[flang][driver] Allow explicit specification of -lFortran_main

[tblah]

Currently, flang-new -lFortran_main will fail on multiple definitions of main.

I can understand there might be differing opinions on whether this is actually a bug.

My thinking is that -lFortran_main should behave the same as -lFortranRuntime.

[llvmbot]

@llvm/pr-subscribers-flang-driver

@llvm/pr-subscribers-clang-driver

Author: Tom Eccles (tblah)

Changes

Currently, flang-new -lFortran_main will fail on multiple definitions of main.

I can understand there might be differing opinions on whether this is actually a bug.

My thinking is that -lFortran_main should behave the same as -lFortranRuntime.

---

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

2 Files Affected:

• (modified) clang/lib/Driver/ToolChains/CommonArgs.cpp (+8)
• (modified) flang/test/Driver/linker-flags.f90 (+3)

diff --git a/clang/lib/Driver/ToolChains/CommonArgs.cpp b/clang/lib/Driver/ToolChains/CommonArgs.cpp
index 385f66f3782bc1..82edb93d157d3f 100644
--- a/clang/lib/Driver/ToolChains/CommonArgs.cpp
+++ b/clang/lib/Driver/ToolChains/CommonArgs.cpp
@@ -1200,6 +1200,14 @@ static void addFortranMain(const ToolChain &TC, const ArgList &Args,
   // TODO: Find an equivalent of `--whole-archive` for Darwin and AIX.
   if (!isWholeArchivePresent(Args) && !TC.getTriple().isMacOSX() &&
       !TC.getTriple().isOSAIX()) {
+    // Adding -lFortran_main with --whole-archive will create an error if the
+    // user specifies -lFortran_main explicitly. Remove the user's
+    // -lFortran_main arguments to avoid this (making sure -lFortran_main
+    // behaves the same as -lFortranRuntime)
+    llvm::erase_if(CmdArgs, [](const char *arg) {
+      return strcmp(arg, "-lFortran_main") == 0;
+    });
+
     CmdArgs.push_back("--whole-archive");
     CmdArgs.push_back("-lFortran_main");
     CmdArgs.push_back("--no-whole-archive");
diff --git a/flang/test/Driver/linker-flags.f90 b/flang/test/Driver/linker-flags.f90
index ea91946316cfaa..0d531cedff4bd2 100644
--- a/flang/test/Driver/linker-flags.f90
+++ b/flang/test/Driver/linker-flags.f90
@@ -12,6 +12,9 @@
 ! RUN: %flang -### --target=x86_64-unknown-haiku %S/Inputs/hello.f90 2>&1 | FileCheck %s --check-prefixes=CHECK,HAIKU
 ! RUN: %flang -### --target=x86_64-windows-gnu %S/Inputs/hello.f90 2>&1 | FileCheck %s --check-prefixes=CHECK,MINGW
 
+! Verify that linking the runtime explicitly doesn't result in a multiple definitions of main error
+! RUN: %flang -lFortran_main -lFortranRuntime -lFortranDecimal %S/Inputs/hello.f90 -o %s.out
+
 ! NOTE: Clang's driver library, clangDriver, usually adds 'oldnames' on Windows,
 !       but it is not needed when compiling Fortran code and they might bring in
 !       additional dependencies. Make sure its not added.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Tom Eccles (tblah)

Changes

Currently, flang-new -lFortran_main will fail on multiple definitions of main.

I can understand there might be differing opinions on whether this is actually a bug.

My thinking is that -lFortran_main should behave the same as -lFortranRuntime.

---

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

2 Files Affected:

• (modified) clang/lib/Driver/ToolChains/CommonArgs.cpp (+8)
• (modified) flang/test/Driver/linker-flags.f90 (+3)

diff --git a/clang/lib/Driver/ToolChains/CommonArgs.cpp b/clang/lib/Driver/ToolChains/CommonArgs.cpp
index 385f66f3782bc1a..82edb93d157d3f1 100644
--- a/clang/lib/Driver/ToolChains/CommonArgs.cpp
+++ b/clang/lib/Driver/ToolChains/CommonArgs.cpp
@@ -1200,6 +1200,14 @@ static void addFortranMain(const ToolChain &TC, const ArgList &Args,
   // TODO: Find an equivalent of `--whole-archive` for Darwin and AIX.
   if (!isWholeArchivePresent(Args) && !TC.getTriple().isMacOSX() &&
       !TC.getTriple().isOSAIX()) {
+    // Adding -lFortran_main with --whole-archive will create an error if the
+    // user specifies -lFortran_main explicitly. Remove the user's
+    // -lFortran_main arguments to avoid this (making sure -lFortran_main
+    // behaves the same as -lFortranRuntime)
+    llvm::erase_if(CmdArgs, [](const char *arg) {
+      return strcmp(arg, "-lFortran_main") == 0;
+    });
+
     CmdArgs.push_back("--whole-archive");
     CmdArgs.push_back("-lFortran_main");
     CmdArgs.push_back("--no-whole-archive");
diff --git a/flang/test/Driver/linker-flags.f90 b/flang/test/Driver/linker-flags.f90
index ea91946316cfaa6..0d531cedff4bd2c 100644
--- a/flang/test/Driver/linker-flags.f90
+++ b/flang/test/Driver/linker-flags.f90
@@ -12,6 +12,9 @@
 ! RUN: %flang -### --target=x86_64-unknown-haiku %S/Inputs/hello.f90 2>&1 | FileCheck %s --check-prefixes=CHECK,HAIKU
 ! RUN: %flang -### --target=x86_64-windows-gnu %S/Inputs/hello.f90 2>&1 | FileCheck %s --check-prefixes=CHECK,MINGW
 
+! Verify that linking the runtime explicitly doesn't result in a multiple definitions of main error
+! RUN: %flang -lFortran_main -lFortranRuntime -lFortranDecimal %S/Inputs/hello.f90 -o %s.out
+
 ! NOTE: Clang's driver library, clangDriver, usually adds 'oldnames' on Windows,
 !       but it is not needed when compiling Fortran code and they might bring in
 !       additional dependencies. Make sure its not added.

[banach-space]

I am not too fond of this.

The current design has taken quite some effort and a few discussions to land in its current form. Also, Flang has gained proper documentation for this:

• https://github.com/llvm/llvm-project/blob/main/flang/docs/FlangDriver.md#linker-driver

This change is effectively undoing an important part of that. In this case:

flang-new -lFortran_main 

the right thing to do would be to check the documentation, which should explain: "don't do this, because of xyz".

In general, I see Fortran_Main as an implementation detail that end users should not require and I like the idea of "hiding" it from the end user.

Having said all that, I'm not going to block this. Howerver, I would like to see that there's consensus before this lands.

[tblah]

I agree that the documentation says not to do this, but existing common build configurations are doing it anyway. For example, building netcdf-parallel (https://github.com/Parallel-NetCDF/PnetCDF) using spack (https://spack.io/).

netcdf-parallel uses autotools. I haven't tried another autotools project to see if it has the same problem.

Of course the long term solution will be to fix autotools (or whatever the culprit is), but I worry it could take a long time for such patches to filter out to distros. In the meantime, I think it is a bad user experience to fail builds with a cryptic linker error.

[kkwli]

How would flang-new -fno-fortran-main t.f -lFortran_main work?

[tblah]

How would flang-new -fno-fortran-main t.f -lFortran_main work?

Good question. This won't work with this patch but currently should work. I'll have to fix this.

Do you agree that flang-new -lFortran_main is something we want to make work?

[mjklemm]

I'm kind of split brain on this. While I do see the issue, I'm not sure if this should be considered a bug or a user error.

One thing that came to my mind though is (remotely related to this) to have a command line flag that reports the proper libraries needed to successfully link a Fortran program using a different linker driver (e.g., clang). Similar to what mpif90 -showme:link does for Open MPI:

$ mpif90 --showme:link
-I/net/software/x86_64/openmpi/5.0.x/aomp/include -I/net/software/x86_64/openmpi/5.0.x/aomp/lib -L/net/software/x86_64/openmpi/5.0.x/aomp/lib -Wl,-rpath -Wl,/net/software/x86_64/openmpi/5.0.x/aomp/lib -Wl,--enable-new-dtags -lmpi_usempif08 -lmpi_usempi_ignore_tkr -lmpi_mpifh -lmpi

So, this would look like this at the moment:

$ flang -fshow-me-link-line
 "-L/net/software/x86_64/llvm-ml/20231225/lib" "--whole-archive" "-lFortran_main" "--no-whole-archive" "-lFortranRuntime" "-lFortranDecimal" "-lm"

[kkwli]

How would flang-new -fno-fortran-main t.f -lFortran_main work?

Good question. This won't work with this patch but currently should work. I'll have to fix this.

Do you agree that flang-new -lFortran_main is something we want to make work?

In my opinion, we should hide 'libFortran_main' as much as possible since it is really an implementation detail. I can see that the proposed behavior will give users less surprise. I am slightly in favor of this behavior.

In a long run, I still think that we need to get rid of Fortran_main. Perhaps, for the minimal, we can rename it to remove Fortran in the name to make people think that it is not required (by the users) to do any linking for Fortran code.

[mjklemm]

In a long run, I still think that we need to get rid of Fortran_main. Perhaps, for the minimal, we can rename it to remove Fortran in the name to make people think that it is not required (by the users) to do any linking for Fortran code.

I'd agree to that! The way gfortran does it, seems the best approach. When there's a program unit in the code, gfortran also add a main definition to that translation unit. Unfortunately, my knowledge about flang internals is not yet there, so I have no clue how to get it to emit the equivalent of the main in libFortran_main. I could get away with having it in .ll file or something and pull it in from disk, but even that seems to exceed my current capabilities. :-)

I could opt-in to make this a learning project for me, but I'd need some hand holding to get it done. Volunteers? :-)

[tblah]

I'd agree to that! The way gfortran does it, seems the best approach. When there's a program unit in the code, gfortran also add a main definition to that translation unit.

+2

I could opt-in to make this a learning project for me, but I'd need some hand holding to get it done. Volunteers? :-)

I'm happy to help, but I think we should discuss this in a community call first

[tblah]

How would flang-new -fno-fortran-main t.f -lFortran_main work?

This works because I only remove -lFortran_main when it is going to be added implicitly. -fno-fortran-main ensures that we never reach this branch. I've added a test to verify this.

[kkwli]

How would flang-new -fno-fortran-main t.f -lFortran_main work?

This works because I only remove -lFortran_main when it is going to be added implicitly. -fno-fortran-main ensures that we never reach this branch. I've added a test to verify this.

I don't know what is the right way to handle the case that users have conflicting flags specified. This behavior is to only remove the implicit -lFortran_main not the explicit one. However, in my opinion, the -fno-fortran-main is an explicit intent that the users do not want the main whether or not -lFortran_main is specified.

[banach-space]

I don't know what is the right way to handle the case that users have conflicting flags specified.

This comment made me realise what might be the source of confusion/friction.

With -fno-fortran-main and -lFortran_main, there are two very different mechanisms to control the same thing - whether Fortran_main.a should be added to the linker invocation. If we replaced -lFortran_main with -ffortran-main then:

• your example would become -fno-fortran-main -ffortran-main and we know how to deal with that (the driver will simply use the rightmost flag)
• -lFortran_main would finally be hidden from end users - everyone seems to agree that that would be the right thing to do.

My suggestion:

• rename Fortran_main.a as e.g. flang_fortran_main.a,
• introduce -ffortran-main on top of -fno-fortran-main,
• disallow -lFortran_main.a and -lflang_fortran_main.a - this library should be kept as an implementation detail of Flang that can change in the future.

The final point is probably key. Flang might change how "main" is defined in the future and we should discourage anyone linking Fortran_main.a directly. Mostly to avoid creating dependency on something that's an implementation detail.

[tblah]

My suggestion:

* rename `Fortran_main.a` as e.g. `flang_fortran_main.a`,

* introduce `-ffortran-main` on top of `-fno-fortran-main`,

* disallow `-lFortran_main.a` and `-lflang_fortran_main.a` - this library should be kept as an implementation detail of Flang that can change in the future.

@banach-space I tentatively support this. But I am concerned that disallowing -lFortran_main and renaming the library will break some existing (badly configured) builds (this is not just theoretical - see my comments above). Maybe we should merge a deprecation warning on -lFortran_main before the LLVM release, then make these changes only after the release has branched?

[mjklemm]

Maybe we should merge a deprecation warning on -lFortran_main before the LLVM release, then make these changes only after the release has branched?

That sounds like a good idea for now. Then, we can buy a bit of time for the RFC and to make decision how we want to deal with the main problem (pun intended :-)).

[banach-space]

@banach-space I tentatively support this. But I am concerned that disallowing -lFortran_main and renaming the library will break some existing (badly configured) builds (this is not just theoretical - see my comments above). Maybe we should merge a deprecation warning on -lFortran_main before the LLVM release, then make these changes only after the release has branched?

These are very good points, +1 to all of the above!

[mjklemm]

Should this PR be closed at some point? It seems to be obsolete with the work that has been done in the recent past to improve the situation.