[Driver] Link Flang runtime on Solaris #65644

rorth · 2023-09-07T17:21:49Z

I noticed that flang-new cannot link Fortran executables on Solaris since the runtime libs are missing.

This patch fixes this, following Gnu.cpp. The linker-flags.f90 testcase is augmented to test for this.

Tested on amd64-pc-solaris2.11, sparcv9-sun-solaris2.11, and x86_64-pc-linux-gnu.

I noticed that `flang-new` cannot link Fortran executables on Solaris since the runtime libs are missing. This patch fixes this, following `Gnu.cpp`. The `linker-flags.f90` testcase is augmented to test for this. Tested on `amd64-pc-solaris2.11`, `sparcv9-sun-solaris2.11`, and `x86_64-pc-linux-gnu`.

banach-space · 2023-09-08T07:25:16Z

flang/test/Driver/linker-flags.f90

@@ -4,6 +4,7 @@

 ! RUN: %flang -### -target ppc64le-linux-gnu %S/Inputs/hello.f90 2>&1 | FileCheck %s --check-prefixes=CHECK,GNU
 ! RUN: %flang -### -target aarch64-apple-darwin %S/Inputs/hello.f90 2>&1 | FileCheck %s --check-prefixes=CHECK,DARWIN
+! RUN: %flang -### -target sparc-sun-solaris2.11 %S/Inputs/hello.f90 2>&1 | FileCheck %s --check-prefixes=CHECK,GNU


Solaris is not GNU ;-) Please use a dedicated prefix.

True, but how many more copies of the same patterns do we want? Add FreeBSD, OpenBSD, ... whenever they arrive? Isn't the common pattern <linker> <object file> <flang rt libs> with or without -lm? How about putting the first under one label and the -lm part under another, used where appropriate?

Sure, reducing duplication would be nice. Could you propose something?

@banach-space How should we proceed with this one? Really introduce separate labels for each OS, even if the checks are identical? Or consolidate labels as I suggested?

I would consolidate like you suggested. Perhaps the common prefix cold be "ALL-RT-LIBS" or "FLANG-RT-LIBS"? Suggestions welcome.

Add FreeBSD, OpenBSD, ... whenever they arrive?

Bumping into this PR that'll be happening much sooner for all of the *BSD's now.

brad0 · 2023-09-15T09:42:07Z

One thing to add to this. I noticed the Gnu addition does not check for nostdlib, nodefaultlibs, or r flags like the C++ library chunk does. With the proposed diff and what I had initially being right under the C++ chunk in the Solaris and *BSD ToolChain they would be checking for said flags.

brad0 · 2023-09-15T09:43:08Z

