[flang][driver] do not crash when fc1 process multiple files

[jeanPerier]

This is a fix for the issue #137126 that turned out to be a driver issue.

The FrontendActions have a loop to process multiple input file and flang -fc1 accept multiple files, but the semantic, lowering, and llvm codegen actions were not re-entrant.

• The SemanticContext cannot be reuse between two files since it is holding the scope, so symbol will clash.
• The mlir and llvm module need to be reset before overriding the previous contexts, otherwise there later deletion will use the old context and cause use after free.

I am unsure it is the intention/a feature to process mutliple input files in a single -fc1 command
Flang invoke one flang -fc1 process per input file, hence the issue does not show up with flang -c foo.f90 bar.f90). I think my fix is fragile (it is hard to keep track of where all the unique pointers are set/deleted, and how they depend on each other) and maybe it would be better to prevent multiple input to fc1. @banach-space, what was the intention?

[llvmbot]

@llvm/pr-subscribers-flang-driver

Author: None (jeanPerier)

Changes

This is a fix for the issue #137126 that turned out to be a driver issue.

The FrontendActions have a loop to process multiple input file and flang -fc1 accept multiple files, but the semantic, lowering, and llvm codegen actions were not re-entrant.

• The SemanticContext cannot be reuse between two files since it is holding the scope, so symbol will clash.
• The mlir and llvm module need to be reset before overriding the previous contexts, otherwise there later deletion will use the old context and cause use after free.

I am unsure it is the intention/a feature to process mutliple input files in a single -fc1 command
Flang invoke one flang -fc1 process per input file, hence the issue does not show up with flang -c foo.f90 bar.f90). I think my fix is fragile (it is hard to keep track of where all the unique pointers are set/deleted, and how they depend on each other) and maybe it would be better to prevent multiple input to fc1. @banach-space, what was the intention?

---

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

5 Files Affected:

• (modified) flang/include/flang/Frontend/CompilerInstance.h (+6)
• (modified) flang/lib/Frontend/CompilerInstance.cpp (-2)
• (modified) flang/lib/Frontend/FrontendAction.cpp (+1-1)
• (modified) flang/lib/Frontend/FrontendActions.cpp (+7)
• (added) flang/test/Driver/multiple-fc1-input.f90 (+9)

