[clang][Driver] Add RPATH by default #115286

pawosm-arm · 2024-11-07T09:45:59Z

The -frtlib-add-rpath is very convenient when linking against various runtimes (OpenMP, Fortran, sanitizers), so much so we could argue that this should be a default behavior.

This patch makes adding RPATH the default behavior that could be prevented by explicitly using the -fno-rtlib-add-rpath flag.

This is an alternative to #115163

The `-frtlib-add-rpath` is very convenient when linking against various runtimes (OpenMP, Fortran, sanitizers), so much so we could argue that this should be a default behavior. This patch makes adding RPATH the default behavior that could be prevented by explicitly using the `-fno-rtlib-add-rpath` flag.

llvmbot · 2024-11-07T09:46:33Z

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-flang-driver

Author: Paul Osmialowski (pawosm-arm)

Changes

The -frtlib-add-rpath is very convenient when linking against various runtimes (OpenMP, Fortran, sanitizers), so much so we could argue that this should be a default behavior.

This patch makes adding RPATH the default behavior that could be prevented by explicitly using the -fno-rtlib-add-rpath flag.

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

4 Files Affected:

(modified) clang/lib/Driver/ToolChains/CommonArgs.cpp (+1-1)
(modified) clang/test/Driver/arch-specific-libdir-rpath.c (+2-2)
(modified) flang/test/Driver/arch-specific-libdir-rpath.f95 (+2-3)
(modified) flang/test/Driver/linker-flags.f90 (+2-1)

diff --git a/clang/lib/Driver/ToolChains/CommonArgs.cpp b/clang/lib/Driver/ToolChains/CommonArgs.cpp
index ca06fb115dfa11..6983ab4567f9a6 100644
--- a/clang/lib/Driver/ToolChains/CommonArgs.cpp
+++ b/clang/lib/Driver/ToolChains/CommonArgs.cpp
@@ -1183,7 +1183,7 @@ void tools::addOpenMPRuntimeLibraryPath(const ToolChain &TC,
 void tools::addArchSpecificRPath(const ToolChain &TC, const ArgList &Args,
                                  ArgStringList &CmdArgs) {
   if (!Args.hasFlag(options::OPT_frtlib_add_rpath,
-                    options::OPT_fno_rtlib_add_rpath, false))
+                    options::OPT_fno_rtlib_add_rpath, true))
     return;
 
   SmallVector<std::string> CandidateRPaths(TC.getArchSpecificLibPaths());
diff --git a/clang/test/Driver/arch-specific-libdir-rpath.c b/clang/test/Driver/arch-specific-libdir-rpath.c
index 1e6bbbc5929ac2..3277da9287233f 100644
--- a/clang/test/Driver/arch-specific-libdir-rpath.c
+++ b/clang/test/Driver/arch-specific-libdir-rpath.c
@@ -2,11 +2,11 @@
 // {RESOURCE_DIR}/lib/linux to the linker search path and to '-rpath'
 //
 // Test the default behavior when neither -frtlib-add-rpath nor
-// -fno-rtlib-add-rpath is specified, which is to skip -rpath
+// -fno-rtlib-add-rpath is specified, which is to add -rpath
 // RUN: %clang %s -### --target=x86_64-linux \
 // RUN:     -fsanitize=address -shared-libasan \
 // RUN:     -resource-dir=%S/Inputs/resource_dir_with_arch_subdir 2>&1 \
-// RUN:   | FileCheck --check-prefixes=RESDIR,LIBPATH-X86_64,NO-RPATH-X86_64 %s
+// RUN:   | FileCheck --check-prefixes=RESDIR,LIBPATH-X86_64,RPATH-X86_64 %s
 //
 // Test that -rpath is not added under -fno-rtlib-add-rpath even if other
 // conditions are met.
