Use CLANG_RESOURCE_DIR more consistently #103388
Merged
Conversation
This file contains hidden or 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
llvmbot
added
clang
|
@llvm/pr-subscribers-lldb @llvm/pr-subscribers-clang-driver Author: Kim Gräsman (kimgr) ChangesWhen Clang is consumed as a library, the CLANG_RESOURCE_DIR definition is not exported from the CMake system, so external clients will be unable to compute the same resource dir as Clang itself would, because they don't know what to pass for the optional CustomResourceDir argument. All call sites except one would pass CLANG_RESOURCE_DIR to Driver::GetResourcesPath. It seems the one exception in libclang CIndexer was an oversight. Move the use of CLANG_RESOURCE_DIR into GetResourcesPath and remove the optional argument to avoid this inconsistency between internal and external clients. Full diff: https://github.com/llvm/llvm-project/pull/103388.diff 5 Files Affected:
diff --git a/clang/include/clang/Driver/Driver.h b/clang/include/clang/Driver/Driver.h
index 04b46782467d6a..0382c8cb155e51 100644
--- a/clang/include/clang/Driver/Driver.h
+++ b/clang/include/clang/Driver/Driver.h
@@ -379,8 +379,7 @@ class Driver {
/// Takes the path to a binary that's either in bin/ or lib/ and returns
/// the path to clang's resource directory.
- static std::string GetResourcesPath(StringRef BinaryPath,
- StringRef CustomResourceDir = "");
+ static std::string GetResourcesPath(StringRef BinaryPath);
Driver(StringRef ClangExecutable, StringRef TargetTriple,
DiagnosticsEngine &Diags, std::string Title = "clang LLVM compiler",
diff --git a/clang/lib/Driver/Driver.cpp b/clang/lib/Driver/Driver.cpp
index 8e44d5afa40e05..dac8185693cf58 100644
--- a/clang/lib/Driver/Driver.cpp
+++ b/clang/lib/Driver/Driver.cpp
@@ -171,18 +171,18 @@ getHIPOffloadTargetTriple(const Driver &D, const ArgList &Args) {
}
// static
-std::string Driver::GetResourcesPath(StringRef BinaryPath,
- StringRef CustomResourceDir) {
+std::string Driver::GetResourcesPath(StringRef BinaryPath) {
// Since the resource directory is embedded in the module hash, it's important
// that all places that need it call this function, so that they get the
// exact same string ("a/../b/" and "b/" get different hashes, for example).
// Dir is bin/ or lib/, depending on where BinaryPath is.
- std::string Dir = std::string(llvm::sys::path::parent_path(BinaryPath));
-
+ StringRef Dir = llvm::sys::path::parent_path(BinaryPath);
SmallString<128> P(Dir);
- if (CustomResourceDir != "") {
- llvm::sys::path::append(P, CustomResourceDir);
+
+ StringRef ConfiguredResourceDir(CLANG_RESOURCE_DIR);
+ if (!ConfiguredResourceDir.empty()) {
+ llvm::sys::path::append(P, ConfiguredResourceDir);
} else {
// On Windows, libclang.dll is in bin/.
// On non-Windows, libclang.so/.dylib is in lib/.
@@ -239,7 +239,7 @@ Driver::Driver(StringRef ClangExecutable, StringRef TargetTriple,
#endif
// Compute the path to the resource directory.
- ResourceDir = GetResourcesPath(ClangExecutable, CLANG_RESOURCE_DIR);
+ ResourceDir = GetResourcesPath(ClangExecutable);
}
void Driver::setDriverMode(StringRef Value) {
diff --git a/clang/lib/Frontend/CompilerInvocation.cpp b/clang/lib/Frontend/CompilerInvocation.cpp
index f6b6c44a4cab6a..6962cb4f32b06a 100644
--- a/clang/lib/Frontend/CompilerInvocation.cpp
+++ b/clang/lib/Frontend/CompilerInvocation.cpp
@@ -3121,7 +3121,7 @@ std::string CompilerInvocation::GetResourcesPath(const char *Argv0,
void *MainAddr) {
std::string ClangExecutable =
llvm::sys::fs::getMainExecutable(Argv0, MainAddr);
- return Driver::GetResourcesPath(ClangExecutable, CLANG_RESOURCE_DIR);
+ return Driver::GetResourcesPath(ClangExecutable);
}
static void GenerateHeaderSearchArgs(const HeaderSearchOptions &Opts,
diff --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangHost.cpp b/lldb/source/Plugins/ExpressionParser/Clang/ClangHost.cpp
index 6064c02c7fd67d..6de851081598fd 100644
--- a/lldb/source/Plugins/ExpressionParser/Clang/ClangHost.cpp
+++ b/lldb/source/Plugins/ExpressionParser/Clang/ClangHost.cpp
@@ -53,7 +53,7 @@ static bool DefaultComputeClangResourceDirectory(FileSpec &lldb_shlib_spec,
std::string raw_path = lldb_shlib_spec.GetPath();
llvm::StringRef parent_dir = llvm::sys::path::parent_path(raw_path);
static const std::string clang_resource_path =
- clang::driver::Driver::GetResourcesPath("bin/lldb", CLANG_RESOURCE_DIR);
+ clang::driver::Driver::GetResourcesPath("bin/lldb");
static const llvm::StringRef kResourceDirSuffixes[] = {
// LLVM.org's build of LLDB uses the clang resource directory placed
diff --git a/lldb/unittests/Expression/ClangParserTest.cpp b/lldb/unittests/Expression/ClangParserTest.cpp
index 6f682f6c97fdb5..70fa49434666ff 100644
--- a/lldb/unittests/Expression/ClangParserTest.cpp
+++ b/lldb/unittests/Expression/ClangParserTest.cpp
@@ -43,7 +43,7 @@ TEST_F(ClangHostTest, ComputeClangResourceDirectory) {
std::string path_to_liblldb = "C:\\foo\\bar\\lib\\";
#endif
std::string path_to_clang_dir = clang::driver::Driver::GetResourcesPath(
- path_to_liblldb + "liblldb", CLANG_RESOURCE_DIR);
+ path_to_liblldb + "liblldb");
llvm::SmallString<256> path_to_clang_lib_dir_real;
llvm::sys::fs::real_path(path_to_clang_dir, path_to_clang_lib_dir_real);
|
|
@llvm/pr-subscribers-clang Author: Kim Gräsman (kimgr) ChangesWhen Clang is consumed as a library, the CLANG_RESOURCE_DIR definition is not exported from the CMake system, so external clients will be unable to compute the same resource dir as Clang itself would, because they don't know what to pass for the optional CustomResourceDir argument. All call sites except one would pass CLANG_RESOURCE_DIR to Driver::GetResourcesPath. It seems the one exception in libclang CIndexer was an oversight. Move the use of CLANG_RESOURCE_DIR into GetResourcesPath and remove the optional argument to avoid this inconsistency between internal and external clients. Full diff: https://github.com/llvm/llvm-project/pull/103388.diff 5 Files Affected:
diff --git a/clang/include/clang/Driver/Driver.h b/clang/include/clang/Driver/Driver.h
index 04b46782467d6a..0382c8cb155e51 100644
--- a/clang/include/clang/Driver/Driver.h
+++ b/clang/include/clang/Driver/Driver.h
@@ -379,8 +379,7 @@ class Driver {
/// Takes the path to a binary that's either in bin/ or lib/ and returns
/// the path to clang's resource directory.
- static std::string GetResourcesPath(StringRef BinaryPath,
- StringRef CustomResourceDir = "");
+ static std::string GetResourcesPath(StringRef BinaryPath);
Driver(StringRef ClangExecutable, StringRef TargetTriple,
DiagnosticsEngine &Diags, std::string Title = "clang LLVM compiler",
diff --git a/clang/lib/Driver/Driver.cpp b/clang/lib/Driver/Driver.cpp
index 8e44d5afa40e05..dac8185693cf58 100644
--- a/clang/lib/Driver/Driver.cpp
+++ b/clang/lib/Driver/Driver.cpp
@@ -171,18 +171,18 @@ getHIPOffloadTargetTriple(const Driver &D, const ArgList &Args) {
}
// static
-std::string Driver::GetResourcesPath(StringRef BinaryPath,
- StringRef CustomResourceDir) {
+std::string Driver::GetResourcesPath(StringRef BinaryPath) {
// Since the resource directory is embedded in the module hash, it's important
// that all places that need it call this function, so that they get the
// exact same string ("a/../b/" and "b/" get different hashes, for example).
// Dir is bin/ or lib/, depending on where BinaryPath is.
- std::string Dir = std::string(llvm::sys::path::parent_path(BinaryPath));
-
+ StringRef Dir = llvm::sys::path::parent_path(BinaryPath);
SmallString<128> P(Dir);
- if (CustomResourceDir != "") {
- llvm::sys::path::append(P, CustomResourceDir);
+
+ StringRef ConfiguredResourceDir(CLANG_RESOURCE_DIR);
+ if (!ConfiguredResourceDir.empty()) {
+ llvm::sys::path::append(P, ConfiguredResourceDir);
} else {
// On Windows, libclang.dll is in bin/.
// On non-Windows, libclang.so/.dylib is in lib/.
@@ -239,7 +239,7 @@ Driver::Driver(StringRef ClangExecutable, StringRef TargetTriple,
#endif
// Compute the path to the resource directory.
- ResourceDir = GetResourcesPath(ClangExecutable, CLANG_RESOURCE_DIR);
+ ResourceDir = GetResourcesPath(ClangExecutable);
}
void Driver::setDriverMode(StringRef Value) {
diff --git a/clang/lib/Frontend/CompilerInvocation.cpp b/clang/lib/Frontend/CompilerInvocation.cpp
index f6b6c44a4cab6a..6962cb4f32b06a 100644
--- a/clang/lib/Frontend/CompilerInvocation.cpp
+++ b/clang/lib/Frontend/CompilerInvocation.cpp
@@ -3121,7 +3121,7 @@ std::string CompilerInvocation::GetResourcesPath(const char *Argv0,
void *MainAddr) {
std::string ClangExecutable =
llvm::sys::fs::getMainExecutable(Argv0, MainAddr);
- return Driver::GetResourcesPath(ClangExecutable, CLANG_RESOURCE_DIR);
+ return Driver::GetResourcesPath(ClangExecutable);
}
static void GenerateHeaderSearchArgs(const HeaderSearchOptions &Opts,
diff --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangHost.cpp b/lldb/source/Plugins/ExpressionParser/Clang/ClangHost.cpp
index 6064c02c7fd67d..6de851081598fd 100644
--- a/lldb/source/Plugins/ExpressionParser/Clang/ClangHost.cpp
+++ b/lldb/source/Plugins/ExpressionParser/Clang/ClangHost.cpp
@@ -53,7 +53,7 @@ static bool DefaultComputeClangResourceDirectory(FileSpec &lldb_shlib_spec,
std::string raw_path = lldb_shlib_spec.GetPath();
llvm::StringRef parent_dir = llvm::sys::path::parent_path(raw_path);
static const std::string clang_resource_path =
- clang::driver::Driver::GetResourcesPath("bin/lldb", CLANG_RESOURCE_DIR);
+ clang::driver::Driver::GetResourcesPath("bin/lldb");
static const llvm::StringRef kResourceDirSuffixes[] = {
// LLVM.org's build of LLDB uses the clang resource directory placed
diff --git a/lldb/unittests/Expression/ClangParserTest.cpp b/lldb/unittests/Expression/ClangParserTest.cpp
index 6f682f6c97fdb5..70fa49434666ff 100644
--- a/lldb/unittests/Expression/ClangParserTest.cpp
+++ b/lldb/unittests/Expression/ClangParserTest.cpp
@@ -43,7 +43,7 @@ TEST_F(ClangHostTest, ComputeClangResourceDirectory) {
std::string path_to_liblldb = "C:\\foo\\bar\\lib\\";
#endif
std::string path_to_clang_dir = clang::driver::Driver::GetResourcesPath(
- path_to_liblldb + "liblldb", CLANG_RESOURCE_DIR);
+ path_to_liblldb + "liblldb");
llvm::SmallString<256> path_to_clang_lib_dir_real;
llvm::sys::fs::real_path(path_to_clang_dir, path_to_clang_lib_dir_real);
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
When Clang is consumed as a library, the CLANG_RESOURCE_DIR definition is not exported from the CMake system, so external clients will be unable to compute the same resource dir as Clang itself would, because they don't know what to pass for the optional CustomResourceDir argument. All call sites except one would pass CLANG_RESOURCE_DIR to Driver::GetResourcesPath. It seems the one exception in libclang CIndexer was an oversight. Move the use of CLANG_RESOURCE_DIR into GetResourcesPath and remove the optional argument to avoid this inconsistency between internal and external clients.
kimgr
force-pushed
the
clang-resource-dir-consistency
branch
from
102517c to
3722d67
Compare
|
@nico You're the original author of this thing back in 2019, maybe you have some additional information. Please take a look if you have a chance. |
bulbazord
approved these changes
Aug 13, 2024
MaskRay
approved these changes
Aug 13, 2024
tuliom
approved these changes
Aug 20, 2024
There was a problem hiding this comment.
I confirmed these changes do solve the issue on Fedora when used together with include-what-you-use/include-what-you-use#1575 .
I created a repository with my builds of clang and include-what-you-use in https://copr.fedorainfracloud.org/coprs/tuliom/bz2299949/builds/
Thank you!
|
Do we need anything more to make progress with this PR? My own primary concerns are basically things I can't check myself:
Not sure who can shed light on these questions. |
@kimgr I have also looked for this and I haven't found an use-case where this would be appropriate. In fact, I think it's wrong to not take |
@kimgr Do you have committer permission? Would you like some help to get this merged? |
@tuliom I think it might be possible that a standalone tool would want to use its own executable path and a hardcoded relative |
Oh, no, I don't, I would need someone to merge this for me. It's still pretty early in the v.20 cycle, right, so maybe this is a good time to try and see if it sticks? |
|
Agreed. Let me merge this. |
|
Thanks! |
kimgr
added a commit
to kimgr/include-what-you-use
that referenced
this pull request
Oct 5, 2024
Clang can configure the relative location of its resource directory (containing built-in headers and compiler runtime libraries) using the CLANG_RESOURCE_DIR CMake option. Unfortunately that variable is not exported by the Clang CMake system, so clients like IWYU can't know post-facto what its value is for a built Clang, which makes it impossible (llvm-19) or hard (llvm-20) to compute the resource dir. See: * https://bugzilla.redhat.com/show_bug.cgi?id=2299949 * llvm/llvm-project#103388 for some more context. Add two variables to IWYU's CMake system, and use a more exhaustive procedure to compute an explicit resource dir for IWYU at runtime: * IWYU_RESOURCE_RELATIVE_TO: which executable to use as a base for the resource dir * IWYU_RESOURCE_DIR: a relative path from the base executable to the resource dir (same function as CLANG_RESOURCE_DIR) The IWYU build system will no longer copy the built-in headers to its build tree, but rather use the resource directory from the base Clang tree by default. Fixes issue include-what-you-use#100.
kimgr
added a commit
to kimgr/include-what-you-use
that referenced
this pull request
Oct 5, 2024
Clang can configure the relative location of its resource directory (containing built-in headers and compiler runtime libraries) using the CLANG_RESOURCE_DIR CMake option. Unfortunately that variable is not exported by the Clang CMake system, so clients like IWYU can't know post-facto what its value is for a built Clang, which makes it impossible (llvm-19) or hard (llvm-20) to compute the resource dir. See: * https://bugzilla.redhat.com/show_bug.cgi?id=2299949 * llvm/llvm-project#103388 for some more context. Add two variables to IWYU's CMake system, and use a more exhaustive procedure to compute an explicit resource dir for IWYU at runtime: * IWYU_RESOURCE_RELATIVE_TO: which executable to use as a base for the resource dir * IWYU_RESOURCE_DIR: a relative path from the base executable to the resource dir (same function as CLANG_RESOURCE_DIR) The IWYU build system will no longer copy the built-in headers to its build tree, but rather use the resource directory from the base Clang tree by default. Fixes issue include-what-you-use#100.
adrian-prantl
pushed a commit
to adrian-prantl/llvm-project
that referenced
this pull request
Oct 12, 2024
When Clang is consumed as a library, the CLANG_RESOURCE_DIR definition is not exported from the CMake system, so external clients will be unable to compute the same resource dir as Clang itself would, because they don't know what to pass for the optional CustomResourceDir argument. All call sites except one would pass CLANG_RESOURCE_DIR to Driver::GetResourcesPath. It seems the one exception in libclang CIndexer was an oversight. Move the use of CLANG_RESOURCE_DIR into GetResourcesPath and remove the optional argument to avoid this inconsistency between internal and external clients. (cherry picked from commit 924a7d8)
kimgr
added a commit
to include-what-you-use/include-what-you-use
that referenced
this pull request
Nov 5, 2024
Clang can configure the relative location of its resource directory (containing built-in headers and compiler runtime libraries) using the CLANG_RESOURCE_DIR CMake option. Unfortunately that variable is not exported by the Clang CMake system, so clients like IWYU can't know post-facto what its value is for a built Clang, which makes it impossible (llvm-19) or hard (llvm-20) to compute the resource dir. See: * https://bugzilla.redhat.com/show_bug.cgi?id=2299949 * llvm/llvm-project#103388 for some more context. Add two variables to IWYU's CMake system, and use a more exhaustive procedure to compute an explicit resource dir for IWYU at runtime: * IWYU_RESOURCE_RELATIVE_TO: which executable to use as a base for the resource dir * IWYU_RESOURCE_DIR: a relative path from the base executable to the resource dir (same function as CLANG_RESOURCE_DIR) The IWYU build system will no longer copy the built-in headers to its build tree, but rather use the resource directory from the base Clang tree by default. Fixes issue #100.
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.
When Clang is consumed as a library, the CLANG_RESOURCE_DIR definition is not exported from the CMake system, so external clients will be unable to compute the same resource dir as Clang itself would, because they don't know what to pass for the optional CustomResourceDir argument.
All call sites except one would pass CLANG_RESOURCE_DIR to Driver::GetResourcesPath. It seems the one exception in libclang CIndexer was an oversight.
Move the use of CLANG_RESOURCE_DIR into GetResourcesPath and remove the optional argument to avoid this inconsistency between internal and external clients.