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

[Alex Lorenz via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL371903: [clang-scan-deps] Fix for headers having the same 
name as a directory (authored by arphaman, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D67091?vs=219976=220181#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D67091

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


Index: cfe/trunk/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
===
--- cfe/trunk/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
+++ cfe/trunk/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
@@ -193,6 +193,9 @@
 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: 
cfe/trunk/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
===
--- 
cfe/trunk/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
+++ 
cfe/trunk/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
@@ -56,6 +56,9 @@
   /// \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: cfe/trunk/test/ClangScanDeps/headerwithdirname.cpp
===
--- cfe/trunk/test/ClangScanDeps/headerwithdirname.cpp
+++ cfe/trunk/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: cfe/trunk/test/ClangScanDeps/Inputs/foodir
===
--- cfe/trunk/test/ClangScanDeps/Inputs/foodir
+++ cfe/trunk/test/ClangScanDeps/Inputs/foodir
@@ -0,0 +1 @@
+// A C++ header with same name as that of a directory in the include path.
Index: cfe/trunk/test/ClangScanDeps/Inputs/headerwithdirname.json
===
--- cfe/trunk/test/ClangScanDeps/Inputs/headerwithdirname.json
+++ cfe/trunk/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: cfe/trunk/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
===
--- cfe/trunk/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
+++ cfe/trunk/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
@@ -193,6 +193,9 @@
 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: cfe/trunk/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
===
--- cfe/trunk/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
+++ cfe/trunk/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
@@ -56,6 +56,9 @@
   /// \returns True if the entry is valid.
   bool isValid() const { return !MaybeStat || MaybeStat->isStatusKnown(); }
 
+  /// \returns True if the current entry points to a 

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

[Alex Lorenz via Phabricator via cfe-commits]

arphaman added a comment.

Sure, I can do that today.


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

[Alex Lorenz via Phabricator via cfe-commits]

arphaman accepted this revision.
arphaman added a comment.
This revision is now accepted and ready to land.

LGTM, 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

[Alex Lorenz via Phabricator via cfe-commits]

arphaman added a subscriber: jkorous.
arphaman added a comment.

In D67091#1667821 , @kousikk wrote:

> 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!


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.

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


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.

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

[Alex Lorenz via Phabricator via cfe-commits]

arphaman added a comment.

Thanks for fixing this! Could you add a test case which verifies that the 
assertion no longer happens? Let me know if you need help coming up with a test.




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

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.


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