[clang][deps] Avoid leaking modulemap paths across unrelated imports

[benlangmuir]

Summary

Use a FileEntryRef when retrieving modulemap paths in the scanner so that we use a path compatible with the original module import, rather than a FileEntry which can allow unrelated modules to leak paths into how we build a module due to FileManager mutating the path.

Note: the current change prevents an "unrelated" path, but does not change how VFS mapped paths are handled (which would be calling getNameAsRequested) nor canonicalize the path. In that sense, this is a narrower version of https://reviews.llvm.org/D135636 that should not be blocked by the VFS concerns.

[jansvoboda11]

LGTM, thanks!

clang/lib/Serialization/ASTWriter.cpp

1560–1561

Do you think resolving this fixme warrants separate patch?

[benlangmuir]

clang/lib/Serialization/ASTWriter.cpp

1560–1561

Good catch, I'll remove this FIXME. Usually diagnostics use the on-disk path, so I think FileEntryRef::getName is correct here and we don't need to change it to getNameAsRequested.

[benlangmuir]

• Removed outdated FIXME
• Ran clang-format
• Attempt to fix Windows test failure: the StringSet of paths was failing to match, so switch to DenseSet<FileEntry *>. Note: the path that ends up in the command-line is from a FileEntryRef, but the equality of FileEntry * is how we check whether to include it at all. For now we just re-lookup the paths which should always hit the path cache in FIleManager. A future improvement could be to track FileEntryRef for the module map paths directly instead of strings.

clang/include/clang/Serialization/ASTReader.h

public:
   void visitInputFiles(serialization::ModuleFile &MF,
                        bool IncludeSystem, bool Complain,
           llvm::function_ref<void(const serialization::InputFile &IF,
                                   bool isSystem)> Visitor);
 
   /// Visit all the top-level module maps loaded when building the given module
   /// file.
   void visitTopLevelModuleMaps(serialization::ModuleFile &MF,
-                               llvm::function_ref<
-                                   void(const FileEntry *)> Visitor);
+                               llvm::function_ref<void(FileEntryRef)> Visitor);
 
   bool isProcessingUpdateRecords() { return ProcessingUpdateRecords; }
 };
 
 } // namespace clang
 
 #endif // LLVM_CLANG_SERIALIZATION_ASTREADER_H

clang/include/clang/Tooling/DependencyScanning/ModuleDepCollector.h

private:
   /// Constructs a CompilerInvocation that can be used to build the given
   /// module, excluding paths to discovered modular dependencies that are yet to
   /// be built.
   CompilerInvocation makeInvocationForModuleBuildWithoutOutputs(
       const ModuleDeps &Deps,
       llvm::function_ref<void(CompilerInvocation &)> Optimize) const;
 
   /// Collect module map files for given modules.
-  llvm::StringSet<>
+  llvm::DenseSet<const FileEntry *>
   collectModuleMapFiles(ArrayRef<ModuleID> ClangModuleDeps) const;
 
   /// Add module map files to the invocation, if needed.
   void addModuleMapFiles(CompilerInvocation &CI,
                          ArrayRef<ModuleID> ClangModuleDeps) const;
   /// Add module files (pcm) to the invocation, if needed.
   void addModuleFiles(CompilerInvocation &CI,
                       ArrayRef<ModuleID> ClangModuleDeps) const;

clang/lib/Frontend/FrontendAction.cpp

