[Driver] Default to -fno-openmp-implicit-rpath

[MaskRay]

Summary

D118493 added -fno-openmp-implicit-rpath and made it the default, with an
argument that it convenients systems installing the toolchain at a non-standard
location.

I'd argue that such systems should specify -Wl,-rpath explicitly or in a Clang
configuration file. libc++, libc++abi, libunwind, and compiler-rt don't add the
extra DT_RUNPATH, it's weird for openmp to diverge. Some build systems want to
handle DT_RUNPATH themselves (e.g. CMAKE_INSTALL_RPATH). Some distributions
(e.g. Fedora) have policies against DT_RUNPATH and this change is causing
trouble.

No test: the test was removed by d14897c7dad8c05c003bada019c0b573ace63a1a.

[MaskRay]

No test probably makes this easy to back port. Eventually there needs to be a test but the original author can do it.

[JonChesterfield]

Added some people I can remember from the original discussion.

The effect of this patch will be that clang -fopenmp foo.cpp will build an executable that doesn't know where its runtime libraries are. If it finds the right ones anyway that'll be fine. If it finds ones from some unrelated toolchain, that application usually won't work.

This is likely to be more painful for people with multiple toolchains installed.

How do the other projects deal with this hazard?

[MaskRay]

In D143306#4104208, @JonChesterfield wrote:

Added some people I can remember from the original discussion.

The effect of this patch will be that clang -fopenmp foo.cpp will build an executable that doesn't know where its runtime libraries are. If it finds the right ones anyway that'll be fine. If it finds ones from some unrelated toolchain, that application usually won't work.

This is likely to be more painful for people with multiple toolchains installed.

How do the other projects deal with this hazard?

For libc++.so libc++abi.so libunwind.so users, there is no implicit -Wl,-rpath=. The user should ensure the built executable will get correct libraries at run-time.
Some build systems may provide some rpath mechanism.

As already mentioned, you can set -Wl,-rpath in your Clang configuration file.

[MaskRay]

Ping for feedback from others

[tianshilei1992]

I prefer to follow established convention.

[jhuber6]

If we're removing this feature it would probably make sense to remove the handling / flag altogether as -fopenmp-implicit-rpath isn't that much different from -Wl,-rpath=/path/to/llvm/install/lib.

Overall, the problem is that we want to tie the libomptarget.so with the same clang. If configuration files can solve this we could distribute those. I do remember being pinged about the Fedora concerns.

[yaxunl]

clang/include/clang/Driver/Options.td

4218–4223

