[flang][Driver] Support -rpath, -shared, and -static in the frontend #66702

tarunprabhu · 2023-09-18T21:24:54Z

Enable -rpath, -shared, and -static for the flang frontend. This brings it in line with clang. Fixes issue #65546.

llvmbot · 2023-09-18T21:25:33Z

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-flang-driver

Changes

Enable -rpath, -shared, and -static for the flang frontend. This brings it in line with clang. Fixes issue #65546.

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

2 Files Affected:

(modified) clang/include/clang/Driver/Options.td (+5-2)
(modified) flang/test/Driver/linker-flags.f90 (+16)

diff --git a/clang/include/clang/Driver/Options.td b/clang/include/clang/Driver/Options.td
index 553c7928c4f949e..bf4c76d4555d152 100644
--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -5165,7 +5165,8 @@ def resource_dir : Separate<["-"], "resource-dir">,
 def resource_dir_EQ : Joined<["-"], "resource-dir=">, Flags<[NoXarchOption]>,
   Visibility<[ClangOption, CLOption, DXCOption]>,
   Alias<resource_dir>;
-def rpath : Separate<["-"], "rpath">, Flags<[LinkerInput]>, Group<Link_Group>;
+def rpath : Separate<["-"], "rpath">, Flags<[LinkerInput]>, Group<Link_Group>,
+  Visibility<[ClangOption, CLOption, DXCOption, FlangOption]>;
 def rtlib_EQ : Joined<["-", "--"], "rtlib=">,
   HelpText<"Compiler runtime library to use">;
 def frtlib_add_rpath: Flag<["-"], "frtlib-add-rpath">, Flags<[NoArgumentUnused]>,
@@ -5216,7 +5217,8 @@ def segs__read__only__addr : Separate<["-"], "segs_read_only_addr">;
 def segs__read__write__addr : Separate<["-"], "segs_read_write_addr">;
 def segs__read__ : Joined<["-"], "segs_read_">;
 def shared_libgcc : Flag<["-"], "shared-libgcc">;
-def shared : Flag<["-", "--"], "shared">, Group<Link_Group>;
+def shared : Flag<["-", "--"], "shared">, Group<Link_Group>,
+  Visibility<[ClangOption, CLOption, DXCOption, FlangOption]>;
 def single__module : Flag<["-"], "single_module">;
 def specs_EQ : Joined<["-", "--"], "specs=">, Group<Link_Group>;
 def specs : Separate<["-", "--"], "specs">, Flags<[Unsupported]>;
@@ -5226,6 +5228,7 @@ def start_no_unused_arguments : Flag<["--"], "start-no-unused-arguments">,
 def static_libgcc : Flag<["-"], "static-libgcc">;
 def static_libstdcxx : Flag<["-"], "static-libstdc++">;
 def static : Flag<["-", "--"], "static">, Group<Link_Group>,
+  Visibility<[ClangOption, CLOption, DXCOption, FlangOption]>,
   Flags<[NoArgumentUnused]>;
 def std_default_EQ : Joined<["-"], "std-default=">;
 def std_EQ : Joined<["-", "--"], "std=">,
diff --git a/flang/test/Driver/linker-flags.f90 b/flang/test/Driver/linker-flags.f90
index 09b8a224df13828..9a9ec5d28acb9c7 100644
--- a/flang/test/Driver/linker-flags.f90
+++ b/flang/test/Driver/linker-flags.f90
@@ -50,3 +50,19 @@
 ! MSVC-SAME: FortranDecimal.lib
 ! MSVC-SAME: /subsystem:console
 ! MSVC-SAME: "[[object_file]]"
+
+! Verify that certain linker flags are known to the frontend and are passed on
+! to the linker.
+
+! RUN: %flang -### -rpath /path/to/dir %s 2>&1 \
+! RUN:   | FileCheck --check-prefix=CHECK-RPATH %s
+! CHECK-RPATH: ld{{.*}} "-rpath" "/path/to/dir"
+
+! RUN: %flang -### -shared %s 2>&1 \
+! RUN:   | FileCheck --check-prefix=CHECK-SHARED %s
+! CHECK-SHARED: ld{{.*}} "-shared"
+
+! RUN: %flang -### -static %s 2>&1 \
+! RUN:   | FileCheck --check-prefix=CHECK-STATIC %s
+! CHECK-STATIC: ld{{.*}} "-static"
+

flang/test/Driver/linker-flags.f90

tarunprabhu · 2023-09-21T19:55:29Z

Thanks @MaskRay and @banach-space for taking a look at this. I have marked the test as unsupported on windows because of a failing buildkite. I don't have access to a Windows machine so I can't add an equivalent test for that platform.

