[PATCH] D70351: [clang][WIP][clang-scan-deps] Add an experimental C API.

[Kousik Kumar via Phabricator via cfe-commits]

kousikk added a comment.

Thanks!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70351/new/

https://reviews.llvm.org/D70351



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70351: [clang][WIP][clang-scan-deps] Add an experimental C API.

[Kousik Kumar via Phabricator via cfe-commits]

kousikk added a comment.

@Bigcheese wondering if there are things with respect to testing we can do, to 
help the patch move forward :)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70351/new/

https://reviews.llvm.org/D70351



___
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

[Kousik Kumar via Phabricator via cfe-commits]

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] D70351: [clang][WIP][clang-scan-deps] Add an experimental C API.

[Kousik Kumar via Phabricator via cfe-commits]

kousikk added inline comments.



Comment at: clang/include/clang-c/Dependencies.h:146
+ */
+typedef struct CXOpaqueDependencyScannerWorker *CXDependencyScannerWorker;
+

It would be simpler if the clients didn't have to worry about the worker?
As far as a user of this lib is concerned, they need to create a shared cache, 
then call scanDeps() in some form with the shared cache and other inputs.

Can we hide the worker inside the implementation? So essentially we create a 
new worker for every invocation (that's assuming worker setup is cheap).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70351/new/

https://reviews.llvm.org/D70351



___
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

[Kousik Kumar via Phabricator via cfe-commits]

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

[Kousik Kumar via Phabricator via cfe-commits]

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

[Kousik Kumar via Phabricator via cfe-commits]

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

[Kousik Kumar via Phabricator via cfe-commits]

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

[Kousik Kumar via Phabricator via cfe-commits]

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

[Kousik Kumar via Phabricator via cfe-commits]

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] D69420: [clang][clang-scan-deps] Add support for extracting full module dependencies.

[Kousik Kumar via Phabricator via cfe-commits]

kousikk added a comment.

A few nit comments




