[Driver] Unify InstalledDir and Dir #80527
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Created using spr 1.3.4
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-driver @llvm/pr-subscribers-clang Author: Fangrui Song (MaskRay) Changes
I believe This patch removes If a user creates a symlink to the regular file If a user creates a symlink to the regular file Full diff: https://github.com/llvm/llvm-project/pull/80527.diff 7 Files Affected:
diff --git a/clang/include/clang/Driver/Driver.h b/clang/include/clang/Driver/Driver.h
index 3ee1bcf2a69c9..afd388dfaf017 100644
--- a/clang/include/clang/Driver/Driver.h
+++ b/clang/include/clang/Driver/Driver.h
@@ -160,7 +160,7 @@ class Driver {
/// Target and driver mode components extracted from clang executable name.
ParsedClangName ClangNameParts;
- /// The path to the installed clang directory, if any.
+ /// TODO: Remove this in favor of Dir.
std::string InstalledDir;
/// The path to the compiler resource directory.
@@ -419,7 +419,6 @@ class Driver {
return InstalledDir.c_str();
return Dir.c_str();
}
- void setInstalledDir(StringRef Value) { InstalledDir = std::string(Value); }
bool isSaveTempsEnabled() const { return SaveTemps != SaveTempsNone; }
bool isSaveTempsObj() const { return SaveTemps == SaveTempsObj; }
diff --git a/clang/test/Driver/darwin-header-search-libcxx.cpp b/clang/test/Driver/darwin-header-search-libcxx.cpp
index 70cc06090a993..5695f53683bab 100644
--- a/clang/test/Driver/darwin-header-search-libcxx.cpp
+++ b/clang/test/Driver/darwin-header-search-libcxx.cpp
@@ -193,7 +193,7 @@
// RUN: ln -sf %t/install/bin/clang %t/symlinked1/bin/clang
// RUN: mkdir -p %t/symlinked1/include/c++/v1
-// RUN: %t/symlinked1/bin/clang -### %s -fsyntax-only 2>&1 \
+// RUN: %t/symlinked1/bin/clang -### %s -no-canonical-prefixes -fsyntax-only 2>&1 \
// RUN: --target=x86_64-apple-darwin \
// RUN: -stdlib=libc++ \
// RUN: -isysroot %S/Inputs/basic_darwin_sdk_usr_cxx_v1 \
diff --git a/clang/test/Driver/mingw-sysroot.cpp b/clang/test/Driver/mingw-sysroot.cpp
index 50152b2ca210d..5d512e6669709 100644
--- a/clang/test/Driver/mingw-sysroot.cpp
+++ b/clang/test/Driver/mingw-sysroot.cpp
@@ -50,10 +50,12 @@
// CHECK_TESTROOT_GCC_EXPLICIT: "-internal-isystem" "{{[^"]+}}/testroot-gcc{{/|\\\\}}include"
-// If there's a matching sysroot next to the clang binary itself, prefer that
+// If -no-canonical-prefixes and there's a matching sysroot next to the clang binary itself, prefer that
// over a gcc in the path:
-// RUN: env "PATH=%T/testroot-gcc/bin:%PATH%" %T/testroot-clang/bin/x86_64-w64-mingw32-clang -target x86_64-w64-mingw32 -rtlib=compiler-rt -stdlib=libstdc++ --sysroot="" -c -### %s 2>&1 | FileCheck -check-prefix=CHECK_TESTROOT_CLANG %s
+// RUN: env "PATH=%T/testroot-gcc/bin:%PATH%" %T/testroot-clang/bin/x86_64-w64-mingw32-clang --target=x86_64-w64-mingw32 -rtlib=compiler-rt -stdlib=libstdc++ --sysroot="" -c -### %s 2>&1 | FileCheck -check-prefix=CHECK_TESTROOT_GCC2 %s
+// RUN: env "PATH=%T/testroot-gcc/bin:%PATH%" %T/testroot-clang/bin/x86_64-w64-mingw32-clang --target=x86_64-w64-mingw32 -rtlib=compiler-rt -stdlib=libstdc++ --sysroot="" -c -### %s -no-canonical-prefixes 2>&1 | FileCheck -check-prefix=CHECK_TESTROOT_CLANG %s
+// CHECK_TESTROOT_GCC2: "{{[^"]+}}/testroot-gcc{{/|\\\\}}x86_64-w64-mingw32{{/|\\\\}}include"
// CHECK_TESTROOT_CLANG: "{{[^"]+}}/testroot-clang{{/|\\\\}}x86_64-w64-mingw32{{/|\\\\}}include"
@@ -82,7 +84,7 @@
// that indicates that we did choose the right base, even if this particular directory
// actually doesn't exist here.
-// RUN: env "PATH=%T/testroot-gcc/bin:%PATH%" %T/testroot-clang-native/bin/clang -target x86_64-w64-mingw32 -rtlib=compiler-rt -stdlib=libstdc++ --sysroot="" -c -### %s 2>&1 | FileCheck -check-prefix=CHECK_TESTROOT_CLANG_NATIVE %s
+// RUN: env "PATH=%T/testroot-gcc/bin:%PATH%" %T/testroot-clang-native/bin/clang -no-canonical-prefixes --target=x86_64-w64-mingw32 -rtlib=compiler-rt -stdlib=libstdc++ --sysroot="" -c -### %s 2>&1 | FileCheck -check-prefix=CHECK_TESTROOT_CLANG_NATIVE %s
// CHECK_TESTROOT_CLANG_NATIVE: "{{[^"]+}}/testroot-clang-native{{/|\\\\}}x86_64-w64-mingw32{{/|\\\\}}include"
@@ -93,12 +95,12 @@
// that defaults to x86_64 mingw, but it's easier to test this in cross setups
// with symlinks, like the other tests here.)
-// RUN: env "PATH=%T/testroot-gcc/bin:%PATH%" %T/testroot-clang/bin/x86_64-w64-mingw32-clang --target=x86_64-w64-mingw32 -m32 -rtlib=compiler-rt -stdlib=libstdc++ --sysroot="" -c -### %s 2>&1 | FileCheck -check-prefix=CHECK_TESTROOT_CLANG_I686 %s
+// RUN: env "PATH=%T/testroot-gcc/bin:%PATH%" %T/testroot-clang/bin/x86_64-w64-mingw32-clang -no-canonical-prefixes --target=x86_64-w64-mingw32 -m32 -rtlib=compiler-rt -stdlib=libstdc++ --sysroot="" -c -### %s 2>&1 | FileCheck -check-prefix=CHECK_TESTROOT_CLANG_I686 %s
// CHECK_TESTROOT_CLANG_I686: "{{[^"]+}}/testroot-clang{{/|\\\\}}i686-w64-mingw32{{/|\\\\}}include"
// If the user calls clang with a custom literal triple, make sure this maps
// to sysroots with the matching spelling.
-// RUN: %T/testroot-custom-triple/bin/clang --target=x86_64-w64-mingw32foo -rtlib=compiler-rt -stdlib=libstdc++ --sysroot="" -c -### %s 2>&1 | FileCheck -check-prefix=CHECK_TESTROOT_CUSTOM_TRIPLE %s
+// RUN: %T/testroot-custom-triple/bin/clang -no-canonical-prefixes --target=x86_64-w64-mingw32foo -rtlib=compiler-rt -stdlib=libstdc++ --sysroot="" -c -### %s 2>&1 | FileCheck -check-prefix=CHECK_TESTROOT_CUSTOM_TRIPLE %s
// CHECK_TESTROOT_CUSTOM_TRIPLE: "{{[^"]+}}/testroot-custom-triple{{/|\\\\}}x86_64-w64-mingw32foo{{/|\\\\}}include"
diff --git a/clang/test/Driver/no-canonical-prefixes.c b/clang/test/Driver/no-canonical-prefixes.c
index fb54f85f959ae..669e56639284a 100644
--- a/clang/test/Driver/no-canonical-prefixes.c
+++ b/clang/test/Driver/no-canonical-prefixes.c
@@ -26,7 +26,7 @@
// RUN: | FileCheck --check-prefix=NON-CANONICAL %s
//
// FIXME: This should really be '.real'.
-// CANONICAL: InstalledDir: {{.*}}.fake
+// CANONICAL: InstalledDir: {{.*}}bin
// CANONICAL: {{[/|\\]*}}clang{{.*}}" -cc1
//
// NON-CANONICAL: InstalledDir: .{{$}}
diff --git a/clang/test/Driver/program-path-priority.c b/clang/test/Driver/program-path-priority.c
index ee931dd7a9a35..c940c4ced9442 100644
--- a/clang/test/Driver/program-path-priority.c
+++ b/clang/test/Driver/program-path-priority.c
@@ -36,7 +36,7 @@
// RUN: touch %t/notreal-none-elf-gcc && chmod +x %t/notreal-none-elf-gcc
// RUN: env "PATH=" %t/clang -### -target notreal-none-elf %s 2>&1 | \
// RUN: FileCheck --check-prefix=PROG_PATH_NOTREAL_GCC %s
-// PROG_PATH_NOTREAL_GCC: notreal-none-elf-gcc"
+// PROG_PATH_NOTREAL_GCC: notreal-none-unknown-elf
/// <triple>-gcc on the PATH is found
// RUN: mkdir -p %t/env
@@ -57,7 +57,7 @@
// RUN: touch %t/gcc && chmod +x %t/gcc
// RUN: env "PATH=" %t/clang -### -target notreal-none-elf %s 2>&1 | \
// RUN: FileCheck --check-prefix=NOTREAL_GCC_PREFERRED %s
-// NOTREAL_GCC_PREFERRED: notreal-none-elf-gcc"
+// NOTREAL_GCC_PREFERRED: notreal-none-unknown-elf"
// NOTREAL_GCC_PREFERRED-NOT: /gcc"
/// <triple>-gcc on the PATH is preferred to gcc in program path
@@ -125,6 +125,9 @@
/// Only if there is nothing in the prefix will we search other paths
/// -f in case $DEFAULT_TRIPLE == %target_triple
// RUN: rm -f %t/prefix/$DEFAULT_TRIPLE-gcc %t/prefix/%target_triple-gcc %t/prefix/gcc
-// RUN: env "PATH=" %t/clang -### -target notreal-none-elf %s -B %t/prefix 2>&1 | \
-// RUN: FileCheck --check-prefix=EMPTY_PREFIX_DIR %s
-// EMPTY_PREFIX_DIR: notreal-none-elf-gcc"
+// RUN: env "PATH=" %t/clang -### -canonical-prefixes --target=notreal-none-elf %s -B %t/prefix 2>&1 | \
+// RUN: FileCheck --check-prefix=EMPTY_PREFIX_DIR1 %s
+// EMPTY_PREFIX_DIR1: gcc"
+// RUN: env "PATH=" %t/clang -### -no-canonical-prefixes --target=notreal-none-elf %s -B %t/prefix 2>&1 | \
+// RUN: FileCheck --check-prefix=EMPTY_PREFIX_DIR2 %s
+// EMPTY_PREFIX_DIR2: notreal-none-elf-gcc"
diff --git a/clang/test/Driver/rocm-detect.hip b/clang/test/Driver/rocm-detect.hip
index 0db994af556f3..8b15c322e3fb3 100644
--- a/clang/test/Driver/rocm-detect.hip
+++ b/clang/test/Driver/rocm-detect.hip
@@ -102,7 +102,7 @@
// RUN: rm -rf %t/rocm-spack
// RUN: cp -r %S/Inputs/rocm-spack %t
// RUN: ln -fs %clang %t/rocm-spack/llvm-amdgpu-4.0.0-ieagcs7inf7runpyfvepqkurasoglq4z/bin/clang
-// RUN: %t/rocm-spack/llvm-amdgpu-4.0.0-ieagcs7inf7runpyfvepqkurasoglq4z/bin/clang -### -v \
+// RUN: %t/rocm-spack/llvm-amdgpu-4.0.0-ieagcs7inf7runpyfvepqkurasoglq4z/bin/clang -### -no-canonical-prefixes -v \
// RUN: -resource-dir=%t/rocm-spack/llvm-amdgpu-4.0.0-ieagcs7inf7runpyfvepqkurasoglq4z/lib/clang \
// RUN: -target x86_64-linux-gnu --cuda-gpu-arch=gfx900 --print-rocm-search-dirs %s 2>&1 \
// RUN: | FileCheck -check-prefixes=SPACK %s
@@ -111,7 +111,7 @@
// ROCm release. --hip-path and --rocm-device-lib-path can be used to specify them.
// RUN: cp -r %t/rocm-spack/hip-* %t/rocm-spack/hip-4.0.0-abcd
-// RUN: %t/rocm-spack/llvm-amdgpu-4.0.0-ieagcs7inf7runpyfvepqkurasoglq4z/bin/clang -### -v \
+// RUN: %t/rocm-spack/llvm-amdgpu-4.0.0-ieagcs7inf7runpyfvepqkurasoglq4z/bin/clang -### -no-canonical-prefixes -v \
// RUN: -target x86_64-linux-gnu --cuda-gpu-arch=gfx900 \
// RUN: --hip-path=%t/rocm-spack/hip-4.0.0-abcd \
// RUN: %s 2>&1 | FileCheck -check-prefixes=SPACK-SET %s
diff --git a/clang/tools/driver/driver.cpp b/clang/tools/driver/driver.cpp
index 1407c7fcdab76..2020230c00399 100644
--- a/clang/tools/driver/driver.cpp
+++ b/clang/tools/driver/driver.cpp
@@ -323,28 +323,6 @@ static void FixupDiagPrefixExeName(TextDiagnosticPrinter *DiagClient,
DiagClient->setPrefix(std::string(ExeBasename));
}
-static void SetInstallDir(SmallVectorImpl<const char *> &argv,
- Driver &TheDriver, bool CanonicalPrefixes) {
- // Attempt to find the original path used to invoke the driver, to determine
- // the installed path. We do this manually, because we want to support that
- // path being a symlink.
- SmallString<128> InstalledPath(argv[0]);
-
- // Do a PATH lookup, if there are no directory components.
- if (llvm::sys::path::filename(InstalledPath) == InstalledPath)
- if (llvm::ErrorOr<std::string> Tmp = llvm::sys::findProgramByName(
- llvm::sys::path::filename(InstalledPath.str())))
- InstalledPath = *Tmp;
-
- // FIXME: We don't actually canonicalize this, we just make it absolute.
- if (CanonicalPrefixes)
- llvm::sys::fs::make_absolute(InstalledPath);
-
- StringRef InstalledPathParent(llvm::sys::path::parent_path(InstalledPath));
- if (llvm::sys::fs::exists(InstalledPathParent))
- TheDriver.setInstalledDir(InstalledPathParent);
-}
-
static int ExecuteCC1Tool(SmallVectorImpl<const char *> &ArgV,
const llvm::ToolContext &ToolContext) {
// If we call the cc1 tool from the clangDriver library (through
@@ -483,7 +461,6 @@ int clang_main(int Argc, char **Argv, const llvm::ToolContext &ToolContext) {
ProcessWarningOptions(Diags, *DiagOpts, /*ReportDiags=*/false);
Driver TheDriver(Path, llvm::sys::getDefaultTargetTriple(), Diags);
- SetInstallDir(Args, TheDriver, CanonicalPrefixes);
auto TargetAndMode = ToolChain::getTargetAndModeFromProgramName(ProgName);
TheDriver.setTargetAndMode(TargetAndMode);
// If -canonical-prefixes is set, GetExecutablePath will have resolved Path
|
This was referenced Feb 3, 2024
|
Ping:) @ldionne I know you are discussing a |
|
From my point of view, I think this sounds fine - it looks like you've looked through this and thought deeper about it than I have at least. I'm not deep enough into this to comfortably press approved on this for now though, so hopefully someone else can chime in as well. |
Thanks. Yes, I think should be done. Ping |
|
Ping:) |
mstorsjo
approved these changes
Feb 28, 2024
MaskRay
deleted the
users/MaskRay/spr/driver-unify-installeddir-and-dir
branch
mylai-mtk
pushed a commit
to mylai-mtk/llvm-project
that referenced
this pull request
Jul 12, 2024
`Driver::ClangExecutable` is derived from: * (-canonical-prefixes default): `realpath` on the executable path * (-no-canonical-prefixes) argv[0] (consult PATH if argv[0] is a word) `Dir` and `ResourceDir` are derived from `ClangExecutable`. Both variables are used to derive certain include and library paths. `InstalledDir` is a related concept used to derive certain other paths. `InstalledDir` is weird in the -canonical-prefixes mode: Clang calls `make_absolute` but does not follow symlinks (FIXME from 9ade6a9). This causes some search and library paths to be mix-and-matched. The "Do a PATH lookup, if there are no directory components." logic makes things worse. `InstalledDir` is different when you invoke it via `PATH`: ``` % which clang /usr/bin/clang % clang -v |& grep InstalledDir InstalledDir: /usr/bin % /usr/lib/llvm-16/bin/clang -v |& grep InstalledDir InstalledDir: /usr/lib/llvm-16/bin ``` I believe `InstalledDir` was a partial solution to `-no-canonical-prefixes` and should be eventually removed. This patch removes `SetInstallDir` and relies on Driver::Driver to set `InstalledDir` to `Dir`. The behavior for regular file `clang` or `-no-canonical-prefixes` is unchanged. If a user creates a symlink to the regular file `clang` and uses the default `-canonical-prefixes`, they now consistently get search and library paths relative to the regular file `clang`, not mix-and-match paths. If a user creates a symlink to the regular file `clang` and replaces some directorys from the actual installation, they should change the symlink to a wrapper that calls the underlying clang with `-no-canonical-prefixes`.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Driver::ClangExecutableis derived from:realpathon the executable pathDirandResourceDirare derived fromClangExecutable. Bothvariables are used to derive certain include and library paths.
InstalledDiris a related concept used to derive certain other paths.InstalledDiris weird in the -canonical-prefixes mode: Clangcalls
make_absolutebut does not follow symlinks(FIXME from 9ade6a9).
This causes some search and library paths to be mix-and-matched.
The "Do a PATH lookup, if there are no directory components." logic makes things worse.
InstalledDiris different when you invoke it viaPATH:I believe
InstalledDirwas a partial solution to-no-canonical-prefixesand should be eventually removed.This patch removes
SetInstallDirand relies on Driver::Driver to setInstalledDirtoDir. The behavior for regular fileclangor-no-canonical-prefixesis unchanged.If a user creates a symlink to the regular file
clangand uses thedefault
-canonical-prefixes, they now consistently get search andlibrary paths relative to the regular file
clang, not mix-and-matchpaths.
If a user creates a symlink to the regular file
clangand replacessome directorys from the actual installation, they should change the
symlink to a wrapper that calls the underlying clang with
-no-canonical-prefixes.