banach-space · 2023-09-22T07:37:41Z

Thanks @MaskRay and @banach-space for taking a look at this. I have marked the test as unsupported on windows because of a failing buildkite. I don't have access to a Windows machine so I can't add an equivalent test for that platform.

Thanks for working for this! We should investigate the Windows situation rather than disabling this test altogether - a lot of effort has gone into making it work and we should maintain that. Perhaps @mstorsjo or @DavidTruby could help?

banach-space

Please re-enable the tests in linker-flags.f90.

If these options don't work on Windows then we should try to document it. Also, tests for -rpath can be moved to a dedicated file if we need to disable them on a particular platform.

MaskRay · 2023-09-24T18:56:24Z

I think Windows does not work likely because ld is absent in PATH. You need -Bxxx/bin to specify a directory that contains ld, but I don't remember whether ld.exe is needed instead.

tarunprabhu · 2023-09-25T13:49:04Z

I think Windows does not work likely because ld is absent in PATH. You need -Bxxx/bin to specify a directory that contains ld, but I don't remember whether ld.exe is needed instead.

I think what I saw on the buildkite was Link.exe, but I don't know if that is always the case.

tarunprabhu · 2023-09-25T19:31:03Z

The buildkite fails yet again because the link flags seem to be mapped differently.

The link line generated on Windows is:

"C:\\BuildTools\\VC\\Tools\\MSVC\\14.29.30133\\bin\\Hostx64\\x64\\link.exe" "-out:a.exe" "-libpath:c:\\ws\\src\\build\\lib" "Fortran_main.lib" "FortranRuntime.lib" "FortranDecimal.lib" "/subsystem:console" "-nologo" "-dll" "-implib:a.lib" "-rpath" "/path/to/dir" "C:\\Users\\ContainerAdministrator\\AppData\\Local\\Temp\\lit-tmp-oaf0t3ny\\linker-flags-de9f45.o"

I don't see -shared or -static. Can someone help me with this? Is -dll is the equivalent of -shared? Does windows support static libraries?

DavidTruby · 2023-10-03T10:16:23Z

Sorry I haven’t had a chance to look at this yet but I can help you resolve the Windows issues here.

Windows does support static libraries and that’s the default for the runtime at the moment, I’ve not actually tried linking the flang runtime dynamically on Windows so that may be the issue. There’s an additional hurdle on Windows as well that libraries statically or dynamically linked against libc are not ABI compatible, so I’m not sure how these flags might interact with that without taking a look

tarunprabhu · 2023-10-03T13:43:45Z

Thanks @DavidTruby.

For this particular patch, the main concern is how the compiler flags are mapped to the corresponding linker flags on Windows. All of that work is done by clang, and I haven't touched any of it so I am not worried about correctness.

The two main concerns are:

What are the linker flags in Windows corresponding to -static and -shared on *nix.
Do the checks in the test need to be expanded to account for different linkers on Windows.

luporl · 2023-10-16T16:49:43Z

I'll try to test this patch on a Windows on ARM machine.
But it may take some time to setup and build flang on it.

tarunprabhu · 2023-10-16T18:05:18Z

I'll try to test this patch on a Windows on ARM machine. But it may take some time to setup and build flang on it.

Thanks @luporl. Any help is most appreciated.

MaskRay · 2023-10-16T21:37:28Z

These tests just need a --target=x86_64-linux-gnu: which is how clang/test/Driver tests these stuff. Non-ELF platforms such as Apple, Windows, and AIX, may not support some options.

Ideally we should express that the test works for every ELF platform or everything except baremetal, but we don't infrastructure for it. Using a specific target triple is somewhat a convention at least for newer tests.

You also don't need Windows to test the test. Just specify --target= with a Windows target triple.

luporl · 2023-10-17T12:42:30Z

Adding --target=x86_64-linux-gnu, as @MaskRay suggests, fixes the test on Windows. But then we are not actually testing linking on Windows.