Comment at: 
clang/include/clang/Tooling/DependencyScanning/DependencyScanningService.h:34
+/// The format that is output by the dependency scanner.
+enum class ScanningFormat {
+  /// This is the Makefile compatible dep format.

"ScanningOutputFormat" or "DependencyOutputFormat" ?



Comment at: 
clang/include/clang/Tooling/DependencyScanning/DependencyScanningService.h:35
+enum class ScanningFormat {
+  /// This is the Makefile compatible dep format.
+  Make,

IIUC, this format would NOT include module dependencies right?

If so, can you explicitly state that in the comment?



Comment at: clang/lib/Tooling/DependencyScanning/DependencyScanningTool.cpp:94
+
+void printDependencies(std::string , StringRef MainFile) {
+  // Sort the modules by name to get a deterministic order.

Should output arguments be at the end?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69420/new/

https://reviews.llvm.org/D69420



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69186: Refactor DependencyScanningTool to its own file

[Kousik Kumar via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGfb042b094fda: Refactor DependencyScanningTool to its own 
file (authored by kousikk).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69186/new/

https://reviews.llvm.org/D69186

Files:
  clang/include/clang/Tooling/DependencyScanning/DependencyScanningTool.h
  clang/lib/Tooling/DependencyScanning/CMakeLists.txt
  clang/lib/Tooling/DependencyScanning/DependencyScanningTool.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
@@ -9,6 +9,7 @@
 #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/InitLLVM.h"
@@ -38,101 +39,6 @@
   raw_ostream 
 };
 
-/// The high-level implementation of the dependency discovery tool that runs on
-/// an individual worker thread.
-class DependencyScanningTool {
-public:
-  /// Construct a dependency scanning tool.
-  ///
-  /// \param Compilations The reference to the compilation database that's
-  /// used by the clang tool.
-  DependencyScanningTool(DependencyScanningService ,
- const tooling::CompilationDatabase ,
- SharedStream , SharedStream )
-  : Worker(Service), Compilations(Compilations), OS(OS), Errs(Errs) {}
-
-  /// Print out the dependency information into a string using the dependency
-  /// file format that is specified in the options (-MD is the default) and
-  /// return it.
-  ///
-  /// \returns A \c StringError with the diagnostic output if clang errors
-  /// occurred, dependency file contents otherwise.
-  llvm::Expected getDependencyFile(const std::string ,
-StringRef CWD) {
-/// Prints out all of the gathered dependencies into a string.
-class DependencyPrinterConsumer : public DependencyConsumer {
-public:
-  void handleFileDependency(const DependencyOutputOptions ,
-StringRef File) override {
-if (!this->Opts)
-  this->Opts = std::make_unique(Opts);
-Dependencies.push_back(File);
-  }
-
-  void printDependencies(std::string ) {
-if (!Opts)
-  return;
-
-class DependencyPrinter : public DependencyFileGenerator {
-public:
-  DependencyPrinter(DependencyOutputOptions ,
-ArrayRef Dependencies)
-  : DependencyFileGenerator(Opts) {
-for (const auto  : Dependencies)
-  addDependency(Dep);
-  }
-
-  void printDependencies(std::string ) {
-llvm::raw_string_ostream OS(S);
-outputDependencyFile(OS);
-  }
-};
-
-DependencyPrinter Generator(*Opts, Dependencies);
-Generator.printDependencies(S);
-  }
-
-private:
-  std::unique_ptr Opts;
-  std::vector Dependencies;
-};
-
-DependencyPrinterConsumer Consumer;
-auto Result =
-Worker.computeDependencies(Input, CWD, Compilations, Consumer);
-if (Result)
-  return std::move(Result);
-std::string Output;
-Consumer.printDependencies(Output);
-return Output;
-  }
-
-  /// Computes the dependencies for the given file and prints them out.
-  ///
-  /// \returns True on error.
-  bool runOnFile(const std::string , StringRef CWD) {
-auto MaybeFile = getDependencyFile(Input, CWD);
-if (!MaybeFile) {
-  llvm::handleAllErrors(
-  MaybeFile.takeError(), [this, ](llvm::StringError ) {
-Errs.applyLocked([&](raw_ostream ) {
-  OS << "Error while scanning dependencies for " << Input << ":\n";
-  OS << Err.getMessage();
-});
-  });
-  return true;
-}
-OS.applyLocked([&](raw_ostream ) { OS << *MaybeFile; });
-return false;
-  }
-
-private:
-  DependencyScanningWorker Worker;
-  const tooling::CompilationDatabase 
-  SharedStream 
-  SharedStream 
-};
-
 llvm::cl::opt Help("h", llvm::cl::desc("Alias for -help"),
  llvm::cl::Hidden);
 
@@ -191,6 +97,27 @@
   return ObjFileName.str();
 }
 
+/// Takes the result of a dependency scan and prints error / dependency files
+/// based on the result.
+///
+/// \returns True on error.
+static bool handleDependencyToolResult(const std::string ,
+   llvm::Expected ,
+   SharedStream , SharedStream ) {
+  if (!MaybeFile) {
+

[PATCH] D69186: Refactor DependencyScanningTool to its own file

[Kousik Kumar via Phabricator via cfe-commits]

kousikk updated this revision to Diff 225994.
kousikk added a comment.

Run clang format


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69186/new/

https://reviews.llvm.org/D69186

Files:
  clang/include/clang/Tooling/DependencyScanning/DependencyScanningTool.h
  clang/lib/Tooling/DependencyScanning/CMakeLists.txt
  clang/lib/Tooling/DependencyScanning/DependencyScanningTool.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
@@ -9,6 +9,7 @@
 #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/InitLLVM.h"
@@ -38,101 +39,6 @@
   raw_ostream 
 };
 
-/// The high-level implementation of the dependency discovery tool that runs on
-/// an individual worker thread.
-class DependencyScanningTool {
-public:
-  /// Construct a dependency scanning tool.
-  ///
-  /// \param Compilations The reference to the compilation database that's
-  /// used by the clang tool.
-  DependencyScanningTool(DependencyScanningService ,
- const tooling::CompilationDatabase ,
- SharedStream , SharedStream )
-  : Worker(Service), Compilations(Compilations), OS(OS), Errs(Errs) {}
-
-  /// Print out the dependency information into a string using the dependency
-  /// file format that is specified in the options (-MD is the default) and
-  /// return it.
-  ///
-  /// \returns A \c StringError with the diagnostic output if clang errors
-  /// occurred, dependency file contents otherwise.
-  llvm::Expected getDependencyFile(const std::string ,
-StringRef CWD) {
-/// Prints out all of the gathered dependencies into a string.
-class DependencyPrinterConsumer : public DependencyConsumer {
-public:
-  void handleFileDependency(const DependencyOutputOptions ,
-StringRef File) override {
-if (!this->Opts)
-  this->Opts = std::make_unique(Opts);
-Dependencies.push_back(File);
-  }
-
-  void printDependencies(std::string ) {
-if (!Opts)
-  return;
-
-class DependencyPrinter : public DependencyFileGenerator {
-public:
-  DependencyPrinter(DependencyOutputOptions ,
-ArrayRef Dependencies)
-  : DependencyFileGenerator(Opts) {
-for (const auto  : Dependencies)
-  addDependency(Dep);
-  }
-
-  void printDependencies(std::string ) {
-llvm::raw_string_ostream OS(S);
-outputDependencyFile(OS);
-  }
-};
-
-DependencyPrinter Generator(*Opts, Dependencies);
-Generator.printDependencies(S);
-  }
-
-private:
-  std::unique_ptr Opts;
-  std::vector Dependencies;
-};
-
-DependencyPrinterConsumer Consumer;
-auto Result =
-Worker.computeDependencies(Input, CWD, Compilations, Consumer);
-if (Result)
-  return std::move(Result);
-std::string Output;
-Consumer.printDependencies(Output);
-return Output;
-  }
-
-  /// Computes the dependencies for the given file and prints them out.
-  ///
-  /// \returns True on error.
-  bool runOnFile(const std::string , StringRef CWD) {
-auto MaybeFile = getDependencyFile(Input, CWD);
-if (!MaybeFile) {
-  llvm::handleAllErrors(
-  MaybeFile.takeError(), [this, ](llvm::StringError ) {
-Errs.applyLocked([&](raw_ostream ) {
-  OS << "Error while scanning dependencies for " << Input << ":\n";
-  OS << Err.getMessage();
-});
-  });
-  return true;
-}
-OS.applyLocked([&](raw_ostream ) { OS << *MaybeFile; });
-return false;
-  }
-
-private:
-  DependencyScanningWorker Worker;
-  const tooling::CompilationDatabase 
-  SharedStream 
-  SharedStream 
-};
-
 llvm::cl::opt Help("h", llvm::cl::desc("Alias for -help"),
  llvm::cl::Hidden);
 
@@ -191,6 +97,27 @@
   return ObjFileName.str();
 }
 
+/// Takes the result of a dependency scan and prints error / dependency files
+/// based on the result.
+///
+/// \returns True on error.
+static bool handleDependencyToolResult(const std::string ,
+   llvm::Expected ,
+   SharedStream , SharedStream ) {
+  if (!MaybeFile) {
+llvm::handleAllErrors(
+MaybeFile.takeError(), [, ](llvm::StringError ) {
+  

[PATCH] D69090: [Try 2] Include sanitize blacklist and other extra deps as part of scan-deps output

[Kousik Kumar via Phabricator via cfe-commits]

kousikk added a comment.

I was able to test this on a Windows machine. The `ExtraDeps` are already being 
added to the dependency output (this code does that - 
https://github.com/llvm/llvm-project/blob/master/clang/lib/Frontend/DependencyFile.cpp#L188).
 My suspicion for why I wasn't seeing them previously was that these ExtraDeps 
are added as relative paths and my consumer code (which integrates with 
scan-deps) was ignoring relative paths. I can handle that in the consumer code 
- I'll leave the behaviour of the tool as it is.

Sorry about the back and forth on this. I'm closing this out.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69090/new/

https://reviews.llvm.org/D69090



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69186: Refactor DependencyScanningTool to its own file

[Kousik Kumar via Phabricator via cfe-commits]

kousikk created this revision.
kousikk added reviewers: arphaman, jkorous, Bigcheese, dexonsmith.
Herald added subscribers: cfe-commits, mgorny.
Herald added a project: clang.
kousikk updated this revision to Diff 225667.
kousikk added a comment.

Lint fixes


There's no behavior change - just moving DependencyScanningTool to its own file 
since this tool can be reused across both clang-scan-deps binary and an 
interface
exposed as part of libClang APIs.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D69186

Files:
  clang/include/clang/Tooling/DependencyScanning/DependencyScanningTool.h
  clang/lib/Tooling/DependencyScanning/CMakeLists.txt
  clang/lib/Tooling/DependencyScanning/DependencyScanningTool.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
@@ -10,6 +10,7 @@
 #include "clang/Tooling/CommonOptionsParser.h"
 #include "clang/Tooling/DependencyScanning/DependencyScanningService.h"
 #include "clang/Tooling/DependencyScanning/DependencyScanningWorker.h"
+#include "clang/Tooling/DependencyScanning/DependencyScanningTool.h"
 #include "clang/Tooling/JSONCompilationDatabase.h"
 #include "llvm/Support/InitLLVM.h"
 #include "llvm/Support/Options.h"
@@ -38,101 +39,6 @@
   raw_ostream 
 };
 
-/// The high-level implementation of the dependency discovery tool that runs on
-/// an individual worker thread.
-class DependencyScanningTool {
-public:
-  /// Construct a dependency scanning tool.
-  ///
-  /// \param Compilations The reference to the compilation database that's
-  /// used by the clang tool.
-  DependencyScanningTool(DependencyScanningService ,
- const tooling::CompilationDatabase ,
- SharedStream , SharedStream )
-  : Worker(Service), Compilations(Compilations), OS(OS), Errs(Errs) {}
-
-  /// Print out the dependency information into a string using the dependency
-  /// file format that is specified in the options (-MD is the default) and
-  /// return it.
-  ///
-  /// \returns A \c StringError with the diagnostic output if clang errors
-  /// occurred, dependency file contents otherwise.
-  llvm::Expected getDependencyFile(const std::string ,
-StringRef CWD) {
-/// Prints out all of the gathered dependencies into a string.
-class DependencyPrinterConsumer : public DependencyConsumer {
-public:
-  void handleFileDependency(const DependencyOutputOptions ,
-StringRef File) override {
-if (!this->Opts)
-  this->Opts = std::make_unique(Opts);
-Dependencies.push_back(File);
-  }
-
-  void printDependencies(std::string ) {
-if (!Opts)
-  return;
-
-class DependencyPrinter : public DependencyFileGenerator {
-public:
-  DependencyPrinter(DependencyOutputOptions ,
-ArrayRef Dependencies)
-  : DependencyFileGenerator(Opts) {
-for (const auto  : Dependencies)
-  addDependency(Dep);
-  }
-
-  void printDependencies(std::string ) {
-llvm::raw_string_ostream OS(S);
-outputDependencyFile(OS);
-  }
-};
-
-DependencyPrinter Generator(*Opts, Dependencies);
-Generator.printDependencies(S);
-  }
-
-private:
-  std::unique_ptr Opts;
-  std::vector Dependencies;
-};
-
-DependencyPrinterConsumer Consumer;
-auto Result =
-Worker.computeDependencies(Input, CWD, Compilations, Consumer);
-if (Result)
-  return std::move(Result);
-std::string Output;
-Consumer.printDependencies(Output);
-return Output;
-  }
-
-  /// Computes the dependencies for the given file and prints them out.
-  ///
-  /// \returns True on error.
-  bool runOnFile(const std::string , StringRef CWD) {
-auto MaybeFile = getDependencyFile(Input, CWD);
-if (!MaybeFile) {
-  llvm::handleAllErrors(
-  MaybeFile.takeError(), [this, ](llvm::StringError ) {
-Errs.applyLocked([&](raw_ostream ) {
-  OS << "Error while scanning dependencies for " << Input << ":\n";
-  OS << Err.getMessage();
-});
-  });
-  return true;
-}
-OS.applyLocked([&](raw_ostream ) { OS << *MaybeFile; });
-return false;
-  }
-
-private:
-  DependencyScanningWorker Worker;
-  const tooling::CompilationDatabase 
-  SharedStream 
-  SharedStream 
-};
-
 llvm::cl::opt Help("h", llvm::cl::desc("Alias for -help"),
  llvm::cl::Hidden);
 
@@ -191,6 +97,28 @@
   return ObjFileName.str();
 }
 
+/// Takes the result of a dependency scan and prints error / dependency files
+/// based on the result.
+///
+/// \returns True on error.
+static bool 

[PATCH] D69186: Refactor DependencyScanningTool to its own file

[Kousik Kumar via Phabricator via cfe-commits]

kousikk updated this revision to Diff 225667.
kousikk added a comment.

Lint fixes


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69186/new/

https://reviews.llvm.org/D69186

Files:
  clang/include/clang/Tooling/DependencyScanning/DependencyScanningTool.h
  clang/lib/Tooling/DependencyScanning/CMakeLists.txt
  clang/lib/Tooling/DependencyScanning/DependencyScanningTool.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
@@ -10,6 +10,7 @@
 #include "clang/Tooling/CommonOptionsParser.h"
 #include "clang/Tooling/DependencyScanning/DependencyScanningService.h"
 #include "clang/Tooling/DependencyScanning/DependencyScanningWorker.h"
+#include "clang/Tooling/DependencyScanning/DependencyScanningTool.h"
 #include "clang/Tooling/JSONCompilationDatabase.h"
 #include "llvm/Support/InitLLVM.h"
 #include "llvm/Support/Options.h"
@@ -38,101 +39,6 @@
   raw_ostream 
 };
 
-/// The high-level implementation of the dependency discovery tool that runs on
-/// an individual worker thread.
-class DependencyScanningTool {
-public:
-  /// Construct a dependency scanning tool.
-  ///
-  /// \param Compilations The reference to the compilation database that's
-  /// used by the clang tool.
-  DependencyScanningTool(DependencyScanningService ,
- const tooling::CompilationDatabase ,
- SharedStream , SharedStream )
-  : Worker(Service), Compilations(Compilations), OS(OS), Errs(Errs) {}
-
-  /// Print out the dependency information into a string using the dependency
-  /// file format that is specified in the options (-MD is the default) and
-  /// return it.
-  ///
-  /// \returns A \c StringError with the diagnostic output if clang errors
-  /// occurred, dependency file contents otherwise.
-  llvm::Expected getDependencyFile(const std::string ,
-StringRef CWD) {
-/// Prints out all of the gathered dependencies into a string.
-class DependencyPrinterConsumer : public DependencyConsumer {
-public:
-  void handleFileDependency(const DependencyOutputOptions ,
-StringRef File) override {
-if (!this->Opts)
-  this->Opts = std::make_unique(Opts);
-Dependencies.push_back(File);
-  }
-
-  void printDependencies(std::string ) {
-if (!Opts)
-  return;
-
-class DependencyPrinter : public DependencyFileGenerator {
-public:
-  DependencyPrinter(DependencyOutputOptions ,
-ArrayRef Dependencies)
-  : DependencyFileGenerator(Opts) {
-for (const auto  : Dependencies)
-  addDependency(Dep);
-  }
-
-  void printDependencies(std::string ) {
-llvm::raw_string_ostream OS(S);
-outputDependencyFile(OS);
-  }
-};
-
-DependencyPrinter Generator(*Opts, Dependencies);
-Generator.printDependencies(S);
-  }
-
-private:
-  std::unique_ptr Opts;
-  std::vector Dependencies;
-};
-
-DependencyPrinterConsumer Consumer;
-auto Result =
-Worker.computeDependencies(Input, CWD, Compilations, Consumer);
-if (Result)
-  return std::move(Result);
-std::string Output;
-Consumer.printDependencies(Output);
-return Output;
-  }
-
-  /// Computes the dependencies for the given file and prints them out.
-  ///
-  /// \returns True on error.
-  bool runOnFile(const std::string , StringRef CWD) {
-auto MaybeFile = getDependencyFile(Input, CWD);
-if (!MaybeFile) {
-  llvm::handleAllErrors(
-  MaybeFile.takeError(), [this, ](llvm::StringError ) {
-Errs.applyLocked([&](raw_ostream ) {
-  OS << "Error while scanning dependencies for " << Input << ":\n";
-  OS << Err.getMessage();
-});
-  });
-  return true;
-}
-OS.applyLocked([&](raw_ostream ) { OS << *MaybeFile; });
-return false;
-  }
-
-private:
-  DependencyScanningWorker Worker;
-  const tooling::CompilationDatabase 
-  SharedStream 
-  SharedStream 
-};
-
 llvm::cl::opt Help("h", llvm::cl::desc("Alias for -help"),
  llvm::cl::Hidden);
 
@@ -191,6 +97,28 @@
   return ObjFileName.str();
 }
 
+/// Takes the result of a dependency scan and prints error / dependency files
+/// based on the result.
+///
+/// \returns True on error.
+static bool handleDependencyToolResult(const std::string ,
+  llvm::Expected ,
+  SharedStream ,
+  SharedStream ) {
+  if (!MaybeFile) {
+llvm::handleAllErrors(
+MaybeFile.takeError(), [, ](llvm::StringError ) {

[PATCH] D69090: [Try 2] Include sanitize blacklist and other extra deps as part of scan-deps output

[Kousik Kumar via Phabricator via cfe-commits]

kousikk added a comment.

Thanks for the comment @jkorous.

> I think you could've just used CHECK-DAG to fix the tests. It *might* be a 
> bit more robust. Although just reordering checks seems perfectly fine too. 
> https://llvm.org/docs/CommandGuide/FileCheck.html#the-check-dag-directive. 
> Using std::set here sounds reasonable to me but I'd like @arphaman to be 
> aware of this.

Thanks for pointing me to it - I have a minor preference towards a `sorted` 
order of outputs vs `order in which we visit the files`. I think a sorted order 
is much more easy to reason about for a client. Having said that, @arphaman 
what do you think? If you and Jan both feel that we should maintain the current 
order, I'll stick to that and not change `Dependencies` to `set`.

> BTW should we change std::vector Dependencies; in 
> DependencyCollector to set too?

I think this would change the output of `*.d` file? If so I'm a little hesitant 
to make that change because of the impact it would have, but once again I'll 
defer to your judgement on this (I'm very new to LLVM codebase).

> Also, I'm wondering why is the behavior different on Windows.

I confirmed that there are duplicate blacklist.txt files without changing 
`vector` to `set`. I'm setting up a Windows machine to try to figure out why 
that is the case. Will share what I find.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69090/new/

https://reviews.llvm.org/D69090



___
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

[Kousik Kumar via Phabricator via cfe-commits]

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

[Kousik Kumar via Phabricator via cfe-commits]

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) {

[PATCH] D69090: [Try 2] Include sanitize blacklist and other extra deps as part of scan-deps output

[Kousik Kumar via Phabricator via cfe-commits]

kousikk updated this revision to Diff 225375.
kousikk added a comment.

Fix tests now that output is in ascending order


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69090/new/

https://reviews.llvm.org/D69090

Files:
  clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
  clang/test/ClangScanDeps/Inputs/non-header-dependency.json
  clang/test/ClangScanDeps/Inputs/sanitize-blacklist.txt
  clang/test/ClangScanDeps/header_stat_before_open.m
  clang/test/ClangScanDeps/headerwithdirname.cpp
  clang/test/ClangScanDeps/headerwithdirnamefollowedbyinclude.cpp
  clang/test/ClangScanDeps/no-werror.cpp
  clang/test/ClangScanDeps/non-header-dependency.cpp
  clang/test/ClangScanDeps/regular_cdb.cpp
  clang/test/ClangScanDeps/subframework_header_dir_symlink.m
  clang/test/ClangScanDeps/symlink.cpp
  clang/test/ClangScanDeps/vfsoverlay.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
@@ -17,6 +17,7 @@
 #include "llvm/Support/Signals.h"
 #include "llvm/Support/Threading.h"
 #include 
+#include 
 #include 
 
 using namespace clang;
@@ -66,7 +67,7 @@
 StringRef File) override {
 if (!this->Opts)
   this->Opts = std::make_unique(Opts);
-Dependencies.push_back(File);
+Dependencies.insert(File);
   }
 
   void printDependencies(std::string ) {
@@ -76,7 +77,7 @@
 class DependencyPrinter : public DependencyFileGenerator {
 public:
   DependencyPrinter(DependencyOutputOptions ,
-ArrayRef Dependencies)
+std::set )
   : DependencyFileGenerator(Opts) {
 for (const auto  : Dependencies)
   addDependency(Dep);
@@ -94,7 +95,7 @@
 
 private:
   std::unique_ptr Opts;
-  std::vector Dependencies;
+  std::set Dependencies;
 };
 
 DependencyPrinterConsumer Consumer;
Index: clang/test/ClangScanDeps/vfsoverlay.cpp
===
--- clang/test/ClangScanDeps/vfsoverlay.cpp
+++ clang/test/ClangScanDeps/vfsoverlay.cpp
@@ -13,5 +13,5 @@
 #include "not_real.h"
 
 // CHECK: vfsoverlay_input.o
-// CHECK-NEXT: vfsoverlay_input.cpp
 // CHECK-NEXT: Inputs{{/|\\}}header.h
+// CHECK-NEXT: vfsoverlay_input.cpp
Index: clang/test/ClangScanDeps/symlink.cpp
===
--- clang/test/ClangScanDeps/symlink.cpp
+++ clang/test/ClangScanDeps/symlink.cpp
@@ -14,10 +14,10 @@
 #include "symlink.h"
 #include "header.h"
 
-// CHECK: symlink_input.cpp
+// CHECK: Inputs{{/|\\}}header.h
 // CHECK-NEXT: Inputs{{/|\\}}symlink.h
-// CHECK-NEXT: Inputs{{/|\\}}header.h
+// CHECK-NEXT: symlink_input.cpp
 
-// CHECK: symlink_input2.cpp
+// CHECK: Inputs{{/|\\}}header.h
 // CHECK-NEXT: Inputs{{/|\\}}symlink.h
-// CHECK-NEXT: Inputs{{/|\\}}header.h
+// CHECK-NEXT: symlink_input2.cpp
\ No newline at end of file
Index: clang/test/ClangScanDeps/subframework_header_dir_symlink.m
===
--- clang/test/ClangScanDeps/subframework_header_dir_symlink.m
+++ clang/test/ClangScanDeps/subframework_header_dir_symlink.m
@@ -20,5 +20,5 @@
 // CHECK: subframework_header_dir_symlink_input.o
 // CHECK-NEXT: subframework_header_dir_symlink_input.m
 // CHECK: subframework_header_dir_symlink_input2.o
-// CHECK-NEXT: subframework_header_dir_symlink_input2.m
 // CHECK-NEXT: Inputs{{/|\\}}frameworks_symlink{{/|\\}}Framework.framework{{/|\\}}Headers{{/|\\}}Framework.h
+// CHECK-NEXT: subframework_header_dir_symlink_input2.m
Index: clang/test/ClangScanDeps/regular_cdb.cpp
===
--- clang/test/ClangScanDeps/regular_cdb.cpp
+++ clang/test/ClangScanDeps/regular_cdb.cpp
@@ -36,10 +36,11 @@
 #include "header.h"
 
 // CHECK1: regular_cdb_input2.cpp
-// CHECK1-NEXT: regular_cdb_input2.cpp
 // CHECK1-NEXT: Inputs{{/|\\}}header.h
 // CHECK1-NEXT: Inputs{{/|\\}}header2.h
+// CHECK1-NEXT: regular_cdb_input2.cpp
 
-// CHECK2: regular_cdb_input.cpp
+// CHECK2: regular_cdb_input.o
 // CHECK2-NEXT: Inputs{{/|\\}}header.h
-// CHECK2NO-NOT: header2
+// CHECK2NO-NOT: header2.h
+// CHECK2-NEXT: regular_cdb_input.cpp
Index: clang/test/ClangScanDeps/non-header-dependency.cpp
===
--- /dev/null
+++ clang/test/ClangScanDeps/non-header-dependency.cpp
@@ -0,0 +1,14 @@
+// RUN: rm -rf %t.dir
+// RUN: rm -rf %t.cdb
+// RUN: mkdir -p %t.dir
+// RUN: cp %s %t.dir/non-header-dependency_input.cpp
+// RUN: mkdir %t.dir/Inputs
+// RUN: cp %S/Inputs/sanitize-blacklist.txt %t.dir/Inputs/sanitize-blacklist.txt
+// RUN: sed -e "s|DIR|%/t.dir|g" 

[PATCH] D69090: [Try 2] Include sanitize blacklist and other extra deps as part of scan-deps output

[Kousik Kumar via Phabricator via cfe-commits]

kousikk created this revision.
kousikk added reviewers: arphaman, dexonsmith, Bigcheese, jkorous.
kousikk added a project: clang.
kousikk edited the summary of this revision.

Clang's -M mode includes these extra dependencies in its output and 
clang-scan-deps
should have equivalent behavior, so adding these extradeps to output just like
how its being done for ".d" file generation mode.

This is a retry of https://reviews.llvm.org/rC375074. From the build-bot 
failure on
Windows, it seemed like somehow the blacklist file was already added as a 
dependency.
So the extra change in this patch is that I add deps to a set in clang-scan-deps
to eliminate duplicates and print in sorted order. Having a set achieves two 
purposes:

1. Prints out the dependencies in sorted order
2. Eliminates duplicates

Windows bot failure: 
http://lab.llvm.org:8011/builders/clang-x64-ninja-win7/builds/15956/steps/ninja%20check%202/logs/stdio

An alternative fix to the test would have been to check only for the presence of
blacklsit file, but I didn't prefer it since explicit testing of all expected 
outputs
is better (and in this case has led us to capture duplicate outputs).


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D69090

Files:
  clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
  clang/test/ClangScanDeps/Inputs/non-header-dependency.json
  clang/test/ClangScanDeps/Inputs/sanitize-blacklist.txt
  clang/test/ClangScanDeps/non-header-dependency.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
@@ -17,6 +17,7 @@
 #include "llvm/Support/Signals.h"
 #include "llvm/Support/Threading.h"
 #include 
+#include 
 #include 
 
 using namespace clang;
@@ -66,7 +67,7 @@
 StringRef File) override {
 if (!this->Opts)
   this->Opts = std::make_unique(Opts);
-Dependencies.push_back(File);
+Dependencies.insert(File);
   }
 
   void printDependencies(std::string ) {
@@ -76,7 +77,7 @@
 class DependencyPrinter : public DependencyFileGenerator {
 public:
   DependencyPrinter(DependencyOutputOptions ,
-ArrayRef Dependencies)
+std::set )
   : DependencyFileGenerator(Opts) {
 for (const auto  : Dependencies)
   addDependency(Dep);
@@ -94,7 +95,7 @@
 
 private:
   std::unique_ptr Opts;
-  std::vector Dependencies;
+  std::set Dependencies;
 };
 
 DependencyPrinterConsumer Consumer;
Index: clang/test/ClangScanDeps/non-header-dependency.cpp
===
--- /dev/null
+++ clang/test/ClangScanDeps/non-header-dependency.cpp
@@ -0,0 +1,14 @@
+// RUN: rm -rf %t.dir
+// RUN: rm -rf %t.cdb
+// RUN: mkdir -p %t.dir
+// RUN: cp %s %t.dir/non-header-dependency_input.cpp
+// RUN: mkdir %t.dir/Inputs
+// RUN: cp %S/Inputs/sanitize-blacklist.txt 
%t.dir/Inputs/sanitize-blacklist.txt
+// RUN: sed -e "s|DIR|%/t.dir|g" %S/Inputs/non-header-dependency.json > %t.cdb
+//
+// RUN: clang-scan-deps -compilation-database %t.cdb -j 1 | FileCheck %s
+
+#define FOO "foo"
+
+// CHECK: Inputs{{/|\\}}sanitize-blacklist.txt
+// CHECK-NEXT: non-header-dependency_input.cpp
Index: clang/test/ClangScanDeps/Inputs/sanitize-blacklist.txt
===
--- /dev/null
+++ clang/test/ClangScanDeps/Inputs/sanitize-blacklist.txt
@@ -0,0 +1 @@
+fun:*
Index: clang/test/ClangScanDeps/Inputs/non-header-dependency.json
===
--- /dev/null
+++ clang/test/ClangScanDeps/Inputs/non-header-dependency.json
@@ -0,0 +1,7 @@
+[
+{
+  "directory": "DIR",
+  "command": "clang -E DIR/non-header-dependency_input.cpp 
-fsanitize=bounds -fsanitize-blacklist=DIR/Inputs/sanitize-blacklist.txt",
+  "file": "DIR/non-header-dependency_input.cpp"
+}
+]
Index: clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
===
--- clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
+++ clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
@@ -36,6 +36,8 @@
   llvm::sys::path::remove_dots(CanonPath, /*remove_dot_dot=*/true);
   C.handleFileDependency(*Opts, CanonPath);
 }
+for (const auto& ExtraDep : Opts->ExtraDeps)
+  C.handleFileDependency(*Opts, ExtraDep);
   }
 
 private:


Index: clang/tools/clang-scan-deps/ClangScanDeps.cpp
===
--- clang/tools/clang-scan-deps/ClangScanDeps.cpp
+++ clang/tools/clang-scan-deps/ClangScanDeps.cpp
@@ -17,6 +17,7 @@
 #include "llvm/Support/Signals.h"
 #include 

[PATCH] D69079: Revert "Include sanitize blacklist and other extra deps as part of scan-deps output"This test is failing on Windows bots, revert for now (will check the right fix and retry the patch).

[Kousik Kumar via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG9e7e36d4c260: Revert Include sanitize blacklist and 
other extra deps as part of scan-deps… (authored by kousikk).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69079/new/

https://reviews.llvm.org/D69079

Files:
  clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
  clang/test/ClangScanDeps/Inputs/non-header-dependency.json
  clang/test/ClangScanDeps/Inputs/sanitize-blacklist.txt
  clang/test/ClangScanDeps/non-header-dependency.cpp


Index: clang/test/ClangScanDeps/non-header-dependency.cpp
===
--- clang/test/ClangScanDeps/non-header-dependency.cpp
+++ /dev/null
@@ -1,14 +0,0 @@
-// RUN: rm -rf %t.dir
-// RUN: rm -rf %t.cdb
-// RUN: mkdir -p %t.dir
-// RUN: cp %s %t.dir/non-header-dependency_input.cpp
-// RUN: mkdir %t.dir/Inputs
-// RUN: cp %S/Inputs/sanitize-blacklist.txt 
%t.dir/Inputs/sanitize-blacklist.txt
-// RUN: sed -e "s|DIR|%/t.dir|g" %S/Inputs/non-header-dependency.json > %t.cdb
-//
-// RUN: clang-scan-deps -compilation-database %t.cdb -j 1 | FileCheck %s
-
-#define FOO "foo"
-
-// CHECK: Inputs{{/|\\}}sanitize-blacklist.txt
-// CHECK-NEXT: non-header-dependency_input.cpp
Index: clang/test/ClangScanDeps/Inputs/sanitize-blacklist.txt
===
--- clang/test/ClangScanDeps/Inputs/sanitize-blacklist.txt
+++ /dev/null
@@ -1 +0,0 @@
-fun:*
Index: clang/test/ClangScanDeps/Inputs/non-header-dependency.json
===
--- clang/test/ClangScanDeps/Inputs/non-header-dependency.json
+++ /dev/null
@@ -1,7 +0,0 @@
-[
-{
-  "directory": "DIR",
-  "command": "clang -E DIR/non-header-dependency_input.cpp 
-fsanitize=bounds -fsanitize-blacklist=DIR/Inputs/sanitize-blacklist.txt",
-  "file": "DIR/non-header-dependency_input.cpp"
-}
-]
Index: clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
===
--- clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
+++ clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
@@ -36,8 +36,6 @@
   llvm::sys::path::remove_dots(CanonPath, /*remove_dot_dot=*/true);
   C.handleFileDependency(*Opts, CanonPath);
 }
-for (const auto& ExtraDep : Opts->ExtraDeps)
-  C.handleFileDependency(*Opts, ExtraDep);
   }
 
 private:


Index: clang/test/ClangScanDeps/non-header-dependency.cpp
===
--- clang/test/ClangScanDeps/non-header-dependency.cpp
+++ /dev/null
@@ -1,14 +0,0 @@
-// RUN: rm -rf %t.dir
-// RUN: rm -rf %t.cdb
-// RUN: mkdir -p %t.dir
-// RUN: cp %s %t.dir/non-header-dependency_input.cpp
-// RUN: mkdir %t.dir/Inputs
-// RUN: cp %S/Inputs/sanitize-blacklist.txt %t.dir/Inputs/sanitize-blacklist.txt
-// RUN: sed -e "s|DIR|%/t.dir|g" %S/Inputs/non-header-dependency.json > %t.cdb
-//
-// RUN: clang-scan-deps -compilation-database %t.cdb -j 1 | FileCheck %s
-
-#define FOO "foo"
-
-// CHECK: Inputs{{/|\\}}sanitize-blacklist.txt
-// CHECK-NEXT: non-header-dependency_input.cpp
Index: clang/test/ClangScanDeps/Inputs/sanitize-blacklist.txt
===
--- clang/test/ClangScanDeps/Inputs/sanitize-blacklist.txt
+++ /dev/null
@@ -1 +0,0 @@
-fun:*
Index: clang/test/ClangScanDeps/Inputs/non-header-dependency.json
===
--- clang/test/ClangScanDeps/Inputs/non-header-dependency.json
+++ /dev/null
@@ -1,7 +0,0 @@
-[
-{
-  "directory": "DIR",
-  "command": "clang -E DIR/non-header-dependency_input.cpp -fsanitize=bounds -fsanitize-blacklist=DIR/Inputs/sanitize-blacklist.txt",
-  "file": "DIR/non-header-dependency_input.cpp"
-}
-]
Index: clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
===
--- clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
+++ clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
@@ -36,8 +36,6 @@
   llvm::sys::path::remove_dots(CanonPath, /*remove_dot_dot=*/true);
   C.handleFileDependency(*Opts, CanonPath);
 }
-for (const auto& ExtraDep : Opts->ExtraDeps)
-  C.handleFileDependency(*Opts, ExtraDep);
   }
 
 private:
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69079: Revert "Include sanitize blacklist and other extra deps as part of scan-deps output"This test is failing on Windows bots, revert for now (will check the right fix and retry the patch).

[Kousik Kumar via Phabricator via cfe-commits]

kousikk created this revision.
kousikk added reviewers: Bigcheese, jkorous, arphaman.
Herald added subscribers: cfe-commits, dexonsmith.
Herald added a project: clang.
kousikk added a comment.

Failure - http://lab.llvm.org:8011/builders/clang-x64-windows-msvc/builds/11583


This reverts commit 962ca076e51c25a7a08f4e0d329c65328a635bdb 
.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D69079

Files:
  clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
  clang/test/ClangScanDeps/Inputs/non-header-dependency.json
  clang/test/ClangScanDeps/Inputs/sanitize-blacklist.txt
  clang/test/ClangScanDeps/non-header-dependency.cpp


Index: clang/test/ClangScanDeps/non-header-dependency.cpp
===
--- clang/test/ClangScanDeps/non-header-dependency.cpp
+++ /dev/null
@@ -1,14 +0,0 @@
-// RUN: rm -rf %t.dir
-// RUN: rm -rf %t.cdb
-// RUN: mkdir -p %t.dir
-// RUN: cp %s %t.dir/non-header-dependency_input.cpp
-// RUN: mkdir %t.dir/Inputs
-// RUN: cp %S/Inputs/sanitize-blacklist.txt 
%t.dir/Inputs/sanitize-blacklist.txt
-// RUN: sed -e "s|DIR|%/t.dir|g" %S/Inputs/non-header-dependency.json > %t.cdb
-//
-// RUN: clang-scan-deps -compilation-database %t.cdb -j 1 | FileCheck %s
-
-#define FOO "foo"
-
-// CHECK: Inputs{{/|\\}}sanitize-blacklist.txt
-// CHECK-NEXT: non-header-dependency_input.cpp
Index: clang/test/ClangScanDeps/Inputs/sanitize-blacklist.txt
===
--- clang/test/ClangScanDeps/Inputs/sanitize-blacklist.txt
+++ /dev/null
@@ -1 +0,0 @@
-fun:*
Index: clang/test/ClangScanDeps/Inputs/non-header-dependency.json
===
--- clang/test/ClangScanDeps/Inputs/non-header-dependency.json
+++ /dev/null
@@ -1,7 +0,0 @@
-[
-{
-  "directory": "DIR",
-  "command": "clang -E DIR/non-header-dependency_input.cpp 
-fsanitize=bounds -fsanitize-blacklist=DIR/Inputs/sanitize-blacklist.txt",
-  "file": "DIR/non-header-dependency_input.cpp"
-}
-]
Index: clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
===
--- clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
+++ clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
@@ -36,8 +36,6 @@
   llvm::sys::path::remove_dots(CanonPath, /*remove_dot_dot=*/true);
   C.handleFileDependency(*Opts, CanonPath);
 }
-for (const auto& ExtraDep : Opts->ExtraDeps)
-  C.handleFileDependency(*Opts, ExtraDep);
   }
 
 private:


Index: clang/test/ClangScanDeps/non-header-dependency.cpp
===
--- clang/test/ClangScanDeps/non-header-dependency.cpp
+++ /dev/null
@@ -1,14 +0,0 @@
-// RUN: rm -rf %t.dir
-// RUN: rm -rf %t.cdb
-// RUN: mkdir -p %t.dir
-// RUN: cp %s %t.dir/non-header-dependency_input.cpp
-// RUN: mkdir %t.dir/Inputs
-// RUN: cp %S/Inputs/sanitize-blacklist.txt %t.dir/Inputs/sanitize-blacklist.txt
-// RUN: sed -e "s|DIR|%/t.dir|g" %S/Inputs/non-header-dependency.json > %t.cdb
-//
-// RUN: clang-scan-deps -compilation-database %t.cdb -j 1 | FileCheck %s
-
-#define FOO "foo"
-
-// CHECK: Inputs{{/|\\}}sanitize-blacklist.txt
-// CHECK-NEXT: non-header-dependency_input.cpp
Index: clang/test/ClangScanDeps/Inputs/sanitize-blacklist.txt
===
--- clang/test/ClangScanDeps/Inputs/sanitize-blacklist.txt
+++ /dev/null
@@ -1 +0,0 @@
-fun:*
Index: clang/test/ClangScanDeps/Inputs/non-header-dependency.json
===
--- clang/test/ClangScanDeps/Inputs/non-header-dependency.json
+++ /dev/null
@@ -1,7 +0,0 @@
-[
-{
-  "directory": "DIR",
-  "command": "clang -E DIR/non-header-dependency_input.cpp -fsanitize=bounds -fsanitize-blacklist=DIR/Inputs/sanitize-blacklist.txt",
-  "file": "DIR/non-header-dependency_input.cpp"
-}
-]
Index: clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
===
--- clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
+++ clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
@@ -36,8 +36,6 @@
   llvm::sys::path::remove_dots(CanonPath, /*remove_dot_dot=*/true);
   C.handleFileDependency(*Opts, CanonPath);
 }
