[clang] 03a0f4b - [clang/HeaderSearch] Make sure `loadSubdirectoryModuleMaps` doesn't cause loading of regular files

[Argyrios Kyrtzidis via cfe-commits]

Author: Argyrios Kyrtzidis
Date: 2023-06-26T10:18:02-07:00
New Revision: 03a0f4b61ca50a267a405a29ff1986473a55f9d9

URL: https://github.com/llvm/llvm-project/commit/03a0f4b61ca50a267a405a29ff1986473a55f9d9
DIFF: https://github.com/llvm/llvm-project/commit/03a0f4b61ca50a267a405a29ff1986473a55f9d9.diff

LOG: [clang/HeaderSearch] Make sure `loadSubdirectoryModuleMaps` doesn't cause loading of regular files

`HeaderSearch::loadSubdirectoryModuleMaps` `stat`s all the files in a directory which causes the dependency scanning
service to load and cache their contents. This is problematic because a file may be in the process of being generated
and could be cached by the dep-scan service while it is still incomplete.

To address this change `loadSubdirectoryModuleMaps` to ignore regular files.

Differential Revision: https://reviews.llvm.org/D153670

Added: 
    

Modified: 
    clang/lib/Lex/HeaderSearch.cpp
    clang/unittests/Tooling/DependencyScannerTest.cpp

Removed: 
    


################################################################################
diff  --git a/clang/lib/Lex/HeaderSearch.cpp b/clang/lib/Lex/HeaderSearch.cpp
index 723479ca4fbbc..6fe655fb24277 100644
--- a/clang/lib/Lex/HeaderSearch.cpp
+++ b/clang/lib/Lex/HeaderSearch.cpp
@@ -1917,6 +1917,8 @@ void HeaderSearch::loadSubdirectoryModuleMaps(DirectoryLookup &SearchDir) {
   llvm::vfs::FileSystem &FS = FileMgr.getVirtualFileSystem();
   for (llvm::vfs::directory_iterator Dir = FS.dir_begin(DirNative, EC), DirEnd;
        Dir != DirEnd && !EC; Dir.increment(EC)) {
+    if (Dir->type() == llvm::sys::fs::file_type::regular_file)
+      continue;
     bool IsFramework = llvm::sys::path::extension(Dir->path()) == ".framework";
     if (IsFramework == SearchDir.isFramework())
       loadModuleMapFile(Dir->path(), SearchDir.isSystemHeaderDirectory(),

diff  --git a/clang/unittests/Tooling/DependencyScannerTest.cpp b/clang/unittests/Tooling/DependencyScannerTest.cpp
index abcc2c787b0d0..8735fcad20046 100644
--- a/clang/unittests/Tooling/DependencyScannerTest.cpp
+++ b/clang/unittests/Tooling/DependencyScannerTest.cpp
@@ -239,3 +239,65 @@ TEST(DependencyScanner, ScanDepsWithFS) {
   EXPECT_EQ(convert_to_slash(DepFile),
             "test.cpp.o: /root/test.cpp /root/header.h\n");
 }
+
+TEST(DependencyScanner, ScanDepsWithModuleLookup) {
+  std::vector<std::string> CommandLine = {
+      "clang",
+      "-target",
+      "x86_64-apple-macosx10.7",
+      "-c",
+      "test.m",
+      "-o"
+      "test.m.o",
+      "-fmodules",
+      "-I/root/SomeSources",
+  };
+  StringRef CWD = "/root";
+
+  auto VFS = new llvm::vfs::InMemoryFileSystem();
+  VFS->setCurrentWorkingDirectory(CWD);
+  auto Sept = llvm::sys::path::get_separator();
+  std::string OtherPath =
+      std::string(llvm::formatv("{0}root{0}SomeSources{0}other.h", Sept));
+  std::string TestPath = std::string(llvm::formatv("{0}root{0}test.m", Sept));
+
+  VFS->addFile(OtherPath, 0, llvm::MemoryBuffer::getMemBuffer("\n"));
+  VFS->addFile(TestPath, 0, llvm::MemoryBuffer::getMemBuffer("@import Foo;\n"));
+
+  struct InterceptorFS : llvm::vfs::ProxyFileSystem {
+    std::vector<std::string> StatPaths;
+    std::vector<std::string> ReadFiles;
+
+    InterceptorFS(IntrusiveRefCntPtr<FileSystem> UnderlyingFS)
+        : ProxyFileSystem(UnderlyingFS) {}
+
+    llvm::ErrorOr<llvm::vfs::Status> status(const Twine &Path) override {
+      StatPaths.push_back(Path.str());
+      return ProxyFileSystem::status(Path);
+    }
+
+    llvm::ErrorOr<std::unique_ptr<llvm::vfs::File>>
+    openFileForRead(const Twine &Path) override {
+      ReadFiles.push_back(Path.str());
+      return ProxyFileSystem::openFileForRead(Path);
+    }
+  };
+
+  auto InterceptFS = llvm::makeIntrusiveRefCnt<InterceptorFS>(VFS);
+
+  DependencyScanningService Service(ScanningMode::DependencyDirectivesScan,
+                                    ScanningOutputFormat::Make);
+  DependencyScanningTool ScanTool(Service, InterceptFS);
+
+  // This will fail with "fatal error: module 'Foo' not found" but it doesn't
+  // matter, the point of the test is to check that files are not read
+  // unnecessarily.
+  std::string DepFile;
+  ASSERT_THAT_ERROR(
+      ScanTool.getDependencyFile(CommandLine, CWD).moveInto(DepFile),
+      llvm::Failed());
+
+  EXPECT_TRUE(llvm::find(InterceptFS->StatPaths, OtherPath) ==
+              InterceptFS->StatPaths.end());
+  EXPECT_EQ(InterceptFS->ReadFiles, std::vector<std::string>{"test.m"});
+}