Because of where the chunk is being added in the Solaris ToolChain (and what I had in my tree for *BSD's) it is checking for the nostdlib, nodefaultlibs, or r flags, where as the Gnu path does not. I just want to make sure the behavior is consistent.

tools::gnutools::Linker::ConstructJob

  if (D.CCCIsCXX() &&
      !Args.hasArg(options::OPT_nostdlib, options::OPT_nodefaultlibs,
                   options::OPT_r)) {
    if (ToolChain.ShouldLinkCXXStdlib(Args)) {
      bool OnlyLibstdcxxStatic = Args.hasArg(options::OPT_static_libstdcxx) &&
                                 !Args.hasArg(options::OPT_static);
      if (OnlyLibstdcxxStatic)
        CmdArgs.push_back("-Bstatic");
      ToolChain.AddCXXStdlibLibArgs(Args, CmdArgs);
      if (OnlyLibstdcxxStatic)
        CmdArgs.push_back("-Bdynamic");
    }
    CmdArgs.push_back("-lm");
  }

  // Silence warnings when linking C code with a C++ '-stdlib' argument.
  Args.ClaimAllArgs(options::OPT_stdlib_EQ);

  // Additional linker set-up and flags for Fortran. This is required in order
  // to generate executables. As Fortran runtime depends on the C runtime,
  // these dependencies need to be listed before the C runtime below (i.e.
  // AddRuntTimeLibs).
  if (D.IsFlangMode()) {
    addFortranRuntimeLibraryPath(ToolChain, Args, CmdArgs);
    addFortranRuntimeLibs(ToolChain, CmdArgs);
    CmdArgs.push_back("-lm");
  }

solaris::Linker::ConstructJob

  if (!Args.hasArg(options::OPT_nostdlib, options::OPT_nodefaultlibs,
                   options::OPT_r)) {
    if (D.CCCIsCXX()) {
      if (getToolChain().ShouldLinkCXXStdlib(Args))
        getToolChain().AddCXXStdlibLibArgs(Args, CmdArgs);
      CmdArgs.push_back("-lm");
    }
    // Additional linker set-up and flags for Fortran. This is required in order
    // to generate executables. As Fortran runtime depends on the C runtime,
    // these dependencies need to be listed before the C runtime below.
    if (D.IsFlangMode()) {
      addFortranRuntimeLibraryPath(getToolChain(), Args, CmdArgs);
      addFortranRuntimeLibs(getToolChain(), CmdArgs);
      CmdArgs.push_back("-lm");
    }

rorth · 2023-09-15T13:01:00Z

I'd argue that handling the Flang runtime libs like C++ is the right thing to do here. However, flang-new currently rejects all of -r, -nostdlib, and -nodefaultlibs on both Linux and Solaris. On top of that, Solaris clang doesn't handle -r correctly, passing -e _start -Bdynamic to ld, which makes it choke...

brad0 · 2023-09-20T07:51:35Z

On top of that, Solaris clang doesn't handle -r correctly, passing -e _start -Bdynamic to ld, which makes it choke...

Looking at the GNU linker it seems to ignore -Bdynamic once -r is specified. Not sure how LLD handles things. The OpenBSD driver might need adjustments for the same thing.

rorth · 2023-09-22T09:39:52Z

The different behaviour can easily be seen since Solaris clang supports linker selection at compile time:

$ clang -r -o hello-r.o hello.o -fuse-ld=gld
/usr/gnu/bin/ld: warning: cannot find entry symbol _start; not setting start address
$ clang -r -o hello-r.o hello.o
ld: fatal: option '-Bdynamic' is incompatible with building a relocatable object
ld: fatal: option '-e _start' is incompatible with building a relocatable object
clang: error: linker command failed with exit code 1 (use -v to see invocation)

I'll either fix the Solaris driver code not to pass -Bdynamic -e _start with -r or do a full comparison of Gnu.cpp and Solaris.cpp to check for undesirable differences.

However, for the patch at hand this is moot while

$ flang-new -r -o hello-r.o hello.o
flang-new: error: unknown argument: '-r'

I wonder how to proceed with the patch at hand.

banach-space · 2023-09-24T10:30:34Z

I wonder how to proceed with the patch at hand.

I'm not a Solaris expert, but based on this discussion I'd consider adding support for -fuse-ld in Flang. This sort of changes are often as simple as updating Options.td. Same for -r.

One important rule of thumb that I'd stick to:

Do whatever Clang, GFortran and GCC do.

In cases where there's different behavior between these compilers, just go for whatever feels most sensible, but please document your design decision.

brad0 · 2023-10-05T20:04:32Z

I'd like for this to move forward.

rorth · 2023-10-11T14:36:12Z

I wonder how to proceed with the patch at hand.

I'm not a Solaris expert, but based on this discussion I'd consider adding support for -fuse-ld in Flang. This sort of changes are often as simple as updating Options.td. Same for -r.

Support of -fuse-ld is not Solaris-specific in any way. Other targets support e.g. -fuse-ld=[bfd|lld]. That would be just another case of Flang needing to support common flags. However, I don't understand how Flang option handling is done, unfortunately. Whatever the case, this is an issue separate from this patch: the only reason we were talking about -r and friends is that my patch guards adding the Flang runtime libs with ! -nostdlib && ! -nodefaultlibs && ! -r, which I believe we have now established is correct.

However, Flang accepts none of those options yet, which it should for compatiblity with both clang++ and gfortran. However, I won't be able to deal with any of this: I've quite a number of other issues on my plate.

One important rule of thumb that I'd stick to:
* Do whatever Clang, GFortran and GCC do.
In cases where there's different behavior between these compilers, just go for whatever feels most sensible, but please document your design decision.

Fully agreed: I've now checked that gfortran handles all of -r, -nostdlib, and -nodefaultlibs, so I believe flang-new should follow suite.

rorth · 2023-10-11T14:40:29Z

I'd like for this to move forward.

Agreed: AFAICS the only open issue is whether the Solaris test should use the GNU label as I have done, introduce an new common one (like UNIX; there's nothing GNU-specific in that test), or really introduce a separate copy of the check under a different label per target (my least preference because it makes the test hard to read for no gain).

brad0 · 2023-10-12T22:03:36Z

Agreed: AFAICS the only open issue is whether the Solaris test should use the GNU label as I have done, introduce an new common one (like UNIX; there's nothing GNU-specific in that test), or really introduce a separate copy of the check under a different label per target (my least preference because it makes the test hard to read for no gain).

I think having additional tests makes sense if there is some variation on what is being checked but not when it's just copying and pasting the same thing with a different label.

banach-space · 2023-10-16T08:52:18Z

Agreed: AFAICS the only open issue is whether the Solaris test should use the GNU label as I have done, introduce an new common one (like UNIX; there's nothing GNU-specific in that test), or really introduce a separate copy of the check under a different label per target (my least preference because it makes the test hard to read for no gain).

I think having additional tests makes sense if there is some variation on what is being checked but not when it's just copying and pasting the same thing with a different label.

I feel that we are bike-shedding here a bit. Please prioritise correctness - using GNU as a label for Solaris would not be correct. The following would be:

duplicating tests with a different label (e.g. SOLARIS) would be correct,
renaming the current label from GNU to e.g. UNIX would be correct.

Unless I am missing something?

rorth · 2023-10-16T10:46:55Z

I feel that we are bike-shedding here a bit. Please prioritise correctness - using GNU as a label for Solaris would not be correct. The following would be:
* duplicating tests with a different label (e.g. `SOLARIS`) would be correct,

* renaming the current label from `GNU` to e.g. `UNIX` would be correct.
Unless I am missing something?

I'd go for renaming the label to UNIX then: it avoids unnecessary duplication and better describes what those cases are about.

brad0 · 2023-10-17T05:14:30Z

renaming the current label from GNU to e.g. UNIX would be correct.

That would be fine with me.

llvmbot · 2023-10-17T18:45:35Z

@llvm/pr-subscribers-clang

Author: Rainer Orth (rorth)

Changes

I noticed that flang-new cannot link Fortran executables on Solaris since the runtime libs are missing.

This patch fixes this, following Gnu.cpp. The linker-flags.f90 testcase is augmented to test for this.

Tested on amd64-pc-solaris2.11, sparcv9-sun-solaris2.11, and x86_64-pc-linux-gnu.

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

2 Files Affected:

(modified) clang/lib/Driver/ToolChains/Solaris.cpp (+8)
(modified) flang/test/Driver/linker-flags.f90 (+8-7)

diff --git a/clang/lib/Driver/ToolChains/Solaris.cpp b/clang/lib/Driver/ToolChains/Solaris.cpp
index 36fe12608eefc6c..252c71d3379eab4 100644
--- a/clang/lib/Driver/ToolChains/Solaris.cpp
+++ b/clang/lib/Driver/ToolChains/Solaris.cpp
@@ -223,6 +223,14 @@ void solaris::Linker::ConstructJob(Compilation &C, const JobAction &JA,
         getToolChain().AddCXXStdlibLibArgs(Args, CmdArgs);
       CmdArgs.push_back("-lm");
     }
+    // Additional linker set-up and flags for Fortran. This is required in order
+    // to generate executables. As Fortran runtime depends on the C runtime,
+    // these dependencies need to be listed before the C runtime below.
+    if (D.IsFlangMode()) {
+      addFortranRuntimeLibraryPath(getToolChain(), Args, CmdArgs);
+      addFortranRuntimeLibs(getToolChain(), CmdArgs);
+      CmdArgs.push_back("-lm");
+    }
     if (Args.hasArg(options::OPT_fstack_protector) ||
         Args.hasArg(options::OPT_fstack_protector_strong) ||
         Args.hasArg(options::OPT_fstack_protector_all)) {
diff --git a/flang/test/Driver/linker-flags.f90 b/flang/test/Driver/linker-flags.f90
index 09b8a224df13828..fd61825eb4023a5 100644
--- a/flang/test/Driver/linker-flags.f90
+++ b/flang/test/Driver/linker-flags.f90
@@ -2,8 +2,9 @@
 ! invocation. These libraries are added on top of other standard runtime
 ! libraries that the Clang driver will include.
 
-! RUN: %flang -### -target ppc64le-linux-gnu %S/Inputs/hello.f90 2>&1 | FileCheck %s --check-prefixes=CHECK,GNU
+! RUN: %flang -### -target ppc64le-linux-gnu %S/Inputs/hello.f90 2>&1 | FileCheck %s --check-prefixes=CHECK,UNIX
 ! RUN: %flang -### -target aarch64-apple-darwin %S/Inputs/hello.f90 2>&1 | FileCheck %s --check-prefixes=CHECK,DARWIN
+! RUN: %flang -### -target sparc-sun-solaris2.11 %S/Inputs/hello.f90 2>&1 | FileCheck %s --check-prefixes=CHECK,UNIX
 ! RUN: %flang -### -target x86_64-windows-gnu %S/Inputs/hello.f90 2>&1 | FileCheck %s --check-prefixes=CHECK,MINGW
 
 ! NOTE: Clang's driver library, clangDriver, usually adds 'libcmt' and
@@ -21,12 +22,12 @@
 !       run on any other platform, such as Windows that use a .exe
 !       suffix. Clang's driver will try to resolve the path to the ld
 !       executable and may find the GNU linker from MinGW or Cygwin.
-! GNU-LABEL:  "{{.*}}ld{{(\.exe)?}}"
-! GNU-SAME: "[[object_file]]"
-! GNU-SAME: -lFortran_main
-! GNU-SAME: -lFortranRuntime
-! GNU-SAME: -lFortranDecimal
-! GNU-SAME: -lm
+! UNIX-LABEL:  "{{.*}}ld{{(\.exe)?}}"
+! UNIX-SAME: "[[object_file]]"
+! UNIX-SAME: -lFortran_main
+! UNIX-SAME: -lFortranRuntime
+! UNIX-SAME: -lFortranDecimal
+! UNIX-SAME: -lm
 
 ! DARWIN-LABEL:  "{{.*}}ld{{(\.exe)?}}"
 ! DARWIN-SAME: "[[object_file]]"

flang/test/Driver/linker-flags.f90

banach-space

LGTM, thanks for all the improvements!

Before merging, could you update the summary and briefly mention the additional changes made here? Otherwise people might think that these changes are required, but that's not strictly the case.

… on OpenBSD (#67254) The entry point symbol handling matches our GCC link spec.. ```%{!shared:%{!nostdlib:%{!r:%{!e*:-e __start}}}}``` Remove usage of -Bdynamic as it is the default for the linker anyway. Came up in discussion here #65644

As discussed in [[Driver] Link Flang runtime on Solaris](llvm#65644), `clang -r` incorrectly passes both `-Bdynamic` and `-e _start` to `ld` which lets the linker choke. This patch fixes this, omitting `-Bdynamic` completely which is the linker default. Tested on `amd64-pc-solaris2.11` and `sparcv9-sun-solaris2.11`.

As discussed in [[Driver] Link Flang runtime on Solaris](#65644), `clang -r` incorrectly passes both `-Bdynamic` and `-e _start` to `ld` which lets the linker choke. This patch fixes this, omitting `-Bdynamic` completely which is the linker default. Tested on `amd64-pc-solaris2.11` and `sparcv9-sun-solaris2.11`.

rorth requested a review from MaskRay September 7, 2023 17:21

rorth requested review from a team as code owners September 7, 2023 17:21

github-actions bot added clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' flang:driver flang Flang issues not falling into any other category labels Sep 7, 2023

banach-space reviewed Sep 8, 2023

View reviewed changes

llvm deleted a comment from banach-space Sep 15, 2023

brad0 mentioned this pull request Sep 24, 2023

[Driver] Corrections for linker flags passed with relocatable linking on OpenBSD #67254

Merged

Rename GNU label to UNIX to better match use.

bf1b865

llvmbot added the clang Clang issues not falling into any other category label Oct 17, 2023

MaskRay reviewed Oct 17, 2023

View reviewed changes

flang/test/Driver/linker-flags.f90 Outdated Show resolved Hide resolved

flang/test/Driver/linker-flags.f90 Outdated Show resolved Hide resolved

Use --target=. Join -l lines for UNIX case.

5e7ca99

banach-space approved these changes Oct 18, 2023

View reviewed changes

rorth merged commit cf1bde9 into llvm:main Oct 18, 2023

madhur13490 mentioned this pull request Oct 20, 2023

Revert commit ba8565fbcb975e2d067ce3ae5a7dbaae4953edd3 madhur13490/llvm-project#3

Closed

banach-space mentioned this pull request Oct 24, 2023

[mlir][vector] Add scalable vectors to tests for vector.contract #70039

Merged

rorth mentioned this pull request Oct 26, 2023

[Driver] Fix -r handling on Solaris #70322

Merged

[Driver] Link Flang runtime on Solaris #65644

[Driver] Link Flang runtime on Solaris #65644

Uh oh!

Conversation

rorth commented Sep 7, 2023

Uh oh!

banach-space Sep 8, 2023

Choose a reason for hiding this comment

Uh oh!

rorth Sep 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

banach-space Sep 10, 2023

Choose a reason for hiding this comment

Uh oh!

rorth Sep 14, 2023

Choose a reason for hiding this comment

Uh oh!

banach-space Sep 14, 2023

Choose a reason for hiding this comment

Uh oh!

brad0 Sep 15, 2023

Choose a reason for hiding this comment

Uh oh!

brad0 commented Sep 15, 2023

Uh oh!

brad0 commented Sep 15, 2023

Uh oh!

rorth commented Sep 15, 2023

Uh oh!

brad0 commented Sep 20, 2023

Uh oh!

rorth commented Sep 22, 2023

Uh oh!

banach-space commented Sep 24, 2023

Uh oh!

brad0 commented Oct 5, 2023

Uh oh!

rorth commented Oct 11, 2023

Uh oh!

rorth commented Oct 11, 2023

Uh oh!

brad0 commented Oct 12, 2023

Uh oh!

banach-space commented Oct 16, 2023

Uh oh!

rorth commented Oct 16, 2023

Uh oh!

brad0 commented Oct 17, 2023

Uh oh!

llvmbot commented Oct 17, 2023

Uh oh!

Uh oh!

Uh oh!

banach-space left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rorth Sep 8, 2023 •

edited

Loading