if (ReplayASTFile) {
     if (auto ASTReader = AST->getASTReader()) {
       auto &MM = ASTReader->getModuleManager();
       auto &PrimaryModule = MM.getPrimaryModule();
 
       for (serialization::ModuleFile &MF : MM)
         if (&MF != &PrimaryModule)
           CI.getFrontendOpts().ModuleFiles.push_back(MF.FileName);
 
-      ASTReader->visitTopLevelModuleMaps(
-          PrimaryModule, [&](const FileEntry *FE) {
-            CI.getFrontendOpts().ModuleMapFiles.push_back(
-                std::string(FE->getName()));
-          });
+      ASTReader->visitTopLevelModuleMaps(PrimaryModule, [&](FileEntryRef FE) {
+            std::string(FE.getName()));
     }
 
     // Set up the input file for replay purposes.
     auto Kind = AST->getInputKind();
     if (Kind.getFormat() == InputKind::ModuleMap) {
       Module *ASTModule =
           AST->getPreprocessor().getHeaderSearchInfo().lookupModule(
               AST->getLangOpts().CurrentModule, SourceLocation(),

clang/lib/Serialization/ASTReader.cpp

for (unsigned I = 0; I < N; ++I) {
     bool IsSystem = I >= NumUserInputs;
     InputFile IF = getInputFile(MF, I+1, Complain);
     Visitor(IF, IsSystem);
   }
 }
 
 void ASTReader::visitTopLevelModuleMaps(
     serialization::ModuleFile &MF,
-    llvm::function_ref<void(const FileEntry *FE)> Visitor) {
+    llvm::function_ref<void(FileEntryRef FE)> Visitor) {
   unsigned NumInputs = MF.InputFilesLoaded.size();
   for (unsigned I = 0; I < NumInputs; ++I) {
     InputFileInfo IFI = readInputFileInfo(MF, I + 1);
     if (IFI.TopLevelModuleMap)
       // FIXME: This unnecessarily re-reads the InputFileInfo.
       if (auto FE = getInputFile(MF, I + 1).getFile())
-        Visitor(FE);
+        Visitor(*FE);
   }
 }
 
 void ASTReader::finishPendingActions() {
   while (!PendingIdentifierInfos.empty() || !PendingFunctionTypes.empty() ||
          !PendingIncompleteDeclChains.empty() || !PendingDeclChains.empty() ||
          !PendingMacroIDs.empty() || !PendingDeclContextInfos.empty() ||
          !PendingUpdateRecords.empty() ||

clang/lib/Serialization/ASTWriter.cpp

WriteInputFiles(Context.SourceMgr,
                   PP.getHeaderSearchInfo().getHeaderSearchOpts());
   Stream.ExitBlock();
 }
 
 namespace  {
 
 /// An input file.
 struct InputFileEntry {
-  const FileEntry *File;
+  FileEntryRef File;
   bool IsSystemFile;
   bool IsTransient;
   bool BufferOverridden;
   bool IsTopLevelModuleMap;
   uint32_t ContentHash[2];
+
+  InputFileEntry(FileEntryRef File) : File(File) {}
 };
 
 } // namespace
 
 void ASTWriter::WriteInputFiles(SourceManager &SourceMgr,
                                 HeaderSearchOptions &HSOpts) {
   using namespace llvm;
 
for (unsigned I = 1, N = SourceMgr.local_sloc_entry_size(); I != N; ++I) {
     const SrcMgr::ContentCache *Cache = &File.getContentCache();
     if (!Cache->OrigEntry)
       continue;
 
     // Do not emit input files that do not affect current module.
     if (!IsSLocAffecting[I])
       continue;
 
-    InputFileEntry Entry;
-    Entry.File = Cache->OrigEntry;
+    InputFileEntry Entry(*Cache->OrigEntry);
     Entry.IsSystemFile = isSystem(File.getFileCharacteristic());
     Entry.IsTransient = Cache->IsTransient;
     Entry.BufferOverridden = Cache->BufferOverridden;
     Entry.IsTopLevelModuleMap = isModuleMap(File.getFileCharacteristic()) &&
                                 File.getIncludeLoc().isInvalid();
 
     auto ContentHash = hash_code(-1);
     if (PP->getHeaderSearchInfo()
             .getHeaderSearchOpts()
             .ValidateASTInputFilesContent) {
       auto MemBuff = Cache->getBufferIfLoaded();
       if (MemBuff)
         ContentHash = hash_value(MemBuff->getBuffer());
       else
-        // FIXME: The path should be taken from the FileEntryRef.
         PP->Diag(SourceLocation(), diag::err_module_unable_to_hash_content)
-            << Entry.File->getName();
+            << Entry.File.getName();
     }
     auto CH = llvm::APInt(64, ContentHash);
     Entry.ContentHash[0] =
         static_cast<uint32_t>(CH.getLoBits(32).getZExtValue());
     Entry.ContentHash[1] =
         static_cast<uint32_t>(CH.getHiBits(32).getZExtValue());
 
     if (Entry.IsSystemFile)
if (!Entry.IsSystemFile)
       ++UserFilesNum;
 
     // Emit size/modification time for this file.
     // And whether this file was overridden.
     {
       RecordData::value_type Record[] = {
           INPUT_FILE,
           InputFileOffsets.size(),
-          (uint64_t)Entry.File->getSize(),
+          (uint64_t)Entry.File.getSize(),
           (uint64_t)getTimestampForOutput(Entry.File),
           Entry.BufferOverridden,
           Entry.IsTransient,
           Entry.IsTopLevelModuleMap};
 
-      // FIXME: The path should be taken from the FileEntryRef.
-      EmitRecordWithPath(IFAbbrevCode, Record, Entry.File->getName());
+      EmitRecordWithPath(IFAbbrevCode, Record, Entry.File.getName());
     }
 
     // Emit content hash for this file.
     {
       RecordData::value_type Record[] = {INPUT_FILE_HASH, Entry.ContentHash[0],
                                          Entry.ContentHash[1]};
       Stream.EmitRecordWithAbbrev(IFHAbbrevCode, Record);
     }

clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp

CI.getFrontendOpts().Inputs.emplace_back(Deps.ClangModuleMapFile,
                                            ModuleMapInputKind);
 
   auto CurrentModuleMapEntry =
       ScanInstance.getFileManager().getFile(Deps.ClangModuleMapFile);
   assert(CurrentModuleMapEntry && "module map file entry not found");
 
   auto DepModuleMapFiles = collectModuleMapFiles(Deps.ClangModuleDeps);
   for (StringRef ModuleMapFile : Deps.ModuleMapFileDeps) {
+    // TODO: Track these as `FileEntryRef` to simplify the equality check below.
+    auto ModuleMapEntry = ScanInstance.getFileManager().getFile(ModuleMapFile);
+    assert(ModuleMapEntry && "module map file entry not found");
+
     // Don't report module maps describing eagerly-loaded dependency. This
     // information will be deserialized from the PCM.
     // TODO: Verify this works fine when modulemap for module A is eagerly
     // loaded from A.pcm, and module map passed on the command line contains
     // definition of a submodule: "explicit module A.Private { ... }".
-    if (EagerLoadModules && DepModuleMapFiles.contains(ModuleMapFile))
+    if (EagerLoadModules && DepModuleMapFiles.contains(*ModuleMapEntry))
       continue;
 
-    // TODO: Track these as `FileEntryRef` to simplify the equality check below.
-    auto ModuleMapEntry = ScanInstance.getFileManager().getFile(ModuleMapFile);
-    assert(ModuleMapEntry && "module map file entry not found");
-
     // Don't report module map file of the current module unless it also
     // describes a dependency (for symmetry).
     if (*ModuleMapEntry == *CurrentModuleMapEntry &&
-        !DepModuleMapFiles.contains(ModuleMapFile))
+        !DepModuleMapFiles.contains(*ModuleMapEntry))
       continue;
 
     CI.getFrontendOpts().ModuleMapFiles.emplace_back(ModuleMapFile);
   }
 
   // Report the prebuilt modules this module uses.
   for (const auto &PrebuiltModule : Deps.PrebuiltModuleDeps)
     CI.getFrontendOpts().ModuleFiles.push_back(PrebuiltModule.PCMFile);
ModuleDepCollector::makeInvocationForModuleBuildWithoutOutputs(
   // reporting dependencies to the clients. Let's make sure we're using
   // **strict** context hash in order to prevent accidental sharing of
   // incompatible modules (e.g. with differences in search paths).
   CI.getHeaderSearchOpts().ModulesStrictContextHash = true;
 
   return CI;
 }
 
-llvm::StringSet<> ModuleDepCollector::collectModuleMapFiles(
+llvm::DenseSet<const FileEntry *> ModuleDepCollector::collectModuleMapFiles(
     ArrayRef<ModuleID> ClangModuleDeps) const {
-  llvm::StringSet<> ModuleMapFiles;
+  llvm::DenseSet<const FileEntry *> ModuleMapFiles;
   for (const ModuleID &MID : ClangModuleDeps) {
     ModuleDeps *MD = ModuleDepsByID.lookup(MID);
     assert(MD && "Inconsistent dependency info");
-    ModuleMapFiles.insert(MD->ClangModuleMapFile);
+    // TODO: Track ClangModuleMapFile as `FileEntryRef`.
+    auto FE = ScanInstance.getFileManager().getFile(MD->ClangModuleMapFile);
+    assert(FE && "Missing module map file that was previously found");
+    ModuleMapFiles.insert(*FE);
   }
   return ModuleMapFiles;
 }
 
 void ModuleDepCollector::addModuleMapFiles(
     CompilerInvocation &CI, ArrayRef<ModuleID> ClangModuleDeps) const {
   if (EagerLoadModules)
     return; // Only pcm is needed for eager load.
MDC.ScanInstance.getASTReader()->visitInputFiles(
       });
 
   llvm::DenseSet<const Module *> SeenDeps;
   addAllSubmodulePrebuiltDeps(M, MD, SeenDeps);
   addAllSubmoduleDeps(M, MD, SeenDeps);
   addAllAffectingClangModules(M, MD, SeenDeps);
 
   MDC.ScanInstance.getASTReader()->visitTopLevelModuleMaps(
-      *MF, [&](const FileEntry *FE) {
-        if (FE->getName().endswith("__inferred_module.map"))
+      *MF, [&](FileEntryRef FE) {
+        if (FE.getName().endswith("__inferred_module.map"))
           return;
-        MD.ModuleMapFileDeps.emplace_back(FE->getName());
+        MD.ModuleMapFileDeps.emplace_back(FE.getName());
       });
 
   CompilerInvocation CI = MDC.makeInvocationForModuleBuildWithoutOutputs(
       MD, [&](CompilerInvocation &BuildInvocation) {
         if (MDC.OptimizeArgs)
           optimizeHeaderSearchOpts(BuildInvocation.getHeaderSearchOpts(),
                                    *MDC.ScanInstance.getASTReader(), *MF);
       });

clang/test/ClangScanDeps/modules-symlink-dir-from-module.c

+// Check that the path of an imported modulemap file is not influenced by
+// modules outside that module's dependency graph. Specifically, the "Foo"
+// module below does not transitively import Mod via a symlink, so it should not
+// see the symlinked path.
+
+// REQUIRES: shell
+
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: sed -e "s|DIR|%/t|g" %t/cdb.json.in > %t/cdb.json
+// RUN: ln -s module %t/include/symlink-to-module
+
+// RUN: clang-scan-deps -compilation-database %t/cdb.json -j 1 \
+// RUN:   -format experimental-full  -mode=preprocess-dependency-directives \
+// RUN:   -optimize-args -module-files-dir %t/build > %t/deps.json
+
+// RUN: cat %t/deps.json | sed 's:\\\\\?:/:g' | FileCheck %s -DPREFIX=%/t
+
+// CHECK: "modules": [
+// CHECK:   {
+// CHECK:     "command-line": [
+// CHECK:       "-fmodule-map-file=[[PREFIX]]/include/module/module.modulemap"
+// CHECK:     ]
+// CHECK:     "name": "Foo"
+// CHECK:   }
+// CHECK:   {
+// CHECK:     "command-line": [
+// FIXME: canonicalize this path
+// CHECK:       "-fmodule-map-file=[[PREFIX]]/include/symlink-to-module/module.modulemap"
+// CHECK:     ]
+// CHECK:     "name": "Foo_Private"
+// CHECK:   }
+// CHECK:   {
+// CHECK:     "command-line": [
+// CHECK:       "[[PREFIX]]/include/module/module.modulemap"
+// CHECK:     ]
+// CHECK:     "name": "Mod"
+// CHECK:   }
+// CHECK:   {
+// CHECK:     "command-line": [
+// FIXME: canonicalize this path
+// CHECK:       "-fmodule-map-file=[[PREFIX]]/include/symlink-to-module/module.modulemap"
+// CHECK:     ]
+// CHECK:     "name": "Other"
+// CHECK:   }
+// CHECK:   {
+// CHECK:     "command-line": [
+// FIXME: canonicalize this path
+// CHECK:       "-fmodule-map-file=[[PREFIX]]/include/symlink-to-module/module.modulemap"
+// CHECK:     ]
+// CHECK:     "name": "Test"
+// CHECK:   }
+// CHECK: ]
+
+//--- cdb.json.in
+[{
+  "directory": "DIR",
+  "command": "clang -fsyntax-only DIR/test.c -F DIR/Frameworks -I DIR/include -fmodules -fimplicit-module-maps -fmodules-cache-path=DIR/module-cache",
+  "file": "DIR/test.c"
+}]
+
+//--- include/module/module.modulemap
+module Mod { header "mod.h" export * }
+
+//--- include/module/mod.h
+
+//--- include/module.modulemap
+module Other { header "other.h" export * }
+
+//--- include/other.h
+#include "symlink-to-module/mod.h"
+#include "module/mod.h"
+
+//--- Frameworks/Foo.framework/Modules/module.modulemap
+framework module Foo { header "Foo.h" export * }
+//--- Frameworks/Foo.framework/Modules/module.private.modulemap
+framework module Foo_Private { header "Priv.h" export * }
+
+//--- Frameworks/Foo.framework/Headers/Foo.h
+#include "module/mod.h"
+
+//--- Frameworks/Foo.framework/PrivateHeaders/Priv.h
+#include <Foo/Foo.h>
+#include "other.h"
+
+//--- module.modulemap
+module Test { header "test.h" export * }
+
+//--- test.h
+#include <Foo/Priv.h>
+#include <Foo/Foo.h>
+
+//--- test.c
+#include "test.h"