[clang][modules][deps] Serialize inputs into PCMs using the "as requested" name

[jansvoboda11]

Summary

This patch changes the PCM serialization logic to refer to input files by their "requested" name. This fixes a bug where the dependency scanner reports the "final" file paths, which can result in failed explicit compiles due to the module.modulemap file not being surrounded by the expected framework directory structure.

Depends on D135634.

[Bigcheese]

lgtm

[jansvoboda11]

Rebase, update test description

[benlangmuir]

clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp

447

Was this canonicalization necessary? If this is needed can we do it in the implicit module build so that it is serialized with the correct path? That seems better for both stability of the implicit module build and avoids re-canonicalizing the path every time it is scanned.

[jansvoboda11]

Rebase, remove unnecessary canonicalization

[jansvoboda11]

clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp

447

Good question. Canonicalization is not necessary for the test to pass, so I removed it. We can do it during serialization if we discover it's necessary.

clang/include/clang/Serialization/ASTReader.h

 //===- ASTReader.h - AST File Reader ----------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
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/lib/Frontend/FrontendAction.cpp

 //===--- FrontendAction.cpp -----------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
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.getNameAsRequested()));
     }
 
     // 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

 //===- ASTReader.cpp - AST File Reader ------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
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

 //===- ASTWriter.cpp - AST File Writer ------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
WriteInputFiles(Context.SourceMgr,
                   AffectingModuleMaps);
   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,
     std::set<const FileEntry *> &AffectingModuleMaps) {
   using namespace llvm;
if (isModuleMap(File.getFileCharacteristic()) &&
         !AffectingModuleMaps.empty() &&
         AffectingModuleMaps.find(Cache->OrigEntry) ==
             AffectingModuleMaps.end()) {
       SkippedModuleMaps.insert(Cache->OrigEntry);
       // Do not emit modulemaps that do not affect current module.
       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.getNameAsRequested();
     }
     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.getNameAsRequested());
     }
 
     // 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

 //===- ModuleDepCollector.cpp - Callbacks to collect deps -------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
MDC.ScanInstance.getASTReader()->visitInputFiles(
       });
 
   llvm::DenseSet<const Module *> SeenDeps;
   addAllSubmodulePrebuiltDeps(M, MD, SeenDeps);
   addAllSubmoduleDeps(M, MD, SeenDeps);
   addAllAffectingModules(M, MD, SeenDeps);
 
   MDC.ScanInstance.getASTReader()->visitTopLevelModuleMaps(
-      *MF, [&](const FileEntry *FE) {
-        if (FE->getName().endswith("__inferred_module.map"))
+      *MF, [&](FileEntryRef FE) {
+        if (FE.getNameAsRequested().endswith("__inferred_module.map"))
           return;
-        MD.ModuleMapFileDeps.emplace_back(FE->getName());
+        SmallString<128> CanonicalPath = FE.getNameAsRequested();
+        ModMapInfo.canonicalizeModuleMapPath(CanonicalPath);
+        MD.ModuleMapFileDeps.emplace_back(CanonicalPath);
       });
 
   CompilerInvocation CI = MDC.makeInvocationForModuleBuildWithoutOutputs(
       MD, [&](CompilerInvocation &BuildInvocation) {
         if (MDC.OptimizeArgs)
           optimizeHeaderSearchOpts(BuildInvocation.getHeaderSearchOpts(),
                                    *MDC.ScanInstance.getASTReader(), *MF);
       });

clang/test/ClangScanDeps/modules-file-name-as-requested.m

+// This test checks that we're reporting module maps affecting the compilation by describing excluded headers.
+
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+
+//--- real/module.modulemap
+framework module FW { header "Header.h" }
+//--- real/Header.h
+//--- overlay.json.template
+{
+  "case-sensitive": "false",
+  "version": "0",
+  "roots": [
+    {
+      "contents": [
+        {
+          "external-contents" : "DIR/real/Header.h",
+          "name" : "Header.h",
+          "type" : "file"
+        }
+      ],
+      "name": "DIR/frameworks/FW.framework/Headers",
+      "type": "directory"
+    },
+    {
+      "contents": [
+        {
+          "external-contents": "DIR/real/module.modulemap",
+          "name": "module.modulemap",
+          "type": "file"
+        }
+      ],
+      "name": "DIR/frameworks/FW.framework/Modules",
+      "type": "directory"
+    }
+  ]
+}
+
+//--- modules/module.modulemap
+module Importer { header "header.h" }
+//--- modules/header.h
+#include <FW/Header.h>
+
+//--- cdb.json.template
+[{
+  "file": "DIR/tu.m",
+  "directory": "DIR",
+  "command": "clang -fmodules -fmodules-cache-path=DIR/cache -Werror=non-modular-include-in-module -ivfsoverlay DIR/overlay.json -F DIR/frameworks -I DIR/modules -c DIR/tu.m -o DIR/tu.o"
+}]
+
+//--- tu.m
+@import Importer;
+
+// RUN: sed -e "s|DIR|%/t|g" %t/overlay.json.template > %t/overlay.json
+// RUN: sed -e "s|DIR|%/t|g" %t/cdb.json.template > %t/cdb.json
+// RUN: clang-scan-deps -compilation-database %t/cdb.json -format experimental-full > %t/result.json
+
+// RUN: %deps-to-rsp %t/result.json --module-name=FW > %t/FW.cc1.rsp
+// RUN: %deps-to-rsp %t/result.json --module-name=Importer > %t/Importer.cc1.rsp
+// RUN: %deps-to-rsp %t/result.json --tu-index=0 > %t/tu.rsp
+// RUN: %clang @%t/FW.cc1.rsp
+// RUN: %clang @%t/Importer.cc1.rsp
+// RUN: %clang @%t/tu.rsp