I am wondering whether this option can be aliased to --offload-add-rpath, which seems to have the same purpose. (https://reviews.llvm.org/D136854)

[tstellar]

FWIW, I'm in favor of this patch. System directory rpaths (e.g. /usr/lib64) are not allowed in Fedora Linux. The current default makes building packages with clang+openmp more difficult.

[JonChesterfield]

I don't mind hugely what mechanism is used but would really like clang++ -fopenmp foo.cpp to build a program that runs. How can we preserve that 'works' feature without setting rpath on the binary?

[tstellar]

In D143306#4142023, @JonChesterfield wrote:

I don't mind hugely what mechanism is used but would really like clang++ -fopenmp foo.cpp to build a program that runs. How can we preserve that 'works' feature without setting rpath on the binary?

This is just for running clang from the build directory, right? I think @MaskRay suggested using a config file before. Maybe we can put a config file in $build/bin/ which makes this work.

[MaskRay]

clang/include/clang/Driver/Options.td

4218–4223

It can be aliased to --[no-]offload-add-rpath, but I think it probably makes sense to drop this option completely. This patch as-is is useful for backporting into 16.x.

[JonChesterfield]

I don't know how this configuration file works. Running clang from the build directory is useful when developing but to avoid user facing regression we'd need it to run successfully from the install directory. The use case is someone builds trunk or a release on a HPC machine and expects it to run without hacking with the environment variables, which is especially important as module systems managing the machine are likely to be hacking with the environment variables.

It's not obvious to me how that helps fedora. Maybe changing rpath is forbidden, but global installs put the shared libraries somewhere that is detectable anyway, and the config file handling has some special case to not set rpath on binaries if it would point to system anyway? That logic would probably work for openmp too - if the library we want to find is the one on the system path, great - the problem is when the library we want to find is not on the system path. However if this machinery is already implemented as something that reads a text configuration file and we can use that instead, great.

I would rather we not apply this patch as-is to trunk, and further not apply it to the previous release, because it'll break anyone running clang -fopenmp who doesn't have clang globally installed. If we can add some config file change to this patch which keeps things working then sure, back-porting is fine.

(p.s. executables specifying where to find their dependencies is quite a big part of the functionality of shared libraries - it's strange to hear that fedora (/ redhat?) would have globally disabled that. I haven't looked into the details)

[JonChesterfield]

So what is this configuration file? Joseph found a Gentoo blog post https://blogs.gentoo.org/mgorny/2022/10/07/clang-in-gentoo-now-sets-default-runtimes-via-config-file/ and I don't have a clang.cfg file in my install dir

Tests in clang look like it's a text file that you write command line flags into and hopefully clang applies them, so if we write -Wl, rpath etc in that text file and successfully distribute it to users, it'll behave identically to setting rpath from clang and fedora will still be unhappy (I don't see how to scope a flag to only -fopenmp, so it might set rpath on every executable clang builds instead of only openmp ones, which will break some applications and fail the rpm build check on all of them)

I don't see how this helps anything?

I'm trying to work out how this works for libc++ and the like and as far as I can see it doesn't - I think the binaries I've built go looking for libc++ in the system libraries and totally ignore the one built as part of the toolchain. I never noticed this because I statically link everything, am I missing something here?

[JonChesterfield]

clang/include/clang/Driver/Options.td

4218–4223

Folding the flags together seems good to me, I'd have probably used offload-add-rpath in the initial implementation if I spotted it.

[JonChesterfield]

Marking this as "no" because as far as I can tell it'll stop anyone running openmp built from source which constitutes a severe regression and I am struggling to find information on what Fedora are doing here. This link https://fedoraproject.org/wiki/Changes/Broken_RPATH_will_fail_rpmbuild suggests changing clang to not set rpath when it would be aiming at a "system directory", which is unfortunately platform specific magic strings, would be fine. That is, maybe Fedora is OK with setting RPATH as long as it isn't set to /usr/lib64 or possibly other unspecified strings.

The use case I want to preserve is people running clang -fopenmp from a local install and without setting environment variables. That means the binary needs to find the shared libraries from that local install and not unrelated files with the same name that happen to be under /usr somewhere.

[jdoerfert]

I'm worried this makes use of LLVM on HPC machines even harder. That said, I'm open to suggestions and I am not well versed in all the ways we can make it work.
Our problem is that there are N libomptarget.so files and N libomptarget.nvptx.so files on a system, including in system directories and in directories you have on your LD_LIBRARY_PATH.
However, we want a clang to pick up its own versions of those files. The former is linked into clang, the latter is dynamically loaded with dlopen. That is, IIRC, roughly our use case.

I'd argue that such systems should specify -Wl,-rpath explicitly or in a Clang configuration file.

Could you work me through this, please. We can't install a config file in a user or system directory. So all we have is the clang install directory.
Should we not set this flag but then install a file (by default) that says -Wl,-rpath=.... Is that what you mean? If so, what's the difference for the user?
Or would we add --offload-add-rpath to the clang build if OpenMP offload is enabled?

clang/include/clang/Driver/Options.td

4218–4223

Merging them makes sense. FWIW, this flag was introduced 10 months earlier (I think).

[MaskRay]

In D143306#4144396, @JonChesterfield wrote:

So what is this configuration file? Joseph found a Gentoo blog post https://blogs.gentoo.org/mgorny/2022/10/07/clang-in-gentoo-now-sets-default-runtimes-via-config-file/ and I don't have a clang.cfg file in my install dir

clang.cfg is not provided by default. You can create it for your needs. The official doc is https://clang.llvm.org/docs/UsersManual.html#configuration-files

Tests in clang look like it's a text file that you write command line flags into and hopefully clang applies them, so if we write -Wl, rpath etc in that text file and successfully distribute it to users, it'll behave identically to setting rpath from clang and fedora will still be unhappy (I don't see how to scope a flag to only -fopenmp, so it might set rpath on every executable clang builds instead of only openmp ones, which will break some applications and fail the rpm build check on all of them)

I don't see how this helps anything?

It's different. With the current status quo, -fopenmp-implicit-rpath is the default and affects all distributions which include users adverse to magic DT_RUNPATH tags.
If you specify -Wl,-rpath=... in your build (possibly HPC related), it just affects your system, and Fedora and others (like I) won't complain.

I'm trying to work out how this works for libc++ and the like and as far as I can see it doesn't - I think the binaries I've built go looking for libc++ in the system libraries and totally ignore the one built as part of the toolchain. I never noticed this because I statically link everything, am I missing something here?

This is the point. Specifying a driver option to use libc++/libc++abi/libunwind doesn't magically change DT_RUNPATH. This is exactly the behavior a user wants for a system Clang.
It does make users with a non-system Clang inconvenient but that's the point that such users should specify rpath by themselves.
openmp should not diverge from libc++/libc++abi/libunwind in this regard.

[MaskRay]

In D143306#4144518, @JonChesterfield wrote:

Marking this as "no" because as far as I can tell it'll stop anyone running openmp built from source which constitutes a severe regression and I am struggling to find information on what Fedora are doing here. This link https://fedoraproject.org/wiki/Changes/Broken_RPATH_will_fail_rpmbuild suggests changing clang to not set rpath when it would be aiming at a "system directory", which is unfortunately platform specific magic strings, would be fine. That is, maybe Fedora is OK with setting RPATH as long as it isn't set to /usr/lib64 or possibly other unspecified strings.

The use case I want to preserve is people running clang -fopenmp from a local install and without setting environment variables. That means the binary needs to find the shared libraries from that local install and not unrelated files with the same name that happen to be under /usr somewhere.

Perhaps we simply fundamentally disagree with each other. My opinion is that D118493 should be reverted.

[MaskRay]

In D143306#4144541, @jdoerfert wrote:

I'm worried this makes use of LLVM on HPC machines even harder. That said, I'm open to suggestions and I am not well versed in all the ways we can make it work.
Our problem is that there are N libomptarget.so files and N libomptarget.nvptx.so files on a system, including in system directories and in directories you have on your LD_LIBRARY_PATH.
However, we want a clang to pick up its own versions of those files. The former is linked into clang, the latter is dynamically loaded with dlopen. That is, IIRC, roughly our use case.

I'd argue that such systems should specify -Wl,-rpath explicitly or in a Clang configuration file.

Could you work me through this, please. We can't install a config file in a user or system directory. So all we have is the clang install directory.
Should we not set this flag but then install a file (by default) that says -Wl,-rpath=.... Is that what you mean? If so, what's the difference for the user?
Or would we add --offload-add-rpath to the clang build if OpenMP offload is enabled?

This part of https://clang.llvm.org/docs/UsersManual.html#configuration-files is relevant "... is searched for sequentially in the directories".

Say my clang executable is at /tmp/opt/Rel/bin/clang. I can just create /tmp/opt/Rel/bin/clang.cfg with one line -Wl,-rpath=/tmp/opt/Rel/lib (there is no magic expansion as in a shell).
Invoking /tmp/opt/Rel/bin/clang will get this default option (without an unused command line option warning) unless --no-default-config is specified.
The difference is that the rpath is now the default, instead of only when -fopenmp=libomp is specified. But such HPC users don't appear to be averse to rpath as much as we do.

It should be fairly trivial to adjust one's llvm-project build/install script to create such a clang.cfg file.

[jdoerfert]

In D143306#4145465, @MaskRay wrote:

In D143306#4144541, @jdoerfert wrote:

I'm worried this makes use of LLVM on HPC machines even harder. That said, I'm open to suggestions and I am not well versed in all the ways we can make it work.
Our problem is that there are N libomptarget.so files and N libomptarget.nvptx.so files on a system, including in system directories and in directories you have on your LD_LIBRARY_PATH.
However, we want a clang to pick up its own versions of those files. The former is linked into clang, the latter is dynamically loaded with dlopen. That is, IIRC, roughly our use case.

I'd argue that such systems should specify -Wl,-rpath explicitly or in a Clang configuration file.

Could you work me through this, please. We can't install a config file in a user or system directory. So all we have is the clang install directory.
Should we not set this flag but then install a file (by default) that says -Wl,-rpath=.... Is that what you mean? If so, what's the difference for the user?
Or would we add --offload-add-rpath to the clang build if OpenMP offload is enabled?

This part of https://clang.llvm.org/docs/UsersManual.html#configuration-files is relevant "... is searched for sequentially in the directories".

Say my clang executable is at /tmp/opt/Rel/bin/clang. I can just create /tmp/opt/Rel/bin/clang.cfg with one line -Wl,-rpath=/tmp/opt/Rel/lib (there is no magic expansion as in a shell).
Invoking /tmp/opt/Rel/bin/clang will get this default option (without an unused command line option warning) unless --no-default-config is specified.
The difference is that the rpath is now the default, instead of only when -fopenmp=libomp is specified. But such HPC users don't appear to be averse to rpath as much as we do.

It should be fairly trivial to adjust one's llvm-project build/install script to create such a clang.cfg file.

@jhuber6 Can you look into the last part, creating such a clang.cfg as part of the build/install process?

The fact that rpath is always set is a difference but I doubt it's that bad. We effectively point only to the llvm runtimes, which should be what users want anyway.
If this is really a problem, maybe we can/should separate the OpenMP runtimes into a subfolder and rpath (-L) that one. Users get by default the OpenMP runtimes associated with the clang, but we would not pollute anything else.

[jhuber6]

In D143306#4145482, @jdoerfert wrote:

@jhuber6 Can you look into the last part, creating such a clang.cfg as part of the build/install process?

The fact that rpath is always set is a difference but I doubt it's that bad. We effectively point only to the llvm runtimes, which should be what users want anyway.
If this is really a problem, maybe we can/should separate the OpenMP runtimes into a subfolder and rpath (-L) that one. Users get by default the OpenMP runtimes associated with the clang, but we would not pollute anything else.

If we do this as part of LLVM, what's the different between the config file and just setting rpath anyway? I figured that MaskRay was suggesting that each user curates their own and the LLVM install itself doesn't stipulate the rpath since it would lead to the same behavior.

[tstellar]

In D143306#4145432, @MaskRay wrote:

This is the point. Specifying a driver option to use libc++/libc++abi/libunwind doesn't magically change DT_RUNPATH. This is exactly the behavior a user wants for a system Clang.
It does make users with a non-system Clang inconvenient but that's the point that such users should specify rpath by themselves.
openmp should not diverge from libc++/libc++abi/libunwind in this regard.

To me this is a really strong argument in favor of this change. Why does libomp need to be different here?

[jdoerfert]

In D143306#4145603, @tstellar wrote:

In D143306#4145432, @MaskRay wrote:

This is the point. Specifying a driver option to use libc++/libc++abi/libunwind doesn't magically change DT_RUNPATH. This is exactly the behavior a user wants for a system Clang.
It does make users with a non-system Clang inconvenient but that's the point that such users should specify rpath by themselves.
openmp should not diverge from libc++/libc++abi/libunwind in this regard.

To me this is a really strong argument in favor of this change. Why does libomp need to be different here?

Because lots of HPC users wouldn't use it otherwise. (In the sense that it doesn't work out-of-the-box and they give up.)

To make more technical arguments why this is different:

• libomp(target) is the only one of these runtimes that has multiple levels of dynamically loaded runtimes (at least that I know of).
• libomp(target) is the only one that is modified and distributed under the same name by ~4 "other compilers" (XL, icx, amdclang, cce) and as such appears on a system multiple times in incompatible but somewhat "same" looking versions.
• libomp(target) is build and used by application developers directly from our main/dev branch

[JonChesterfield]

If fedora has a specific set of paths it rejects in DT_RUNPATH

• and libomptarget is on one of those by default
• and user executables find it there
• and we can detect we're building on fedora

then we can disable the rpath assignment for fedora installs to system root, while keeping openmp binaries working when the toolchain is installed elsewhere.

Are those constraints satisfiable?

[tstellar]

@JonChesterfield I don't think we should be putting any Fedora specific logic into clang's build system or the driver, if that's what you are suggesting. Fedora can always patch the compiler or install a config file to change the default behavior, even though this is something we try really hard to avoid.

[MaskRay]

Abandoned by a revert of D118493

[tstellar]

Should we apply this patch to the release/16.x branch to create a more smooth transition since the option has been removed in main?

[MaskRay]

In D143306#4183759, @tstellar wrote:

Should we apply this patch to the release/16.x branch to create a more smooth transition since the option has been removed in main?

Yes, it will be a good idea to apply this to release/16.x (as I acked on Discord in the morning).
Feel free to do it.

(I am going to be mostly out for the next 4 weeks and may not spend much time on reviews.llvm.org.)

clang/include/clang/Driver/Options.td

 def frtlib_add_rpath: Flag<["-"], "frtlib-add-rpath">, Flags<[NoArgumentUnused]>,
   HelpText<"Add -rpath with architecture-specific resource directory to the linker flags">;
 def fno_rtlib_add_rpath: Flag<["-"], "fno-rtlib-add-rpath">, Flags<[NoArgumentUnused]>,
   HelpText<"Do not add -rpath with architecture-specific resource directory to the linker flags">;
 def offload_add_rpath: Flag<["--"], "offload-add-rpath">, Flags<[NoArgumentUnused]>,
   HelpText<"Add -rpath with HIP runtime library directory to the linker flags">;
 def no_offload_add_rpath: Flag<["--"], "no-offload-add-rpath">, Flags<[NoArgumentUnused]>,
   HelpText<"Do not add -rpath with HIP runtime library directory to the linker flags">;
 defm openmp_implicit_rpath: BoolFOption<"openmp-implicit-rpath",
   LangOpts<"OpenMP">,
-  DefaultTrue,
+  DefaultFalse,
   PosFlag<SetTrue, [], "Set rpath on OpenMP executables">,
   NegFlag<SetFalse>,
   BothFlags<[NoArgumentUnused]>>;
 def r : Flag<["-"], "r">, Flags<[LinkerInput,NoArgumentUnused]>,
         Group<Link_Group>;
 def save_temps_EQ : Joined<["-", "--"], "save-temps=">, Flags<[CC1Option, FlangOption, NoXarchOption]>,
   HelpText<"Save intermediate compilation results.">;
 def save_temps : Flag<["-", "--"], "save-temps">, Flags<[FlangOption, NoXarchOption]>,
   Alias<save_temps_EQ>, AliasArgs<["cwd"]>,
   HelpText<"Save intermediate compilation results">;
 def save_stats_EQ : Joined<["-", "--"], "save-stats=">, Flags<[NoXarchOption]>,

clang/lib/Driver/ToolChains/CommonArgs.cpp

addMachineOutlinerArgs(D, Args, CmdArgs, ToolChain.getEffectiveTriple(),
                          /*IsLTO=*/true, PluginOptPrefix);
 }
 
 void tools::addOpenMPRuntimeSpecificRPath(const ToolChain &TC,
                                           const ArgList &Args,
                                           ArgStringList &CmdArgs) {
 
   if (Args.hasFlag(options::OPT_fopenmp_implicit_rpath,
-                   options::OPT_fno_openmp_implicit_rpath, true)) {
+                   options::OPT_fno_openmp_implicit_rpath, false)) {
     // Default to clang lib / lib64 folder, i.e. the same location as device
     // runtime
     SmallString<256> DefaultLibPath =
         llvm::sys::path::parent_path(TC.getDriver().Dir);
     llvm::sys::path::append(DefaultLibPath, CLANG_INSTALL_LIBDIR_BASENAME);
     CmdArgs.push_back("-rpath");
     CmdArgs.push_back(Args.MakeArgString(DefaultLibPath));
   }