[flang][driver] Remove Fortain_main static library from linking stages #75816

mjklemm · 2023-12-18T15:48:40Z

At present, when building static or shared libraries, Flang adds -lFortran_main.a (or /WHOLEARCHIVE:Fortran.*.lib pon Windows) to the link line. This leads to the problem that _QQmain and _QQEnvironmentDefaults (as of the time of this PR) are symbols marked as used, while main is being defined. This should not happen and this PR fixes this by detecting if -shared or -static is used on the Flang command line and removing the static Fortran_main library.

llvmbot · 2023-12-18T15:49:07Z

@llvm/pr-subscribers-flang-driver
@llvm/pr-subscribers-clang-driver

@llvm/pr-subscribers-clang

Author: Michael Klemm (mjklemm)

Changes

At present, when building static or shared libraries, Flang adds -lFortran_main.a (or /WHOLEARCHIVE:Fortran.*.lib pon Windows) to the link line. This leads to the problem that _QQmain and _QQEnvironmentDefaults (as of the time of this PR) are symbols marked as used, while main is being defined. This should not happen and this PR fixes this by detecting if -shared or -static is used on the Flang command line and removing the static Fortran_main library.

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

2 Files Affected:

(modified) clang/lib/Driver/ToolChains/CommonArgs.cpp (+20)
(modified) flang/test/Driver/dynamic-linker.f90 (+6-2)

diff --git a/clang/lib/Driver/ToolChains/CommonArgs.cpp b/clang/lib/Driver/ToolChains/CommonArgs.cpp
index 45901ee7157f77..05ebd42829c95d 100644
--- a/clang/lib/Driver/ToolChains/CommonArgs.cpp
+++ b/clang/lib/Driver/ToolChains/CommonArgs.cpp
@@ -1133,6 +1133,16 @@ static bool isWholeArchivePresent(const ArgList &Args) {
   return WholeArchiveActive;
 }
 