-for (const auto& ExtraDep : Opts->ExtraDeps)
-  C.handleFileDependency(*Opts, ExtraDep);
   }
 
 private:
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69079: Revert "Include sanitize blacklist and other extra deps as part of scan-deps output"This test is failing on Windows bots, revert for now (will check the right fix and retry the patch).

[Kousik Kumar via Phabricator via cfe-commits]

kousikk added a comment.

Failure - http://lab.llvm.org:8011/builders/clang-x64-windows-msvc/builds/11583


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69079/new/

https://reviews.llvm.org/D69079



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69017: Include sanitize blacklist and other extra deps as part of scan-deps output

[Kousik Kumar via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG962ca076e51c: Include sanitize blacklist and other extra 
deps as part of scan-deps output (authored by kousikk).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69017/new/

https://reviews.llvm.org/D69017

Files:
  clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
  clang/test/ClangScanDeps/Inputs/non-header-dependency.json
  clang/test/ClangScanDeps/Inputs/sanitize-blacklist.txt
  clang/test/ClangScanDeps/non-header-dependency.cpp


Index: clang/test/ClangScanDeps/non-header-dependency.cpp
===
--- /dev/null
+++ clang/test/ClangScanDeps/non-header-dependency.cpp
@@ -0,0 +1,14 @@
+// RUN: rm -rf %t.dir
+// RUN: rm -rf %t.cdb
+// RUN: mkdir -p %t.dir
+// RUN: cp %s %t.dir/non-header-dependency_input.cpp
+// RUN: mkdir %t.dir/Inputs
+// RUN: cp %S/Inputs/sanitize-blacklist.txt 
%t.dir/Inputs/sanitize-blacklist.txt
+// RUN: sed -e "s|DIR|%/t.dir|g" %S/Inputs/non-header-dependency.json > %t.cdb
+//
+// RUN: clang-scan-deps -compilation-database %t.cdb -j 1 | FileCheck %s
+
+#define FOO "foo"
+
+// CHECK: Inputs{{/|\\}}sanitize-blacklist.txt
+// CHECK-NEXT: non-header-dependency_input.cpp
Index: clang/test/ClangScanDeps/Inputs/sanitize-blacklist.txt
===
--- /dev/null
+++ clang/test/ClangScanDeps/Inputs/sanitize-blacklist.txt
@@ -0,0 +1 @@
+fun:*
Index: clang/test/ClangScanDeps/Inputs/non-header-dependency.json
===
--- /dev/null
+++ clang/test/ClangScanDeps/Inputs/non-header-dependency.json
@@ -0,0 +1,7 @@
+[
+{
+  "directory": "DIR",
+  "command": "clang -E DIR/non-header-dependency_input.cpp 
-fsanitize=bounds -fsanitize-blacklist=DIR/Inputs/sanitize-blacklist.txt",
+  "file": "DIR/non-header-dependency_input.cpp"
+}
+]
Index: clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
===
--- clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
+++ clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
@@ -36,6 +36,8 @@
   llvm::sys::path::remove_dots(CanonPath, /*remove_dot_dot=*/true);
   C.handleFileDependency(*Opts, CanonPath);
 }
+for (const auto& ExtraDep : Opts->ExtraDeps)
+  C.handleFileDependency(*Opts, ExtraDep);
   }
 
 private:


Index: clang/test/ClangScanDeps/non-header-dependency.cpp
===
--- /dev/null
+++ clang/test/ClangScanDeps/non-header-dependency.cpp
@@ -0,0 +1,14 @@
+// RUN: rm -rf %t.dir
+// RUN: rm -rf %t.cdb
+// RUN: mkdir -p %t.dir
+// RUN: cp %s %t.dir/non-header-dependency_input.cpp
+// RUN: mkdir %t.dir/Inputs
+// RUN: cp %S/Inputs/sanitize-blacklist.txt %t.dir/Inputs/sanitize-blacklist.txt
+// RUN: sed -e "s|DIR|%/t.dir|g" %S/Inputs/non-header-dependency.json > %t.cdb
+//
+// RUN: clang-scan-deps -compilation-database %t.cdb -j 1 | FileCheck %s
+
+#define FOO "foo"
+
+// CHECK: Inputs{{/|\\}}sanitize-blacklist.txt
+// CHECK-NEXT: non-header-dependency_input.cpp
Index: clang/test/ClangScanDeps/Inputs/sanitize-blacklist.txt
===
--- /dev/null
+++ clang/test/ClangScanDeps/Inputs/sanitize-blacklist.txt
@@ -0,0 +1 @@
+fun:*
Index: clang/test/ClangScanDeps/Inputs/non-header-dependency.json
===
--- /dev/null
+++ clang/test/ClangScanDeps/Inputs/non-header-dependency.json
@@ -0,0 +1,7 @@
+[
+{
+  "directory": "DIR",
+  "command": "clang -E DIR/non-header-dependency_input.cpp -fsanitize=bounds -fsanitize-blacklist=DIR/Inputs/sanitize-blacklist.txt",
+  "file": "DIR/non-header-dependency_input.cpp"
+}
+]
Index: clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
===
--- clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
+++ clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
@@ -36,6 +36,8 @@
   llvm::sys::path::remove_dots(CanonPath, /*remove_dot_dot=*/true);
   C.handleFileDependency(*Opts, CanonPath);
 }
+for (const auto& ExtraDep : Opts->ExtraDeps)
+  C.handleFileDependency(*Opts, ExtraDep);
   }
 
 private:
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69017: Include sanitize blacklist and other extra deps as part of scan-deps output

[Kousik Kumar via Phabricator via cfe-commits]

kousikk updated this revision to Diff 225153.
kousikk added a comment.

Lint fixes


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69017/new/

https://reviews.llvm.org/D69017

Files:
  clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
  clang/test/ClangScanDeps/Inputs/non-header-dependency.json
  clang/test/ClangScanDeps/Inputs/sanitize-blacklist.txt
  clang/test/ClangScanDeps/non-header-dependency.cpp


Index: clang/test/ClangScanDeps/non-header-dependency.cpp
===
--- /dev/null
+++ clang/test/ClangScanDeps/non-header-dependency.cpp
@@ -0,0 +1,14 @@
+// RUN: rm -rf %t.dir
+// RUN: rm -rf %t.cdb
+// RUN: mkdir -p %t.dir
+// RUN: cp %s %t.dir/non-header-dependency_input.cpp
+// RUN: mkdir %t.dir/Inputs
+// RUN: cp %S/Inputs/sanitize-blacklist.txt 
%t.dir/Inputs/sanitize-blacklist.txt
+// RUN: sed -e "s|DIR|%/t.dir|g" %S/Inputs/non-header-dependency.json > %t.cdb
+//
+// RUN: clang-scan-deps -compilation-database %t.cdb -j 1 | FileCheck %s
+
+#define FOO "foo"
+
+// CHECK: Inputs{{/|\\}}sanitize-blacklist.txt
+// CHECK-NEXT: non-header-dependency_input.cpp
Index: clang/test/ClangScanDeps/Inputs/sanitize-blacklist.txt
===
--- /dev/null
+++ clang/test/ClangScanDeps/Inputs/sanitize-blacklist.txt
@@ -0,0 +1 @@
+fun:*
Index: clang/test/ClangScanDeps/Inputs/non-header-dependency.json
===
--- /dev/null
+++ clang/test/ClangScanDeps/Inputs/non-header-dependency.json
@@ -0,0 +1,7 @@
+[
+{
+  "directory": "DIR",
+  "command": "clang -E DIR/non-header-dependency_input.cpp 
-fsanitize=bounds -fsanitize-blacklist=DIR/Inputs/sanitize-blacklist.txt",
+  "file": "DIR/non-header-dependency_input.cpp"
+}
+]
Index: clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
===
--- clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
+++ clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
@@ -36,6 +36,8 @@
   llvm::sys::path::remove_dots(CanonPath, /*remove_dot_dot=*/true);
   C.handleFileDependency(*Opts, CanonPath);
 }
+for (const auto& ExtraDep : Opts->ExtraDeps)
+  C.handleFileDependency(*Opts, ExtraDep);
   }
 
 private:


Index: clang/test/ClangScanDeps/non-header-dependency.cpp
===
--- /dev/null
+++ clang/test/ClangScanDeps/non-header-dependency.cpp
@@ -0,0 +1,14 @@
+// RUN: rm -rf %t.dir
+// RUN: rm -rf %t.cdb
+// RUN: mkdir -p %t.dir
+// RUN: cp %s %t.dir/non-header-dependency_input.cpp
+// RUN: mkdir %t.dir/Inputs
+// RUN: cp %S/Inputs/sanitize-blacklist.txt %t.dir/Inputs/sanitize-blacklist.txt
+// RUN: sed -e "s|DIR|%/t.dir|g" %S/Inputs/non-header-dependency.json > %t.cdb
+//
+// RUN: clang-scan-deps -compilation-database %t.cdb -j 1 | FileCheck %s
+
+#define FOO "foo"
+
+// CHECK: Inputs{{/|\\}}sanitize-blacklist.txt
+// CHECK-NEXT: non-header-dependency_input.cpp
Index: clang/test/ClangScanDeps/Inputs/sanitize-blacklist.txt
===
--- /dev/null
+++ clang/test/ClangScanDeps/Inputs/sanitize-blacklist.txt
@@ -0,0 +1 @@
+fun:*
Index: clang/test/ClangScanDeps/Inputs/non-header-dependency.json
===
--- /dev/null
+++ clang/test/ClangScanDeps/Inputs/non-header-dependency.json
@@ -0,0 +1,7 @@
+[
+{
+  "directory": "DIR",
+  "command": "clang -E DIR/non-header-dependency_input.cpp -fsanitize=bounds -fsanitize-blacklist=DIR/Inputs/sanitize-blacklist.txt",
+  "file": "DIR/non-header-dependency_input.cpp"
+}
+]
Index: clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
===
--- clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
+++ clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
@@ -36,6 +36,8 @@
   llvm::sys::path::remove_dots(CanonPath, /*remove_dot_dot=*/true);
   C.handleFileDependency(*Opts, CanonPath);
 }
+for (const auto& ExtraDep : Opts->ExtraDeps)
+  C.handleFileDependency(*Opts, ExtraDep);
   }
 
 private:
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69017: Include sanitize blacklist and other extra deps as part of scan-deps output

[Kousik Kumar via Phabricator via cfe-commits]

kousikk created this revision.
kousikk added reviewers: arphaman, dexonsmith, Bigcheese, jkorous.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
kousikk updated this revision to Diff 225153.
kousikk added a comment.

