[Lldb-commits] [clang] [lldb] Use CLANG_RESOURCE_DIR more consistently (PR #103388)
https://github.com/kimgr created
https://github.com/llvm/llvm-project/pull/103388
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.
From 102517cd9d53695c9ae56135492bab09df9d90ee Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Kim=20Gr=C3=A4sman?=
Date: Mon, 5 Aug 2024 11:49:32 +0200
Subject: [PATCH] Use CLANG_RESOURCE_DIR more consistently
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.
---
clang/include/clang/Driver/Driver.h| 3 +--
clang/lib/Driver/Driver.cpp| 14 +++---
clang/lib/Frontend/CompilerInvocation.cpp | 2 +-
.../Plugins/ExpressionParser/Clang/ClangHost.cpp | 2 +-
lldb/unittests/Expression/ClangParserTest.cpp | 2 +-
5 files changed, 11 insertions(+), 12 deletions(-)
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/Clan
[Lldb-commits] [clang] [lldb] Use CLANG_RESOURCE_DIR more consistently (PR #103388)
https://github.com/kimgr updated
https://github.com/llvm/llvm-project/pull/103388
From 3722d673ee409c1320c9d66aa2f3f6568ffdfd77 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Kim=20Gr=C3=A4sman?=
Date: Mon, 5 Aug 2024 11:49:32 +0200
Subject: [PATCH] Use CLANG_RESOURCE_DIR more consistently
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.
---
clang/include/clang/Driver/Driver.h| 3 +--
clang/lib/Driver/Driver.cpp| 14 +++---
clang/lib/Frontend/CompilerInvocation.cpp | 2 +-
.../Plugins/ExpressionParser/Clang/ClangHost.cpp | 2 +-
lldb/unittests/Expression/ClangParserTest.cpp | 4 ++--
5 files changed, 12 insertions(+), 13 deletions(-)
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");
[Lldb-commits] [clang] [lldb] Use CLANG_RESOURCE_DIR more consistently (PR #103388)
kimgr wrote: @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. https://github.com/llvm/llvm-project/pull/103388 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [clang] [lldb] Use CLANG_RESOURCE_DIR more consistently (PR #103388)
kimgr wrote: Do we need anything more to make progress with this PR? My own primary concerns are basically things I can't check myself: * Is there an out-of-tree scenario where `CLANG_RESOURCE_DIR` needs to be replaced with something else at runtime, i.e. a real use-case for the optional `CustomResourceDir` optional argument I removed? * The only call in-tree whose behavior changed is https://github.com/llvm/llvm-project/blob/99b85cae628c1cc5641944290712cd84ccf1f6c8/clang/tools/libclang/CIndexer.cpp#L156. Is there any reason to think libclang should behave differently? Not sure who can shed light on these questions. https://github.com/llvm/llvm-project/pull/103388 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [clang] [lldb] Use CLANG_RESOURCE_DIR more consistently (PR #103388)
kimgr wrote: > > Is there an out-of-tree scenario where CLANG_RESOURCE_DIR needs to be > > replaced with something else at runtime, i.e. a real use-case for the > > optional CustomResourceDir optional argument I removed? > > @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 > `CLANG_RESOURCE_DIR` as part of the output from > `driver::Driver::GetResourcesPath()` because that would generate wrong > results for builds that do set `CLANG_RESOURCE_DIR`, e.g. Fedora's builds. @tuliom I think it might be possible that a standalone tool would want to use its own executable path and a hardcoded relative `CustomResourceDir`, but then maybe it would be better to just build the relevant path without calling `GetResourcesPath`. https://github.com/llvm/llvm-project/pull/103388 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [clang] [lldb] Use CLANG_RESOURCE_DIR more consistently (PR #103388)
kimgr wrote: > > Do we need anything more to make progress with this PR? > > @kimgr Do you have committer permission? Would you like some help to get this > merged? 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? https://github.com/llvm/llvm-project/pull/103388 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [clang] [lldb] Use CLANG_RESOURCE_DIR more consistently (PR #103388)
kimgr wrote: Thanks! https://github.com/llvm/llvm-project/pull/103388 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