+/// Determine if driver is invoked to create a shared object library (-static)
+static bool isSharedLinkage(const ArgList &Args) {
+  return Args.hasArg(options::OPT_shared);
+}
+
+/// Determine if driver is invoked to create a static object library (-shared)
+static bool isStaticLinkage(const ArgList &Args) {
+  return Args.hasArg(options::OPT_static);
+}
+
 /// Add Fortran runtime libs for MSVC
 static void addFortranRuntimeLibsMSVC(const ArgList &Args,
                                       llvm::opt::ArgStringList &CmdArgs) {
@@ -1164,6 +1174,16 @@ static void addFortranRuntimeLibsMSVC(const ArgList &Args,
 // Add FortranMain runtime lib
 static void addFortranMain(const ToolChain &TC, const ArgList &Args,
                            llvm::opt::ArgStringList &CmdArgs) {
+  // 0. Shared-library linkage
+  // If we are attempting to link a library, we should not add
+  // -lFortran_main.a to the link line, as the `main` symbol is not
+  // required for a library and should also be provided by one of
+  // the translation units of the code that this shared library
+  // will be linked against eventually.
+  if (isSharedLinkage(Args) || isStaticLinkage(Args)) {
+    return;
+  }
+
   // 1. MSVC
   if (TC.getTriple().isKnownWindowsMSVCEnvironment()) {
     addFortranRuntimeLibsMSVC(Args, CmdArgs);
diff --git a/flang/test/Driver/dynamic-linker.f90 b/flang/test/Driver/dynamic-linker.f90
index df119c22a2ea51..af07e2483f93fa 100644
--- a/flang/test/Driver/dynamic-linker.f90
+++ b/flang/test/Driver/dynamic-linker.f90
@@ -3,18 +3,22 @@
 
 ! RUN: %flang -### --target=x86_64-linux-gnu -rpath /path/to/dir -shared \
 ! RUN:     -static %s 2>&1 | FileCheck \
-! RUN:     --check-prefixes=GNU-LINKER-OPTIONS %s
+! RUN:     --check-prefixes=GNU-LINKER-OPTIONS \
+! RUN:     --implicit-check-not=GNU-LINKER-OPTIONS-NOT %s
 ! RUN: %flang -### --target=x86_64-windows-msvc -rpath /path/to/dir -shared \
 ! RUN:     -static %s 2>&1 | FileCheck \
-! RUN:     --check-prefixes=MSVC-LINKER-OPTIONS %s
+! RUN:     --check-prefixes=MSVC-LINKER-OPTIONS \
+! RUN:     --implicit-check-not=MSVC-LINKER-OPTIONS-NOT %s
 
 ! TODO: Could the linker have an extension or a suffix?
 ! GNU-LINKER-OPTIONS: "{{.*}}ld{{(.exe)?}}"
 ! GNU-LINKER-OPTIONS-SAME: "-shared"
 ! GNU-LINKER-OPTIONS-SAME: "-static"
 ! GNU-LINKER-OPTIONS-SAME: "-rpath" "/path/to/dir"
+! GNU-LINKER-OPTIONS-NOT: "-lFortran_main.a"
 
 ! For MSVC, adding -static does not add any additional linker options.
 ! MSVC-LINKER-OPTIONS: "{{.*}}link{{(.exe)?}}"
 ! MSVC-LINKER-OPTIONS-SAME: "-dll"
 ! MSVC-LINKER-OPTIONS-SAME: "-rpath" "/path/to/dir"
+! MSVC-LINKER-OPTIONS-NOT: "/WHOLEARCHIVE:Fortran_main"

banach-space · 2023-12-18T19:15:09Z

Thanks! Mostly looks good, but I have a question:

This leads to the problem that _QQmain and _QQEnvironmentDefaults (as of the time of this PR) are symbols marked as used, while main is being defined.

Sorry for being pedantic, but not sure I follow. FortranMain defines main, which calls _QQmain. However, the latter wouldn't be available in a library, right?

Also, not sure what defines _QQEnvironmentDefaults and what exactly makes it problematic?

Btw, would you mind adding a note to the driver docs?

flang/test/Driver/dynamic-linker.f90

clang/lib/Driver/ToolChains/CommonArgs.cpp

mjklemm · 2023-12-18T19:48:16Z

Thanks! Mostly looks good, but I have a question:

This leads to the problem that _QQmain and _QQEnvironmentDefaults (as of the time of this PR) are symbols marked as used, while main is being defined.

Sorry for being pedantic, but not sure I follow. FortranMain defines main, which calls _QQmain. However, the latter wouldn't be available in a library, right?

Yes, that's correct. The main symbol calls _QQmain, which happens to be the entry point for the Fortran program unit.

Also, not sure what defines _QQEnvironmentDefaults and what exactly makes it problematic?

The main entrypoint also calls _QQEnvironmentDefaults to treat argc, argv, etc. So, when main is included in the shared object, so will be those symbols are used symbols, which makes no sense.

Btw, would you mind adding a note to the driver docs?

Sure, I was planning to do this for the previous PR already but got distracted. Can you point me to the right place to add this? Thanks!

mjklemm · 2023-12-18T19:56:30Z

Also, not sure what defines _QQEnvironmentDefaults and what exactly makes it problematic?

The main entrypoint also calls _QQEnvironmentDefaults to treat argc, argv, etc. So, when main is included in the shared object, so will be those symbols are used symbols, which makes no sense.

I need to correct myself: _QQEnvironmentDefaults is not a function that is called, but rather an external file-scope variable that creates that symbol. The symbol used to initialize that is _FortranAProgramStart. Plus, there's _FortranAProgramEndStatement that also used by Fortran_main.a. So, lot's of symbols that should not be in a dynamic library for no reason. :-)

banach-space · 2023-12-18T20:42:03Z

Sure, I was planning to do this for the previous PR already but got distracted. Can you point me to the right place to add this? Thanks!

Somewhere here:

https://github.com/llvm/llvm-project/blob/main/flang/docs/FlangDriver.md

I would probably create a section on building executables and just mention that 3 runtime libs will be linked auto-magically. Unless somebody uses -fno-fortran-main ;-) Make it as short or as long as you prefer - your contribution is greatly appreciated 🙏🏻 :)

mjklemm · 2023-12-19T14:17:13Z

I would probably create a section on building executables and just mention that 3 runtime libs will be linked auto-magically. Unless somebody uses -fno-fortran-main ;-) Make it as short or as long as you prefer - your contribution is greatly appreciated 🙏🏻 :)

I've have written up a first draft. Please have a look and let me know what you think.

mjklemm · 2023-12-19T20:24:03Z

@banach-space How did you draw the pictures in the MD file that I'm changing? If you have some sort of source file, I can try to add a nice chart to the explanation, I'm adding.

