Add support to find out resource dir and add it as compilation args

[kousikk]

Summary

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.

[jkorous]

clang/tools/clang-scan-deps/ClangScanDeps.cpp

71

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?

[arphaman]

@kousikk could we please add support for generic -extra-arg, which can then add -resource-dir like the other clang tools?

[kousikk]

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 most 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?

[Bigcheese]

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.

[MaskRay]

clang/lib/Tooling/ArgumentsAdjusters.cpp

43 ↗

(On Diff #225468)

Just delete /*unused*/

[Bigcheese]

I've added Manuel as a reviewer as this patch is also changing the tooling APIs.

[klimek]

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.

[kousikk]

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.

[Bigcheese]

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.

[kousikk]

@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.

[kousikk]

Forgot to run clang-format

[kousikk]

Ping

[Bigcheese]

lgtm with the changes to FindResourceDir.

clang/tools/clang-scan-deps/ClangScanDeps.cpp

50

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.

50

FindResourceDir should be findResourceDir.

75–79

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.

[kousikk]

Address review comments

clang/tools/clang-scan-deps/ClangScanDeps.cpp

75–79

Yup - I'm going to leave it as such for now.

[Bigcheese]

Thanks, you're good to commit.

clang/tools/clang-scan-deps/ClangScanDeps.cpp

 //===- ClangScanDeps.cpp - Implementation of clang-scan-deps --------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Tooling/CommonOptionsParser.h"
 #include "clang/Tooling/DependencyScanning/DependencyScanningService.h"
 #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"
 #include "llvm/Support/Signals.h"
 #include "llvm/Support/Threading.h"
 #include <mutex>
 #include <thread>
 
void applyLocked(llvm::function_ref<void(raw_ostream &OS)> Fn) {
     OS.flush();
   }
 
 private:
   std::mutex Lock;
   raw_ostream &OS;
 };
 
+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 &Args) {
+    if (Args.size() < 1)
+      return "";
+
+    const std::string &ClangBinaryPath = Args[0];
+    if (!llvm::sys::path::is_absolute(ClangBinaryPath))
+      return "";
+
+    const std::string &ClangBinaryName =
+        llvm::sys::path::filename(ClangBinaryPath);
+
+    std::unique_lock<std::mutex> LockGuard(CacheLock);
+    const auto &CachedResourceDir = Cache.find(ClangBinaryPath);
+    if (CachedResourceDir != Cache.end())
+      return CachedResourceDir->second;
+
+    std::vector<StringRef> 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<StringRef> 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<std::string, std::string> Cache;
+  std::mutex CacheLock;
+};
+
 llvm::cl::opt<bool> Help("h", llvm::cl::desc("Alias for -help"),
                          llvm::cl::Hidden);
 
 llvm::cl::OptionCategory DependencyScannerCategory("Tool options");
 
 static llvm::cl::opt<ScanningMode> ScanMode(
     "mode",
     llvm::cl::desc("The preprocessing mode used to compute the dependencies"),
int main(int argc, const char **argv) {
   }
 
   llvm::cl::PrintOptionValues();
 
   // The command options are rewritten to run Clang in preprocessor only mode.
   auto AdjustingCompilations =
       std::make_unique<tooling::ArgumentsAdjustingCompilations>(
           std::move(Compilations));
+  ResourceDirectoryCache ResourceDirCache;
   AdjustingCompilations->appendArgumentsAdjuster(
-      [](const tooling::CommandLineArguments &Args, StringRef FileName) {
+      [&ResourceDirCache](const tooling::CommandLineArguments &Args,
+                          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;
           for (auto It = Args.rbegin(); It != Args.rend(); ++It) {
             if (It != Args.rbegin()) {
               if (Args[Idx] == "-o")
                 LastO = Args[Idx + 1];
               if (Args[Idx] == "-MT")
                 HasMT = true;
               if (Args[Idx] == "-MQ")
                 HasMQ = true;
               if (Args[Idx] == "-MD")
                 HasMD = true;
+              if (Args[Idx] == "-resource-dir")
+                HasResourceDir = true;
             }
             --Idx;
           }
         }
         // If there's no -MT/-MQ Driver would add -MT with the value of the last
         // -o option.
         tooling::CommandLineArguments AdjustedArgs = Args;
         AdjustedArgs.push_back("-o");
AdjustingCompilations->appendArgumentsAdjuster(
             AdjustedArgs.push_back(FileName);
           }
         }
         AdjustedArgs.push_back("-Xclang");
         AdjustedArgs.push_back("-Eonly");
         AdjustedArgs.push_back("-Xclang");
         AdjustedArgs.push_back("-sys-header-deps");
         AdjustedArgs.push_back("-Wno-error");
+
+        if (!HasResourceDir) {
+          const std::string &ResourceDir =
+              ResourceDirCache.FindResourceDir(Args);
+          if (!ResourceDir.empty()) {
+            AdjustedArgs.push_back("-resource-dir");
+            AdjustedArgs.push_back(ResourceDir);
+          }
+        }
         return AdjustedArgs;
       });
   AdjustingCompilations->appendArgumentsAdjuster(
       tooling::getClangStripSerializeDiagnosticAdjuster());
 
   SharedStream Errs(llvm::errs());
   // Print out the dependency results to STDOUT by default.
   SharedStream DependencyOS(llvm::outs());