diff --git a/flang/test/Driver/arch-specific-libdir-rpath.f95 b/flang/test/Driver/arch-specific-libdir-rpath.f95
index 23fb52abfbd574..5ce57dab96c260 100644
--- a/flang/test/Driver/arch-specific-libdir-rpath.f95
+++ b/flang/test/Driver/arch-specific-libdir-rpath.f95
@@ -1,12 +1,11 @@
-! REQUIRES: x86-registered-target
 ! Test that the driver adds an arch-specific subdirectory in
 ! {RESOURCE_DIR}/lib/linux to the linker search path and to '-rpath'
 !
 ! Test the default behavior when neither -frtlib-add-rpath nor
-! -fno-rtlib-add-rpath is specified, which is to skip -rpath
+! -fno-rtlib-add-rpath is specified, which is to add -rpath
 ! RUN: %flang %s -### --target=x86_64-linux \
 ! RUN:     -resource-dir=%S/../../../clang/test/Driver/Inputs/resource_dir_with_arch_subdir 2>&1 \
-! RUN:   | FileCheck --check-prefixes=RESDIR,LIBPATH-X86_64,NO-RPATH-X86_64 %s
+! RUN:   | FileCheck --check-prefixes=RESDIR,LIBPATH-X86_64,RPATH-X86_64 %s
 !
 ! Test that -rpath is not added under -fno-rtlib-add-rpath
 ! RUN: %flang %s -### --target=x86_64-linux \
diff --git a/flang/test/Driver/linker-flags.f90 b/flang/test/Driver/linker-flags.f90
index ac9500d7c45cec..c4ee26c5dacab8 100644
--- a/flang/test/Driver/linker-flags.f90
+++ b/flang/test/Driver/linker-flags.f90
@@ -33,7 +33,8 @@
 ! SOLARIS-F128NONE-NOT: FortranFloat128Math
 ! UNIX-F128LIBQUADMATH-SAME: "-lFortranFloat128Math" "--as-needed" "-lquadmath" "--no-as-needed"
 ! SOLARIS-F128LIBQUADMATH-SAME: "-lFortranFloat128Math" "-z" "ignore" "-lquadmath" "-z" "record"
-! UNIX-SAME: "-lFortranRuntime" "-lFortranDecimal" "-lm"
+! UNIX-SAME: "-lFortranRuntime" "-lFortranDecimal"
+! UNIX-SAME: "-lm"
 ! COMPILER-RT: "{{.*}}{{\\|/}}libclang_rt.builtins.a"
 
 ! DARWIN-LABEL:  "{{.*}}ld{{(\.exe)?}}"

llvmbot · 2024-11-07T09:46:34Z

@llvm/pr-subscribers-clang-driver

Author: Paul Osmialowski (pawosm-arm)

Changes

The -frtlib-add-rpath is very convenient when linking against various runtimes (OpenMP, Fortran, sanitizers), so much so we could argue that this should be a default behavior.

This patch makes adding RPATH the default behavior that could be prevented by explicitly using the -fno-rtlib-add-rpath flag.

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

4 Files Affected:

(modified) clang/lib/Driver/ToolChains/CommonArgs.cpp (+1-1)
(modified) clang/test/Driver/arch-specific-libdir-rpath.c (+2-2)
(modified) flang/test/Driver/arch-specific-libdir-rpath.f95 (+2-3)
(modified) flang/test/Driver/linker-flags.f90 (+2-1)

diff --git a/clang/lib/Driver/ToolChains/CommonArgs.cpp b/clang/lib/Driver/ToolChains/CommonArgs.cpp
index ca06fb115dfa11..6983ab4567f9a6 100644
--- a/clang/lib/Driver/ToolChains/CommonArgs.cpp
+++ b/clang/lib/Driver/ToolChains/CommonArgs.cpp
@@ -1183,7 +1183,7 @@ void tools::addOpenMPRuntimeLibraryPath(const ToolChain &TC,
 void tools::addArchSpecificRPath(const ToolChain &TC, const ArgList &Args,
                                  ArgStringList &CmdArgs) {
   if (!Args.hasFlag(options::OPT_frtlib_add_rpath,
-                    options::OPT_fno_rtlib_add_rpath, false))
+                    options::OPT_fno_rtlib_add_rpath, true))
     return;
 
   SmallVector<std::string> CandidateRPaths(TC.getArchSpecificLibPaths());
