[flang] Safer hermetic module file reading

[klausler]

When a hermetic module file is read, use a new scope to hold its dependent modules so that they don't conflict with any modules in the global scope.

[llvmbot]

@llvm/pr-subscribers-flang-semantics

Author: Peter Klausler (klausler)

Changes

When a hermetic module file is read, use a new scope to hold its dependent modules so that they don't conflict with any modules in the global scope.

---

Full diff: https://github.com/llvm/llvm-project/pull/121002.diff

4 Files Affected:

• (modified) flang/docs/ModFiles.md (+10)
• (modified) flang/include/flang/Semantics/semantics.h (+7)
• (modified) flang/lib/Semantics/mod-file.cpp (+19)
• (modified) flang/lib/Semantics/resolve-names.cpp (+11-3)

diff --git a/flang/docs/ModFiles.md b/flang/docs/ModFiles.md
index 7463454c8563a2..a4c2395d308fb2 100644
--- a/flang/docs/ModFiles.md
+++ b/flang/docs/ModFiles.md
@@ -164,3 +164,13 @@ a diagnostic but we still wouldn't have line numbers.
 To provide line numbers and character positions or source lines as the user
 wrote them we would have to save some amount of provenance information in the
 module file as well.
+
+## Hermetic modules files
+
+Top-level module files for libraries can be build with `-fhermetic-module-files`.
+This option causes these module files to contain copies of all of the non-intrinsic
+modules on which they depend, so that non-top-level local modules and the
+modules of dependent libraries need not also be packaged with the library.
+When the compiler reads a hermetic module file, the copies of the dependent
+modules are read into their own scope, and will not conflict with other modules
+of the same name that client code might `USE`.
diff --git a/flang/include/flang/Semantics/semantics.h b/flang/include/flang/Semantics/semantics.h
index c981d86fbd94cb..821ce021b32264 100644
--- a/flang/include/flang/Semantics/semantics.h
+++ b/flang/include/flang/Semantics/semantics.h
@@ -110,6 +110,12 @@ class SemanticsContext {
   }
   Scope &globalScope() { return globalScope_; }
   Scope &intrinsicModulesScope() { return intrinsicModulesScope_; }
+  Scope *currentHermeticModuleFileScope() {
+    return currentHermeticModuleFileScope_;
+  }
+  void set_currentHermeticModuleFileScope(Scope *scope) {
+    currentHermeticModuleFileScope_ = scope;
+  }
   parser::Messages &messages() { return messages_; }
   evaluate::FoldingContext &foldingContext() { return foldingContext_; }
   parser::AllCookedSources &allCookedSources() { return allCookedSources_; }