Lint fixes


Clang's -M mode includes these extra dependencies in its output and 
clang-scan-deps
should have equivalent behavior, so adding these extradeps to output just like
how its being done for ".d" file generation mode.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D69017

Files:
  clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
  clang/test/ClangScanDeps/Inputs/non-header-dependency.json
  clang/test/ClangScanDeps/Inputs/sanitize-blacklist.txt
  clang/test/ClangScanDeps/non-header-dependency.cpp


Index: clang/test/ClangScanDeps/non-header-dependency.cpp
===
--- /dev/null
+++ clang/test/ClangScanDeps/non-header-dependency.cpp
@@ -0,0 +1,14 @@
+// RUN: rm -rf %t.dir
+// RUN: rm -rf %t.cdb
+// RUN: mkdir -p %t.dir
+// RUN: cp %s %t.dir/non-header-dependency_input.cpp
+// RUN: mkdir %t.dir/Inputs
+// RUN: cp %S/Inputs/sanitize-blacklist.txt 
%t.dir/Inputs/sanitize-blacklist.txt
+// RUN: sed -e "s|DIR|%/t.dir|g" %S/Inputs/non-header-dependency.json > %t.cdb
+//
+// RUN: clang-scan-deps -compilation-database %t.cdb -j 1 | FileCheck %s
+
+#define FOO "foo"
+
+// CHECK: Inputs{{/|\\}}sanitize-blacklist.txt
+// CHECK-NEXT: non-header-dependency_input.cpp
Index: clang/test/ClangScanDeps/Inputs/sanitize-blacklist.txt
===
--- /dev/null
+++ clang/test/ClangScanDeps/Inputs/sanitize-blacklist.txt
@@ -0,0 +1 @@
+fun:*
Index: clang/test/ClangScanDeps/Inputs/non-header-dependency.json
===
--- /dev/null
+++ clang/test/ClangScanDeps/Inputs/non-header-dependency.json
@@ -0,0 +1,7 @@
+[
+{
+  "directory": "DIR",
+  "command": "clang -E DIR/non-header-dependency_input.cpp 
-fsanitize=bounds -fsanitize-blacklist=DIR/Inputs/sanitize-blacklist.txt",
+  "file": "DIR/non-header-dependency_input.cpp"
+}
+]
Index: clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
===
--- clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
+++ clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
@@ -36,6 +36,8 @@
   llvm::sys::path::remove_dots(CanonPath, /*remove_dot_dot=*/true);
   C.handleFileDependency(*Opts, CanonPath);
 }
+for (const auto& ExtraDep : Opts->ExtraDeps)
+  C.handleFileDependency(*Opts, ExtraDep);
   }
 
 private:


Index: clang/test/ClangScanDeps/non-header-dependency.cpp
===
--- /dev/null
+++ clang/test/ClangScanDeps/non-header-dependency.cpp
@@ -0,0 +1,14 @@
+// RUN: rm -rf %t.dir
+// RUN: rm -rf %t.cdb
+// RUN: mkdir -p %t.dir
+// RUN: cp %s %t.dir/non-header-dependency_input.cpp
+// RUN: mkdir %t.dir/Inputs
+// RUN: cp %S/Inputs/sanitize-blacklist.txt %t.dir/Inputs/sanitize-blacklist.txt
+// RUN: sed -e "s|DIR|%/t.dir|g" %S/Inputs/non-header-dependency.json > %t.cdb
+//
+// RUN: clang-scan-deps -compilation-database %t.cdb -j 1 | FileCheck %s
+
+#define FOO "foo"
+
+// CHECK: Inputs{{/|\\}}sanitize-blacklist.txt
+// CHECK-NEXT: non-header-dependency_input.cpp
Index: clang/test/ClangScanDeps/Inputs/sanitize-blacklist.txt
===
--- /dev/null
+++ clang/test/ClangScanDeps/Inputs/sanitize-blacklist.txt
@@ -0,0 +1 @@
+fun:*
Index: clang/test/ClangScanDeps/Inputs/non-header-dependency.json
===
--- /dev/null
+++ clang/test/ClangScanDeps/Inputs/non-header-dependency.json
@@ -0,0 +1,7 @@
+[
+{
+  "directory": "DIR",
+  "command": "clang -E DIR/non-header-dependency_input.cpp -fsanitize=bounds -fsanitize-blacklist=DIR/Inputs/sanitize-blacklist.txt",
+  "file": "DIR/non-header-dependency_input.cpp"
+}
+]
Index: clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
===
--- clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
+++ clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
@@ -36,6 +36,8 @@
   llvm::sys::path::remove_dots(CanonPath, /*remove_dot_dot=*/true);
   C.handleFileDependency(*Opts, CanonPath);
 }
+for (const auto& ExtraDep : Opts->ExtraDeps)
+  C.handleFileDependency(*Opts, ExtraDep);
   }
 
 private:
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68193: In openFileForRead don't cache erroneous entries if the error relates to them being directories. Add tests.

[Kousik Kumar via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG4abac5330277: In openFileForRead dont cache erroneous 
entries if the error relates to them… (authored by kousikk).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68193/new/

https://reviews.llvm.org/D68193

Files:
  clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
  clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
  clang/test/ClangScanDeps/Inputs/headerwithdirnamefollowedbyinclude.json
  clang/test/ClangScanDeps/headerwithdirnamefollowedbyinclude.cpp

Index: clang/test/ClangScanDeps/headerwithdirnamefollowedbyinclude.cpp
===
--- /dev/null
+++ clang/test/ClangScanDeps/headerwithdirnamefollowedbyinclude.cpp
@@ -0,0 +1,21 @@
+// RUN: rm -rf %t.dir
+// RUN: rm -rf %t.dir/foodir
+// RUN: rm -rf %t.cdb
+
+// RUN: mkdir -p %t.dir
+// RUN: mkdir -p %t.dir/foodir
+
+// RUN: cp %S/Inputs/header.h %t.dir/foodir/foodirheader.h
+// RUN: cp %s %t.dir/headerwithdirname_input.cpp
+// RUN: mkdir %t.dir/Inputs
+// RUN: cp %S/Inputs/foodir %t.dir/Inputs/foodir
+// RUN: sed -e "s|DIR|%/t.dir|g" %S/Inputs/headerwithdirnamefollowedbyinclude.json > %t.cdb
+//
+// RUN: clang-scan-deps -compilation-database %t.cdb -j 1 | FileCheck %s
+
+#include 
+#include "foodir/foodirheader.h"
+
+// CHECK: headerwithdirname_input.o
+// CHECK-NEXT: headerwithdirname_input.cpp
+// CHECK-NEXT: Inputs{{/|\\}}foodir
Index: clang/test/ClangScanDeps/Inputs/headerwithdirnamefollowedbyinclude.json
===
--- /dev/null
+++ clang/test/ClangScanDeps/Inputs/headerwithdirnamefollowedbyinclude.json
@@ -0,0 +1,7 @@
+[
+{
+  "directory": "DIR",
+  "command": "clang -c -IDIR -IInputs DIR/headerwithdirname_input.cpp",
+  "file": "DIR/headerwithdirname_input.cpp"
+}
+]
Index: clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
===
--- clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
+++ clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
@@ -122,14 +122,11 @@
   return It.first->getValue();
 }
 
-llvm::ErrorOr
-DependencyScanningWorkerFilesystem::status(const Twine ) {
-  SmallString<256> OwnedFilename;
-  StringRef Filename = Path.toStringRef(OwnedFilename);
-
-  // Check the local cache first.
-  if (const CachedFileSystemEntry *Entry = getCachedEntry(Filename))
-return Entry->getStatus();
+llvm::ErrorOr
+DependencyScanningWorkerFilesystem::getOrCreateFileSystemEntry(const StringRef Filename) {
+  if (const CachedFileSystemEntry* Entry = getCachedEntry(Filename)) {
+return Entry;
+  }
 
   // FIXME: Handle PCM/PCH files.
   // FIXME: Handle module map files.
@@ -160,7 +157,17 @@
 
   // Store the result in the local cache.
   setCachedEntry(Filename, Result);
-  return Result->getStatus();
+  return Result;
+}
+
+llvm::ErrorOr
+DependencyScanningWorkerFilesystem::status(const Twine ) {
+  SmallString<256> OwnedFilename;
+  StringRef Filename = Path.toStringRef(OwnedFilename);
+  const llvm::ErrorOr Result = getOrCreateFileSystemEntry(Filename);
+  if (!Result)
+return Result.getError();
+  return (*Result)->getStatus();
 }
 
 namespace {
@@ -217,30 +224,8 @@
   SmallString<256> OwnedFilename;
   StringRef Filename = Path.toStringRef(OwnedFilename);
 
-  // Check the local cache first.
-  if (const CachedFileSystemEntry *Entry = getCachedEntry(Filename))
-return createFile(Entry, PPSkipMappings);
-
-  // FIXME: Handle PCM/PCH files.
-  // FIXME: Handle module map files.
-
-  bool KeepOriginalSource = IgnoredFiles.count(Filename);
-  DependencyScanningFilesystemSharedCache::SharedFileSystemEntry
-   = SharedCache.get(Filename);
-  const CachedFileSystemEntry *Result;
-  {
-std::unique_lock LockGuard(SharedCacheEntry.ValueLock);
-CachedFileSystemEntry  = SharedCacheEntry.Value;
-
-if (!CacheEntry.isValid()) {
-  CacheEntry = CachedFileSystemEntry::createFileEntry(
-  Filename, getUnderlyingFS(), !KeepOriginalSource);
-}
-
-Result = 
-  }
-
-  // Store the result in the local cache.
-  setCachedEntry(Filename, Result);
-  return createFile(Result, PPSkipMappings);
+  const llvm::ErrorOr Result = getOrCreateFileSystemEntry(Filename);
+  if (!Result)
+return Result.getError();
+  return createFile(Result.get(), PPSkipMappings);
 }
Index: clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
===
--- clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
+++ clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
@@ -168,6 +168,9 @@
 return It == Cache.end() ? nullptr : It->getValue();
   }
 
+  llvm::ErrorOr
+  

[PATCH] D68193: In openFileForRead don't cache erroneous entries if the error relates to them being directories. Add tests.

[Kousik Kumar via Phabricator via cfe-commits]

kousikk updated this revision to Diff 224228.
kousikk added a comment.

Remove getError() function that's now unused


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68193/new/

https://reviews.llvm.org/D68193

Files:
  clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
  clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
  clang/test/ClangScanDeps/Inputs/headerwithdirnamefollowedbyinclude.json
  clang/test/ClangScanDeps/headerwithdirnamefollowedbyinclude.cpp

Index: clang/test/ClangScanDeps/headerwithdirnamefollowedbyinclude.cpp
===
--- /dev/null
+++ clang/test/ClangScanDeps/headerwithdirnamefollowedbyinclude.cpp
@@ -0,0 +1,21 @@
+// RUN: rm -rf %t.dir
+// RUN: rm -rf %t.dir/foodir
+// RUN: rm -rf %t.cdb
+
+// RUN: mkdir -p %t.dir
+// RUN: mkdir -p %t.dir/foodir
+
+// RUN: cp %S/Inputs/header.h %t.dir/foodir/foodirheader.h
+// RUN: cp %s %t.dir/headerwithdirname_input.cpp
+// RUN: mkdir %t.dir/Inputs
+// RUN: cp %S/Inputs/foodir %t.dir/Inputs/foodir
+// RUN: sed -e "s|DIR|%/t.dir|g" %S/Inputs/headerwithdirnamefollowedbyinclude.json > %t.cdb
+//
+// RUN: clang-scan-deps -compilation-database %t.cdb -j 1 | FileCheck %s
+
+#include 
+#include "foodir/foodirheader.h"
+
+// CHECK: headerwithdirname_input.o
+// CHECK-NEXT: headerwithdirname_input.cpp
+// CHECK-NEXT: Inputs{{/|\\}}foodir
Index: clang/test/ClangScanDeps/Inputs/headerwithdirnamefollowedbyinclude.json
===
--- /dev/null
+++ clang/test/ClangScanDeps/Inputs/headerwithdirnamefollowedbyinclude.json
@@ -0,0 +1,7 @@
+[
+{
+  "directory": "DIR",
+  "command": "clang -c -IDIR -IInputs DIR/headerwithdirname_input.cpp",
+  "file": "DIR/headerwithdirname_input.cpp"
+}
+]
Index: clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
===
--- clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
+++ clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
@@ -122,14 +122,11 @@
   return It.first->getValue();
 }
 
-llvm::ErrorOr
-DependencyScanningWorkerFilesystem::status(const Twine ) {
-  SmallString<256> OwnedFilename;
-  StringRef Filename = Path.toStringRef(OwnedFilename);
-
-  // Check the local cache first.
-  if (const CachedFileSystemEntry *Entry = getCachedEntry(Filename))
-return Entry->getStatus();
+llvm::ErrorOr
+DependencyScanningWorkerFilesystem::getOrCreateFileSystemEntry(const StringRef Filename) {
+  if (const CachedFileSystemEntry* Entry = getCachedEntry(Filename)) {
+return Entry;
+  }
 
   // FIXME: Handle PCM/PCH files.
   // FIXME: Handle module map files.
@@ -160,7 +157,17 @@
 
   // Store the result in the local cache.
   setCachedEntry(Filename, Result);
-  return Result->getStatus();
+  return Result;
+}
+
+llvm::ErrorOr
+DependencyScanningWorkerFilesystem::status(const Twine ) {
+  SmallString<256> OwnedFilename;
+  StringRef Filename = Path.toStringRef(OwnedFilename);
+  const llvm::ErrorOr Result = getOrCreateFileSystemEntry(Filename);
+  if (!Result)
+return Result.getError();
+  return (*Result)->getStatus();
 }
 
 namespace {
@@ -217,30 +224,8 @@
   SmallString<256> OwnedFilename;
   StringRef Filename = Path.toStringRef(OwnedFilename);
 
-  // Check the local cache first.
-  if (const CachedFileSystemEntry *Entry = getCachedEntry(Filename))
-return createFile(Entry, PPSkipMappings);
-
-  // FIXME: Handle PCM/PCH files.
-  // FIXME: Handle module map files.
-
-  bool KeepOriginalSource = IgnoredFiles.count(Filename);
-  DependencyScanningFilesystemSharedCache::SharedFileSystemEntry
-   = SharedCache.get(Filename);
-  const CachedFileSystemEntry *Result;
-  {
-std::unique_lock LockGuard(SharedCacheEntry.ValueLock);
-CachedFileSystemEntry  = SharedCacheEntry.Value;
-
-if (!CacheEntry.isValid()) {
-  CacheEntry = CachedFileSystemEntry::createFileEntry(
-  Filename, getUnderlyingFS(), !KeepOriginalSource);
-}
-
-Result = 
-  }
-
-  // Store the result in the local cache.
-  setCachedEntry(Filename, Result);
-  return createFile(Result, PPSkipMappings);
+  const llvm::ErrorOr Result = getOrCreateFileSystemEntry(Filename);
+  if (!Result)
+return Result.getError();
+  return createFile(Result.get(), PPSkipMappings);
 }
Index: clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
===
--- clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
+++ clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
@@ -168,6 +168,9 @@
 return It == Cache.end() ? nullptr : It->getValue();
   }
 
+  llvm::ErrorOr
+  getOrCreateFileSystemEntry(const StringRef Filename);
+
   DependencyScanningFilesystemSharedCache 
   /// 

[PATCH] D68193: In openFileForRead don't cache erroneous entries if the error relates to them being directories. Add tests.

[Kousik Kumar via Phabricator via cfe-commits]

kousikk updated this revision to Diff 224219.
kousikk added a comment.

- Revert changes to createFileEntry since that may result in a race condition 
if stat is called before file open


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68193/new/

https://reviews.llvm.org/D68193

Files:
  clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
  clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
  clang/test/ClangScanDeps/Inputs/headerwithdirnamefollowedbyinclude.json
  clang/test/ClangScanDeps/headerwithdirnamefollowedbyinclude.cpp

Index: clang/test/ClangScanDeps/headerwithdirnamefollowedbyinclude.cpp
===
--- /dev/null
+++ clang/test/ClangScanDeps/headerwithdirnamefollowedbyinclude.cpp
@@ -0,0 +1,21 @@
+// RUN: rm -rf %t.dir
+// RUN: rm -rf %t.dir/foodir
+// RUN: rm -rf %t.cdb
+
+// RUN: mkdir -p %t.dir
+// RUN: mkdir -p %t.dir/foodir
+
+// RUN: cp %S/Inputs/header.h %t.dir/foodir/foodirheader.h
+// RUN: cp %s %t.dir/headerwithdirname_input.cpp
+// RUN: mkdir %t.dir/Inputs
+// RUN: cp %S/Inputs/foodir %t.dir/Inputs/foodir
+// RUN: sed -e "s|DIR|%/t.dir|g" %S/Inputs/headerwithdirnamefollowedbyinclude.json > %t.cdb
+//
+// RUN: clang-scan-deps -compilation-database %t.cdb -j 1 | FileCheck %s
+
+#include 
+#include "foodir/foodirheader.h"
+
+// CHECK: headerwithdirname_input.o
+// CHECK-NEXT: headerwithdirname_input.cpp
+// CHECK-NEXT: Inputs{{/|\\}}foodir
Index: clang/test/ClangScanDeps/Inputs/headerwithdirnamefollowedbyinclude.json
===
--- /dev/null
+++ clang/test/ClangScanDeps/Inputs/headerwithdirnamefollowedbyinclude.json
@@ -0,0 +1,7 @@
+[
+{
+  "directory": "DIR",
+  "command": "clang -c -IDIR -IInputs DIR/headerwithdirname_input.cpp",
+  "file": "DIR/headerwithdirname_input.cpp"
+}
+]
Index: clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
===
--- clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
+++ clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
@@ -122,14 +122,11 @@
   return It.first->getValue();
 }
 
