[flang][driver] Don't use -whole-archive on Darwin #75393

banach-space · 2023-12-13T22:07:53Z

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:

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

llvmbot · 2023-12-13T22:08:21Z

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

@llvm/pr-subscribers-clang

Author: Andrzej Warzyński (banach-space)

Changes

Direct follow-up of #7312 - the linker on Darwin does not support
-whole-archive, so that needs to be removed from the linker
invocation.

For context:

SCCP miscompiles sitofp(undef) #7312

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

1 Files Affected:

(modified) clang/lib/Driver/ToolChains/CommonArgs.cpp (+9-5)

diff --git a/clang/lib/Driver/ToolChains/CommonArgs.cpp b/clang/lib/Driver/ToolChains/CommonArgs.cpp
index 51b336216c5653..4ce456d52c3e5e 100644
--- a/clang/lib/Driver/ToolChains/CommonArgs.cpp
+++ b/clang/lib/Driver/ToolChains/CommonArgs.cpp
@@ -1128,20 +1128,24 @@ void tools::addFortranRuntimeLibs(const ToolChain &TC, const ArgList &Args,
     // --whole-archive flag to the link line.  If it's not, add a proper
     // --whole-archive/--no-whole-archive bracket to the link line.
     bool WholeArchiveActive = false;
-    for (auto *Arg : Args.filtered(options::OPT_Wl_COMMA))
-      if (Arg)
+    for (auto *Arg : Args.filtered(options::OPT_Wl_COMMA)) {
+      if (Arg) {
         for (StringRef ArgValue : Arg->getValues()) {
           if (ArgValue == "--whole-archive")
             WholeArchiveActive = true;
           if (ArgValue == "--no-whole-archive")
             WholeArchiveActive = false;
         }
+      }
+    }
 
-    if (!WholeArchiveActive)
+    if (!WholeArchiveActive && !TC.getTriple().isMacOSX()) {
       CmdArgs.push_back("--whole-archive");
-    CmdArgs.push_back("-lFortran_main");
-    if (!WholeArchiveActive)
+      CmdArgs.push_back("-lFortran_main");
       CmdArgs.push_back("--no-whole-archive");
+    } else {
+      CmdArgs.push_back("-lFortran_main");
+    }
 
     // Perform regular linkage of the remaining runtime libraries.
     CmdArgs.push_back("-lFortranRuntime");

luporl

LGTM. Worked fine on my machine.

NOTE: tested by replacing CommonArgs.cpp in main, to avoid conflicts.

Direct follow-up of llvm#7312 - the linker on Darwin does not support `-whole-archive`, so that needs to be removed from the linker invocation. For context: * llvm#7312

banach-space · 2023-12-14T12:08:14Z

LGTM. Worked fine on my machine.

NOTE: tested by replacing CommonArgs.cpp in main, to avoid conflicts.

Thanks for checking and apologies for the merge conflict - I thought that I was up to date :( I've just rebased and force-pushed.

I will be landing this today if there are no further comments 🙏🏻

mjklemm · 2023-12-14T12:17:43Z

LGTM. Worked fine on my machine.

NOTE: tested by replacing CommonArgs.cpp in main, to avoid conflicts.

Thanks for checking and apologies for the merge conflict - I thought that I was up to date :( I've just rebased and force-pushed.

I will be landing this today if there are no further comments 🙏🏻

This patch seems to hide the original problem of the compiler not erroring out when multiple definitions of main happen. I think we should rather inject the proper linker equivalent of --whole-archive on Darwin, too.

I'd be OK to land this patch for now, if we agree to have a follow up PR to (re-)instantiate the error handling also for Darwin.

banach-space · 2023-12-14T12:39:26Z

LGTM. Worked fine on my machine.

NOTE: tested by replacing CommonArgs.cpp in main, to avoid conflicts.

Thanks for checking and apologies for the merge conflict - I thought that I was up to date :( I've just rebased and force-pushed.
I will be landing this today if there are no further comments 🙏🏻

This patch seems to hide the original problem of the compiler not erroring out when multiple definitions of main happen.

To clarify - this patch is a warkaround to avoid reverting #73124. And to buy ourselves some time. I did a bit of research and couldn't find any equivalents for Darwin for -whole-archive :( I am a bit concerned that this might be the only option TBH, but hopefully I am wrong.

I'd be OK to land this patch for now, if we agree to have a follow up PR to (re-)instantiate the error handling also for Darwin.

That would be ideal, but I won't have the bandwidth for that. We'll need a volunteer. Another option is to revert #73124. Either way, we need to unblock our Darwin users ASAP :)

Add an extra comment

luporl · 2023-12-14T12:52:27Z

I've run flang/test/Driver/no-duplicate-main.f90 manually on Darwin and noticed that the third RUN line fails, because flang doesn't return an error:
! RUN: not %flang -o %t.exe %t %t.c-object 2>&1

So maybe -whole-archive is just not needed on it?

The equivalent of -whole-archive is needed on Darwin, I had misunderstood the test.

luporl · 2023-12-14T13:07:51Z

Some sources (https://stackoverflow.com/questions/16082470/osx-how-do-i-convert-a-static-library-to-a-dynamic-one) suggest the use of -force_load:

     -force_load path_to_archive
             Loads all members of the specified static archive library.  Note: -all_load forces all members of all archives to be
             loaded.  This option allows you to target a specific archive.

Using it I get a duplicate symbol '_main' error in flang/test/Driver/no-duplicate-main.f90, as expected.
The downside is that it doesn't work with -lFortran_main, but need the path to the static library instead.

kkwli · 2023-12-14T15:25:33Z

I build and check-flang on arm64-apple-darwin22.6.0. Driver/no-duplicate-main.f90 fails in the 3rd RUN. The test passes with HEAD.

kelvin@neutrino2 build % /Users/kelvin/wrk/llvm/tmp/build/bin/flang-new -o /Users/kelvin/wrk/llvm/tmp/build/tools/flang/test/Driver/Output/no-duplicate-main.f90.tmp.exe /Users/kelvin/wrk/llvm/tmp/build/tools/flang/test/Driver/Output/no-duplicate-main.f90.tmp /Users/kelvin/wrk/llvm/tmp/build/tools/flang/test/Driver/Output/no-duplicate-main.f90.tmp.c-object

kelvin@neutrino2 build % echo $?
0

JasonWoodArm · 2023-12-14T16:45:16Z

I tested this with our build on Mac and we are working again so it fixed the issue we were having. Thanks.

mjklemm · 2023-12-14T17:11:12Z

Let's get this in. I agree with the assessment by @banach-space. The PR looks good to me.

luporl · 2023-12-14T17:25:23Z

I build and check-flang on arm64-apple-darwin22.6.0. Driver/no-duplicate-main.f90 fails in the 3rd RUN. The test passes with HEAD.

kelvin@neutrino2 build % /Users/kelvin/wrk/llvm/tmp/build/bin/flang-new -o /Users/kelvin/wrk/llvm/tmp/build/tools/flang/test/Driver/Output/no-duplicate-main.f90.tmp.exe /Users/kelvin/wrk/llvm/tmp/build/tools/flang/test/Driver/Output/no-duplicate-main.f90.tmp /Users/kelvin/wrk/llvm/tmp/build/tools/flang/test/Driver/Output/no-duplicate-main.f90.tmp.c-object

kelvin@neutrino2 build % echo $?
0

Unrelated to this PR, but may I ask how do you configure flang, to make it link without errors and without extra flags?
I need to add -L/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/lib.

Disable the test on Darwin

banach-space · 2023-12-14T17:52:26Z

Unrelated to this PR, but may I ask how do you configure flang, to make it link without errors and without extra flags? I need to add -L/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/lib.

AFAIK, what you really want is this: -isysroot $(xcrun --sdk-path). Today this happens to be:

-L/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/lib.

But there's no guarantee that this won't change in the future. Now, one should be able to "fix it" within one build of Clang with DEFAULT_SYSROOT. If that works for you - could you document in the Flang docs? 🙏🏻

kkwli · 2023-12-14T18:26:54Z

I build and check-flang on arm64-apple-darwin22.6.0. Driver/no-duplicate-main.f90 fails in the 3rd RUN. The test passes with HEAD.
kelvin@neutrino2 build % /Users/kelvin/wrk/llvm/tmp/build/bin/flang-new -o /Users/kelvin/wrk/llvm/tmp/build/tools/flang/test/Driver/Output/no-duplicate-main.f90.tmp.exe /Users/kelvin/wrk/llvm/tmp/build/tools/flang/test/Driver/Output/no-duplicate-main.f90.tmp /Users/kelvin/wrk/llvm/tmp/build/tools/flang/test/Driver/Output/no-duplicate-main.f90.tmp.c-object

kelvin@neutrino2 build % echo $?
0
Unrelated to this PR, but may I ask how do you configure flang, to make it link without errors and without extra flags? I need to add -L/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/lib.

I do not have any special configure. Here is my cmake command:

cmake -G "Unix Makefiles" -DCMAKE_BUILD_TYPE=Release $HOME/wrk/llvm/tmp/llvm-project/llvm \
  -DCMAKE_INSTALL_PREFIX=$HOME/wrk/llvm/tmp/llvm-install -DLLVM_TARGETS_TO_BUILD="AArch64" \
  -DCMAKE_OSX_ARCHITECTURES="arm64" -DLLVM_ENABLE_PROJECTS="clang;flang;mlir" \
  -DLLVM_ENABLE_RUNTIMES="openmp;compiler-rt" -DDEFAULT_SYSROOT="$(xcrun --show-sdk-path)" \
  -DCMAKE_CXX_COMPILER=$HOME/wrk/llvm/17.0.4/bin/clang++ \
  -DCMAKE_C_COMPILER=$HOME/wrk/llvm/17.0.4/bin/clang \
  -DBUILTINS_CMAKE_ARGS=-DCOMPILER_RT_ENABLE_IOS=OFF \
  -DLLVM_LIT_ARGS="--threads=8 -v --show-unsupported"

kkwli · 2023-12-14T18:28:12Z

Unrelated to this PR, but may I ask how do you configure flang, to make it link without errors and without extra flags? I need to add -L/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/lib.

AFAIK, what you really want is this: -isysroot $(xcrun --sdk-path). Today this happens to be:
* `-L/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/lib`.
But there's no guarantee that this won't change in the future. Now, one should be able to "fix it" within one build of Clang with DEFAULT_SYSROOT. If that works for you - could you document in the Flang docs? 🙏🏻

I already have -DDEFAULT_SYSROOT="$(xcrun --show-sdk-path)" in my cmake command.

luporl · 2023-12-14T18:42:21Z

Ok, thanks, I'll try with -DDEFAULT_SYSROOT="$(xcrun --show-sdk-path)".

But it's good to know that using -isysroot with flang is the right solution. I'll try to come up with a patch to make flang support it, when I have some spare time.

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

banach-space requested review from mjklemm and luporl December 13, 2023 22:08

banach-space mentioned this pull request Dec 13, 2023

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

Merged

luporl approved these changes Dec 14, 2023

View reviewed changes

[flang][driver] Don't use -whole-archive on Darwin

95b4db0

Direct follow-up of llvm#7312 - the linker on Darwin does not support `-whole-archive`, so that needs to be removed from the linker invocation. For context: * llvm#7312

banach-space force-pushed the andrzej/update_darwin_link branch from 954ebbb to 95b4db0 Compare December 14, 2023 12:06

fixup! [flang][driver] Don't use -whole-archive on Darwin

fd2c65b

Add an extra comment

fixup! [flang][driver] Don't use -whole-archive on Darwin

aed183e

Disable the test on Darwin

llvmbot added flang:driver flang Flang issues not falling into any other category labels Dec 14, 2023

banach-space merged commit 1b6c828 into llvm:main Dec 14, 2023
5 of 6 checks passed

banach-space deleted the andrzej/update_darwin_link branch December 14, 2023 21:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[flang][driver] Don't use -whole-archive on Darwin #75393

[flang][driver] Don't use -whole-archive on Darwin #75393

banach-space commented Dec 13, 2023 •

edited

llvmbot commented Dec 13, 2023 •

edited

luporl left a comment

banach-space commented Dec 14, 2023

mjklemm commented Dec 14, 2023 •

edited

banach-space commented Dec 14, 2023

luporl commented Dec 14, 2023 •

edited

luporl commented Dec 14, 2023 •

edited

kkwli commented Dec 14, 2023

JasonWoodArm commented Dec 14, 2023

mjklemm commented Dec 14, 2023

luporl commented Dec 14, 2023

banach-space commented Dec 14, 2023 •

edited

kkwli commented Dec 14, 2023

kkwli commented Dec 14, 2023

luporl commented Dec 14, 2023

[flang][driver] Don't use -whole-archive on Darwin #75393

[flang][driver] Don't use -whole-archive on Darwin #75393

Conversation

banach-space commented Dec 13, 2023 • edited

llvmbot commented Dec 13, 2023 • edited

luporl left a comment

Choose a reason for hiding this comment

banach-space commented Dec 14, 2023

mjklemm commented Dec 14, 2023 • edited

banach-space commented Dec 14, 2023

luporl commented Dec 14, 2023 • edited

luporl commented Dec 14, 2023 • edited

kkwli commented Dec 14, 2023

JasonWoodArm commented Dec 14, 2023

mjklemm commented Dec 14, 2023

luporl commented Dec 14, 2023

banach-space commented Dec 14, 2023 • edited

kkwli commented Dec 14, 2023

kkwli commented Dec 14, 2023

luporl commented Dec 14, 2023

banach-space commented Dec 13, 2023 •

edited

llvmbot commented Dec 13, 2023 •

edited

mjklemm commented Dec 14, 2023 •

edited

luporl commented Dec 14, 2023 •

edited

luporl commented Dec 14, 2023 •

edited

banach-space commented Dec 14, 2023 •

edited