diff --git a/clang/test/Driver/arch-specific-libdir-rpath.c b/clang/test/Driver/arch-specific-libdir-rpath.c
index 1e6bbbc5929ac2..3277da9287233f 100644
--- a/clang/test/Driver/arch-specific-libdir-rpath.c
+++ b/clang/test/Driver/arch-specific-libdir-rpath.c
@@ -2,11 +2,11 @@
 // {RESOURCE_DIR}/lib/linux to the linker search path and to '-rpath'
 //
 // Test the default behavior when neither -frtlib-add-rpath nor
-// -fno-rtlib-add-rpath is specified, which is to skip -rpath
+// -fno-rtlib-add-rpath is specified, which is to add -rpath
 // RUN: %clang %s -### --target=x86_64-linux \
 // RUN:     -fsanitize=address -shared-libasan \
 // RUN:     -resource-dir=%S/Inputs/resource_dir_with_arch_subdir 2>&1 \
-// RUN:   | FileCheck --check-prefixes=RESDIR,LIBPATH-X86_64,NO-RPATH-X86_64 %s
+// RUN:   | FileCheck --check-prefixes=RESDIR,LIBPATH-X86_64,RPATH-X86_64 %s
 //
 // Test that -rpath is not added under -fno-rtlib-add-rpath even if other
 // conditions are met.
diff --git a/flang/test/Driver/arch-specific-libdir-rpath.f95 b/flang/test/Driver/arch-specific-libdir-rpath.f95
index 23fb52abfbd574..5ce57dab96c260 100644
--- a/flang/test/Driver/arch-specific-libdir-rpath.f95
+++ b/flang/test/Driver/arch-specific-libdir-rpath.f95
@@ -1,12 +1,11 @@
-! REQUIRES: x86-registered-target
 ! Test that the driver adds an arch-specific subdirectory in
 ! {RESOURCE_DIR}/lib/linux to the linker search path and to '-rpath'
 !
 ! Test the default behavior when neither -frtlib-add-rpath nor
-! -fno-rtlib-add-rpath is specified, which is to skip -rpath
+! -fno-rtlib-add-rpath is specified, which is to add -rpath
 ! RUN: %flang %s -### --target=x86_64-linux \
 ! RUN:     -resource-dir=%S/../../../clang/test/Driver/Inputs/resource_dir_with_arch_subdir 2>&1 \
-! RUN:   | FileCheck --check-prefixes=RESDIR,LIBPATH-X86_64,NO-RPATH-X86_64 %s
+! RUN:   | FileCheck --check-prefixes=RESDIR,LIBPATH-X86_64,RPATH-X86_64 %s
 !
 ! Test that -rpath is not added under -fno-rtlib-add-rpath
 ! RUN: %flang %s -### --target=x86_64-linux \
diff --git a/flang/test/Driver/linker-flags.f90 b/flang/test/Driver/linker-flags.f90
index ac9500d7c45cec..c4ee26c5dacab8 100644
--- a/flang/test/Driver/linker-flags.f90
+++ b/flang/test/Driver/linker-flags.f90
@@ -33,7 +33,8 @@
 ! SOLARIS-F128NONE-NOT: FortranFloat128Math
 ! UNIX-F128LIBQUADMATH-SAME: "-lFortranFloat128Math" "--as-needed" "-lquadmath" "--no-as-needed"
 ! SOLARIS-F128LIBQUADMATH-SAME: "-lFortranFloat128Math" "-z" "ignore" "-lquadmath" "-z" "record"
-! UNIX-SAME: "-lFortranRuntime" "-lFortranDecimal" "-lm"
+! UNIX-SAME: "-lFortranRuntime" "-lFortranDecimal"
+! UNIX-SAME: "-lm"
 ! COMPILER-RT: "{{.*}}{{\\|/}}libclang_rt.builtins.a"
 
 ! DARWIN-LABEL:  "{{.*}}ld{{(\.exe)?}}"