-llvm::ErrorOr
-DependencyScanningWorkerFilesystem::status(const Twine ) {
-  SmallString<256> OwnedFilename;
-  StringRef Filename = Path.toStringRef(OwnedFilename);
-
-  // Check the local cache first.
-  if (const CachedFileSystemEntry *Entry = getCachedEntry(Filename))
-return Entry->getStatus();
+llvm::ErrorOr
+DependencyScanningWorkerFilesystem::getOrCreateFileSystemEntry(const StringRef Filename) {
+  if (const CachedFileSystemEntry* Entry = getCachedEntry(Filename)) {
+return Entry;
+  }
 
   // FIXME: Handle PCM/PCH files.
   // FIXME: Handle module map files.
@@ -160,7 +157,17 @@
 
   // Store the result in the local cache.
   setCachedEntry(Filename, Result);
-  return Result->getStatus();
+  return Result;
+}
+
+llvm::ErrorOr
+DependencyScanningWorkerFilesystem::status(const Twine ) {
+  SmallString<256> OwnedFilename;
+  StringRef Filename = Path.toStringRef(OwnedFilename);
+  const llvm::ErrorOr Result = getOrCreateFileSystemEntry(Filename);
+  if (!Result)
+return Result.getError();
+  return (*Result)->getStatus();
 }
 
 namespace {
@@ -217,30 +224,8 @@
   SmallString<256> OwnedFilename;
   StringRef Filename = Path.toStringRef(OwnedFilename);
 
-  // Check the local cache first.
-  if (const CachedFileSystemEntry *Entry = getCachedEntry(Filename))
-return createFile(Entry, PPSkipMappings);
-
-  // FIXME: Handle PCM/PCH files.
-  // FIXME: Handle module map files.
-
-  bool KeepOriginalSource = IgnoredFiles.count(Filename);
-  DependencyScanningFilesystemSharedCache::SharedFileSystemEntry
-   = SharedCache.get(Filename);
-  const CachedFileSystemEntry *Result;
-  {
-std::unique_lock LockGuard(SharedCacheEntry.ValueLock);
-CachedFileSystemEntry  = SharedCacheEntry.Value;
-
-if (!CacheEntry.isValid()) {
-  CacheEntry = CachedFileSystemEntry::createFileEntry(
-  Filename, getUnderlyingFS(), !KeepOriginalSource);
-}
-
-Result = 
-  }
-
-  // Store the result in the local cache.
-  setCachedEntry(Filename, Result);
-  return createFile(Result, PPSkipMappings);
+  const llvm::ErrorOr Result = getOrCreateFileSystemEntry(Filename);
+  if (!Result)
+return Result.getError();
+  return createFile(Result.get(), PPSkipMappings);
 }
Index: clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
===
--- clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
+++ clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
@@ -80,6 +80,11 @@
 return MaybeStat->getName();
   }
 