@@ -313,6 +319,7 @@ class SemanticsContext {
   evaluate::TargetCharacteristics targetCharacteristics_;
   Scope globalScope_;
   Scope &intrinsicModulesScope_;
+  Scope *currentHermeticModuleFileScope_{nullptr};
   ScopeIndex scopeIndex_;
   parser::Messages messages_;
   evaluate::FoldingContext foldingContext_;
diff --git a/flang/lib/Semantics/mod-file.cpp b/flang/lib/Semantics/mod-file.cpp
index c0065045ebee0b..fe3a65e784e0a7 100644
--- a/flang/lib/Semantics/mod-file.cpp
+++ b/flang/lib/Semantics/mod-file.cpp
@@ -1366,6 +1366,12 @@ Scope *ModFileReader::Read(SourceName name, std::optional<bool> isIntrinsic,
       name.ToString(), isIntrinsic.value_or(false))};
   if (!isIntrinsic.value_or(false) && !ancestor) {
     // Already present in the symbol table as a usable non-intrinsic module?
+    if (Scope * hermeticScope{context_.currentHermeticModuleFileScope()}) {
+      auto it{hermeticScope->find(name)};
+      if (it != hermeticScope->end()) {
+        return it->second->scope();
+      }
+    }
     auto it{context_.globalScope().find(name)};
     if (it != context_.globalScope().end()) {
       Scope *scope{it->second->scope()};
@@ -1543,9 +1549,22 @@ Scope *ModFileReader::Read(SourceName name, std::optional<bool> isIntrinsic,
   // Process declarations from the module file
   auto wasModuleFileName{context_.foldingContext().moduleFileName()};
   context_.foldingContext().set_moduleFileName(name);
+  // Are there multiple modules in the module file due to it having been
+  // created under -fhermetic-module-files?  If so, process them first in
+  // their own nested scope that will be visible only to USE statements
+  // within the module file.
+  if (parseTree.v.size() > 1) {
+    parser::Program hermeticModules{std::move(parseTree.v)};
+    parseTree.v.emplace_back(std::move(hermeticModules.v.front()));
+    hermeticModules.v.pop_front();
+    Scope &hermeticScope{topScope.MakeScope(Scope::Kind::Global)};
+    context_.set_currentHermeticModuleFileScope(&hermeticScope);
+    ResolveNames(context_, hermeticModules, hermeticScope);
+  }
   GetModuleDependences(context_.moduleDependences(), sourceFile->content());
   ResolveNames(context_, parseTree, topScope);
   context_.foldingContext().set_moduleFileName(wasModuleFileName);
+  context_.set_currentHermeticModuleFileScope(nullptr);
   if (!moduleSymbol) {
     // Submodule symbols' storage are owned by their parents' scopes,
     // but their names are not in their parents' dictionaries -- we
diff --git a/flang/lib/Semantics/resolve-names.cpp b/flang/lib/Semantics/resolve-names.cpp
index 122c0a2ebb646a..6a2cccea334c06 100644
--- a/flang/lib/Semantics/resolve-names.cpp
+++ b/flang/lib/Semantics/resolve-names.cpp
@@ -2548,9 +2548,11 @@ void ScopeHandler::PopScope() {
     ConvertToObjectEntity(*pair.second);
   }
   funcResultStack_.Pop();
-  // If popping back into a global scope, pop back to the main global scope.
-  SetScope(currScope_->parent().IsGlobal() ? context().globalScope()
-                                           : currScope_->parent());
+  // If popping back into a global scope, pop back to the top scope.
+  Scope *hermetic{context().currentHermeticModuleFileScope()};
+  SetScope(currScope_->parent().IsGlobal()
+          ? (hermetic ? *hermetic : context().globalScope())
+          : currScope_->parent());
 }
 void ScopeHandler::SetScope(Scope &scope) {
   currScope_ = &scope;
@@ -9375,6 +9377,12 @@ template <typename A> std::set<SourceName> GetUses(const A &x) {
 }
 
 bool ResolveNamesVisitor::Pre(const parser::Program &x) {
+  if (Scope * hermetic{context().currentHermeticModuleFileScope()}) {
+    // Processing either the dependent modules or first module of a
+    // hermetic module file; ensure that the hermetic module scope has
+    // its implicit rules map entry.
+    ImplicitRulesVisitor::BeginScope(*hermetic);
+  }
   std::map<SourceName, const parser::ProgramUnit *> modules;
   std::set<SourceName> uses;
   bool disordered{false};

[eugeneepshteyn]

Are there already tests for this?

[klausler]

Are there already tests for this?

Yes. Grep flang/test/Semantics for fhermetic-module-files.

[jeanPerier]

The patch removed the ambiguous error about the module, but more advanced scenarios where subroutine/variable are referred are still causing semantic errors.

Example:

! my_mpi.f90

module my_mpi
! version 1.0
implicit none
integer :: my_mpi_var = 1
contains
subroutine some_mpi_subroutine
  print *, my_mpi_var
end subroutine
end module

! my_lib.f90

module my_lib
  use my_mpi
  implicit none
contains
subroutine some_lib_subroutine
  print *, my_mpi_var
  call some_mpi_subroutine()
end subroutine
end module

! my_mpi_v2.f90

module my_mpi
! version 2.0 that is backward compatible (only adding things).
implicit none
integer :: my_mpi_var = 1
integer :: my_mpi_new_var
contains
subroutine some_mpi_subroutine
  print *, my_mpi_var
end subroutine
end module

! main.f90

main.f90 
  use my_mpi
  use my_lib
  implicit none
  print *, my_mpi_var
  call some_lib_subroutine()
  call some_mpi_subroutine()
end

build script:

FC=flang

# Build library with MY_MPI version 1.
rm -rf my_mpi.o my_mpi.mod
$FC -c my_mpi.f90
rm -rf my_lib.o my_lib.mod
$FC -c -fhermetic-module-files my_lib.f90

# Build application with MY_MPI version 2.
rm -rf my_mpi.o my_mpi.mod
$FC -c my_mpi.f90
$FC -c my_mpi_v2.f90 -o my_mpi.o

$FC main.f90 my_lib.o my_mpi.o

With this patch, I am getting the following semantic errors:

error: Semantic errors in main.f90
./main.f90:4:12: error: Reference to 'my_mpi_var' is ambiguous
    print *, my_mpi_var
             ^^^^^^^^^^
./main.f90:1:7: 'my_mpi_var' was use-associated from module 'my_mpi'
    use my_mpi
        ^^^^^^
./main.f90:2:7: 'my_mpi_var' was use-associated from module 'my_lib'
    use my_lib
        ^^^^^^
./main.f90:6:8: error: Reference to 'some_mpi_subroutine' is ambiguous
    call some_mpi_subroutine()
         ^^^^^^^^^^^^^^^^^^^
./main.f90:1:7: 'some_mpi_subroutine' was use-associated from module 'my_mpi'
    use my_mpi
        ^^^^^^
./main.f90:2:7: 'some_mpi_subroutine' was use-associated from module 'my_lib'
    use my_lib

[klausler]

I've updated the patch to allow for compatible module contents in situations like these.

[jeanPerier]

I really like the solution here. This provides flexibility over library versions while still providing extra safety compared to what exists.

Beware of the windows issue with the current RUN line from the added test.

[klausler]

RUN lines changed in new modfile71.F90 test; will ensure that everything is fine on Linux and Windows before merging.

[klausler]

Updated the patch to convert incompatible symbols into UseErrorDetails, where possible. This allows incompatible symbols (such as named constant library version numbers) to exist so long as they are not actually used in the associating scope.

[klausler]

Updated to generate more helpful attachments to error messages. Will merge soon; take another look if you like.