pawosm-arm · 2024-11-07T13:57:09Z

@paulwalker-arm and @DavidTruby Could you share your rationale here, please?

DavidTruby · 2024-11-07T14:28:03Z

I'd argue that this provides the least surprising behaviour to the end user: building a file with clang -fopenmp or clang++ -stdlib=libc++ where openmp/libc++ were enabled on that LLVM build gives a binary that can just be run on that system, without setting any environment variables or playing with anything else. It seems to me that clang-built binaries should know where to look for the libraries from that toolchain. GCC does this with libstdc++ and libgcc_s as far as I know (I haven't checked in a while so I could be wrong here).

MaskRay · 2024-11-07T16:48:20Z

Note: Some build systems want to handle DT_RUNPATH themselves (e.g. CMAKE_INSTALL_RPATH). Some distributions (e.g. Fedora) have policies against DT_RUNPATH for system packages.

While I can understand why some folks want to make -frtlib-add-rpath the default (I believe the opinion is more strongly held in OpenMP distributions), I am not sure this is the majority of opinions.

pawosm-arm · 2024-11-07T17:21:23Z

Some distributions (e.g. Fedora) have policies against DT_RUNPATH for system packages.

For as long as I remember, distros (and Fedora is a good example of this phenomenon) were patching the software packages heavily, so adding one more patch to ensure their well established policies wouldn't make a huge difference.

DavidTruby · 2024-11-07T21:30:47Z

It's also of course possible to have this as the sensible default (making clang+openmp work out of the box, making flang work out of the box etc) and expect people wanting the opposite to use config files to add -fno-rtlib-add-rpath. I guess that it's possible that this would be a breaking change for CMake though, which would be bad.

DavidTruby · 2024-11-18T14:38:03Z

@pawosm-arm it doesn't look like we are going to get consensus on this change so perhaps we should just drop it?

llvmbot added clang Clang issues not falling into any other category clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' flang:driver flang Flang issues not falling into any other category labels Nov 7, 2024

pawosm-arm requested a review from banach-space November 7, 2024 09:46

pawosm-arm requested review from DavidTruby, MaskRay and Meinersbur November 7, 2024 09:46

pawosm-arm mentioned this pull request Nov 7, 2024

[clang][Driver] Add the ability to specify that RPATH should be added by default #115163

Closed

pawosm-arm requested a review from paulwalker-arm November 7, 2024 13:56

pawosm-arm closed this Nov 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[clang][Driver] Add RPATH by default #115286

[clang][Driver] Add RPATH by default #115286

Uh oh!

pawosm-arm commented Nov 7, 2024 •

edited

Loading

Uh oh!

llvmbot commented Nov 7, 2024 •

edited

Loading

Uh oh!

llvmbot commented Nov 7, 2024

Uh oh!

pawosm-arm commented Nov 7, 2024 •

edited

Loading

Uh oh!

DavidTruby commented Nov 7, 2024

Uh oh!

MaskRay commented Nov 7, 2024 •

edited

Loading

Uh oh!

pawosm-arm commented Nov 7, 2024 •

edited

Loading

Uh oh!

DavidTruby commented Nov 7, 2024

Uh oh!

DavidTruby commented Nov 18, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

[clang][Driver] Add RPATH by default #115286

[clang][Driver] Add RPATH by default #115286

Uh oh!

Conversation

pawosm-arm commented Nov 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Nov 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Nov 7, 2024

Uh oh!

pawosm-arm commented Nov 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DavidTruby commented Nov 7, 2024

Uh oh!

MaskRay commented Nov 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pawosm-arm commented Nov 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DavidTruby commented Nov 7, 2024

Uh oh!

DavidTruby commented Nov 18, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

pawosm-arm commented Nov 7, 2024 •

edited

Loading

llvmbot commented Nov 7, 2024 •

edited

Loading

pawosm-arm commented Nov 7, 2024 •

edited

Loading

MaskRay commented Nov 7, 2024 •

edited

Loading

pawosm-arm commented Nov 7, 2024 •

edited

Loading