+  std::error_code getError() const {
+assert(isValid() && "not initialized");
+  

[PATCH] D68193: In openFileForRead don't cache erroneous entries if the error relates to them being directories. Add tests.

[Kousik Kumar via Phabricator via cfe-commits]

kousikk added a comment.

@arphaman I don't mind changing this if there are race conditions as you say, 
but isn't the assumption of the tool that the filesystem remains unchanged for 
a single run of the tool? If so, should we actually throw error conditions 
instead of crashing in those cases?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68193/new/

https://reviews.llvm.org/D68193



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68193: In openFileForRead don't cache erroneous entries if the error relates to them being directories. Add tests.

[Kousik Kumar via Phabricator via cfe-commits]

kousikk added a comment.

@dexonsmith when you get a chance, can you take another look at this? Thanks!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68193/new/

https://reviews.llvm.org/D68193



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68436: [clang-scan-deps] Improve string/character literal skipping

[Kousik Kumar via Phabricator via cfe-commits]

kousikk accepted this revision.
kousikk marked an inline comment as done.
kousikk added inline comments.



Comment at: clang/lib/Lex/DependencyDirectivesSourceMinimizer.cpp:206
+if (*First == '\\') {
+  if (++First == End)
+return;

dexonsmith wrote:
> > Should you also check if the character right after a backslash is equal to 
> > Terminator and if it is, continue on without terminating?
> > 
> 
> @kousikk, that was handled before this patch.  This `++First` combined with 
> the one in the loop increment handles skipping over an escaped terminator.
Ah makes sense, thanks.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68436/new/

https://reviews.llvm.org/D68436



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68436: [clang-scan-deps] Improve string/character literal skipping

[Kousik Kumar via Phabricator via cfe-commits]

kousikk added inline comments.



Comment at: clang/lib/Lex/DependencyDirectivesSourceMinimizer.cpp:205
+  return;
+if (*First == '\\') {
+  if (++First == End)

Should you also check if the character right after a backslash is equal to 
Terminator and if it is, continue on without terminating? The case I'm thinking 
of is:

```
#define FOO "FOO \"doublequote\""
```

The testcase would be something like:

```
StringRef Source = "#define FOO \"FOO \\\"doublequote\\\"\"
... do rest
```



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68436/new/

https://reviews.llvm.org/D68436



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68193: In openFileForRead don't cache erroneous entries if the error relates to them being directories. Add tests.

[Kousik Kumar via Phabricator via cfe-commits]

kousikk updated this revision to Diff 222953.
kousikk added a comment.

Update diff to include all commits


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68193/new/

https://reviews.llvm.org/D68193

Files:
  clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
  clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
  clang/test/ClangScanDeps/Inputs/headerwithdirnamefollowedbyinclude.json
  clang/test/ClangScanDeps/headerwithdirnamefollowedbyinclude.cpp

Index: clang/test/ClangScanDeps/headerwithdirnamefollowedbyinclude.cpp
===
--- /dev/null
+++ clang/test/ClangScanDeps/headerwithdirnamefollowedbyinclude.cpp
@@ -0,0 +1,21 @@
+// RUN: rm -rf %t.dir
+// RUN: rm -rf %t.dir/foodir
+// RUN: rm -rf %t.cdb
+
+// RUN: mkdir -p %t.dir
+// RUN: mkdir -p %t.dir/foodir
+
+// RUN: cp %S/Inputs/header.h %t.dir/foodir/foodirheader.h
+// RUN: cp %s %t.dir/headerwithdirname_input.cpp
+// RUN: mkdir %t.dir/Inputs
+// RUN: cp %S/Inputs/foodir %t.dir/Inputs/foodir
+// RUN: sed -e "s|DIR|%/t.dir|g" %S/Inputs/headerwithdirnamefollowedbyinclude.json > %t.cdb
+//
+// RUN: clang-scan-deps -compilation-database %t.cdb -j 1 | FileCheck %s
+
+#include 
+#include "foodir/foodirheader.h"
+
+// CHECK: headerwithdirname_input.o
+// CHECK-NEXT: headerwithdirname_input.cpp
+// CHECK-NEXT: Inputs{{/|\\}}foodir
Index: clang/test/ClangScanDeps/Inputs/headerwithdirnamefollowedbyinclude.json
===
--- /dev/null
+++ clang/test/ClangScanDeps/Inputs/headerwithdirnamefollowedbyinclude.json
@@ -0,0 +1,7 @@
+[
+{
+  "directory": "DIR",
+  "command": "clang -c -IDIR -IInputs DIR/headerwithdirname_input.cpp",
+  "file": "DIR/headerwithdirname_input.cpp"
+}
+]
Index: clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
===
--- clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
+++ clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
@@ -15,20 +15,19 @@
 using namespace tooling;
 using namespace dependencies;
 
+
 CachedFileSystemEntry CachedFileSystemEntry::createFileEntry(
-StringRef Filename, llvm::vfs::FileSystem , bool Minimize) {
+llvm::vfs::Status &, llvm::vfs::FileSystem , bool Minimize) {
   // Load the file and its content from the file system.
+  StringRef Filename = Stat.getName();
   llvm::ErrorOr> MaybeFile =
   FS.openFileForRead(Filename);
   if (!MaybeFile)
 return MaybeFile.getError();
-  llvm::ErrorOr Stat = (*MaybeFile)->status();
-  if (!Stat)
-return Stat.getError();
 
   llvm::vfs::File  = **MaybeFile;
   llvm::ErrorOr> MaybeBuffer =
-  F.getBuffer(Stat->getName());
+  F.getBuffer(Filename);
   if (!MaybeBuffer)
 return MaybeBuffer.getError();
 
@@ -42,7 +41,7 @@
 // if the minimization failed.
 // FIXME: Propage the diagnostic if desired by the client.
 CachedFileSystemEntry Result;
-Result.MaybeStat = std::move(*Stat);
+Result.MaybeStat = Stat;
 Result.Contents.reserve(Buffer->getBufferSize() + 1);
 Result.Contents.append(Buffer->getBufferStart(), Buffer->getBufferEnd());
 // Implicitly null terminate the contents for Clang's lexer.
@@ -53,10 +52,10 @@
 
   CachedFileSystemEntry Result;
   size_t Size = MinimizedFileContents.size();
-  Result.MaybeStat = llvm::vfs::Status(Stat->getName(), Stat->getUniqueID(),
-   Stat->getLastModificationTime(),
-   Stat->getUser(), Stat->getGroup(), Size,
-   Stat->getType(), Stat->getPermissions());
+  Result.MaybeStat = llvm::vfs::Status(Stat.getName(), Stat.getUniqueID(),
+   Stat.getLastModificationTime(),
+   Stat.getUser(), Stat.getGroup(), Size,
+   Stat.getType(), Stat.getPermissions());
   // The contents produced by the minimizer must be null terminated.
   assert(MinimizedFileContents.data()[MinimizedFileContents.size()] == '\0' &&
  "not null terminated contents");
@@ -122,14 +121,11 @@
   return It.first->getValue();
 }
 
-llvm::ErrorOr
-DependencyScanningWorkerFilesystem::status(const Twine ) {
-  SmallString<256> OwnedFilename;
-  StringRef Filename = Path.toStringRef(OwnedFilename);
-
-  // Check the local cache first.
-  if (const CachedFileSystemEntry *Entry = getCachedEntry(Filename))
-return Entry->getStatus();
+llvm::ErrorOr
+DependencyScanningWorkerFilesystem::getOrCreateFileSystemEntry(const StringRef Filename) {
+  if (const CachedFileSystemEntry* Entry = getCachedEntry(Filename)) {
+return Entry;
+  }
 
   // FIXME: Handle PCM/PCH files.
   // FIXME: Handle module map files.
@@ -152,7 +148,7 @@
 std::move(*MaybeStatus));
   

[PATCH] D68193: In openFileForRead don't cache erroneous entries if the error relates to them being directories. Add tests.

[Kousik Kumar via Phabricator via cfe-commits]

kousikk added a comment.

In D68193#1692293 , @arphaman wrote:

> I don't quite understand the specific issue you're hitting, as it sounds that 
> the logic right now should handle it:
>
> > It seems that when the CachingFileSystem is first given a file to open that 
> > is actually a directory, it incorrectly
> > caches that path to be errenous and throws an error when subsequently a 
> > directory open >call is made for the same
> > path.
>
> In this case, right now (without this patch) `createFileEntry` will return 
> `std::errc::is_a_directory`. 
> `DependencyScanningWorkerFilesystem::openFileForRead` will then invalidate 
> the entry in the global cache:


`createFileEntry` does return std::errc::is_a_directory, but the cache entry is 
NOT invalidated from the global cache. It stays on in the global cache, as an 
error entry for that directory path. I think the current version is slightly 
better because the first time we see a path, we determine if its a file / 
directory and store the appropriate CachedFileSystemEntry in the global cache.

> 
> 
>   if (CacheEntry.getError() == std::errc::is_a_directory) {
>   // Reset the CacheEntry to avoid setting an error entry in the
>   // cache for the given directory path.
>   CacheEntry = CachedFileSystemEntry();
>
> 
> Which means that when the call to `stat` happens, it should be fine as the 
> global cache doesn't have a valid entry, so it will be able to recognize a 
> directory and won't return an error.
> 
> Does this not happen in your case?

I think the difference is the global cache does have the entry (but its set to 
an Error object). The test-case I added should currently fail without the 
changes in this CL.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68193/new/

https://reviews.llvm.org/D68193



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68193: In openFileForRead don't cache erroneous entries if the error relates to them being directories. Add tests.

[Kousik Kumar via Phabricator via cfe-commits]

kousikk updated this revision to Diff 222669.
kousikk added a comment.

Add missed const


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68193/new/

https://reviews.llvm.org/D68193

Files:
  clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
  clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp

Index: clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
===
--- clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
+++ clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
@@ -15,22 +15,19 @@
 using namespace tooling;
 using namespace dependencies;
 
+
 CachedFileSystemEntry CachedFileSystemEntry::createFileEntry(
-StringRef Filename, llvm::vfs::FileSystem , bool Minimize) {
+llvm::vfs::Status &, llvm::vfs::FileSystem , bool Minimize) {
   // Load the file and its content from the file system.
+  StringRef Filename = Stat.getName();
   llvm::ErrorOr> MaybeFile =
   FS.openFileForRead(Filename);
   if (!MaybeFile)
 return MaybeFile.getError();
-  llvm::ErrorOr Stat = (*MaybeFile)->status();
-  if (!Stat)
-return Stat.getError();
-  if (Stat->isDirectory())
-return std::make_error_code(std::errc::is_a_directory);
 
   llvm::vfs::File  = **MaybeFile;
   llvm::ErrorOr> MaybeBuffer =
-  F.getBuffer(Stat->getName());
+  F.getBuffer(Filename);
   if (!MaybeBuffer)
 return MaybeBuffer.getError();
 
@@ -44,7 +41,7 @@
 // if the minimization failed.
 // FIXME: Propage the diagnostic if desired by the client.
 CachedFileSystemEntry Result;
-Result.MaybeStat = std::move(*Stat);
+Result.MaybeStat = Stat;
 Result.Contents.reserve(Buffer->getBufferSize() + 1);
 Result.Contents.append(Buffer->getBufferStart(), Buffer->getBufferEnd());
 // Implicitly null terminate the contents for Clang's lexer.
@@ -55,10 +52,10 @@
 
   CachedFileSystemEntry Result;
   size_t Size = MinimizedFileContents.size();
-  Result.MaybeStat = llvm::vfs::Status(Stat->getName(), Stat->getUniqueID(),
-   Stat->getLastModificationTime(),
-   Stat->getUser(), Stat->getGroup(), Size,
-   Stat->getType(), Stat->getPermissions());
+  Result.MaybeStat = llvm::vfs::Status(Stat.getName(), Stat.getUniqueID(),
+   Stat.getLastModificationTime(),
+   Stat.getUser(), Stat.getGroup(), Size,
+   Stat.getType(), Stat.getPermissions());
   // The contents produced by the minimizer must be null terminated.
   assert(MinimizedFileContents.data()[MinimizedFileContents.size()] == '\0' &&
  "not null terminated contents");
@@ -124,14 +121,11 @@
   return It.first->getValue();
 }
 
-llvm::ErrorOr
-DependencyScanningWorkerFilesystem::status(const Twine ) {
-  SmallString<256> OwnedFilename;
-  StringRef Filename = Path.toStringRef(OwnedFilename);
-
-  // Check the local cache first.
-  if (const CachedFileSystemEntry *Entry = getCachedEntry(Filename))
-return Entry->getStatus();
+llvm::ErrorOr
+DependencyScanningWorkerFilesystem::getOrCreateFileSystemEntry(const StringRef Filename) {
+  if (const CachedFileSystemEntry* Entry = getCachedEntry(Filename)) {
+return Entry;
+  }
 
   // FIXME: Handle PCM/PCH files.
   // FIXME: Handle module map files.
@@ -154,7 +148,7 @@
 std::move(*MaybeStatus));
   else
 CacheEntry = CachedFileSystemEntry::createFileEntry(
-Filename, FS, !KeepOriginalSource);
+std::move(*MaybeStatus), FS, !KeepOriginalSource);
 }
 
 Result = 
@@ -162,7 +156,17 @@
 
   // Store the result in the local cache.
   setCachedEntry(Filename, Result);
-  return Result->getStatus();
+  return Result;
+}
+
+llvm::ErrorOr
+DependencyScanningWorkerFilesystem::status(const Twine ) {
+  SmallString<256> OwnedFilename;
+  StringRef Filename = Path.toStringRef(OwnedFilename);
+  const llvm::ErrorOr Result = getOrCreateFileSystemEntry(Filename);
+  if (!Result)
+return Result.getError();
+  return (*Result)->getStatus();
 }
 
 namespace {
@@ -219,36 +223,8 @@
   SmallString<256> OwnedFilename;
   StringRef Filename = Path.toStringRef(OwnedFilename);
 
-  // Check the local cache first.
-  if (const CachedFileSystemEntry *Entry = getCachedEntry(Filename))
-return createFile(Entry, PPSkipMappings);
-
-  // FIXME: Handle PCM/PCH files.
-  // FIXME: Handle module map files.
-
-  bool KeepOriginalSource = IgnoredFiles.count(Filename);
-  DependencyScanningFilesystemSharedCache::SharedFileSystemEntry
-   = SharedCache.get(Filename);
-  const CachedFileSystemEntry *Result;
-  {
-std::unique_lock LockGuard(SharedCacheEntry.ValueLock);
-CachedFileSystemEntry  = SharedCacheEntry.Value;
-
-if 

[PATCH] D68193: In openFileForRead don't cache erroneous entries if the error relates to them being directories. Add tests.

[Kousik Kumar via Phabricator via cfe-commits]

kousikk added a comment.

In D68193#1688838 , @dexonsmith wrote:

> Sorry for bouncing you around, but I just had a look at the other user of 
> `createFileEntry` and I think the right thing to do is to somehow share the 
> code between `DependencyScanningWorkerFilesystem::status` and this.  I 
> suggest splitting a function out of `status` (called 
> `getOrCreateFileSystemEntry`?) that returns a `CachedFileSystemEntry` (or an 
> error).
>
> The fix I just asked you to make (to only call `stat` once) could be done on 
> `getOrCreateFileSystemEntry` as a follow-up, using 
> `std::unique_ptr` and changing `getFileEntry` to take that 
> instead of a filename.


IIUC, 
`DependencyScanningWorkerFilesystem::status()` and 
`DependencyScanningWorkerFilesystem::openFileForRead()` both inturn call into 
`CachedFileSystemEntry::createFileEntry()`.

Given that, I made `getOrCreateFileEntry()` a private-member of 
`DependencyScanningWorkerFilesystem` and made both these functions use it first.

In a subsequent step, I passed in `llvm::vfs::Status` object to 
`createFileEntry()` instead of `Filename`. I hope this aligns with the 
code-organization you are looking for. Let me know if this aligns with how you 
wanted the code to look (happy to re-write if not).

I also ran it against a canonical set of tests I'm maintaining internally and I 
didn't notice a difference in either performance and didn't get new crashes of 
the tool, so I think there should NOT be any regressions because of this change.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68193/new/

https://reviews.llvm.org/D68193



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68193: In openFileForRead don't cache erroneous entries if the error relates to them being directories. Add tests.

[Kousik Kumar via Phabricator via cfe-commits]

kousikk updated this revision to Diff 222667.
kousikk marked an inline comment as done.
kousikk added a comment.

Address code-reorg comment


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68193/new/

https://reviews.llvm.org/D68193

Files:
  clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
  clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp

Index: clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
===
--- clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
+++ clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
@@ -15,22 +15,19 @@
 using namespace tooling;
 using namespace dependencies;
 
+
 CachedFileSystemEntry CachedFileSystemEntry::createFileEntry(
-StringRef Filename, llvm::vfs::FileSystem , bool Minimize) {
+llvm::vfs::Status &, llvm::vfs::FileSystem , bool Minimize) {
   // Load the file and its content from the file system.
+  StringRef Filename = Stat.getName();
   llvm::ErrorOr> MaybeFile =
   FS.openFileForRead(Filename);
   if (!MaybeFile)
 return MaybeFile.getError();
-  llvm::ErrorOr Stat = (*MaybeFile)->status();
-  if (!Stat)
-return Stat.getError();
-  if (Stat->isDirectory())
-return std::make_error_code(std::errc::is_a_directory);
 
   llvm::vfs::File  = **MaybeFile;
   llvm::ErrorOr> MaybeBuffer =
-  F.getBuffer(Stat->getName());
+  F.getBuffer(Filename);
   if (!MaybeBuffer)
 return MaybeBuffer.getError();
 
@@ -44,7 +41,7 @@
 // if the minimization failed.
 // FIXME: Propage the diagnostic if desired by the client.
 CachedFileSystemEntry Result;
-Result.MaybeStat = std::move(*Stat);
+Result.MaybeStat = Stat;
 Result.Contents.reserve(Buffer->getBufferSize() + 1);
 Result.Contents.append(Buffer->getBufferStart(), Buffer->getBufferEnd());
 // Implicitly null terminate the contents for Clang's lexer.
@@ -55,10 +52,10 @@
 
   CachedFileSystemEntry Result;
   size_t Size = MinimizedFileContents.size();
-  Result.MaybeStat = llvm::vfs::Status(Stat->getName(), Stat->getUniqueID(),
-   Stat->getLastModificationTime(),
-   Stat->getUser(), Stat->getGroup(), Size,
-   Stat->getType(), Stat->getPermissions());
+  Result.MaybeStat = llvm::vfs::Status(Stat.getName(), Stat.getUniqueID(),
+   Stat.getLastModificationTime(),
+   Stat.getUser(), Stat.getGroup(), Size,
+   Stat.getType(), Stat.getPermissions());
   // The contents produced by the minimizer must be null terminated.
   assert(MinimizedFileContents.data()[MinimizedFileContents.size()] == '\0' &&
  "not null terminated contents");
@@ -124,14 +121,11 @@
   return It.first->getValue();
 }
 
-llvm::ErrorOr
-DependencyScanningWorkerFilesystem::status(const Twine ) {
-  SmallString<256> OwnedFilename;
-  StringRef Filename = Path.toStringRef(OwnedFilename);
-
-  // Check the local cache first.
-  if (const CachedFileSystemEntry *Entry = getCachedEntry(Filename))
-return Entry->getStatus();
+llvm::ErrorOr
+DependencyScanningWorkerFilesystem::getOrCreateFileSystemEntry(const StringRef Filename) {
+  if (const CachedFileSystemEntry* Entry = getCachedEntry(Filename)) {
+return Entry;
+  }
 
   // FIXME: Handle PCM/PCH files.
   // FIXME: Handle module map files.
@@ -139,7 +133,7 @@
   bool KeepOriginalSource = IgnoredFiles.count(Filename);
   DependencyScanningFilesystemSharedCache::SharedFileSystemEntry
= SharedCache.get(Filename);
-  const CachedFileSystemEntry *Result;
+  CachedFileSystemEntry *Result;
   {
 std::unique_lock LockGuard(SharedCacheEntry.ValueLock);
 CachedFileSystemEntry  = SharedCacheEntry.Value;
@@ -154,7 +148,7 @@
 std::move(*MaybeStatus));
   else
 CacheEntry = CachedFileSystemEntry::createFileEntry(
-Filename, FS, !KeepOriginalSource);
+std::move(*MaybeStatus), FS, !KeepOriginalSource);
 }
 
 Result = 
@@ -162,7 +156,17 @@
 
   // Store the result in the local cache.
   setCachedEntry(Filename, Result);
-  return Result->getStatus();
+  return Result;
+}
+
+llvm::ErrorOr
+DependencyScanningWorkerFilesystem::status(const Twine ) {
+  SmallString<256> OwnedFilename;
+  StringRef Filename = Path.toStringRef(OwnedFilename);
+  const llvm::ErrorOr Result = getOrCreateFileSystemEntry(Filename);
+  if (!Result)
+return Result.getError();
+  return (*Result)->getStatus();
 }
 
 namespace {
@@ -219,36 +223,8 @@
   SmallString<256> OwnedFilename;
   StringRef Filename = Path.toStringRef(OwnedFilename);
 
-  // Check the local cache first.
-  if (const CachedFileSystemEntry *Entry = getCachedEntry(Filename))
-return createFile(Entry, PPSkipMappings);

[PATCH] D68193: In openFileForRead don't cache erroneous entries if the error relates to them being directories. Add tests.

[Kousik Kumar via Phabricator via cfe-commits]

kousikk marked 3 inline comments as done.
kousikk added inline comments.



Comment at: 
clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp:236-237
 if (!CacheEntry.isValid()) {
+  llvm::vfs::FileSystem  = getUnderlyingFS();
+  auto MaybeStatus = FS.status(Filename);
+

dexonsmith wrote:
> It seems wasteful to do an extra stat here when the file is already open in 
> `createFileEntry`.
> 
> Can you instead change `createFileEntry` to return 
> `std::errc::is_a_directory` as appropriate to avoid the extra filesystem 
> access?  (Is it possible that it's already doing that, and you just need to 
> check for that?)
Thanks for the suggestion, done.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68193/new/

https://reviews.llvm.org/D68193



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68193: In openFileForRead don't cache erroneous entries if the error relates to them being directories. Add tests.

[Kousik Kumar via Phabricator via cfe-commits]

kousikk updated this revision to Diff 222474.
kousikk added a comment.

Avoid the extra stat() call


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68193/new/

https://reviews.llvm.org/D68193

Files:
  clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
  clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp


Index: clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
===
--- clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
+++ clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
@@ -25,6 +25,8 @@
   llvm::ErrorOr Stat = (*MaybeFile)->status();
   if (!Stat)
 return Stat.getError();
+  if (Stat->isDirectory())
+return std::make_error_code(std::errc::is_a_directory);
 
   llvm::vfs::File  = **MaybeFile;
   llvm::ErrorOr> MaybeBuffer =
@@ -233,15 +235,14 @@
 CachedFileSystemEntry  = SharedCacheEntry.Value;
 
 if (!CacheEntry.isValid()) {
-  llvm::vfs::FileSystem  = getUnderlyingFS();
-  auto MaybeStatus = FS.status(Filename);
-
-  if (MaybeStatus && MaybeStatus->isDirectory())
-return llvm::ErrorOr>(
-  std::make_error_code(std::errc::is_a_directory));
-
   CacheEntry = CachedFileSystemEntry::createFileEntry(
-  Filename, FS, !KeepOriginalSource);
+Filename, getUnderlyingFS(), !KeepOriginalSource);
+  if (CacheEntry.getError() == std::errc::is_a_directory) {
+// Reset the CacheEntry to avoid setting an error entry in the
+// cache for the given directory path.
+CacheEntry = CachedFileSystemEntry();
+return 
llvm::ErrorOr>(std::errc::is_a_directory);
+  }
 }
 
 Result = 
Index: 
clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
===
--- 
clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
+++ 
clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
@@ -80,6 +80,11 @@
 return MaybeStat->getName();
   }
 
+  std::error_code getError() const {
+assert(isValid() && "not initialized");
+return MaybeStat.getError();
+  }
+
   /// Return the mapping between location -> distance that is used to speed up
   /// the block skipping in the preprocessor.
   const PreprocessorSkippedRangeMapping () const {


Index: clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
===
--- clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
+++ clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
@@ -25,6 +25,8 @@
   llvm::ErrorOr Stat = (*MaybeFile)->status();
   if (!Stat)
 return Stat.getError();
+  if (Stat->isDirectory())
+return std::make_error_code(std::errc::is_a_directory);
 
   llvm::vfs::File  = **MaybeFile;
   llvm::ErrorOr> MaybeBuffer =
@@ -233,15 +235,14 @@
 CachedFileSystemEntry  = SharedCacheEntry.Value;
 
 if (!CacheEntry.isValid()) {
-  llvm::vfs::FileSystem  = getUnderlyingFS();
-  auto MaybeStatus = FS.status(Filename);
-
-  if (MaybeStatus && MaybeStatus->isDirectory())
-return llvm::ErrorOr>(
-  std::make_error_code(std::errc::is_a_directory));
-
   CacheEntry = CachedFileSystemEntry::createFileEntry(
-  Filename, FS, !KeepOriginalSource);
+Filename, getUnderlyingFS(), !KeepOriginalSource);
+  if (CacheEntry.getError() == std::errc::is_a_directory) {
+// Reset the CacheEntry to avoid setting an error entry in the
+// cache for the given directory path.
+CacheEntry = CachedFileSystemEntry();
+return llvm::ErrorOr>(std::errc::is_a_directory);
+  }
 }
 
 Result = 
Index: clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
===
--- clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
+++ clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
@@ -80,6 +80,11 @@
 return MaybeStat->getName();
   }
 
+  std::error_code getError() const {
+assert(isValid() && "not initialized");
+return MaybeStat.getError();
+  }
+
   /// Return the mapping between location -> distance that is used to speed up
   /// the block skipping in the preprocessor.
   const PreprocessorSkippedRangeMapping () const {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68193: In openFileForRead don't cache erroneous entries if the error relates to them being directories. Add tests.

[Kousik Kumar via Phabricator via cfe-commits]

kousikk created this revision.
kousikk added a reviewer: arphaman.
Herald added subscribers: cfe-commits, dexonsmith.
Herald added a project: clang.

It seems that when the CachingFileSystem is first given a file to open that is 
actually a directory, it incorrectly
caches that path to be errenous and throws an error when subsequently a 
directory open call is made for the same
path.
This change makes it so that we do NOT cache a path if it turns out we asked 
for a file when its a directory.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D68193

Files:
  clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
  clang/test/ClangScanDeps/Inputs/headerwithdirnamefollowedbyinclude.json
  clang/test/ClangScanDeps/headerwithdirnamefollowedbyinclude.cpp


Index: clang/test/ClangScanDeps/headerwithdirnamefollowedbyinclude.cpp
===
--- /dev/null
+++ clang/test/ClangScanDeps/headerwithdirnamefollowedbyinclude.cpp
@@ -0,0 +1,21 @@
+// RUN: rm -rf %t.dir
+// RUN: rm -rf %t.dir/foodir
+// RUN: rm -rf %t.cdb
+
+// RUN: mkdir -p %t.dir
+// RUN: mkdir -p %t.dir/foodir
+
+// RUN: cp %S/Inputs/header.h %t.dir/foodir/foodirheader.h
+// RUN: cp %s %t.dir/headerwithdirname_input.cpp
+// RUN: mkdir %t.dir/Inputs
+// RUN: cp %S/Inputs/foodir %t.dir/Inputs/foodir
+// RUN: sed -e "s|DIR|%/t.dir|g" 
%S/Inputs/headerwithdirnamefollowedbyinclude.json > %t.cdb
+//
+// RUN: clang-scan-deps -compilation-database %t.cdb -j 1 | FileCheck %s
+
+#include 
+#include "foodir/foodirheader.h"
+
+// CHECK: headerwithdirname_input.o
+// CHECK-NEXT: headerwithdirname_input.cpp
+// CHECK-NEXT: Inputs{{/|\\}}foodir
Index: clang/test/ClangScanDeps/Inputs/headerwithdirnamefollowedbyinclude.json
===
--- /dev/null
+++ clang/test/ClangScanDeps/Inputs/headerwithdirnamefollowedbyinclude.json
@@ -0,0 +1,7 @@
+[
+{
+  "directory": "DIR",
+  "command": "clang -c -IDIR -IInputs DIR/headerwithdirname_input.cpp",
+  "file": "DIR/headerwithdirname_input.cpp"
+}
+]
Index: clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
===
--- clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
+++ clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
@@ -233,8 +233,15 @@
 CachedFileSystemEntry  = SharedCacheEntry.Value;
 
 if (!CacheEntry.isValid()) {
+  llvm::vfs::FileSystem  = getUnderlyingFS();
+  auto MaybeStatus = FS.status(Filename);
+
+  if (MaybeStatus && MaybeStatus->isDirectory())
+return llvm::ErrorOr>(
+  std::make_error_code(std::errc::is_a_directory));
+
   CacheEntry = CachedFileSystemEntry::createFileEntry(
-  Filename, getUnderlyingFS(), !KeepOriginalSource);
+  Filename, FS, !KeepOriginalSource);
 }
 
 Result = 


Index: clang/test/ClangScanDeps/headerwithdirnamefollowedbyinclude.cpp
===
--- /dev/null
+++ clang/test/ClangScanDeps/headerwithdirnamefollowedbyinclude.cpp
@@ -0,0 +1,21 @@
+// RUN: rm -rf %t.dir
+// RUN: rm -rf %t.dir/foodir
+// RUN: rm -rf %t.cdb
+
+// RUN: mkdir -p %t.dir
+// RUN: mkdir -p %t.dir/foodir
+
+// RUN: cp %S/Inputs/header.h %t.dir/foodir/foodirheader.h
+// RUN: cp %s %t.dir/headerwithdirname_input.cpp
+// RUN: mkdir %t.dir/Inputs
+// RUN: cp %S/Inputs/foodir %t.dir/Inputs/foodir
+// RUN: sed -e "s|DIR|%/t.dir|g" %S/Inputs/headerwithdirnamefollowedbyinclude.json > %t.cdb
+//
+// RUN: clang-scan-deps -compilation-database %t.cdb -j 1 | FileCheck %s
+
+#include 
+#include "foodir/foodirheader.h"
+
+// CHECK: headerwithdirname_input.o
+// CHECK-NEXT: headerwithdirname_input.cpp
+// CHECK-NEXT: Inputs{{/|\\}}foodir
Index: clang/test/ClangScanDeps/Inputs/headerwithdirnamefollowedbyinclude.json
===
--- /dev/null
+++ clang/test/ClangScanDeps/Inputs/headerwithdirnamefollowedbyinclude.json
@@ -0,0 +1,7 @@
+[
+{
+  "directory": "DIR",
+  "command": "clang -c -IDIR -IInputs DIR/headerwithdirname_input.cpp",
+  "file": "DIR/headerwithdirname_input.cpp"
+}
+]
Index: clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
===
--- clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
+++ clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
@@ -233,8 +233,15 @@
 CachedFileSystemEntry  = SharedCacheEntry.Value;
 
 if (!CacheEntry.isValid()) {
+  llvm::vfs::FileSystem  = getUnderlyingFS();
+  auto MaybeStatus = FS.status(Filename);
+
+  if (MaybeStatus && MaybeStatus->isDirectory())
+return llvm::ErrorOr>(
+  std::make_error_code(std::errc::is_a_directory));
+
   CacheEntry = 

[PATCH] D67635: Fix for stringized function-macro args continued across lines

[Kousik Kumar via Phabricator via cfe-commits]

kousikk added a comment.

> I think you should obtain commit access for your future patches/commits. You 
> can follow the instructions here: 
> https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access

Thanks, I've done that!


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67635/new/

https://reviews.llvm.org/D67635



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D67635: Fix for stringized function-macro args continued across lines

[Kousik Kumar via Phabricator via cfe-commits]

kousikk added a comment.

Thanks! I will need you to merge this one too!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67635/new/

https://reviews.llvm.org/D67635



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D67635: Fix for stringized function-macro args continued across lines

[Kousik Kumar via Phabricator via cfe-commits]

kousikk added a comment.

Ping on the review for this :)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67635/new/

https://reviews.llvm.org/D67635



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D67635: Fix for stringized function-macro args continued across lines

[Kousik Kumar via Phabricator via cfe-commits]

kousikk created this revision.
kousikk added a reviewer: arphaman.
Herald added subscribers: cfe-commits, dexonsmith.
Herald added a project: clang.

In case of certain #define'd macros, there's a space just before line 
continuation
that the minimized-source lexer was missing to include, resulting in invalid 
stringize.
For example:
#define foo(x) x+7
#define bar(a,b) \

  #a \
  foo(b)

should result in "#a  foo(b)", but 
actually results in "#afoo(b)" (which is invalid).

Fixing it by keeping last but one space. I am confident that keeping the space 
is the right fix,
although I'm not sure if I've done it in the right way :)


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D67635

Files:
  clang/lib/Lex/DependencyDirectivesSourceMinimizer.cpp
  clang/unittests/Lex/DependencyDirectivesSourceMinimizerTest.cpp


Index: clang/unittests/Lex/DependencyDirectivesSourceMinimizerTest.cpp
===
--- clang/unittests/Lex/DependencyDirectivesSourceMinimizerTest.cpp
+++ clang/unittests/Lex/DependencyDirectivesSourceMinimizerTest.cpp
@@ -157,19 +157,19 @@
 
   ASSERT_FALSE(minimizeSourceToDependencyDirectives(
   "#define MACRO(\t)\tcon \t tent\t", Out));
-  EXPECT_STREQ("#define MACRO() con \t tent\n", Out.data());
+  EXPECT_STREQ("#define MACRO() con \t tent\t\n", Out.data());
 
   ASSERT_FALSE(minimizeSourceToDependencyDirectives(
   "#define MACRO(\f)\fcon \f tent\f", Out));
-  EXPECT_STREQ("#define MACRO() con \f tent\n", Out.data());
+  EXPECT_STREQ("#define MACRO() con \f tent\f\n", Out.data());
 
   ASSERT_FALSE(minimizeSourceToDependencyDirectives(
   "#define MACRO(\v)\vcon \v tent\v", Out));
-  EXPECT_STREQ("#define MACRO() con \v tent\n", Out.data());
+  EXPECT_STREQ("#define MACRO() con \v tent\v\n", Out.data());
 
   ASSERT_FALSE(minimizeSourceToDependencyDirectives(
   "#define MACRO \t\v\f\v\t con\f\t\vtent\v\f \v", Out));
-  EXPECT_STREQ("#define MACRO con\f\t\vtent\n", Out.data());
+  EXPECT_STREQ("#define MACRO con\f\t\vtent\v\n", Out.data());
 }
 
 TEST(MinimizeSourceToDependencyDirectivesTest, DefineMultilineArgs) {
@@ -187,7 +187,7 @@
"call((a),  \\\n"
" (b))",
Out));
-  EXPECT_STREQ("#define MACRO(a,b) call((a),(b))\n", Out.data());
+  EXPECT_STREQ("#define MACRO(a,b) call((a), (b))\n", Out.data());
 }
 
 TEST(MinimizeSourceToDependencyDirectivesTest,
@@ -200,7 +200,19 @@
"call((a),  \\\r"
" (b))",
Out));
-  EXPECT_STREQ("#define MACRO(a,b) call((a),(b))\n", Out.data());
+  EXPECT_STREQ("#define MACRO(a,b) call((a), (b))\n", Out.data());
+}
+
+TEST(MinimizeSourceToDependencyDirectivesTest,
+ DefineMultilineArgsStringize) {
+  SmallVector Out;
+
+  ASSERT_FALSE(
+  minimizeSourceToDependencyDirectives("#define MACRO(a,b) \\\n"
+   "#a \\\n"
+   "#b",
+   Out));
+  EXPECT_STREQ("#define MACRO(a,b) #a #b\n", Out.data());
 }
 
 TEST(MinimizeSourceToDependencyDirectivesTest,
@@ -213,7 +225,7 @@
"call((a),  \\\r\n"
" (b))",
Out));
-  EXPECT_STREQ("#define MACRO(a,b) call((a),(b))\n", Out.data());
+  EXPECT_STREQ("#define MACRO(a,b) call((a), (b))\n", Out.data());
 }
 
 TEST(MinimizeSourceToDependencyDirectivesTest,
@@ -226,7 +238,7 @@
"call((a),  \\\n\r"
" (b))",
Out));
-  EXPECT_STREQ("#define MACRO(a,b) call((a),(b))\n", Out.data());
+  EXPECT_STREQ("#define MACRO(a,b) call((a), (b))\n", Out.data());
 }
 
 TEST(MinimizeSourceToDependencyDirectivesTest, DefineNumber) {
Index: clang/lib/Lex/DependencyDirectivesSourceMinimizer.cpp
===
--- clang/lib/Lex/DependencyDirectivesSourceMinimizer.cpp
+++ clang/lib/Lex/DependencyDirectivesSourceMinimizer.cpp
@@ -246,9 +246,12 @@
 
 static const char *reverseOverSpaces(const char *First, const char *Last) {
   assert(First <= Last);
-  while (First != Last && isHorizontalWhitespace(Last[-1]))
+  const char *PrevLast = Last;
+  while (First != Last && isHorizontalWhitespace(Last[-1])) {
+PrevLast = Last;
 --Last;
-  return Last;
+  }
+  return PrevLast;
 }
 
 static void skipLineComment(const char *, const char *const End) {


Index: 

[PATCH] D67091: Fix for headers having the same name as a directory

[Kousik Kumar via Phabricator via cfe-commits]

kousikk added a comment.

I don't have commit access - do you mind submitting this for me? Thanks!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67091/new/

https://reviews.llvm.org/D67091



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D67091: Fix for headers having the same name as a directory

[Kousik Kumar via Phabricator via cfe-commits]

kousikk added a comment.

> I was unable to reproduce the failures when I moved the check to createFile. 
> What kind of failures did you see? Did you move it right to the start of the 
> function? I would recommend rebasing the patch and retrying.

I saw an assertion error - but I realize now that it was because it was not at 
the top of the function. I had put it after `Entry->getContents()` which was 
throwing an assertion. I now realize that for `getContents()` to work, the 
entry needs to be a file. Thanks for correcting me!

> Also, could you please rename headerwithdirname.cpp to 
> headerwithdirname_input.cpp when copying it to ensure FileCheck can match the 
> filename without matching the temporary path (see 
> https://reviews.llvm.org/D67379 for test fixes that @jkorous applied).

Done


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67091/new/

https://reviews.llvm.org/D67091



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D67091: Fix for headers having the same name as a directory

[Kousik Kumar via Phabricator via cfe-commits]

kousikk updated this revision to Diff 219976.
kousikk added a comment.

- Add validation inside the createFile() function


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67091/new/

https://reviews.llvm.org/D67091

Files:
  clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
  clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
  clang/test/ClangScanDeps/Inputs/foodir
  clang/test/ClangScanDeps/Inputs/headerwithdirname.json
  clang/test/ClangScanDeps/headerwithdirname.cpp


Index: clang/test/ClangScanDeps/headerwithdirname.cpp
===
--- /dev/null
+++ clang/test/ClangScanDeps/headerwithdirname.cpp
@@ -0,0 +1,17 @@
+// RUN: rm -rf %t.dir
+// RUN: rm -rf %t.dir/foodir
+// RUN: rm -rf %t.cdb
+// RUN: mkdir -p %t.dir
+// RUN: mkdir -p %t.dir/foodir
+// RUN: cp %s %t.dir/headerwithdirname_input.cpp
+// RUN: mkdir %t.dir/Inputs
+// RUN: cp %S/Inputs/foodir %t.dir/Inputs/foodir
+// RUN: sed -e "s|DIR|%/t.dir|g" %S/Inputs/headerwithdirname.json > %t.cdb
+//
+// RUN: clang-scan-deps -compilation-database %t.cdb -j 1 | FileCheck %s
+
+#include 
+
+// CHECK: headerwithdirname_input.o
+// CHECK-NEXT: headerwithdirname_input.cpp
+// CHECK-NEXT: Inputs{{/|\\}}foodir
Index: clang/test/ClangScanDeps/Inputs/headerwithdirname.json
===
--- /dev/null
+++ clang/test/ClangScanDeps/Inputs/headerwithdirname.json
@@ -0,0 +1,7 @@
+[
+{
+  "directory": "DIR",
+  "command": "clang -c -IDIR -IDIR/foodir -IInputs 
DIR/headerwithdirname_input.cpp",
+  "file": "DIR/headerwithdirname_input.cpp"
+}
+]
Index: clang/test/ClangScanDeps/Inputs/foodir
===
--- /dev/null
+++ clang/test/ClangScanDeps/Inputs/foodir
@@ -0,0 +1 @@
+// A C++ header with same name as that of a directory in the include path.
Index: clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
===
--- clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
+++ clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
@@ -193,6 +193,8 @@
 llvm::ErrorOr>
 createFile(const CachedFileSystemEntry *Entry,
ExcludedPreprocessorDirectiveSkipMapping *PPSkipMappings) {
+  if (Entry->isDirectory())
+return 
llvm::ErrorOr>(std::make_error_code(std::errc::is_a_directory));
   llvm::ErrorOr Contents = Entry->getContents();
   if (!Contents)
 return Contents.getError();
Index: 
clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
===
--- 
clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
+++ 
clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
@@ -56,6 +56,11 @@
   /// \returns True if the entry is valid.
   bool isValid() const { return !MaybeStat || MaybeStat->isStatusKnown(); }
 
+  /// \returns True if the current entry points to a directory.
+  bool isDirectory() const {
+return MaybeStat && MaybeStat->isDirectory();
+  }
+
   /// \returns The error or the file's contents.
   llvm::ErrorOr getContents() const {
 if (!MaybeStat)


Index: clang/test/ClangScanDeps/headerwithdirname.cpp
===
--- /dev/null
+++ clang/test/ClangScanDeps/headerwithdirname.cpp
@@ -0,0 +1,17 @@
+// RUN: rm -rf %t.dir
+// RUN: rm -rf %t.dir/foodir
+// RUN: rm -rf %t.cdb
+// RUN: mkdir -p %t.dir
+// RUN: mkdir -p %t.dir/foodir
+// RUN: cp %s %t.dir/headerwithdirname_input.cpp
+// RUN: mkdir %t.dir/Inputs
+// RUN: cp %S/Inputs/foodir %t.dir/Inputs/foodir
+// RUN: sed -e "s|DIR|%/t.dir|g" %S/Inputs/headerwithdirname.json > %t.cdb
+//
+// RUN: clang-scan-deps -compilation-database %t.cdb -j 1 | FileCheck %s
+
+#include 
+
+// CHECK: headerwithdirname_input.o
+// CHECK-NEXT: headerwithdirname_input.cpp
+// CHECK-NEXT: Inputs{{/|\\}}foodir
Index: clang/test/ClangScanDeps/Inputs/headerwithdirname.json
===
--- /dev/null
+++ clang/test/ClangScanDeps/Inputs/headerwithdirname.json
@@ -0,0 +1,7 @@
+[
+{
+  "directory": "DIR",
+  "command": "clang -c -IDIR -IDIR/foodir -IInputs DIR/headerwithdirname_input.cpp",
+  "file": "DIR/headerwithdirname_input.cpp"
+}
+]
Index: clang/test/ClangScanDeps/Inputs/foodir
===
--- /dev/null
+++ clang/test/ClangScanDeps/Inputs/foodir
@@ -0,0 +1 @@
+// A C++ header with same name as that of a directory in the include path.
Index: clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
===
--- 

[PATCH] D67091: Fix for headers having the same name as a directory

[Kousik Kumar via Phabricator via cfe-commits]

kousikk updated this revision to Diff 219975.
kousikk added a comment.

- Add validation inside the createFile() function


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67091/new/

https://reviews.llvm.org/D67091

Files:
  clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
  clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
  clang/test/ClangScanDeps/Inputs/foodir
  clang/test/ClangScanDeps/Inputs/headerwithdirname.json
  clang/test/ClangScanDeps/headerwithdirname.cpp


Index: clang/test/ClangScanDeps/headerwithdirname.cpp
===
--- /dev/null
+++ clang/test/ClangScanDeps/headerwithdirname.cpp
@@ -0,0 +1,17 @@
+// RUN: rm -rf %t.dir
+// RUN: rm -rf %t.dir/foodir
+// RUN: rm -rf %t.cdb
+// RUN: mkdir -p %t.dir
+// RUN: mkdir -p %t.dir/foodir
+// RUN: cp %s %t.dir/headerwithdirname_input.cpp
+// RUN: mkdir %t.dir/Inputs
+// RUN: cp %S/Inputs/foodir %t.dir/Inputs/foodir
+// RUN: sed -e "s|DIR|%/t.dir|g" %S/Inputs/headerwithdirname.json > %t.cdb
+//
+// RUN: clang-scan-deps -compilation-database %t.cdb -j 1 | FileCheck %s
+
+#include 
+
+// CHECK: headerwithdirname_input.o
+// CHECK-NEXT: headerwithdirname_input.cpp
+// CHECK-NEXT: Inputs{{/|\\}}foodir
Index: clang/test/ClangScanDeps/Inputs/headerwithdirname.json
===
--- /dev/null
+++ clang/test/ClangScanDeps/Inputs/headerwithdirname.json
@@ -0,0 +1,7 @@
+[
+{
+  "directory": "DIR",
+  "command": "clang -c -IDIR -IDIR/foodir -IInputs 
DIR/headerwithdirname_input.cpp",
+  "file": "DIR/headerwithdirname_input.cpp"
+}
+]
Index: clang/test/ClangScanDeps/Inputs/foodir
===
--- /dev/null
+++ clang/test/ClangScanDeps/Inputs/foodir
@@ -0,0 +1 @@
+// A C++ header with same name as that of a directory in the include path.
Index: clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
===
--- clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
+++ clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
@@ -193,6 +193,8 @@
 llvm::ErrorOr>
 createFile(const CachedFileSystemEntry *Entry,
ExcludedPreprocessorDirectiveSkipMapping *PPSkipMappings) {
+  if (Entry->isDirectory())
+return 
llvm::ErrorOr>(std::make_error_code(std::errc::is_a_directory));
   llvm::ErrorOr Contents = Entry->getContents();
   if (!Contents)
 return Contents.getError();
@@ -215,8 +217,9 @@
   StringRef Filename = Path.toStringRef(OwnedFilename);
 
   // Check the local cache first.
-  if (const CachedFileSystemEntry *Entry = getCachedEntry(Filename))
+  if (const CachedFileSystemEntry *Entry = getCachedEntry(Filename)) {
 return createFile(Entry, PPSkipMappings);
+  }
 
   // FIXME: Handle PCM/PCH files.
   // FIXME: Handle module map files.
Index: 
clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
===
--- 
clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
+++ 
clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
@@ -56,6 +56,11 @@
   /// \returns True if the entry is valid.
   bool isValid() const { return !MaybeStat || MaybeStat->isStatusKnown(); }
 
+  /// \returns True if the current entry points to a directory.
+  bool isDirectory() const {
+return MaybeStat && MaybeStat->isDirectory();
+  }
+
   /// \returns The error or the file's contents.
   llvm::ErrorOr getContents() const {
 if (!MaybeStat)


Index: clang/test/ClangScanDeps/headerwithdirname.cpp
===
--- /dev/null
+++ clang/test/ClangScanDeps/headerwithdirname.cpp
@@ -0,0 +1,17 @@
+// RUN: rm -rf %t.dir
+// RUN: rm -rf %t.dir/foodir
+// RUN: rm -rf %t.cdb
+// RUN: mkdir -p %t.dir
+// RUN: mkdir -p %t.dir/foodir
+// RUN: cp %s %t.dir/headerwithdirname_input.cpp
+// RUN: mkdir %t.dir/Inputs
+// RUN: cp %S/Inputs/foodir %t.dir/Inputs/foodir
+// RUN: sed -e "s|DIR|%/t.dir|g" %S/Inputs/headerwithdirname.json > %t.cdb
+//
+// RUN: clang-scan-deps -compilation-database %t.cdb -j 1 | FileCheck %s
+
+#include 
+
+// CHECK: headerwithdirname_input.o
+// CHECK-NEXT: headerwithdirname_input.cpp
+// CHECK-NEXT: Inputs{{/|\\}}foodir
Index: clang/test/ClangScanDeps/Inputs/headerwithdirname.json
===
--- /dev/null
+++ clang/test/ClangScanDeps/Inputs/headerwithdirname.json
@@ -0,0 +1,7 @@
+[
+{
+  "directory": "DIR",
+  "command": "clang -c -IDIR -IDIR/foodir -IInputs DIR/headerwithdirname_input.cpp",
+  "file": "DIR/headerwithdirname_input.cpp"
+}
+]
Index: clang/test/ClangScanDeps/Inputs/foodir

[PATCH] D67091: Fix for headers having the same name as a directory

[Kousik Kumar via Phabricator via cfe-commits]

kousikk added a comment.

Sorry about the delay on this - I was OOO (back now).

1. I added tests.
2. I couldn't add isDirectory() check to createFile() since that resulted in 
failures to normal scenarios where it was previously passing.

PTAL!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67091/new/

https://reviews.llvm.org/D67091



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D67091: Fix for headers having the same name as a directory

[Kousik Kumar via Phabricator via cfe-commits]

kousikk updated this revision to Diff 219918.
kousikk added a comment.

- Add validation inside the createFile() function


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67091/new/

https://reviews.llvm.org/D67091

Files:
  clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
  clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
  clang/test/ClangScanDeps/Inputs/foodir
  clang/test/ClangScanDeps/Inputs/headerwithdirname.json
  clang/test/ClangScanDeps/headerwithdirname.cpp


Index: clang/test/ClangScanDeps/headerwithdirname.cpp
===
--- /dev/null
+++ clang/test/ClangScanDeps/headerwithdirname.cpp
@@ -0,0 +1,17 @@
+// RUN: rm -rf %t.dir
+// RUN: rm -rf %t.dir/foodir
+// RUN: rm -rf %t.cdb
+// RUN: mkdir -p %t.dir
+// RUN: mkdir -p %t.dir/foodir
+// RUN: cp %s %t.dir/headerwithdirname.cpp
+// RUN: mkdir %t.dir/Inputs
+// RUN: cp %S/Inputs/foodir %t.dir/Inputs/foodir
+// RUN: sed -e "s|DIR|%/t.dir|g" %S/Inputs/headerwithdirname.json > %t.cdb
+//
+// RUN: clang-scan-deps -compilation-database %t.cdb -j 1 | FileCheck %s
+
+#include 
+
+// CHECK: clang-scan-deps dependency
+// CHECK-NEXT: headerwithdirname.cpp
+// CHECK-NEXT: Inputs{{/|\\}}foodir
\ No newline at end of file
Index: clang/test/ClangScanDeps/Inputs/headerwithdirname.json
===
--- /dev/null
+++ clang/test/ClangScanDeps/Inputs/headerwithdirname.json
@@ -0,0 +1,7 @@
+[
+{
+  "directory": "DIR",
+  "command": "clang -c -IDIR -IDIR/foodir -IInputs 
DIR/headerwithdirname.cpp",
+  "file": "DIR/headerwithdirname.cpp"
+}
+]
Index: clang/test/ClangScanDeps/Inputs/foodir
===
--- /dev/null
+++ clang/test/ClangScanDeps/Inputs/foodir
@@ -0,0 +1 @@
+// A C++ header with same name as that of a directory in the include path.
Index: clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
===
--- clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
+++ clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
@@ -190,8 +190,11 @@
   StringRef Filename = Path.toStringRef(OwnedFilename);
 
   // Check the local cache first.
-  if (const CachedFileSystemEntry *Entry = getCachedEntry(Filename))
+  if (const CachedFileSystemEntry *Entry = getCachedEntry(Filename)) {
+if (Entry->isDirectory())
+  return 
llvm::ErrorOr>(std::make_error_code(std::errc::is_a_directory));
 return createFile(Entry);
+  }
 
   // FIXME: Handle PCM/PCH files.
   // FIXME: Handle module map files.
Index: 
clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
===
--- 
clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
+++ 
clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
@@ -55,6 +55,11 @@
   /// \returns True if the entry is valid.
   bool isValid() const { return !MaybeStat || MaybeStat->isStatusKnown(); }
 
+  /// \returns True if the current entry points to a directory.
+  bool isDirectory() const {
+return MaybeStat && MaybeStat->isDirectory();
+  }
+
   /// \returns The error or the file's contents.
   llvm::ErrorOr getContents() const {
 if (!MaybeStat)


Index: clang/test/ClangScanDeps/headerwithdirname.cpp
===
--- /dev/null
+++ clang/test/ClangScanDeps/headerwithdirname.cpp
@@ -0,0 +1,17 @@
+// RUN: rm -rf %t.dir
+// RUN: rm -rf %t.dir/foodir
+// RUN: rm -rf %t.cdb
+// RUN: mkdir -p %t.dir
+// RUN: mkdir -p %t.dir/foodir
+// RUN: cp %s %t.dir/headerwithdirname.cpp
+// RUN: mkdir %t.dir/Inputs
+// RUN: cp %S/Inputs/foodir %t.dir/Inputs/foodir
+// RUN: sed -e "s|DIR|%/t.dir|g" %S/Inputs/headerwithdirname.json > %t.cdb
+//
+// RUN: clang-scan-deps -compilation-database %t.cdb -j 1 | FileCheck %s
+
+#include 
+
+// CHECK: clang-scan-deps dependency
+// CHECK-NEXT: headerwithdirname.cpp
+// CHECK-NEXT: Inputs{{/|\\}}foodir
\ No newline at end of file
Index: clang/test/ClangScanDeps/Inputs/headerwithdirname.json
===
--- /dev/null
+++ clang/test/ClangScanDeps/Inputs/headerwithdirname.json
@@ -0,0 +1,7 @@
+[
+{
+  "directory": "DIR",
+  "command": "clang -c -IDIR -IDIR/foodir -IInputs DIR/headerwithdirname.cpp",
+  "file": "DIR/headerwithdirname.cpp"
+}
+]
Index: clang/test/ClangScanDeps/Inputs/foodir
===
--- /dev/null
+++ clang/test/ClangScanDeps/Inputs/foodir
@@ -0,0 +1 @@
+// A C++ header with same name as that of a directory in the include path.
Index: clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp

[PATCH] D67091: Fix for headers having the same name as a directory

[Kousik Kumar via Phabricator via cfe-commits]

kousikk updated this revision to Diff 219917.
kousikk added a comment.

- Add tests and remove the fix inside createFile since it fails in other cases


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67091/new/

https://reviews.llvm.org/D67091

Files:
  clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
  clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
  clang/test/ClangScanDeps/Inputs/foodir
  clang/test/ClangScanDeps/Inputs/headerwithdirname.json
  clang/test/ClangScanDeps/headerwithdirname.cpp


Index: clang/test/ClangScanDeps/headerwithdirname.cpp
===
--- /dev/null
+++ clang/test/ClangScanDeps/headerwithdirname.cpp
@@ -0,0 +1,17 @@
+// RUN: rm -rf %t.dir
+// RUN: rm -rf %t.dir/foodir
+// RUN: rm -rf %t.cdb
+// RUN: mkdir -p %t.dir
+// RUN: mkdir -p %t.dir/foodir
+// RUN: cp %s %t.dir/headerwithdirname.cpp
+// RUN: mkdir %t.dir/Inputs
+// RUN: cp %S/Inputs/foodir %t.dir/Inputs/foodir
+// RUN: sed -e "s|DIR|%/t.dir|g" %S/Inputs/headerwithdirname.json > %t.cdb
+//
+// RUN: clang-scan-deps -compilation-database %t.cdb -j 1 | FileCheck %s
+
+#include 
+
+// CHECK: clang-scan-deps dependency
+// CHECK-NEXT: headerwithdirname.cpp
+// CHECK-NEXT: Inputs{{/|\\}}foodir
\ No newline at end of file
Index: clang/test/ClangScanDeps/Inputs/headerwithdirname.json
===
--- /dev/null
+++ clang/test/ClangScanDeps/Inputs/headerwithdirname.json
@@ -0,0 +1,7 @@
+[
+{
+  "directory": "DIR",
+  "command": "clang -c -IDIR -IDIR/foodir -IInputs 
DIR/headerwithdirname.cpp",
+  "file": "DIR/headerwithdirname.cpp"
+}
+]
Index: clang/test/ClangScanDeps/Inputs/foodir
===
--- /dev/null
+++ clang/test/ClangScanDeps/Inputs/foodir
@@ -0,0 +1 @@
+// A C++ header with same name as that of a directory in the include path.
\ No newline at end of file
Index: clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
===
--- clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
+++ clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
@@ -190,8 +190,11 @@
   StringRef Filename = Path.toStringRef(OwnedFilename);
 
   // Check the local cache first.
-  if (const CachedFileSystemEntry *Entry = getCachedEntry(Filename))
+  if (const CachedFileSystemEntry *Entry = getCachedEntry(Filename)) {
+if (Entry->isDirectory())
+  return 
llvm::ErrorOr>(std::make_error_code(std::errc::is_a_directory));
 return createFile(Entry);
+  }
 
   // FIXME: Handle PCM/PCH files.
   // FIXME: Handle module map files.
Index: 
clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
===
--- 
clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
+++ 
clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
@@ -55,6 +55,11 @@
   /// \returns True if the entry is valid.
   bool isValid() const { return !MaybeStat || MaybeStat->isStatusKnown(); }
 
+  /// \returns True if the current entry points to a directory.
+  bool isDirectory() const {
+return MaybeStat && MaybeStat->isDirectory();
+  }
+
   /// \returns The error or the file's contents.
   llvm::ErrorOr getContents() const {
 if (!MaybeStat)


Index: clang/test/ClangScanDeps/headerwithdirname.cpp
===
--- /dev/null
+++ clang/test/ClangScanDeps/headerwithdirname.cpp
@@ -0,0 +1,17 @@
+// RUN: rm -rf %t.dir
+// RUN: rm -rf %t.dir/foodir
+// RUN: rm -rf %t.cdb
+// RUN: mkdir -p %t.dir
+// RUN: mkdir -p %t.dir/foodir
+// RUN: cp %s %t.dir/headerwithdirname.cpp
+// RUN: mkdir %t.dir/Inputs
+// RUN: cp %S/Inputs/foodir %t.dir/Inputs/foodir
+// RUN: sed -e "s|DIR|%/t.dir|g" %S/Inputs/headerwithdirname.json > %t.cdb
+//
+// RUN: clang-scan-deps -compilation-database %t.cdb -j 1 | FileCheck %s
+
+#include 
+
+// CHECK: clang-scan-deps dependency
+// CHECK-NEXT: headerwithdirname.cpp
+// CHECK-NEXT: Inputs{{/|\\}}foodir
\ No newline at end of file
Index: clang/test/ClangScanDeps/Inputs/headerwithdirname.json
===
--- /dev/null
+++ clang/test/ClangScanDeps/Inputs/headerwithdirname.json
@@ -0,0 +1,7 @@
+[
+{
+  "directory": "DIR",
+  "command": "clang -c -IDIR -IDIR/foodir -IInputs DIR/headerwithdirname.cpp",
+  "file": "DIR/headerwithdirname.cpp"
+}
+]
Index: clang/test/ClangScanDeps/Inputs/foodir
===
--- /dev/null
+++ clang/test/ClangScanDeps/Inputs/foodir
@@ -0,0 +1 @@
+// A C++ header with same name as that of a directory in the include path.
\ No newline at end of 

[PATCH] D67091: Fix for headers having the same name as a directory

[Kousik Kumar via Phabricator via cfe-commits]

kousikk updated this revision to Diff 219890.
kousikk added a comment.

- Add validation inside the createFile() function


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67091/new/

https://reviews.llvm.org/D67091

Files:
  clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
  clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp


Index: clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
===
--- clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
+++ clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
@@ -176,6 +176,8 @@
   llvm::ErrorOr Contents = Entry->getContents();
   if (!Contents)
 return Contents.getError();
+  if (Entry->isDirectory())
+return 
llvm::ErrorOr>(std::make_error_code(std::errc::is_a_directory));
   return std::make_unique(
   llvm::MemoryBuffer::getMemBuffer(*Contents, Entry->getName(),
/*RequiresNullTerminator=*/false),
@@ -191,7 +193,7 @@
 
   // Check the local cache first.
   if (const CachedFileSystemEntry *Entry = getCachedEntry(Filename))
-return createFile(Entry);
+createFile(Entry);
 
   // FIXME: Handle PCM/PCH files.
   // FIXME: Handle module map files.
Index: 
clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
===
--- 
clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
+++ 
clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
@@ -55,6 +55,11 @@
   /// \returns True if the entry is valid.
   bool isValid() const { return !MaybeStat || MaybeStat->isStatusKnown(); }
 
+  /// \returns True if the current entry points to a directory.
+  bool isDirectory() const {
+return MaybeStat && MaybeStat->isDirectory();
+  }
+
   /// \returns The error or the file's contents.
   llvm::ErrorOr getContents() const {
 if (!MaybeStat)


Index: clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
===
--- clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
+++ clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
@@ -176,6 +176,8 @@
   llvm::ErrorOr Contents = Entry->getContents();
   if (!Contents)
 return Contents.getError();
+  if (Entry->isDirectory())
+return llvm::ErrorOr>(std::make_error_code(std::errc::is_a_directory));
   return std::make_unique(
   llvm::MemoryBuffer::getMemBuffer(*Contents, Entry->getName(),
/*RequiresNullTerminator=*/false),
@@ -191,7 +193,7 @@
 
   // Check the local cache first.
   if (const CachedFileSystemEntry *Entry = getCachedEntry(Filename))
-return createFile(Entry);
+createFile(Entry);
 
   // FIXME: Handle PCM/PCH files.
   // FIXME: Handle module map files.
Index: clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
===
--- clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
+++ clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
@@ -55,6 +55,11 @@
   /// \returns True if the entry is valid.
   bool isValid() const { return !MaybeStat || MaybeStat->isStatusKnown(); }
 
+  /// \returns True if the current entry points to a directory.
+  bool isDirectory() const {
+return MaybeStat && MaybeStat->isDirectory();
+  }
+
   /// \returns The error or the file's contents.
   llvm::ErrorOr getContents() const {
 if (!MaybeStat)
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D67091: Fix for headers having the same name as a directory

[Kousik Kumar via Phabricator via cfe-commits]

kousikk updated this revision to Diff 219891.
kousikk added a comment.

- Add validation inside the createFile() function


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67091/new/

https://reviews.llvm.org/D67091

Files:
  clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
  clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp


Index: clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
===
--- clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
+++ clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
@@ -176,6 +176,8 @@
   llvm::ErrorOr Contents = Entry->getContents();
   if (!Contents)
 return Contents.getError();
+  if (Entry->isDirectory())
+return 
llvm::ErrorOr>(std::make_error_code(std::errc::is_a_directory));
   return std::make_unique(
   llvm::MemoryBuffer::getMemBuffer(*Contents, Entry->getName(),
/*RequiresNullTerminator=*/false),
Index: 
clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
===
--- 
clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
+++ 
clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
@@ -55,6 +55,11 @@
   /// \returns True if the entry is valid.
   bool isValid() const { return !MaybeStat || MaybeStat->isStatusKnown(); }
 
+  /// \returns True if the current entry points to a directory.
+  bool isDirectory() const {
+return MaybeStat && MaybeStat->isDirectory();
+  }
+
   /// \returns The error or the file's contents.
   llvm::ErrorOr getContents() const {
 if (!MaybeStat)


Index: clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
===
--- clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
+++ clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
@@ -176,6 +176,8 @@
   llvm::ErrorOr Contents = Entry->getContents();
   if (!Contents)
 return Contents.getError();
+  if (Entry->isDirectory())
+return llvm::ErrorOr>(std::make_error_code(std::errc::is_a_directory));
   return std::make_unique(
   llvm::MemoryBuffer::getMemBuffer(*Contents, Entry->getName(),
/*RequiresNullTerminator=*/false),
Index: clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
===
--- clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
+++ clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
@@ -55,6 +55,11 @@
   /// \returns True if the entry is valid.
   bool isValid() const { return !MaybeStat || MaybeStat->isStatusKnown(); }
 
+  /// \returns True if the current entry points to a directory.
+  bool isDirectory() const {
+return MaybeStat && MaybeStat->isDirectory();
+  }
+
   /// \returns The error or the file's contents.
   llvm::ErrorOr getContents() const {
 if (!MaybeStat)
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D67091: Fix for headers having the same name as a directory

[Kousik Kumar via Phabricator via cfe-commits]

kousikk added a comment.

Sure I'll add a test case!




Comment at: 
clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp:196
+  return 
llvm::ErrorOr>(std::make_error_code(std::errc::is_a_directory));
+}
+  }

arphaman wrote:
> This change dropped the createFile call, and didn't fix the issue where the 
> same could happen at the end of the function. Could you please perform this 
> check and return inside of `createFile` instead? This would ensure that both 
> uses are fixed.
Ah sorry, I didn't mean to drop the `createFile()` call. Sure will fix inside 
createFile() instead.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67091/new/

https://reviews.llvm.org/D67091



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D67091: Fix for headers having the same name as a directory

[Kousik Kumar via Phabricator via cfe-commits]

kousikk created this revision.
kousikk added a reviewer: arphaman.
Herald added subscribers: cfe-commits, jfb, dexonsmith.
Herald added a project: clang.

Scan deps tool crashes when called on a C++ file, containing an include
that has the same name as a directory. For example:

test.cpp:
#include 
int main() { return 0; }

In directory foo/ :
dir/ bar/dir(file)

Now if the compile command is:
clang++ -I foo/ -I foo/bar/ -c test.cpp

The tool crashes since it finds foo/dir and tries to read that as a file and 
fails.
In this change, I add a defence against that scenario in the Dependency Scanning
File system.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D67091

Files:
  clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
  clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp


Index: clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
===
--- clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
+++ clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
@@ -190,8 +190,11 @@
   StringRef Filename = Path.toStringRef(OwnedFilename);
 
   // Check the local cache first.
-  if (const CachedFileSystemEntry *Entry = getCachedEntry(Filename))
-return createFile(Entry);
+  if (const CachedFileSystemEntry *Entry = getCachedEntry(Filename)) {
+if (Entry->isDirectory()) {
+  return 
llvm::ErrorOr>(std::make_error_code(std::errc::is_a_directory));
+}
+  }
 
   // FIXME: Handle PCM/PCH files.
   // FIXME: Handle module map files.
Index: 
clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
===
--- 
clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
+++ 
clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
@@ -55,6 +55,11 @@
   /// \returns True if the entry is valid.
   bool isValid() const { return !MaybeStat || MaybeStat->isStatusKnown(); }
 
+  /// \returns True if the current entry points to a directory.
+  bool isDirectory() const {
+return MaybeStat && MaybeStat->isDirectory();
+  }
+
   /// \returns The error or the file's contents.
   llvm::ErrorOr getContents() const {
 if (!MaybeStat)


Index: clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
===
--- clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
+++ clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
@@ -190,8 +190,11 @@
   StringRef Filename = Path.toStringRef(OwnedFilename);
 
   // Check the local cache first.
-  if (const CachedFileSystemEntry *Entry = getCachedEntry(Filename))
-return createFile(Entry);
+  if (const CachedFileSystemEntry *Entry = getCachedEntry(Filename)) {
+if (Entry->isDirectory()) {
+  return llvm::ErrorOr>(std::make_error_code(std::errc::is_a_directory));
+}
+  }
 
   // FIXME: Handle PCM/PCH files.
   // FIXME: Handle module map files.
Index: clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
===
--- clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
+++ clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
@@ -55,6 +55,11 @@
   /// \returns True if the entry is valid.
   bool isValid() const { return !MaybeStat || MaybeStat->isStatusKnown(); }
 
+  /// \returns True if the current entry points to a directory.
+  bool isDirectory() const {
+return MaybeStat && MaybeStat->isDirectory();
+  }
+
   /// \returns The error or the file's contents.
   llvm::ErrorOr getContents() const {
 if (!MaybeStat)
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits