[PATCH] D118493: Set rpath on openmp executables

JonChesterfield added a comment. That is great news, phabricator's list of branches has mislead me. Thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D118493/new/ https://reviews.llvm.org/D118493 ___

[PATCH] D118493: Set rpath on openmp executables

MaskRay added a comment. In D118493#4182583 , @JonChesterfield wrote: > Duplicating a comment from the commit thread so it's easier for me to find > later. > > You've applied this to the release branches going back as far as 14. It's a > user facing br

[PATCH] D118493: Set rpath on openmp executables

tstellar added a comment. In D118493#4182583 , @JonChesterfield wrote: > Duplicating a comment from the commit thread so it's easier for me to find > later. > > You've applied this to the release branches going back as far as 14. It's a > user facing b

[PATCH] D118493: Set rpath on openmp executables

JonChesterfield added a comment. Duplicating a comment from the commit thread so it's easier for me to find later. You've applied this to the release branches going back as far as 14. It's a user facing breaking change. As in people who have a working openmp toolchain and update to the point r

[PATCH] D118493: Set rpath on openmp executables

MaskRay added a comment. In D118493#4178250 , @JonChesterfield wrote: > Reporting after another round of discussion. I don't have much support from > the llvm openmp working group that we should maintain the current divergence > from libc++ and the lik

[PATCH] D118493: Set rpath on openmp executables

JonChesterfield added a comment. Reporting after another round of discussion. I don't have much support from the llvm openmp working group that we should maintain the current divergence from libc++ and the like so I'm backing down. Let's revert this and regress for all users who don't install g

[PATCH] D118493: Set rpath on openmp executables

MaskRay added a comment. In D118493#4075970 , @JonChesterfield wrote: > That works if you have one toolchain installed globally or you are happy to > cobble together a working system using environment variables. If you have > multiple toolchains, they

[PATCH] D118493: Set rpath on openmp executables

JonChesterfield added a comment. That works if you have one toolchain installed globally or you are happy to cobble together a working system using environment variables. If you have multiple toolchains, they can't all sit in the global directory. If you don't have root, you can't install them

[PATCH] D118493: Set rpath on openmp executables

MaskRay added a comment. Enabled-by-default `-fopenmp-implicit-rpath` is odd. For other shared objects (e.g. `libclang_rt.*.so`) we don't do this. For libraries in a well-known system directory, we just rely on the default `/etc/ld.so.conf*`. For libraries in other directories or when `--sysroot

[PATCH] D118493: Set rpath on openmp executables

JonChesterfield added a comment. In D118493#3401448 , @tstellar wrote: > The rule that is broken is "standard RPATHs" Fedora installs libomp to > /usr/lib64. > > ... > > I think what we'll do in Fedora is just add -fno-openmp-implicit-rpath to > the d

[PATCH] D118493: Set rpath on openmp executables

tstellar added a comment. In D118493#3398808 , @JonChesterfield wrote: > It lets applications run with the libomp and libomptarget built with the > toolchain. For users, they don't have to be root. For compiler devs, we test > our work instead of whate

[PATCH] D118493: Set rpath on openmp executables

JonChesterfield added a subscriber: estewart08. JonChesterfield added a comment. @estewart08 Fedora are rejecting some uses of rpath (and probably runpath). I can't find a corresponding page for redhat. This may become a problem for our aomp/rocm builds. Repository: rG LLVM Github Monorepo

[PATCH] D118493: Set rpath on openmp executables

JonChesterfield added a comment. It lets applications run with the libomp and libomptarget built with the toolchain. For users, they don't have to be root. For compiler devs, we test our work instead of whatever the distro had installed. There's no info at that link. What's 'broken rpath'? If t

[PATCH] D118493: Set rpath on openmp executables

tstellar added a comment. Herald added a project: All. Fedora recently banned RUNPATH, so packages built with clang and -fopenmp are rejected by the build system with this change. Can you elaborate more about the intend

[PATCH] D118493: Set rpath on openmp executables

thakis added a comment. The behavior change can still be observed by a `-###` test -- just check that `-rpath` gets passed to the linker. Clang tests generally can't rely on running linkers. Linkers tend to be platform-specific. Windows link.exe can't produce elf binaries, macOS's ld64 can't p

