Fix for headers having the same name as a directory

[kousikk]

Summary

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

[arphaman]

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.

clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp

199

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.

[kousikk]

Sure I'll add a test case!

clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp

199

Ah sorry, I didn't mean to drop the createFile() call. Sure will fix inside createFile() instead.

[kousikk]

• Add validation inside the createFile() function

[kousikk]

• Add validation inside the createFile() function

[kousikk]

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

[kousikk]

• Add validation inside the createFile() function

[kousikk]

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!

[arphaman]

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

[kousikk]

• Add validation inside the createFile() function

[kousikk]

• Add validation inside the createFile() function

[kousikk]

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

[arphaman]

LGTM, Thanks!

[kousikk]

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

[arphaman]

Sure, I can do that today.

clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h

static CachedFileSystemEntry createFileEntry(StringRef Filename,
                                                bool Minimize = true);
 
   /// Create an entry that represents a directory on the filesystem.
   static CachedFileSystemEntry createDirectoryEntry(llvm::vfs::Status &&Stat);
 
   /// \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<StringRef> getContents() const {
     if (!MaybeStat)
       return MaybeStat.getError();
     assert(!MaybeStat->isDirectory() && "not a file");
     assert(isValid() && "not initialized");
     return StringRef(Contents);
   }

clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp

private:
   llvm::vfs::Status Stat;
 };
 
 llvm::ErrorOr<std::unique_ptr<llvm::vfs::File>>
 createFile(const CachedFileSystemEntry *Entry) {
   llvm::ErrorOr<StringRef> Contents = Entry->getContents();
   if (!Contents)
     return Contents.getError();
+  if (Entry->isDirectory())
+    return llvm::ErrorOr<std::unique_ptr<llvm::vfs::File>>(std::make_error_code(std::errc::is_a_directory));
   return std::make_unique<MinimizedVFSFile>(
       llvm::MemoryBuffer::getMemBuffer(*Contents, Entry->getName(),
                                        /*RequiresNullTerminator=*/false),
       *Entry->getStatus());
 }
 
 } // end anonymous namespace
 
 llvm::ErrorOr<std::unique_ptr<llvm::vfs::File>>
 DependencyScanningWorkerFilesystem::openFileForRead(const Twine &Path) {
   SmallString<256> OwnedFilename;
   StringRef Filename = Path.toStringRef(OwnedFilename);
 
   // 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.
 
   bool KeepOriginalSource = IgnoredFiles.count(Filename);
   DependencyScanningFilesystemSharedCache::SharedFileSystemEntry
       &SharedCacheEntry = SharedCache.get(Filename);
   const CachedFileSystemEntry *Result;
   {
     std::unique_lock<std::mutex> LockGuard(SharedCacheEntry.ValueLock);
     CachedFileSystemEntry &CacheEntry = SharedCacheEntry.Value;