banach-space

Make sense, thank you! I really appreciate you fixing this 🙏🏻

@kiranchandramohan - could you also take a look?

flang/docs/FlangDriver.md

kiranchandramohan · 2023-12-20T12:50:03Z

Looks reasonable to me.

I guess the environment settings that are there in the EnvironmentDefaults apply only when the main program is compiled with some flags. It would be good to check the options in EnvironmentDefaults and see that they are all applicable only when compiled for the main program.

flang/docs/FlangDriver.md

kkwli

LG. Thanks.

flang/docs/FlangDriver.md

Co-authored-by: kkwli <[email protected]>

mjklemm · 2023-12-26T07:34:03Z

Thanks for all the good suggestions!

h-vetinari · 2024-05-17T02:23:48Z

For anyone (like me) having to deal with correctly linking Fortran_main with flang 18+, it's worth noting that much of this got removed again in 8d53866.

mjklemm requested review from DavidTruby and banach-space December 18, 2023 15:48

llvmbot added clang Clang issues not falling into any other category clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' flang:driver flang Flang issues not falling into any other category labels Dec 18, 2023

mjklemm self-assigned this Dec 18, 2023

banach-space reviewed Dec 18, 2023

View reviewed changes

flang/test/Driver/dynamic-linker.f90 Show resolved Hide resolved

clang/lib/Driver/ToolChains/CommonArgs.cpp Show resolved Hide resolved

banach-space approved these changes Dec 19, 2023

View reviewed changes

flang/docs/FlangDriver.md Outdated Show resolved Hide resolved

flang/docs/FlangDriver.md Show resolved Hide resolved

banach-space requested review from kiranchandramohan and tblah December 19, 2023 21:27

banach-space mentioned this pull request Dec 20, 2023

[flang][Driver] Let the linker fail on multiple definitions of main() #73124

Merged

kkwli reviewed Dec 22, 2023

View reviewed changes

flang/docs/FlangDriver.md Outdated Show resolved Hide resolved

flang/docs/FlangDriver.md Outdated Show resolved Hide resolved

flang/docs/FlangDriver.md Outdated Show resolved Hide resolved

mjklemm requested a review from kkwli December 24, 2023 09:26

kkwli approved these changes Dec 25, 2023

View reviewed changes

flang/docs/FlangDriver.md Outdated Show resolved Hide resolved

mjklemm and others added 7 commits December 25, 2023 16:16

Remove -lFortran_main from the link line when -shared is present

b3fa63a

Update dynamic_linker.f90 test and clean up a bit

a480e16

Write up for Flang creates the link line and the thoughts behind that

bdd5d9b

Make small addition about static and shared libraries

a2e41ca

Try to improve the wording a bit

c9a9bb1

Apply suggested edit by @kkwli

b31e990

Incorporate change by @kkwli

b29a7fd

Co-authored-by: kkwli <[email protected]>

mjklemm force-pushed the fix_shared_lib_linking branch from 8ff6553 to b29a7fd Compare December 25, 2023 16:35

Update flang/docs/FlangDriver.md

8a72662

Co-authored-by: kkwli <[email protected]>

mjklemm merged commit 9d6837d into llvm:main Dec 25, 2023

mjklemm deleted the fix_shared_lib_linking branch December 27, 2023 06:34

[flang][driver] Remove Fortain_main static library from linking stages #75816

[flang][driver] Remove Fortain_main static library from linking stages #75816

Uh oh!

Conversation

mjklemm commented Dec 18, 2023

Uh oh!

llvmbot commented Dec 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

banach-space commented Dec 18, 2023

Uh oh!

Uh oh!

Uh oh!

mjklemm commented Dec 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mjklemm commented Dec 18, 2023

Uh oh!

banach-space commented Dec 18, 2023

Uh oh!

mjklemm commented Dec 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mjklemm commented Dec 19, 2023

Uh oh!

banach-space left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kiranchandramohan commented Dec 20, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kkwli left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mjklemm commented Dec 26, 2023

Uh oh!

h-vetinari commented May 17, 2024

Uh oh!

Uh oh!

llvmbot commented Dec 18, 2023 •

edited

Loading

mjklemm commented Dec 18, 2023 •

edited

Loading

mjklemm commented Dec 19, 2023 •

edited

Loading