On my Windows machine, the test output is practically the same as that reported by buildkite, as mentioned by @tarunprabhu above. I see no differences in the output with or without -static. Using -shared adds -dll -implib:a.lib and the output becomes a dll instead of an exe, which makes sense, according to MSVC documentation (https://learn.microsoft.com/en-us/cpp/build/reference/dll-build-a-dll?view=msvc-170).

I've also tried linking a hello world program with and without -static and it runs fine. With -shared, it also produces an a.exe file (-o a.dll is needed to create the output file with the right extension), but that doesn't run, because it's actually a dll it seems, although I didn't try to use it as such.

In short, when using MSVC linker, -rpath is passed to it, -static doesn't change its invocation and -shared is translated to -dll.

luporl · 2023-10-17T12:44:17Z

flang/test/Driver/linker-flags.f90

+! RUN: %flang -### -rpath /path/to/dir -shared -static %s 2>&1 \
+! RUN:   | FileCheck --check-prefix=CHECK-LINKER-OPTIONS %s
+! CHECK-LINKER-OPTIONS-DAG: "-rpath" "/path/to/dir"
+! CHECK-LINKER-OPTIONS-DAG: "-shared"
+! CHECK-LINKER-OPTIONS-DAG: "-static"


Suggested change

! RUN: %flang -### -rpath /path/to/dir -shared -static %s 2>&1 \

! RUN: | FileCheck --check-prefix=CHECK-LINKER-OPTIONS %s

! CHECK-LINKER-OPTIONS-DAG: "-rpath" "/path/to/dir"

! CHECK-LINKER-OPTIONS-DAG: "-shared"

! CHECK-LINKER-OPTIONS-DAG: "-static"

! RUN: %flang -### --target=x86_64-linux-gnu -rpath /path/to/dir -shared \

! RUN: -static %s 2>&1 | FileCheck \

! RUN: --check-prefixes=CHECK-LINKER-OPTIONS,GNU-LINKER-OPTIONS %s

! RUN: %flang -### --target=x86_64-windows-msvc -rpath /path/to/dir -shared \

! RUN: -static %s 2>&1 | FileCheck \

! RUN: --check-prefixes=CHECK-LINKER-OPTIONS,MSVC-LINKER-OPTIONS %s

! CHECK-LINKER-OPTIONS-DAG: "-rpath" "/path/to/dir"

! GNU-LINKER-OPTIONS-DAG: "-shared"

! MSVC-LINKER-OPTIONS-DAG: "-dll"

! GNU-LINKER-OPTIONS-DAG: "-static"

kiranchandramohan · 2023-11-13T13:54:52Z

@banach-space Is this patch OK with you?

banach-space · 2023-11-13T22:25:02Z

flang/test/Driver/linker-flags.f90

+
+! Verify that certain linker flags are known to the frontend and are passed on
+! to the linker.
+
+! RUN: %flang -### --target=x86_64-linux-gnu -rpath /path/to/dir -shared \
+! RUN:     -static %s 2>&1 | FileCheck \
+! RUN:     --check-prefixes=CHECK-LINKER-OPTIONS,GNU-LINKER-OPTIONS %s
+! RUN: %flang -### --target=x86_64-windows-msvc -rpath /path/to/dir -shared \
+! RUN:     -static %s 2>&1 | FileCheck \
+! RUN:     --check-prefixes=CHECK-LINKER-OPTIONS,MSVC-LINKER-OPTIONS %s
+! CHECK-LINKER-OPTIONS-DAG: "-rpath" "/path/to/dir"
+! GNU-LINKER-OPTIONS-DAG: "-shared"
+! MSVC-LINKER-OPTIONS-DAG: "-dll"
+! GNU-LINKER-OPTIONS-DAG: "-static"


I have a few suggestions:

The tests above this block test for default (i.e. generated by the driver automatically) linker flags when generating an executable. This block does something very different. I would separate the two things into different files.

Why would we test -shared and -static in one invocation?

We should make sure that what's being tested is indeed the linker invocation. This can be done by e.g. adding something akin ! UNIX-LABEL: "{{.*}}ld{{(\.exe)?}}" to make sure that it's indeed the linker invocation that's being matched.

I would separate the two things into different files.

Done.

* Why would we test `-shared` and `-static` in one invocation?

This was because of a suggestion by @MaskRay in an earlier comment

* We should make sure that what's being tested is indeed the linker invocation.

Done.

banach-space · 2023-11-13T22:25:45Z

@banach-space Is this patch OK with you?

Yup, I'm ok with this direction. Made a few small suggestions, but nothing substantial.

banach-space

Thanks for seeing this through, that's much appreciated!

banach-space · 2023-11-15T21:03:20Z

flang/test/Driver/dynamic-linker.f90

[nit] Dynamic linker will only be used with shared objects, but this file also tests static. So the name is a bit confusing. But I don't have any great suggestion myself. Perhaps update the comment to clarify that this test is specifically for static/shared?

clang has a test that is similar and in a file with that name.

Enable -rpath, -shared, and -static for the flang frontend. This brings it in line with clang. Fixes issue llvm#65546.

…lvm#66702) Enable -rpath, -shared, and -static for the flang frontend. This brings it in line with clang. Fixes issue llvm#65546.

tarunprabhu added the flang:driver label Sep 18, 2023

tarunprabhu requested review from MaskRay, banach-space, kiranchandramohan and rovka September 18, 2023 21:24

llvmbot added clang Clang issues not falling into any other category flang Flang issues not falling into any other category labels Sep 18, 2023

tarunprabhu linked an issue Sep 18, 2023 that may be closed by this pull request

flang-new: error: unknown argument: '-rpath' #65546

Closed

tarunprabhu requested a review from dpalermo September 18, 2023 21:27

ronlieb self-requested a review September 19, 2023 23:09

MaskRay reviewed Sep 20, 2023

View reviewed changes

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

MaskRay approved these changes Sep 21, 2023

View reviewed changes

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

banach-space approved these changes Sep 21, 2023

View reviewed changes

banach-space requested changes Sep 22, 2023

View reviewed changes

luporl reviewed Oct 17, 2023

View reviewed changes

tarunprabhu force-pushed the rpath-shared-static branch from fae3410 to 3bc2d83 Compare October 26, 2023 14:23

kawashima-fj mentioned this pull request Oct 31, 2023

configury: support flang-new open-mpi/ompi#11840

Merged

banach-space reviewed Nov 13, 2023

View reviewed changes

tarunprabhu force-pushed the rpath-shared-static branch from 3bc2d83 to ba1ac3b Compare November 14, 2023 15:38

banach-space approved these changes Nov 15, 2023

View reviewed changes

[flang][Driver] Support -rpath, -shared, and -static in the frontend

68733f1

Enable -rpath, -shared, and -static for the flang frontend. This brings it in line with clang. Fixes issue llvm#65546.

tarunprabhu force-pushed the rpath-shared-static branch from 7edd9f4 to 68733f1 Compare November 16, 2023 18:16

tarunprabhu merged commit 34e4e5e into llvm:main Nov 16, 2023
2 of 3 checks passed

tarunprabhu deleted the rpath-shared-static branch November 16, 2023 19:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[flang][Driver] Support -rpath, -shared, and -static in the frontend #66702

[flang][Driver] Support -rpath, -shared, and -static in the frontend #66702

tarunprabhu commented Sep 18, 2023

llvmbot commented Sep 18, 2023 •

edited

tarunprabhu commented Sep 21, 2023

banach-space commented Sep 22, 2023

banach-space left a comment

MaskRay commented Sep 24, 2023

tarunprabhu commented Sep 25, 2023

tarunprabhu commented Sep 25, 2023

DavidTruby commented Oct 3, 2023

tarunprabhu commented Oct 3, 2023

luporl commented Oct 16, 2023

tarunprabhu commented Oct 16, 2023

MaskRay commented Oct 16, 2023 •

edited

luporl commented Oct 17, 2023

luporl Oct 17, 2023

kiranchandramohan commented Nov 13, 2023

banach-space Nov 13, 2023

tarunprabhu Nov 14, 2023

banach-space commented Nov 13, 2023

banach-space left a comment

banach-space Nov 15, 2023

tarunprabhu Nov 16, 2023

[flang][Driver] Support -rpath, -shared, and -static in the frontend #66702

[flang][Driver] Support -rpath, -shared, and -static in the frontend #66702

Conversation

tarunprabhu commented Sep 18, 2023

llvmbot commented Sep 18, 2023 • edited

tarunprabhu commented Sep 21, 2023

banach-space commented Sep 22, 2023

banach-space left a comment

Choose a reason for hiding this comment

MaskRay commented Sep 24, 2023

tarunprabhu commented Sep 25, 2023

tarunprabhu commented Sep 25, 2023

DavidTruby commented Oct 3, 2023

tarunprabhu commented Oct 3, 2023

luporl commented Oct 16, 2023

tarunprabhu commented Oct 16, 2023

MaskRay commented Oct 16, 2023 • edited

luporl commented Oct 17, 2023

luporl Oct 17, 2023

Choose a reason for hiding this comment

kiranchandramohan commented Nov 13, 2023

banach-space Nov 13, 2023

Choose a reason for hiding this comment

tarunprabhu Nov 14, 2023

Choose a reason for hiding this comment

banach-space commented Nov 13, 2023

banach-space left a comment

Choose a reason for hiding this comment

banach-space Nov 15, 2023

Choose a reason for hiding this comment

tarunprabhu Nov 16, 2023

Choose a reason for hiding this comment

llvmbot commented Sep 18, 2023 •

edited

MaskRay commented Oct 16, 2023 •

edited