Page MenuHomePhabricator

[Driver] Add DEFAULT_DYLD_PREFIX and DEFAULT_RPATH to complement DEFAULT_SYSROOT
AcceptedPublic

Authored by hubert.reinterpretcast on May 20 2020, 8:19 AM.

Details

Summary

Some target toolchains use more than just a default --sysroot, but also include a default --dyld-prefix and an implicitly-added -rpath.

For example, using a vanilla build of Clang with the IBM Advance Toolchain for Linux on Power would require specifying --sysroot=/opt/at12.0 --dyld-prefix=/opt/at12.0 -rpath /opt/at12.0/lib64. The GCC compiler provided with the Advance Toolchain is preconfigured such that adding such options is not necessary. This patch adds the configuration hooks that would allow Clang to be preconfigured similarly.

Various tests not compatible with builds that set a non-empty DEFAULT_DYLD_PREFIX (because they are sensitive to the dynamic linker path) are updated with --dyld-prefix="" in a manner analogous to what was done in D66834 with --sysroot="" in relation to DEFAULT_SYSROOT.

Note: The DEFAULT_RPATH behaviour has only been implemented for "GNU" toolchains.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptMay 20 2020, 8:19 AM
hubert.reinterpretcast edited the summary of this revision. (Show Details)May 20 2020, 8:36 AM
nemanjai accepted this revision.May 21 2020, 3:29 AM
nemanjai added subscribers: tstellar, brad, craig.topper, joerg.

Thank you for sorting this out. I think it is quite useful to be able to configure the compiler to use complete non-standard toolchains that include a dynamic linker.
Could you please ensure that the commit message describes that the --dyld-prefix option is added to test cases since they check for the dynamic linker and would fail on builds that specify a default prefix?
Also, you might want to add someone from some of the other targets/platforms as a reviewer to ensure there is no objections (perhaps @craig.topper, @tstellar, @brad or @joerg).

LGTM but maybe give others a few days to have a look.

clang/lib/Driver/ToolChains/Gnu.cpp
452

Is this just an orthogonal NFC change? If so, can you please commit it separately in an NFC commit?

This revision is now accepted and ready to land.May 21 2020, 3:29 AM
hubert.reinterpretcast edited the summary of this revision. (Show Details)May 21 2020, 6:34 AM
hubert.reinterpretcast added inline comments.
clang/lib/Driver/ToolChains/Gnu.cpp
452

Yes; all of the other changes made around here disappeared during the development process. Would it be okay to split it out on the commit instead of updating the patch?

joerg added a comment.May 21 2020, 8:39 AM

I do agree with the feature request, but I'm not sure about the implementation. It doesn't seem to work well with the cross-compiling support in the driver as clearly shown by the amount of tests that need patching. Is cross-compiling a concern for you at all? Otherwise I would suggest going a slightly different route and use default values iff no "-target" or "target-command" mechanism is active. Might need a way to temporarily disable unused flag warnings, but that way would be pretty much toolchain independent?

It doesn't seem to work well with the cross-compiling support in the driver as clearly shown by the amount of tests that need patching.

I don't see the test changes as reflecting that at all. If a user was able to use --dyld-prefix with the value being set as the default, then it just works. That would depend on the user's environment, which is something that the packager/builder of Clang might know something about or have control over.

I would suggest going a slightly different route and use default values iff no "-target" or "target-command" mechanism is active.

I'm not sure I see enough of a difference between the newly added customization points and DEFAULT_SYSROOT. The latter does not act conditionally in the manner proposed above.

Might need a way to temporarily disable unused flag warnings, but that way would be pretty much toolchain independent?

Can you elaborate on this? I'm not sure I caught the line of reasoning that led to this comment.

I see that more as a short-coming in the existing DEFAULT_SYSROOT behavior and less an argument for making more cases like it. So the general idea is that for turnkey toolchains,
we want to allow customizing the "default" target of the toolchain to hard-code options like --sysroot, --target, -Wl,-rpath etc. Those are all related, so when using a different target, they no longer make sense. One way to deal with all those options in a consistent manner is hook into the logic for determining the current target, if none is explicitly specified on the command line or implicit from the executable name, then prepend some additional flags on the command line based on some cmake variable or so. This flags shouldn't trigger the unused argument warnings, so you can always pass -Wl,-rpath, --sysroot etc, independent of whether the compiler is in preprocessing / compile / assemble / link mode. That seems to be a more general and systematic approach than adding many additional build-time options.

So the general idea is that for turnkey toolchains,
we want to allow customizing the "default" target of the toolchain to hard-code options like --sysroot, --target, -Wl,-rpath etc. Those are all related, so when using a different target, they no longer make sense.

I don't think there is anything precluding the packager from setting up the availability of a "turnkey" multi-target toolchain (with fat binaries if necessary) that would still make sense to pass --target to.

One way to deal with all those options in a consistent manner is hook into the logic for determining the current target, if none is explicitly specified on the command line or implicit from the executable name, then prepend some additional flags on the command line based on some cmake variable or so. This flags shouldn't trigger the unused argument warnings, so you can always pass -Wl,-rpath, --sysroot etc, independent of whether the compiler is in preprocessing / compile / assemble / link mode. That seems to be a more general and systematic approach than adding many additional build-time options.

This might make sense if we believe that the ability to add arbitrary "implied default target options" is not harmful. I am not certain people would agree on that point. The solutions do have overlap in terms of the problems they could be applied to, but I think each of them address some use cases that the other does not.

@joerg, are you okay with this patch landing?

hubert.reinterpretcast marked 3 inline comments as done.May 31 2020, 1:55 PM
hubert.reinterpretcast added inline comments.
clang/lib/Driver/ToolChains/Gnu.cpp
452

The NFC change has be separately committed under rGc15d5d12c625.

hubert.reinterpretcast marked an inline comment as done.Jun 2 2020, 8:19 PM

I believe I have responded to @joerg's comments with rationale to support the current direction. I intend to commit this patch within a week or two.

joerg added a comment.Jun 3 2020, 6:31 AM

I don't agree with the justification at all, but it also seems that noone else cares about the build option creep here.

According to https://clang.llvm.org/docs/CrossCompilation.html (under Toolchain Options option 2) it is quite likely that a user that desires to cross-compile will have the necessary toolchain installed into a directory that will not require the use of --sysroot.

So I think that letting --target override the default --sysroot/--dyld-prefix/--rpath is reasonable. What I am suggesting is that if --target is specified on the command line, it clears any default setting of these three options (i.e. clears them unless they were explicitly specified on the command line).

Namely, something along the lines of:

diff --git a/clang/lib/Driver/Driver.cpp b/clang/lib/Driver/Driver.cpp
index 5c726b2..9a85394 100644
--- a/clang/lib/Driver/Driver.cpp
+++ b/clang/lib/Driver/Driver.cpp
@@ -1073,8 +1073,13 @@ Compilation *Driver::BuildCompilation(ArrayRef<const char *> ArgList) {
     T.setObjectFormat(llvm::Triple::COFF);
     TargetTriple = T.str();
   }
-  if (const Arg *A = Args.getLastArg(options::OPT_target))
+  if (const Arg *A = Args.getLastArg(options::OPT_target)) {
     TargetTriple = A->getValue();
+    if (!Args.getLastArg(options::OPT__sysroot_EQ))
+      SysRoot = "";
+    if (!Args.getLastArg(options::OPT__dyld_prefix_EQ))
+      DyldPrefix = "";
+  }

And something similar for DEFAULT_RPATH.

Would something like this be satisfactory for everyone here? Or do others think this is the "worst of both worlds"? :)