diff --git a/flang/include/flang/Frontend/CompilerInstance.h b/flang/include/flang/Frontend/CompilerInstance.h
index e37ef5e236871..4ad95c9df42d7 100644
--- a/flang/include/flang/Frontend/CompilerInstance.h
+++ b/flang/include/flang/Frontend/CompilerInstance.h
@@ -147,6 +147,12 @@ class CompilerInstance {
   /// @name Semantic analysis
   /// {
 
+  Fortran::semantics::SemanticsContext &createNewSemanticsContext() {
+    semaContext =
+        getInvocation().getSemanticsCtx(*allCookedSources, getTargetMachine());
+    return *semaContext;
+  }
+
   Fortran::semantics::SemanticsContext &getSemanticsContext() {
     return *semaContext;
   }
diff --git a/flang/lib/Frontend/CompilerInstance.cpp b/flang/lib/Frontend/CompilerInstance.cpp
index f7ed969f03bf4..cbd2c58eeeb47 100644
--- a/flang/lib/Frontend/CompilerInstance.cpp
+++ b/flang/lib/Frontend/CompilerInstance.cpp
@@ -162,8 +162,6 @@ bool CompilerInstance::executeAction(FrontendAction &act) {
   allSources->set_encoding(invoc.getFortranOpts().encoding);
   if (!setUpTargetMachine())
     return false;
-  // Create the semantics context
-  semaContext = invoc.getSemanticsCtx(*allCookedSources, getTargetMachine());
   // Set options controlling lowering to FIR.
   invoc.setLoweringOptions();
 
diff --git a/flang/lib/Frontend/FrontendAction.cpp b/flang/lib/Frontend/FrontendAction.cpp
index ab77d143fa4b6..d178fd6a9395d 100644
--- a/flang/lib/Frontend/FrontendAction.cpp
+++ b/flang/lib/Frontend/FrontendAction.cpp
@@ -183,7 +183,7 @@ bool FrontendAction::runSemanticChecks() {
 
   // Transfer any pending non-fatal messages from parsing to semantics
   // so that they are merged and all printed in order.
-  auto &semanticsCtx{ci.getSemanticsContext()};
+  auto &semanticsCtx{ci.createNewSemanticsContext()};
   semanticsCtx.messages().Annex(std::move(ci.getParsing().messages()));
   semanticsCtx.set_debugModuleWriter(ci.getInvocation().getDebugModuleDir());
 
diff --git a/flang/lib/Frontend/FrontendActions.cpp b/flang/lib/Frontend/FrontendActions.cpp
index c1f47b12abee2..e5a15c555fa5e 100644
--- a/flang/lib/Frontend/FrontendActions.cpp
+++ b/flang/lib/Frontend/FrontendActions.cpp
@@ -171,6 +171,10 @@ static void addDependentLibs(mlir::ModuleOp mlirModule, CompilerInstance &ci) {
 }
 
 bool CodeGenAction::beginSourceFileAction() {
+  // Delete previous LLVM module depending on old context before making a new
+  // one.
+  if (llvmModule)
+    llvmModule.reset(nullptr);
   llvmCtx = std::make_unique<llvm::LLVMContext>();
   CompilerInstance &ci = this->getInstance();
   mlir::DefaultTimingManager &timingMgr = ci.getTimingManager();
@@ -197,6 +201,9 @@ bool CodeGenAction::beginSourceFileAction() {
     return true;
   }
 
+  // Reset MLIR module if it was set before overriding the old context.
+  if (mlirModule)
+    mlirModule = mlir::OwningOpRef<mlir::ModuleOp>(nullptr);
   // Load the MLIR dialects required by Flang
   mlirCtx = std::make_unique<mlir::MLIRContext>();
   fir::support::loadDialects(*mlirCtx);
diff --git a/flang/test/Driver/multiple-fc1-input.f90 b/flang/test/Driver/multiple-fc1-input.f90
new file mode 100644
index 0000000000000..57f7c5e92b4c4
--- /dev/null
+++ b/flang/test/Driver/multiple-fc1-input.f90
@@ -0,0 +1,9 @@
+! Test that flang -fc1 can be called with several input files without
+! crashing.
+! Regression tests for: https://github.com/llvm/llvm-project/issues/137126
+
+! RUN: %flang_fc1 -emit-fir %s %s -o - | FileCheck %s
+subroutine foo()
+end subroutine
+! CHECK: func @_QPfoo() 
+! CHECK: func @_QPfoo() 

[DavidTruby]

LGTM thanks for the fix!

[eugeneepshteyn]

I'm not the deciding vote, but this LGTM

[inaki-amatria]

TYSM @jeanPerier! We really appreciate you taking the time to look into this issue.

The way I reported the issue, invoking two files with flang -fc1, was intentional. We use both Clang and Flang as libraries, and this was the most effective way to report the issue in a form that aligns with your typical workflow, where Flang is primarily used as a binary.

Regardless of whether flang -fc1 is expected to accept multiple input files directly, we believe the fix is still of great value for Flang library users like us.

Again, thanks so much for looking into this @jeanPerier.

[DavidTruby]

Sorry, @inaki-amatria's comment has made me realise I didn't look at this closely enough; I clearly only skim read the commit message, and the code change looked sensible to me...
I've checked what clang -cc1 does here, and that only accepts the last flag. I.e. clang -cc1 -o test.o -x c test.c -o test2.o -x c test2.c will only genereate test2.o. It does this silently, i.e. it doesn't give you any warning that you've passed multiple files.

I'm not opposed to flang -fc1 accepting it even though clang -cc1 doesn't, but I'm not actually clear what the expected behaviour would be with flang -fc1 accepting multiple input files. What happens with this change?

I do think that making this re-entrant is sensible anyway for library users as @inaki-amatria mentioned so my accept still stands.

[jeanPerier]

I'm not opposed to flang -fc1 accepting it even though clang -cc1 doesn't, but I'm not actually clear what the expected behavior would be with flang -fc1 accepting multiple input files. What happens with this change?

With this change, instead of crashing, flang -fc1 -emit-obj bar.f90 foo.f90 now emits two object files, bar.o and foo.o (just like if they had been processed individually).