[PATCH] D69122: Add support to find out resource dir and add it as compilation args
This revision was automatically updated to reflect the committed changes.
Closed by commit rG26fa9e31f58a: Add support to find out resource dir and add
it as compilation args (authored by kousikk).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D69122/new/
https://reviews.llvm.org/D69122
Files:
clang/tools/clang-scan-deps/ClangScanDeps.cpp
Index: clang/tools/clang-scan-deps/ClangScanDeps.cpp
===
--- clang/tools/clang-scan-deps/ClangScanDeps.cpp
+++ clang/tools/clang-scan-deps/ClangScanDeps.cpp
@@ -13,6 +13,7 @@
#include "clang/Tooling/DependencyScanning/DependencyScanningWorker.h"
#include "clang/Tooling/JSONCompilationDatabase.h"
#include "llvm/Support/CommandLine.h"
+#include "llvm/Support/FileUtilities.h"
#include "llvm/Support/InitLLVM.h"
#include "llvm/Support/Program.h"
#include "llvm/Support/Signals.h"
@@ -39,6 +40,64 @@
raw_ostream
};
+class ResourceDirectoryCache {
+public:
+ /// findResourceDir finds the resource directory relative to the clang
+ /// compiler being used in Args, by running it with "-print-resource-dir"
+ /// option and cache the results for reuse. \returns resource directory path
+ /// associated with the given invocation command or empty string if the
+ /// compiler path is NOT an absolute path.
+ StringRef findResourceDir(const tooling::CommandLineArguments ) {
+if (Args.size() < 1)
+ return "";
+
+const std::string = Args[0];
+if (!llvm::sys::path::is_absolute(ClangBinaryPath))
+ return "";
+
+const std::string =
+llvm::sys::path::filename(ClangBinaryPath);
+
+std::unique_lock LockGuard(CacheLock);
+const auto = Cache.find(ClangBinaryPath);
+if (CachedResourceDir != Cache.end())
+ return CachedResourceDir->second;
+
+std::vector PrintResourceDirArgs{ClangBinaryName,
+"-print-resource-dir"};
+llvm::SmallString<64> OutputFile, ErrorFile;
+llvm::sys::fs::createTemporaryFile("print-resource-dir-output",
+ "" /*no-suffix*/, OutputFile);
+llvm::sys::fs::createTemporaryFile("print-resource-dir-error",
+ "" /*no-suffix*/, ErrorFile);
+llvm::FileRemover OutputRemover(OutputFile.c_str());
+llvm::FileRemover ErrorRemover(ErrorFile.c_str());
+llvm::Optional Redirects[] = {
+{""}, // Stdin
+StringRef(OutputFile),
+StringRef(ErrorFile),
+};
+if (const int RC = llvm::sys::ExecuteAndWait(
+ClangBinaryPath, PrintResourceDirArgs, {}, Redirects)) {
+ auto ErrorBuf = llvm::MemoryBuffer::getFile(ErrorFile.c_str());
+ llvm::errs() << ErrorBuf.get()->getBuffer();
+ return "";
+}
+
+auto OutputBuf = llvm::MemoryBuffer::getFile(OutputFile.c_str());
+if (!OutputBuf)
+ return "";
+StringRef Output = OutputBuf.get()->getBuffer().rtrim('\n');
+
+Cache[ClangBinaryPath] = Output.str();
+return Cache[ClangBinaryPath];
+ }
+
+private:
+ std::map Cache;
+ std::mutex CacheLock;
+};
+
llvm::cl::opt Help("h", llvm::cl::desc("Alias for -help"),
llvm::cl::Hidden);
@@ -169,12 +228,15 @@
auto AdjustingCompilations =
std::make_unique(
std::move(Compilations));
+ ResourceDirectoryCache ResourceDirCache;
AdjustingCompilations->appendArgumentsAdjuster(
- [](const tooling::CommandLineArguments , StringRef FileName) {
+ [](const tooling::CommandLineArguments ,
+ StringRef FileName) {
std::string LastO = "";
bool HasMT = false;
bool HasMQ = false;
bool HasMD = false;
+bool HasResourceDir = false;
// We need to find the last -o value.
if (!Args.empty()) {
std::size_t Idx = Args.size() - 1;
@@ -188,6 +250,8 @@
HasMQ = true;
if (Args[Idx] == "-MD")
HasMD = true;
+ if (Args[Idx] == "-resource-dir")
+HasResourceDir = true;
}
--Idx;
}
@@ -215,6 +279,15 @@
AdjustedArgs.push_back("-Xclang");
AdjustedArgs.push_back("-sys-header-deps");
AdjustedArgs.push_back("-Wno-error");
+
+if (!HasResourceDir) {
+ StringRef ResourceDir =
+ ResourceDirCache.findResourceDir(Args);
+ if (!ResourceDir.empty()) {
+AdjustedArgs.push_back("-resource-dir");
+AdjustedArgs.push_back(ResourceDir);
+ }
+}
return AdjustedArgs;
});
AdjustingCompilations->appendArgumentsAdjuster(
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D69122: Add support to find out resource dir and add it as compilation args
Bigcheese added a comment. Thanks, you're good to commit. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69122/new/ https://reviews.llvm.org/D69122 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D69122: Add support to find out resource dir and add it as compilation args
kousikk added inline comments.
Comment at: clang/tools/clang-scan-deps/ClangScanDeps.cpp:75-79
+llvm::Optional Redirects[] = {
+{""}, // Stdin
+StringRef(OutputFile),
+StringRef(ErrorFile),
+};
Bigcheese wrote:
> It would be nice if this didn't need to be a real file, but it looks like
> `llvm::sys::Execute` can only handle file paths.
Yup - I'm going to leave it as such for now.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D69122/new/
https://reviews.llvm.org/D69122
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D69122: Add support to find out resource dir and add it as compilation args
kousikk updated this revision to Diff 230508.
kousikk marked 4 inline comments as done.
kousikk added a comment.
Address review comments
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D69122/new/
https://reviews.llvm.org/D69122
Files:
clang/tools/clang-scan-deps/ClangScanDeps.cpp
Index: clang/tools/clang-scan-deps/ClangScanDeps.cpp
===
--- clang/tools/clang-scan-deps/ClangScanDeps.cpp
+++ clang/tools/clang-scan-deps/ClangScanDeps.cpp
@@ -13,6 +13,7 @@
#include "clang/Tooling/DependencyScanning/DependencyScanningWorker.h"
#include "clang/Tooling/JSONCompilationDatabase.h"
#include "llvm/Support/CommandLine.h"
+#include "llvm/Support/FileUtilities.h"
#include "llvm/Support/InitLLVM.h"
#include "llvm/Support/Program.h"
#include "llvm/Support/Signals.h"
@@ -39,6 +40,64 @@
raw_ostream
};
+class ResourceDirectoryCache {
+public:
+ /// findResourceDir finds the resource directory relative to the clang
+ /// compiler being used in Args, by running it with "-print-resource-dir"
+ /// option and cache the results for reuse. \returns resource directory path
+ /// associated with the given invocation command or empty string if the
+ /// compiler path is NOT an absolute path.
+ StringRef findResourceDir(const tooling::CommandLineArguments ) {
+if (Args.size() < 1)
+ return "";
+
+const std::string = Args[0];
+if (!llvm::sys::path::is_absolute(ClangBinaryPath))
+ return "";
+
+const std::string =
+llvm::sys::path::filename(ClangBinaryPath);
+
+std::unique_lock LockGuard(CacheLock);
+const auto = Cache.find(ClangBinaryPath);
+if (CachedResourceDir != Cache.end())
+ return CachedResourceDir->second;
+
+std::vector PrintResourceDirArgs{ClangBinaryName,
+"-print-resource-dir"};
+llvm::SmallString<64> OutputFile, ErrorFile;
+llvm::sys::fs::createTemporaryFile("print-resource-dir-output",
+ "" /*no-suffix*/, OutputFile);
+llvm::sys::fs::createTemporaryFile("print-resource-dir-error",
+ "" /*no-suffix*/, ErrorFile);
+llvm::FileRemover OutputRemover(OutputFile.c_str());
+llvm::FileRemover ErrorRemover(ErrorFile.c_str());
+llvm::Optional Redirects[] = {
+{""}, // Stdin
+StringRef(OutputFile),
+StringRef(ErrorFile),
+};
+if (const int RC = llvm::sys::ExecuteAndWait(
+ClangBinaryPath, PrintResourceDirArgs, {}, Redirects)) {
+ auto ErrorBuf = llvm::MemoryBuffer::getFile(ErrorFile.c_str());
+ llvm::errs() << ErrorBuf.get()->getBuffer();
+ return "";
+}
+
+auto OutputBuf = llvm::MemoryBuffer::getFile(OutputFile.c_str());
+if (!OutputBuf)
+ return "";
+StringRef Output = OutputBuf.get()->getBuffer().rtrim('\n');
+
+Cache[ClangBinaryPath] = Output.str();
+return Cache[ClangBinaryPath];
+ }
+
+private:
+ std::map Cache;
+ std::mutex CacheLock;
+};
+
llvm::cl::opt Help("h", llvm::cl::desc("Alias for -help"),
llvm::cl::Hidden);
@@ -169,12 +228,15 @@
auto AdjustingCompilations =
std::make_unique(
std::move(Compilations));
+ ResourceDirectoryCache ResourceDirCache;
AdjustingCompilations->appendArgumentsAdjuster(
- [](const tooling::CommandLineArguments , StringRef FileName) {
+ [](const tooling::CommandLineArguments ,
+ StringRef FileName) {
std::string LastO = "";
bool HasMT = false;
bool HasMQ = false;
bool HasMD = false;
+bool HasResourceDir = false;
// We need to find the last -o value.
if (!Args.empty()) {
std::size_t Idx = Args.size() - 1;
@@ -188,6 +250,8 @@
HasMQ = true;
if (Args[Idx] == "-MD")
HasMD = true;
+ if (Args[Idx] == "-resource-dir")
+HasResourceDir = true;
}
--Idx;
}
@@ -215,6 +279,15 @@
AdjustedArgs.push_back("-Xclang");
AdjustedArgs.push_back("-sys-header-deps");
AdjustedArgs.push_back("-Wno-error");
+
+if (!HasResourceDir) {
+ StringRef ResourceDir =
+ ResourceDirCache.findResourceDir(Args);
+ if (!ResourceDir.empty()) {
+AdjustedArgs.push_back("-resource-dir");
+AdjustedArgs.push_back(ResourceDir);
+ }
+}
return AdjustedArgs;
});
AdjustingCompilations->appendArgumentsAdjuster(
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D69122: Add support to find out resource dir and add it as compilation args
Bigcheese accepted this revision.
Bigcheese added a comment.
This revision is now accepted and ready to land.
lgtm with the changes to `FindResourceDir`.
Comment at: clang/tools/clang-scan-deps/ClangScanDeps.cpp:50
+ /// compiler path is NOT an absolute path.
+ std::string FindResourceDir(const tooling::CommandLineArguments ) {
+if (Args.size() < 1)
This doesn't need to return a `std::string` as the lifetime of the returned
string is the same as the lifetime of `ResourceDirectoryCache::Cache`. It can
be a `StringRef` instead.
Comment at: clang/tools/clang-scan-deps/ClangScanDeps.cpp:50
+ /// compiler path is NOT an absolute path.
+ std::string FindResourceDir(const tooling::CommandLineArguments ) {
+if (Args.size() < 1)
Bigcheese wrote:
> This doesn't need to return a `std::string` as the lifetime of the returned
> string is the same as the lifetime of `ResourceDirectoryCache::Cache`. It can
> be a `StringRef` instead.
`FindResourceDir` should be `findResourceDir`.
Comment at: clang/tools/clang-scan-deps/ClangScanDeps.cpp:75-79
+llvm::Optional Redirects[] = {
+{""}, // Stdin
+StringRef(OutputFile),
+StringRef(ErrorFile),
+};
It would be nice if this didn't need to be a real file, but it looks like
`llvm::sys::Execute` can only handle file paths.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D69122/new/
https://reviews.llvm.org/D69122
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D69122: Add support to find out resource dir and add it as compilation args
kousikk added a comment. Ping Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69122/new/ https://reviews.llvm.org/D69122 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D69122: Add support to find out resource dir and add it as compilation args
kousikk updated this revision to Diff 227601.
kousikk added a comment.
Forgot to run clang-format
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D69122/new/
https://reviews.llvm.org/D69122
Files:
clang/tools/clang-scan-deps/ClangScanDeps.cpp
Index: clang/tools/clang-scan-deps/ClangScanDeps.cpp
===
--- clang/tools/clang-scan-deps/ClangScanDeps.cpp
+++ clang/tools/clang-scan-deps/ClangScanDeps.cpp
@@ -12,6 +12,7 @@
#include "clang/Tooling/DependencyScanning/DependencyScanningTool.h"
#include "clang/Tooling/DependencyScanning/DependencyScanningWorker.h"
#include "clang/Tooling/JSONCompilationDatabase.h"
+#include "llvm/Support/FileUtilities.h"
#include "llvm/Support/InitLLVM.h"
#include "llvm/Support/Options.h"
#include "llvm/Support/Program.h"
@@ -39,6 +40,64 @@
raw_ostream
};
+class ResourceDirectoryCache {
+public:
+ /// FindResourceDir finds the resource directory relative to the clang
+ /// compiler being used in Args, by running it with "-print-resource-dir"
+ /// option and cache the results for reuse. \returns resource directory path
+ /// associated with the given invocation command or empty string if the
+ /// compiler path is NOT an absolute path.
+ std::string FindResourceDir(const tooling::CommandLineArguments ) {
+if (Args.size() < 1)
+ return "";
+
+const std::string = Args[0];
+if (!llvm::sys::path::is_absolute(ClangBinaryPath))
+ return "";
+
+const std::string =
+llvm::sys::path::filename(ClangBinaryPath);
+
+std::unique_lock LockGuard(CacheLock);
+const auto = Cache.find(ClangBinaryPath);
+if (CachedResourceDir != Cache.end())
+ return CachedResourceDir->second;
+
+std::vector PrintResourceDirArgs{ClangBinaryName,
+"-print-resource-dir"};
+llvm::SmallString<64> OutputFile, ErrorFile;
+llvm::sys::fs::createTemporaryFile("print-resource-dir-output",
+ "" /*no-suffix*/, OutputFile);
+llvm::sys::fs::createTemporaryFile("print-resource-dir-error",
+ "" /*no-suffix*/, ErrorFile);
+llvm::FileRemover OutputRemover(OutputFile.c_str());
+llvm::FileRemover ErrorRemover(ErrorFile.c_str());
+llvm::Optional Redirects[] = {
+{""}, // Stdin
+StringRef(OutputFile),
+StringRef(ErrorFile),
+};
+if (const int RC = llvm::sys::ExecuteAndWait(
+ClangBinaryPath, PrintResourceDirArgs, {}, Redirects)) {
+ auto ErrorBuf = llvm::MemoryBuffer::getFile(ErrorFile.c_str());
+ llvm::errs() << ErrorBuf.get()->getBuffer();
+ return "";
+}
+
+auto OutputBuf = llvm::MemoryBuffer::getFile(OutputFile.c_str());
+if (!OutputBuf)
+ return "";
+StringRef Output = OutputBuf.get()->getBuffer().rtrim('\n');
+
+Cache[ClangBinaryPath] = Output.str();
+return Cache[ClangBinaryPath];
+ }
+
+private:
+ std::map Cache;
+ std::mutex CacheLock;
+};
+
llvm::cl::opt Help("h", llvm::cl::desc("Alias for -help"),
llvm::cl::Hidden);
@@ -169,12 +228,15 @@
auto AdjustingCompilations =
std::make_unique(
std::move(Compilations));
+ ResourceDirectoryCache ResourceDirCache;
AdjustingCompilations->appendArgumentsAdjuster(
- [](const tooling::CommandLineArguments , StringRef FileName) {
+ [](const tooling::CommandLineArguments ,
+ StringRef FileName) {
std::string LastO = "";
bool HasMT = false;
bool HasMQ = false;
bool HasMD = false;
+bool HasResourceDir = false;
// We need to find the last -o value.
if (!Args.empty()) {
std::size_t Idx = Args.size() - 1;
@@ -188,6 +250,8 @@
HasMQ = true;
if (Args[Idx] == "-MD")
HasMD = true;
+ if (Args[Idx] == "-resource-dir")
+HasResourceDir = true;
}
--Idx;
}
@@ -215,6 +279,15 @@
AdjustedArgs.push_back("-Xclang");
AdjustedArgs.push_back("-sys-header-deps");
AdjustedArgs.push_back("-Wno-error");
+
+if (!HasResourceDir) {
+ const std::string =
+ ResourceDirCache.FindResourceDir(Args);
+ if (!ResourceDir.empty()) {
+AdjustedArgs.push_back("-resource-dir");
+AdjustedArgs.push_back(ResourceDir);
+ }
+}
return AdjustedArgs;
});
AdjustingCompilations->appendArgumentsAdjuster(
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D69122: Add support to find out resource dir and add it as compilation args
kousikk updated this revision to Diff 227600.
kousikk marked an inline comment as done.
kousikk added a comment.
@BigCheese - thanks, all the three points make sense!
> An alternative would be to only do FindResourceDir when given an absolute
> path to a compiler, instead of trying to guess. I think I prefer this
> approach, as if you have an installed clang in your path, there should be a
> clang-scan-deps next to it, which would use the same resource dir.
I really like the idea of adding resource-dir only if the given clang path is
an absolute path.
As you pointed out, this probably solves all the three concerns you mentioned.
Updated the patch with the change and reverted changes to clang/lib/Tooling.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D69122/new/
https://reviews.llvm.org/D69122
Files:
clang/tools/clang-scan-deps/ClangScanDeps.cpp
Index: clang/tools/clang-scan-deps/ClangScanDeps.cpp
===
--- clang/tools/clang-scan-deps/ClangScanDeps.cpp
+++ clang/tools/clang-scan-deps/ClangScanDeps.cpp
@@ -13,6 +13,7 @@
#include "clang/Tooling/DependencyScanning/DependencyScanningWorker.h"
#include "clang/Tooling/JSONCompilationDatabase.h"
#include "llvm/Support/InitLLVM.h"
+#include "llvm/Support/FileUtilities.h"
#include "llvm/Support/Options.h"
#include "llvm/Support/Program.h"
#include "llvm/Support/Signals.h"
@@ -39,6 +40,59 @@
raw_ostream
};
+class ResourceDirectoryCache {
+public:
+ /// FindResourceDir finds the resource directory relative to the clang compiler
+ /// being used in Args, by running it with "-print-resource-dir" option and
+ /// cache the results for reuse.
+ /// \returns resource directory path associated with the given invocation command or empty string if the compiler path is NOT an absolute path.
+ std::string FindResourceDir(const tooling::CommandLineArguments ) {
+if (Args.size() < 1)
+ return "";
+
+const std::string = Args[0];
+if (!llvm::sys::path::is_absolute(ClangBinaryPath))
+ return "";
+
+const std::string =
+llvm::sys::path::filename(ClangBinaryPath);
+
+std::unique_lock LockGuard(CacheLock);
+const auto& CachedResourceDir = Cache.find(ClangBinaryPath);
+if (CachedResourceDir != Cache.end())
+ return CachedResourceDir->second;
+
+std::vector PrintResourceDirArgs{ClangBinaryName, "-print-resource-dir"};
+llvm::SmallString<64> OutputFile, ErrorFile;
+llvm::sys::fs::createTemporaryFile("print-resource-dir-output", "" /*no-suffix*/, OutputFile);
+llvm::sys::fs::createTemporaryFile("print-resource-dir-error", ""/*no-suffix*/, ErrorFile);
+llvm::FileRemover OutputRemover(OutputFile.c_str());
+llvm::FileRemover ErrorRemover(ErrorFile.c_str());
+llvm::Optional Redirects[] = {
+ {""}, //Stdin
+ StringRef(OutputFile),
+ StringRef(ErrorFile),
+};
+if (const int RC = llvm::sys::ExecuteAndWait(ClangBinaryPath, PrintResourceDirArgs, {}, Redirects)) {
+ auto ErrorBuf = llvm::MemoryBuffer::getFile(ErrorFile.c_str());
+ llvm::errs() << ErrorBuf.get()->getBuffer();
+ return "";
+}
+
+auto OutputBuf = llvm::MemoryBuffer::getFile(OutputFile.c_str());
+if (!OutputBuf)
+ return "";
+StringRef Output = OutputBuf.get()->getBuffer().rtrim('\n');
+
+Cache[ClangBinaryPath] = Output.str();
+return Cache[ClangBinaryPath];
+ }
+
+private:
+ std::map Cache;
+ std::mutex CacheLock;
+};
+
llvm::cl::opt Help("h", llvm::cl::desc("Alias for -help"),
llvm::cl::Hidden);
@@ -169,12 +223,14 @@
auto AdjustingCompilations =
std::make_unique(
std::move(Compilations));
+ ResourceDirectoryCache ResourceDirCache;
AdjustingCompilations->appendArgumentsAdjuster(
- [](const tooling::CommandLineArguments , StringRef FileName) {
+ [](const tooling::CommandLineArguments , StringRef FileName) {
std::string LastO = "";
bool HasMT = false;
bool HasMQ = false;
bool HasMD = false;
+bool HasResourceDir = false;
// We need to find the last -o value.
if (!Args.empty()) {
std::size_t Idx = Args.size() - 1;
@@ -188,6 +244,8 @@
HasMQ = true;
if (Args[Idx] == "-MD")
HasMD = true;
+ if (Args[Idx] == "-resource-dir")
+HasResourceDir = true;
}
--Idx;
}
@@ -215,6 +273,14 @@
AdjustedArgs.push_back("-Xclang");
AdjustedArgs.push_back("-sys-header-deps");
AdjustedArgs.push_back("-Wno-error");
+
+if (!HasResourceDir) {
+ const std::string = ResourceDirCache.FindResourceDir(Args);
+ if (!ResourceDir.empty()) {
+AdjustedArgs.push_back("-resource-dir");
+AdjustedArgs.push_back(ResourceDir);
+ }
+}
return
[PATCH] D69122: Add support to find out resource dir and add it as compilation args
Bigcheese requested changes to this revision. Bigcheese added a comment. This revision now requires changes to proceed. So I agree solving this problem makes sense, but I have some issues with the current patch. - This defaults to doing the lookup. The test suite, and some real outputs, just uses `"clang ..."` as the command, not an absolute path. If we always do the lookup then this would be whatever `clang` is in `PATH`, not the `clang` we're testing. - While looking into the above, I think I discovered that the way `FindResourceDir ` constructs `ClangBinaryPath` is broken. It uses the `"directory"` entry from the compilation database, which is the working directory of where the compile occurred. It should be extremely rare for this ever to contain a `clang`, and even rarer for it to be the one that was intended. - This touches a lot of `clang/lib/Tooling` that I don't think it needs to. For the first issue we may just want to fix the tests to point to a specific `clang`, it's annoying, but doable. An alternative would be to //only// do `FindResourceDir` when given an absolute path to a compiler, instead of trying to guess. I think I prefer this approach, as if you have an installed `clang` in your path, there should be a `clang-scan-deps` next to it, which would use the same resource dir. For the second and third issues, I think you can just remove the `StringRef Directory` arg and use `FindInEnvPath` to get the correct `clang`. You shouldn't need it anymore, but you can replace `MakeAbsolutePath` with `llvm::sys::path::make_absolute`. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69122/new/ https://reviews.llvm.org/D69122 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D69122: Add support to find out resource dir and add it as compilation args
kousikk added a comment. I discussed with @klimek about this. The conclusion we arrived at was: Allowing override of `resource-dir` for all tools is not something we should do (for reasons that @klimek has outlined above). However, the dependency scanner tool can choose to override `resource-dir`, iff we are sure that it would work across different versions of clang. And my reasoning for why `resource-dir`'s behaviour when overridden in `clang-scan-deps` will remain the same across different versions of clang is: 1. The tool runs only the preprocessor. The preprocessor spec hasn't changed in a backwards incompatible way as far as I know. 2. In addition to (1), the tool doesn't use anything other than header files from `resource-dir`, for the purpose of dependency scanning from the preprocessor. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69122/new/ https://reviews.llvm.org/D69122 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D69122: Add support to find out resource dir and add it as compilation args
klimek added a comment. > since resource directory should be picked relative > to the path of the clang-compiler in the compilation command. The resource dir should be the resource dir that shipped with the clang source code that the *tool* was built with. We can think about the resource dir as files that should really have been compiled into the tool. If the compiler has a different version, its resource dir might break the tool. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69122/new/ https://reviews.llvm.org/D69122 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D69122: Add support to find out resource dir and add it as compilation args
Bigcheese added a reviewer: klimek. Bigcheese added a comment. I've added Manuel as a reviewer as this patch is also changing the tooling APIs. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69122/new/ https://reviews.llvm.org/D69122 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D69122: Add support to find out resource dir and add it as compilation args
MaskRay added inline comments.
Comment at: clang/lib/Tooling/ArgumentsAdjusters.cpp:43
ArgumentsAdjuster getClangStripOutputAdjuster() {
- return [](const CommandLineArguments , StringRef /*unused*/) {
+ return [](const CommandLineArguments , StringRef /*unused*/, StringRef
/*unused*/) {
CommandLineArguments AdjustedArgs;
Just delete ` /*unused*/`
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D69122/new/
https://reviews.llvm.org/D69122
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D69122: Add support to find out resource dir and add it as compilation args
Bigcheese added a comment. The current assumption is that the clang-scan-deps binary is the one that comes next to the clang binary you are using. There are lots of other differences between clang versions than just the resource-dir. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69122/new/ https://reviews.llvm.org/D69122 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D69122: Add support to find out resource dir and add it as compilation args
kousikk added a comment.
> I haven't looked into this in detail but it feels kind of wasteful to start a
> process just to get this value.
Right, that's why I cache this information. So multiple compile commands
sharing the same compiler process will trigger at 1 subprocess and then
subsequently use the cached information.
> Can't we just expose it in some of the clang libs we already link against?
The problem is, the value of `resource-dir` depends on `CLANG_VERSION` and
`ARCH` #define's, that's set when the clang binary is built. So lets say you
use `/home/kousikk/clang/7.0/bin/clang`, then if your compilation database is:
[
{ "command": "/home/kousikk/clang/7.0/bin/clang -c /home/kousikk/test.cpp",
"file": "/home/kousikk/test.cpp",
"directory": "/home/kousikk/"
}
]
then the value of `-resource-dir` for compilation of `test.cpp` must be
something like `/home/kousikk/clang/7.0/lib64/clang/7.0.0/`. Any libs used as
part of building `clang-scan-deps` itself is going to point to its own
resource-directory which is NOT the behaviour we want.
---
> could we please add support for generic -extra-arg, which can then add
> -resource-dir like the other clang tools?
@arphaman would this be an option to `clang-scan-deps`? If so it might not work
since commands in the compilation-database can be using more than 1 version of
clang?
In addition, every project on which we use scan-deps will inturn have to
specify this extra argument - wouldn't it be better if they didn't have to do
that?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D69122/new/
https://reviews.llvm.org/D69122
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D69122: Add support to find out resource dir and add it as compilation args
arphaman added a comment. @kousikk could we please add support for generic `-extra-arg`, which can then add `-resource-dir` like the other clang tools? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69122/new/ https://reviews.llvm.org/D69122 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D69122: Add support to find out resource dir and add it as compilation args
jkorous added inline comments.
Comment at: clang/tools/clang-scan-deps/ClangScanDeps.cpp:70
+};
+if (const int RC = llvm::sys::ExecuteAndWait(ClangBinaryPath,
PrintResourceDirArgs, {}, Redirects)) {
+ auto ErrorBuf = llvm::MemoryBuffer::getFile(ErrorFile.c_str());
I haven't looked into this in detail but it feels kind of wasteful to start a
process just to get this value. Can't we just expose it in some of the clang
libs we already link against?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D69122/new/
https://reviews.llvm.org/D69122
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D69122: Add support to find out resource dir and add it as compilation args
kousikk created this revision.
kousikk added reviewers: arphaman, Bigcheese, jkorous, dexonsmith.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
kousikk edited the summary of this revision.
kousikk edited the summary of this revision.
If -resource-dir is not specified as part of the compilation command, then by
default
clang-scan-deps picks up a directory relative to its own path as
resource-directory.
This is probably not the right behavior - since resource directory should be
picked relative
to the path of the clang-compiler in the compilation command.
This patch adds support for it along with a cache to store the resource-dir
paths based on
compiler paths.
Notes:
1. "-resource-dir" is a behavior that's specific to clang, gcc does not have
that flag. That's why if I'm not able to find a resource-dir, I quietly ignore
it.
2. Should I also use the mtime of the compiler in the cache? I think its not
strictly necessary since we assume the filesystem is immutable.
3. From my testing, this does not regress performance.
4. Will try to get this tested on Windows.
But basically the problem that this patch is trying to solve is, clients might
not always want to specify
"-resource-dir" in their compile commands, so scan-deps must auto-infer it
correctly.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D69122
Files:
clang/include/clang/Tooling/ArgumentsAdjusters.h
clang/lib/Tooling/ArgumentsAdjusters.cpp
clang/lib/Tooling/CommonOptionsParser.cpp
clang/lib/Tooling/Tooling.cpp
clang/tools/clang-scan-deps/ClangScanDeps.cpp
Index: clang/tools/clang-scan-deps/ClangScanDeps.cpp
===
--- clang/tools/clang-scan-deps/ClangScanDeps.cpp
+++ clang/tools/clang-scan-deps/ClangScanDeps.cpp
@@ -12,6 +12,7 @@
#include "clang/Tooling/DependencyScanning/DependencyScanningWorker.h"
#include "clang/Tooling/JSONCompilationDatabase.h"
#include "llvm/Support/InitLLVM.h"
+#include "llvm/Support/FileUtilities.h"
#include "llvm/Support/Options.h"
#include "llvm/Support/Program.h"
#include "llvm/Support/Signals.h"
@@ -38,6 +39,64 @@
raw_ostream
};
+class ResourceDirectoryCache {
+public:
+ /// FindResourceDir finds the resource directory relative to the clang compiler
+ /// being used in Args, by running it with "-print-resource-dir" option and
+ /// cache the results for reuse.
+ /// \returns resource directory path associated with the given invocation command.
+ std::string FindResourceDir(const tooling::CommandLineArguments , StringRef Directory) {
+if (Args.size() < 1)
+ return "";
+
+const std::string = MakeAbsolutePath(Directory, Args[0]);
+
+std::unique_lock LockGuard(CacheLock);
+const auto& CachedResourceDir = Cache.find(ClangBinaryPath);
+if (CachedResourceDir != Cache.end())
+ return CachedResourceDir->second;
+
+std::vector PrintResourceDirArgs{"clang", "-print-resource-dir"};
+llvm::SmallString<64> OutputFile, ErrorFile;
+llvm::sys::fs::createTemporaryFile("print-resource-dir-output", "" /*no-suffix*/, OutputFile);
+llvm::sys::fs::createTemporaryFile("print-resource-dir-error", ""/*no-suffix*/, ErrorFile);
+llvm::FileRemover OutputRemover(OutputFile.c_str());
+llvm::FileRemover ErrorRemover(ErrorFile.c_str());
+llvm::Optional Redirects[] = {
+ {""}, //Stdin
+ StringRef(OutputFile),
+ StringRef(ErrorFile),
+};
+if (const int RC = llvm::sys::ExecuteAndWait(ClangBinaryPath, PrintResourceDirArgs, {}, Redirects)) {
+ auto ErrorBuf = llvm::MemoryBuffer::getFile(ErrorFile.c_str());
+ llvm::errs() << ErrorBuf.get()->getBuffer();
+ return "";
+}
+
+auto OutputBuf = llvm::MemoryBuffer::getFile(OutputFile.c_str());
+if (!OutputBuf)
+ return "";
+StringRef Output = OutputBuf.get()->getBuffer().rtrim('\n');
+
+Cache[ClangBinaryPath] = Output.str();
+return Cache[ClangBinaryPath];
+ }
+
+private:
+ /// \returns the absolute path formed by joining the current directory with the given path.
+ std::string MakeAbsolutePath(StringRef CurrentDir, StringRef Path) {
+if (Path.empty())
+ return "";
+llvm::SmallString<256> InitialDirectory(CurrentDir);
+llvm::SmallString<256> AbsolutePath(Path);
+llvm::sys::fs::make_absolute(InitialDirectory, AbsolutePath);
+return AbsolutePath.str();
+ }
+
+ std::map Cache;
+ std::mutex CacheLock;
+};
+
/// The high-level implementation of the dependency discovery tool that runs on
/// an individual worker thread.
class DependencyScanningTool {
@@ -218,12 +277,14 @@
auto AdjustingCompilations =
std::make_unique(
std::move(Compilations));
+ ResourceDirectoryCache ResourceDirCache;
AdjustingCompilations->appendArgumentsAdjuster(
- [](const tooling::CommandLineArguments , StringRef FileName) {
+ [](const tooling::CommandLineArguments , StringRef FileName, StringRef Directory) {