[PATCH] D118493: Set rpath on openmp executables

JonChesterfield added a comment. In D118493#3284781 , @thakis wrote: > In D118493#3284663 , > @JonChesterfield wrote: > >> In D118493#3284617 , @thakis wrote: >> >>> look

[PATCH] D118493: Set rpath on openmp executables

thakis added a comment. In D118493#3284663 , @JonChesterfield wrote: > In D118493#3284617 , @thakis wrote: > >> looks like the mac linker doesn't like this test either: >> http://45.33.8.238/macm1/26834/step_7.t

[PATCH] D118493: Set rpath on openmp executables

JonChesterfield added a comment. In D118493#3284617 , @thakis wrote: > looks like the mac linker doesn't like this test either: > http://45.33.8.238/macm1/26834/step_7.txt Error message is ld: file too small (length=8) file '/Users/thakis/src/llvm-proj

[PATCH] D118493: Set rpath on openmp executables

thakis added a comment. looks like the mac linker doesn't like this test either: http://45.33.8.238/macm1/26834/step_7.txt Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D118493/new/ https://reviews.llvm.org/D118493

[PATCH] D118493: Set rpath on openmp executables

JonChesterfield added a reviewer: kparzysz. JonChesterfield added a subscriber: kparzysz. JonChesterfield added a comment. This test doesn't work on hexagon because hexagon doesn't have a linker (??), emailed @kparzysz asking how to disable tests on hexagon as I can't find a likely looking UNSUP

[PATCH] D118493: Set rpath on openmp executables

JonChesterfield added a comment. Apologies for the noise, turns out 'llvm/lib' and 'llvm/lib64' are not the only two options. Made it more permissive and reapplied. Also looks like ppc64le defaults to rpath as opposed to runpath, so that's worth knowing. Repository: rG LLVM Github Monorepo

[PATCH] D118493: Set rpath on openmp executables

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGa80d5c34e4b9: Set rpath on openmp executables (authored by JonChesterfield). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION http

[PATCH] D118493: Set rpath on openmp executables

JonChesterfield updated this revision to Diff 404536. JonChesterfield added a comment. - More paranoid regex - not sure what windows uses as the path separator in the dynamic table Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D118493/new/ https://

[PATCH] D118493: Set rpath on openmp executables

JonChesterfield added a comment. Yep. I'm not totally confident what windows builds will embed in the dynamic table - might be windows style \ or it might be linux style /. Or it might be \\, which can be matched as in D109057 . I think I'm going with the very

[PATCH] D118493: Set rpath on openmp executables

jhuber6 accepted this revision. jhuber6 added a comment. This revision is now accepted and ready to land. LGTM, unless someone else has reservations. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D118493/new/ https://reviews.llvm.org/D118493 __

[PATCH] D118493: Set rpath on openmp executables

jhuber6 added inline comments. Comment at: clang/test/OpenMP/implicit_rpath.c:28 +// CHECK-COMPOSABLE: ({{R|RUN}}PATH) Library {{r|run}}path: [early:late:{{.*}}llvm/lib] + +int main() {} JonChesterfield wrote: > This ^ probably has path separator issues on windo

[PATCH] D118493: Set rpath on openmp executables

JonChesterfield added inline comments. Comment at: clang/test/OpenMP/implicit_rpath.c:28 +// CHECK-COMPOSABLE: ({{R|RUN}}PATH) Library {{r|run}}path: [early:late:{{.*}}llvm/lib] + +int main() {} This ^ probably has path separator issues on windows, will try to f

[PATCH] D118493: Set rpath on openmp executables

JonChesterfield updated this revision to Diff 404499. JonChesterfield added a comment. - rebase and format Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D118493/new/ https://reviews.llvm.org/D118493 Files: clang/include/clang/Driver/Options.td

[PATCH] D118493: Set rpath on openmp executables

JonChesterfield updated this revision to Diff 404498. JonChesterfield added a comment. Herald added a project: OpenMP. Herald added a subscriber: openmp-commits. - Disable implicit rpath for the tests so we can be certain the tests aren't picking up unrelated libs Repository: rG LLVM Github M