[flang][Driver] Let the linker fail on multiple definitions of main() #73124
Conversation
llvmbot
added
clang
|
@llvm/pr-subscribers-flang-driver @llvm/pr-subscribers-clang Author: Michael Klemm (mjklemm) ChangesThe flang driver was silently ignoring the Full diff: https://github.com/llvm/llvm-project/pull/73124.diff 1 Files Affected:
diff --git a/clang/lib/Driver/ToolChains/CommonArgs.cpp b/clang/lib/Driver/ToolChains/CommonArgs.cpp
index 5d2cd1959b06925..30c249d05677ce5 100644
--- a/clang/lib/Driver/ToolChains/CommonArgs.cpp
+++ b/clang/lib/Driver/ToolChains/CommonArgs.cpp
@@ -1018,7 +1018,20 @@ void tools::addFortranRuntimeLibs(const ToolChain &TC, const ArgList &Args,
break;
}
} else {
+ // --whole-archive needs to be part of the link line to make sure
+ // that the main() function from Fortran_main.a is pulled in by
+ // the linker.
+ //
+ // We are using this --whole-archive/--no-whole-archive bracket w/o
+ // any further checks, because -Wl,--whole-archive at the flang-new new
+ // line will not sucessfully complete, unless the user correctly specified
+ // -Wl,--no-whole-archive (e.g., -Wl,--whole-archive -ldummy
+ // -Wl,--no-whole-archive).
+ CmdArgs.push_back("--whole-archive");
CmdArgs.push_back("-lFortran_main");
+ CmdArgs.push_back("--no-whole-archive");
+
+ // Perform regular linkage of the remaining runtime libraries.
CmdArgs.push_back("-lFortranRuntime");
CmdArgs.push_back("-lFortranDecimal");
}
@@ -1029,7 +1042,7 @@ void tools::addFortranRuntimeLibraryPath(const ToolChain &TC,
ArgStringList &CmdArgs) {
// Default to the <driver-path>/../lib directory. This works fine on the
// platforms that we have tested so far. We will probably have to re-fine
- // this in the future. In particular, on some platforms, we may need to use
+ // this in the future. In particular, on some platforms, we may need to useq
// lib64 instead of lib.
SmallString<256> DefaultLibPath =
llvm::sys::path::parent_path(TC.getDriver().Dir);
|
|
@llvm/pr-subscribers-clang-driver Author: Michael Klemm (mjklemm) ChangesThe flang driver was silently ignoring the Full diff: https://github.com/llvm/llvm-project/pull/73124.diff 1 Files Affected:
diff --git a/clang/lib/Driver/ToolChains/CommonArgs.cpp b/clang/lib/Driver/ToolChains/CommonArgs.cpp
index 5d2cd1959b06925..30c249d05677ce5 100644
--- a/clang/lib/Driver/ToolChains/CommonArgs.cpp
+++ b/clang/lib/Driver/ToolChains/CommonArgs.cpp
@@ -1018,7 +1018,20 @@ void tools::addFortranRuntimeLibs(const ToolChain &TC, const ArgList &Args,
break;
}
} else {
+ // --whole-archive needs to be part of the link line to make sure
+ // that the main() function from Fortran_main.a is pulled in by
+ // the linker.
+ //
+ // We are using this --whole-archive/--no-whole-archive bracket w/o
+ // any further checks, because -Wl,--whole-archive at the flang-new new
+ // line will not sucessfully complete, unless the user correctly specified
+ // -Wl,--no-whole-archive (e.g., -Wl,--whole-archive -ldummy
+ // -Wl,--no-whole-archive).
+ CmdArgs.push_back("--whole-archive");
CmdArgs.push_back("-lFortran_main");
+ CmdArgs.push_back("--no-whole-archive");
+
+ // Perform regular linkage of the remaining runtime libraries.
CmdArgs.push_back("-lFortranRuntime");
CmdArgs.push_back("-lFortranDecimal");
}
@@ -1029,7 +1042,7 @@ void tools::addFortranRuntimeLibraryPath(const ToolChain &TC,
ArgStringList &CmdArgs) {
// Default to the <driver-path>/../lib directory. This works fine on the
// platforms that we have tested so far. We will probably have to re-fine
- // this in the future. In particular, on some platforms, we may need to use
+ // this in the future. In particular, on some platforms, we may need to useq
// lib64 instead of lib.
SmallString<256> DefaultLibPath =
llvm::sys::path::parent_path(TC.getDriver().Dir);
|
|
Would it be possible to test this? |
Sure thing! That's the next thing I can look at. I'd like to probe though if the general solution is acceptable. |
Makes sense to me - looks like a great improvement! |
llvmbot
added
flang:driver
flang
I have added a simple test. It checks that the linker indeed fails, but does not check the actual linker error message. PLease let me know if that's desired. I could then provide the error messages for LD and LLD. For other environments, I'd have to rely on the community to help me getting the proper linker error messages for the test. |
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
There was a problem hiding this comment.
LGTM, thank you for taking care of this 🙏🏻
Dare I ask - what's "dupes"? I only found dupe. Also, please wait for @kiranchandramohan to approve before merging this :)
I used "dupes" in the sense of being fooled. I can of course still change the name to something like "main_linktime_duplicate.f90" or the likes. |
Right, "dupes" wasn't that obvious to me :) I would probably go for your other suggestion or just plain "duplicate_main.f90". Either would be a bit more descriptive IMHO 🙏🏻 . Thanks again for addressing my comments and for this contribution! |
|
Chipping into the discussion, since this patch I can also no longer build OpenBLAS or PETSc. OpenBLAS for example fails with |
Thanks for the report! Can you please tell me how OpenBLAS was built? I'm trying to replicate this, but I do not see a reference to |
Hi @mjklemm! This was on an AArch64 box (not that that should make a difference) doing something like: The error shows up when linking a C program with a Fortran shared library, so maybe you weren't enabling building shared libraries? |
|
Thanks for the reproducer.
I was building OpenBLAS using the "old" Makefile-based build. There the issue indeed does not happen. When I switched to your CMake configuration line, I was able to reproduce the problem. The solution is to add It seems like |
Thanks, sounds like a good workaround to me, though as you say I find strange the need to explicitly specify "don't include main" when building a library! |
I am trying to recall the rationale behind that, but it's been a while :( Here's a relevant discussion/bug that hasn't been resolved yet. It might feel off-topic at first, but it's in fact a very similar problem - where and when should |
…rtran_main from link line (#74139) This PR adds the `-fno-fortran-main` command line option to remove `Fortran_main.a` from the link and to allow for linking Fortran code w/o program unit with C/C++ translation units that provide the `main()` entrypoint. When linking Fortran code with C/C++ code (Fortran calling into C/C++), PR #73124 introduced a proper error message that would prevent successful linkage, if there was a program unit from Fortran *and* `main()` function coming from C/C++. Alas, this caused some breakage of code that would call Fortran code from C/C++ and rightfully provided the `main()` entrypoint. Classic Flang had the command-line option `-fno-fortran-main` to then remove the entrypoints for the Fortran program unit from the linker stage. This PR is related to PR #74120 and (merged) PR #73124. --------- Co-authored-by: Andrzej Warzyński <[email protected]>
|
Since this patch I can't build binaries with flang-new on macOS anymore: The command line above works fine when I revert this patch (and #74139, to avoid dealing with conflicts). I can also workaround it by adding |
|
We are also seeing the same issue when linking on Mac regarding the |
Is there an equivalent option for the MacOS linker? |
|
Could folks confirm whether this fixes the issue on Darwin: #75393? |
|
@mjklemm This PR caused some regressions of C-interop test cases in our local test run. The test cases typically have a Fortran main (compiled with Flang) that calls a C function (compiled with clang). The linking is by Reverting |
Sorry that you are hitting this, but please note that compiling mixed sources is untested and as such not really supported yet.
Could you share a repro? This way we could start testing this properly. And claim that it's supported :) |
|
I see. So Fortran and C interoperability of F2003/F2008 is not supported yet in Flang? Those ~100ish regression test cases we have were passing before this PR though. |
That's not really what I had in mind. It worked for you so everything that's needed is there, but no upstream testing indicates that you might be the first person trying it. And that nobody has actually tried verifying it and making sure that relevant various bits are integrated in the intended way.
That's a lot - I am sorry about that :(
It sounds like you are hitting something fairly fundamental that should be reproducible with a couple of files (C and Fortran), few lines each. And without a repro, it will be very hard to fix that. We could revert this change, but then again - without a simple repro we won't be able to improve this for the future. |
It's rather untested and I'm working hard to get it fixed, so that we have well-defined behavior that is similar to other Fortran compilers like ifx and gfortran.
Please share as many of these reproducers as you can, so that I can debug the regression and fix it. This error message is a strong indication of potentially two things:
|
|
Thanks for the initial analysis. I will take a closer look of the failure and prepare a reproducer and post it here. |
|
The test cases actually have C main indeed and call to Fortran procedures as opposed to what I thought (the other way around). Adding |
It was always generated before, but the bug was hidden by the linker doing the right thing by incidence. I have fixed that so that such an error does no longer unnoticed. |
|
Yep the situation is something like using It is good that this situation is caught by most linkers (unfortunately, AIX linker does not complain and picks up one of the |
|
Ok. I see. The Fortran |
|
@kkwli and @DanielCChen - could you review #75816 and see whether that makes sense to you? I am referring specifically to the documentation that @mjklemm is kindly adding. Hopefully that will help folks avoid similar issues in the future. |
|
Thanks @banach-space for the pointer! |
Direct follow-up of #73124 - the linker on Darwin does not support `-whole-archive`, so that needs to be removed from the linker invocation. For context: * llvm/llvm-project#73124
The flang driver was silently ignoring the
main()function inFortran_main.afor entry into the Fortran program unit if an externalmain()as supplied (e.g., via cross-language linkage with Fortran and C/C++). This PR fixes this by making sure that the linker always pulls in themain()definition fromFortran_main.aand consequently fails due to multiple definitions of the same symbol if another object file also has a definition ofmain().