[LinkerWrapper] Add support for --no-canonical-prefixes#184160
Conversation
Created using spr 1.3.7
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-driver @llvm/pr-subscribers-clang Author: Aiden Grossman (boomanaiden154) ChangesThis is necessary to support build environments where the Full diff: https://github.com/llvm/llvm-project/pull/184160.diff 3 Files Affected:
diff --git a/clang/test/Driver/linker-wrapper-canonical-prefixes.c b/clang/test/Driver/linker-wrapper-canonical-prefixes.c
new file mode 100644
index 0000000000000..79152cc69c078
--- /dev/null
+++ b/clang/test/Driver/linker-wrapper-canonical-prefixes.c
@@ -0,0 +1,25 @@
+// UNSUPPORTED: system-windows
+// REQUIRES: x86-registered-target
+// REQUIRES: amdgpu-registered-target
+
+// An externally visible variable so static libraries extract.
+__attribute__((visibility("protected"), used)) int x;
+
+// RUN: rm -rf %t.test_dir && mkdir -p %t.test_dir
+// RUN: touch %t.test_dir/clang
+// RUN: chmod +x %t.test_dir/clang
+// RUN: ln -s clang-linker-wrapper %t.test_dir/clang-linker-wrapper
+
+// RUN: %clang -cc1 %s -triple x86_64-unknown-linux-gnu -emit-obj -o %t.elf.o
+// RUN: %clang -cc1 %s -triple amdgcn-amd-amdhsa -emit-llvm-bc -o %t.amdgpu.bc
+
+// RUN: llvm-offload-binary -o %t.out \
+// RUN: --image=file=%t.elf.o,kind=openmp,triple=amdgcn-amd-amdhsa,arch=gfx908 \
+// RUN: --image=file=%t.elf.o,kind=openmp,triple=amdgcn-amd-amdhsa,arch=gfx908
+// RUN: %clang -cc1 %s -triple x86_64-unknown-linux-gnu -emit-obj -o %t.o -fembed-offload-object=%t.out
+// RUN: %t.test_dir/clang-linker-wrapper --host-triple=x86_64-unknown-linux-gnu --dry-run \
+// RUN: --linker-path=/usr/bin/ld %t.o -o a.out --no-canonical-prefixes 2>&1 | FileCheck %s
+
+// Check that we resolve clang to the symlink rather than the bin/ directory
+// and that the sub-clang invocation was passed -no-canonical-prefixes.
+// CHECK: test_dir/clang" {{.*}} -no-canonical-prefixes
diff --git a/clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp b/clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp
index c49ce44432e5a..b2b6910cd7e0b 100644
--- a/clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp
+++ b/clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp
@@ -140,6 +140,9 @@ static std::list<SmallString<128>> TempFiles;
/// Codegen flags for LTO backend.
static codegen::RegisterCodeGenFlags CodeGenFlags;
+/// Whether or not to look through symlinks when resolving binaries.
+static bool CanonicalPrefixes = true;
+
using OffloadingImage = OffloadBinary::OffloadingImage;
namespace llvm {
@@ -219,6 +222,8 @@ void printCommands(ArrayRef<StringRef> CmdArgs) {
}
std::string getMainExecutable(const char *Name) {
+ if (!CanonicalPrefixes)
+ return sys::path::parent_path(LinkerExecutable).str();
void *Ptr = (void *)(intptr_t)&getMainExecutable;
auto COWPath = sys::fs::getMainExecutable(Name, Ptr);
return sys::path::parent_path(COWPath).str();
@@ -539,6 +544,9 @@ Expected<StringRef> clang(ArrayRef<StringRef> InputFiles, const ArgList &Args,
Args.MakeArgString("--target=" + Triple.getTriple()),
};
+ if (!CanonicalPrefixes)
+ CmdArgs.push_back("-no-canonical-prefixes");
+
if (!Arch.empty())
Triple.isAMDGPU() ? CmdArgs.push_back(Args.MakeArgString("-mcpu=" + Arch))
: CmdArgs.push_back(Args.MakeArgString("-march=" + Arch));
@@ -1350,6 +1358,7 @@ int main(int Argc, char **Argv) {
DryRun = Args.hasArg(OPT_dry_run);
SaveTemps = Args.hasArg(OPT_save_temps);
CudaBinaryPath = Args.getLastArgValue(OPT_cuda_path_EQ).str();
+ CanonicalPrefixes = !Args.hasArg(OPT_no_canonical_prefixes);
llvm::Triple Triple(
Args.getLastArgValue(OPT_host_triple_EQ, sys::getDefaultTargetTriple()));
diff --git a/clang/tools/clang-linker-wrapper/LinkerWrapperOpts.td b/clang/tools/clang-linker-wrapper/LinkerWrapperOpts.td
index ef3a16b2f58bb..973e5bb51a507 100644
--- a/clang/tools/clang-linker-wrapper/LinkerWrapperOpts.td
+++ b/clang/tools/clang-linker-wrapper/LinkerWrapperOpts.td
@@ -69,6 +69,10 @@ def emit_fatbin_only
Flags<[WrapperOnlyOption]>,
HelpText<"Emit fat binary directly without wrapping or host linking">;
+def no_canonical_prefixes : Flag<["--"], "no-canonical-prefixes">,
+ Flags<[WrapperOnlyOption]>,
+ HelpText<"Do not resolve symbolic links, turn relative paths into absolute ones, or do anything else to identify the executable">;
+
// Flags passed to the device linker.
def arch_EQ : Joined<["--"], "arch=">,
Flags<[DeviceOnlyOption, HelpHidden]>, MetaVarName<"<arch>">,
|
boomanaiden154
requested review from
Artem-B,
jhuber6 and
sarnex
and removed request for
jhuber6
| // RUN: %clang -cc1 %s -triple amdgcn-amd-amdhsa -emit-llvm-bc -o %t.amdgpu.bc | ||
|
|
||
| // RUN: llvm-offload-binary -o %t.out \ | ||
| // RUN: --image=file=%t.elf.o,kind=openmp,triple=amdgcn-amd-amdhsa,arch=gfx908 \ |
There was a problem hiding this comment.
Is having the same --image line twice intentional?
There was a problem hiding this comment.
Nope. Just me copying and pasting from other tests. Removed.
| Flags<[WrapperOnlyOption]>, | ||
| HelpText<"Emit fat binary directly without wrapping or host linking">; | ||
|
|
||
| def no_canonical_prefixes : Flag<["--"], "no-canonical-prefixes">, |
There was a problem hiding this comment.
Is this something other tools handle? Is that handling uniform?
There was a problem hiding this comment.
Only tools that need to resolve binary paths/invoke them. So probably not most tools, but definitely this one and the clang driver (which already supports it). In my limited understanding, support is relatively uniform across the standard toolchain binaries.
| }; | ||
|
|
||
| if (!CanonicalPrefixes) | ||
| CmdArgs.push_back("-no-canonical-prefixes"); |
There was a problem hiding this comment.
Arguments like this that are meant to be propagated are better handled via the list at the top of when we make the clang-linker-wrapper job. The onyl change here seems to be for getting the directory of the current executable, but I'm not sure why that needs to be changed. If there's another way to do this then it's just a one line change to add that to the list.
There was a problem hiding this comment.
List at the top as in the definition of CmdArgs right above?
We don't want to propagate this unconditionally because it could change the behavior and would also make this inconsistent with the default behavior of the clang driver.
We need to change the way we get the directory of the current executable because otherwise we cannot resolve the path to clang if clang-linker-wrapper is a symlink in a directory that contains clang to a file in a directory that does not contain a binary named clang.
There was a problem hiding this comment.
Okay, so the part that forwards it to the embedded job should just be handled by the list. The part that changes the relative search path for the main executable. If there's no way to detect if we should do that without the option then it should probably be a separate thing.
There was a problem hiding this comment.
If there's no way to detect if we should do that without the option then it should probably be a separate thing.
What exactly do you mean here by "a separate thing"?
There was a problem hiding this comment.
I was just wondering if it would be easier to rely on the forwarding behavior, but since we also need the option for the linker wrapper itself I suppose it's easier to have a single option.
Can we use the args list here instead?
There was a problem hiding this comment.
Done. Required making clang-linker-wrapper accept -no-canonical-prefixes rather than --no-canonical-prefixes which I guess is inconsistent with the rest of the clang-linker-wrapper options, but makes it more consistent with the clang driver.
|
Having two PRs for this is confusing, can you just close the other one and put that change here? |
| OPT_save_temps_EQ, | ||
| OPT_mcode_object_version_EQ, | ||
| OPT_load, | ||
| OPT_no_canonical_prefixes, |
There was a problem hiding this comment.
This will automatically forward it to the internal clang, but not the internal uses of the tools. I'm wondering if we shouldn't just pass some directory from clang in this option is set. I think we do something similar with the CUDA path and this is basically the same thing IIRC.
Does that sound reasonable? We'd have clang pass in a path and that would be used instead of looking it up from the main executable if we assume the clang driver to be the arbiter of truth here.
There was a problem hiding this comment.
This is in LinkerWrapper::ConstructJob. It forwards specifically to clang-linker-wrapper as far as I can tell.
We could pass the directory to clang down into clang-linker-wrapper, but then you run into the same problem with any other tool that clang-linker-wrapper could invoke like llvm-objcopy. I don't think passing in flags for all the tools makes sense.
There was a problem hiding this comment.
The clang driver computes the paths for these tools, I was more referring to passing forward the root of the 'clang-toolchain' since this config basically assumes they're disparate as far as I'm understanding. Not dissimilar to finding the GCC prefix or CUDA root?
There was a problem hiding this comment.
Sure, that would work.
I don't see the advantage of that approach over supporting this flag though. If we do what you're proposing, we end up with the following:
- Propagation of
-no-canonical-prefixesthrough--device-compilerto sub-clang invocations as currently. - Replacement of
-no-canonical-prefixespassed toclang-linker-wrapperwith the clang driver passing something like-toolchain-dir=<resolved toolchain dir>. Instead of updatinggetExecutableDir, we need to updatefindProgramto take the new flag into account.
I think the complexity ends up being about the same at the cost of divergence. Instead of having one way that both the driver and clang-linker-wrapper can specify this behavior, now we have two different ways of achieving the same thing.
jhuber6
left a comment
jhuber6
left a comment
There was a problem hiding this comment.
Seems fine from what I understand
boomanaiden154
deleted the
users/boomanaiden154/linkerwrapper-add-support-for-no-canonical-prefixes
branch
This is necessary to support build environments where the compiler/associated tools are actually just symlinks into a CAS. Without this, we try and resolve binaries relative to the real path of clang-linker-wrapper, which is usually in a directory prefixed with the first couple characters of a SHA digest and named with a SHA digest. We also need to ensure that we propagate --no-canonical-prefixes to sub clang invocations so that clang is able to resolve lld in such environments. Reviewers: jhuber6, Artem-B, sarnex Pull Request: llvm/llvm-project#184160
4 participants
This is necessary to support build environments where the
compiler/associated tools are actually just symlinks into a CAS. Without
this, we try and resolve binaries relative to the real path of
clang-linker-wrapper, which is usually in a directory prefixed with the
first couple characters of a SHA digest and named with a SHA digest. We
also need to ensure that we propagate --no-canonical-prefixes to sub
clang invocations so that clang is able to resolve lld in such
environments.