[clang] [clang][modules] Preserve the module map that allowed inferring (PR #113389)
Jan Svoboda via cfe-commits
cfe-commits at lists.llvm.org
Tue Oct 22 15:07:01 PDT 2024
https://github.com/jansvoboda11 created https://github.com/llvm/llvm-project/pull/113389
With inferred modules, the dependency scanner takes care to replace the fake "__inferred_module.map" path with the file that allowed the module to be inferred. However, this only worked when such a module was imported directly in the TU. Whenever such module got loaded transitively, the scanner would fail to perform the replacement. This is caused by the fact that PCM files are lossy and drop this information.
This patch makes sure that PCMs include this file, fixes one existing test with an incorrect assertion, and does a little drive-by refactoring of `ModuleMap`.
>From efcd62d35fe296b2bd4fe5cdbec9ab96493a885f Mon Sep 17 00:00:00 2001
From: Jan Svoboda <jan_svoboda at apple.com>
Date: Tue, 22 Oct 2024 14:01:45 -0700
Subject: [PATCH] [clang][modules] Preserve the module map that allowed
inferring
---
.../include/clang/Serialization/ASTBitCodes.h | 4 ++--
clang/include/clang/Serialization/ASTReader.h | 2 ++
clang/lib/Frontend/FrontendAction.cpp | 1 -
clang/lib/Lex/ModuleMap.cpp | 11 ++++-------
clang/lib/Serialization/ASTReader.cpp | 5 +++--
clang/lib/Serialization/ASTWriter.cpp | 7 +++++++
.../DependencyScanning/ModuleDepCollector.cpp | 18 +++++++-----------
clang/test/ClangScanDeps/link-libraries.c | 7 +++----
8 files changed, 28 insertions(+), 27 deletions(-)
diff --git a/clang/include/clang/Serialization/ASTBitCodes.h b/clang/include/clang/Serialization/ASTBitCodes.h
index e397dff097652b..9f50e28e94ff0c 100644
--- a/clang/include/clang/Serialization/ASTBitCodes.h
+++ b/clang/include/clang/Serialization/ASTBitCodes.h
@@ -44,7 +44,7 @@ namespace serialization {
/// Version 4 of AST files also requires that the version control branch and
/// revision match exactly, since there is no backward compatibility of
/// AST files at this time.
-const unsigned VERSION_MAJOR = 31;
+const unsigned VERSION_MAJOR = 32;
/// AST file minor version number supported by this version of
/// Clang.
@@ -54,7 +54,7 @@ const unsigned VERSION_MAJOR = 31;
/// for the previous version could still support reading the new
/// version by ignoring new kinds of subblocks), this number
/// should be increased.
-const unsigned VERSION_MINOR = 1;
+const unsigned VERSION_MINOR = 0;
/// An ID number that refers to an identifier in an AST file.
///
diff --git a/clang/include/clang/Serialization/ASTReader.h b/clang/include/clang/Serialization/ASTReader.h
index b476a40ebd2c8c..070c1c9a54f48c 100644
--- a/clang/include/clang/Serialization/ASTReader.h
+++ b/clang/include/clang/Serialization/ASTReader.h
@@ -2335,6 +2335,8 @@ class ASTReader
/// Translate a FileID from another module file's FileID space into ours.
FileID TranslateFileID(ModuleFile &F, FileID FID) const {
assert(FID.ID >= 0 && "Reading non-local FileID.");
+ if (FID.isInvalid())
+ return FID;
return FileID::get(F.SLocEntryBaseID + FID.ID - 1);
}
diff --git a/clang/lib/Frontend/FrontendAction.cpp b/clang/lib/Frontend/FrontendAction.cpp
index 81eea9c4c4dc58..a647e3e7ec6f64 100644
--- a/clang/lib/Frontend/FrontendAction.cpp
+++ b/clang/lib/Frontend/FrontendAction.cpp
@@ -534,7 +534,6 @@ static Module *prepareToBuildModule(CompilerInstance &CI,
}
if (*OriginalModuleMap != CI.getSourceManager().getFileEntryRefForID(
CI.getSourceManager().getMainFileID())) {
- M->IsInferred = true;
auto FileCharacter =
M->IsSystem ? SrcMgr::C_System_ModuleMap : SrcMgr::C_User_ModuleMap;
FileID OriginalModuleMapFID = CI.getSourceManager().getOrCreateFileID(
diff --git a/clang/lib/Lex/ModuleMap.cpp b/clang/lib/Lex/ModuleMap.cpp
index fd6bc17ae9cdac..31617c7e86fddb 100644
--- a/clang/lib/Lex/ModuleMap.cpp
+++ b/clang/lib/Lex/ModuleMap.cpp
@@ -657,8 +657,7 @@ ModuleMap::findOrCreateModuleForHeaderInUmbrellaDir(FileEntryRef File) {
llvm::sys::path::stem(SkippedDir.getName()), NameBuf);
Result = findOrCreateModule(Name, Result, /*IsFramework=*/false,
Explicit).first;
- InferredModuleAllowedBy[Result] = UmbrellaModuleMap;
- Result->IsInferred = true;
+ setInferredModuleAllowedBy(Result, UmbrellaModuleMap);
// Associate the module and the directory.
UmbrellaDirs[SkippedDir] = Result;
@@ -675,8 +674,7 @@ ModuleMap::findOrCreateModuleForHeaderInUmbrellaDir(FileEntryRef File) {
llvm::sys::path::stem(File.getName()), NameBuf);
Result = findOrCreateModule(Name, Result, /*IsFramework=*/false,
Explicit).first;
- InferredModuleAllowedBy[Result] = UmbrellaModuleMap;
- Result->IsInferred = true;
+ setInferredModuleAllowedBy(Result, UmbrellaModuleMap);
Result->addTopHeader(File);
// If inferred submodules export everything they import, add a
@@ -1097,8 +1095,7 @@ Module *ModuleMap::inferFrameworkModule(DirectoryEntryRef FrameworkDir,
Module *Result = new (ModulesAlloc.Allocate())
Module(ModuleConstructorTag{}, ModuleName, SourceLocation(), Parent,
/*IsFramework=*/true, /*IsExplicit=*/false, NumCreatedModules++);
- InferredModuleAllowedBy[Result] = ModuleMapFID;
- Result->IsInferred = true;
+ setInferredModuleAllowedBy(Result, ModuleMapFID);
if (!Parent) {
if (LangOpts.CurrentModule == ModuleName)
SourceModule = Result;
@@ -1345,7 +1342,7 @@ ModuleMap::getModuleMapFileForUniquing(const Module *M) const {
}
void ModuleMap::setInferredModuleAllowedBy(Module *M, FileID ModMapFID) {
- assert(M->IsInferred && "module not inferred");
+ M->IsInferred = true;
InferredModuleAllowedBy[M] = ModMapFID;
}
diff --git a/clang/lib/Serialization/ASTReader.cpp b/clang/lib/Serialization/ASTReader.cpp
index 60b708067dc597..4fa41338024930 100644
--- a/clang/lib/Serialization/ASTReader.cpp
+++ b/clang/lib/Serialization/ASTReader.cpp
@@ -5826,6 +5826,7 @@ llvm::Error ASTReader::ReadSubmoduleBlock(ModuleFile &F,
SubmoduleID Parent = getGlobalSubmoduleID(F, Record[Idx++]);
Module::ModuleKind Kind = (Module::ModuleKind)Record[Idx++];
SourceLocation DefinitionLoc = ReadSourceLocation(F, Record[Idx++]);
+ FileID InferredAllowedBy = ReadFileID(F, Record, Idx);
bool IsFramework = Record[Idx++];
bool IsExplicit = Record[Idx++];
bool IsSystem = Record[Idx++];
@@ -5847,8 +5848,6 @@ llvm::Error ASTReader::ReadSubmoduleBlock(ModuleFile &F,
ModMap.findOrCreateModule(Name, ParentModule, IsFramework, IsExplicit)
.first;
- // FIXME: Call ModMap.setInferredModuleAllowedBy()
-
SubmoduleID GlobalIndex = GlobalID - NUM_PREDEF_SUBMODULE_IDS;
if (GlobalIndex >= SubmodulesLoaded.size() ||
SubmodulesLoaded[GlobalIndex])
@@ -5879,6 +5878,8 @@ llvm::Error ASTReader::ReadSubmoduleBlock(ModuleFile &F,
CurrentModule->DefinitionLoc = DefinitionLoc;
CurrentModule->Signature = F.Signature;
CurrentModule->IsFromModuleFile = true;
+ if (InferredAllowedBy.isValid())
+ ModMap.setInferredModuleAllowedBy(CurrentModule, InferredAllowedBy);
CurrentModule->IsSystem = IsSystem || CurrentModule->IsSystem;
CurrentModule->IsExternC = IsExternC;
CurrentModule->InferSubmodules = InferSubmodules;
diff --git a/clang/lib/Serialization/ASTWriter.cpp b/clang/lib/Serialization/ASTWriter.cpp
index 938d7b525cb959..5479ef5fe5a918 100644
--- a/clang/lib/Serialization/ASTWriter.cpp
+++ b/clang/lib/Serialization/ASTWriter.cpp
@@ -2914,6 +2914,7 @@ void ASTWriter::WriteSubmodules(Module *WritingModule) {
Abbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 6)); // Parent
Abbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 4)); // Kind
Abbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 8)); // Definition location
+ Abbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 4)); // Inferred allowed by
Abbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // IsFramework
Abbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // IsExplicit
Abbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // IsSystem
@@ -3018,6 +3019,11 @@ void ASTWriter::WriteSubmodules(Module *WritingModule) {
SourceLocationEncoding::RawLocEncoding DefinitionLoc =
getRawSourceLocationEncoding(getAdjustedLocation(Mod->DefinitionLoc));
+ ModuleMap &ModMap = PP->getHeaderSearchInfo().getModuleMap();
+ FileID InferredFID =
+ Mod->IsInferred ? ModMap.getModuleMapFileIDForUniquing(Mod) : FileID();
+ int Inferred = getAdjustedFileID(InferredFID).getOpaqueValue();
+
// Emit the definition of the block.
{
RecordData::value_type Record[] = {SUBMODULE_DEFINITION,
@@ -3025,6 +3031,7 @@ void ASTWriter::WriteSubmodules(Module *WritingModule) {
ParentID,
(RecordData::value_type)Mod->Kind,
DefinitionLoc,
+ (RecordData::value_type)Inferred,
Mod->IsFramework,
Mod->IsExplicit,
Mod->IsSystem,
diff --git a/clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp b/clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
index 77f9d07175c2c1..637416cd1fc621 100644
--- a/clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
+++ b/clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
@@ -587,9 +587,7 @@ ModuleDepCollectorPP::handleTopLevelModule(const Module *M) {
ModuleMap &ModMapInfo =
MDC.ScanInstance.getPreprocessor().getHeaderSearchInfo().getModuleMap();
- OptionalFileEntryRef ModuleMap = ModMapInfo.getModuleMapFileForUniquing(M);
-
- if (ModuleMap) {
+ if (auto ModuleMap = ModMapInfo.getModuleMapFileForUniquing(M)) {
SmallString<128> Path = ModuleMap->getNameAsRequested();
ModMapInfo.canonicalizeModuleMapPath(Path);
MD.ClangModuleMapFile = std::string(Path);
@@ -601,15 +599,13 @@ ModuleDepCollectorPP::handleTopLevelModule(const Module *M) {
MDC.ScanInstance.getASTReader()->visitInputFileInfos(
*MF, /*IncludeSystem=*/true,
[&](const serialization::InputFileInfo &IFI, bool IsSystem) {
- // __inferred_module.map is the result of the way in which an implicit
- // module build handles inferred modules. It adds an overlay VFS with
- // this file in the proper directory and relies on the rest of Clang to
- // handle it like normal. With explicitly built modules we don't need
- // to play VFS tricks, so replace it with the correct module map.
- if (StringRef(IFI.Filename).ends_with("__inferred_module.map")) {
- MDC.addFileDep(MD, ModuleMap->getName());
+ // The __inferred_module.map file is an insignificant implementation
+ // detail of implicitly-built modules. The PCM will also report the
+ // actual on-disk module map file that allowed inferring the module,
+ // which is what we need for building the module explicitly
+ // Let's ignore this file.
+ if (StringRef(IFI.Filename).ends_with("__inferred_module.map"))
return;
- }
MDC.addFileDep(MD, IFI.Filename);
});
diff --git a/clang/test/ClangScanDeps/link-libraries.c b/clang/test/ClangScanDeps/link-libraries.c
index c09691d2356efc..bc0b0c546ea032 100644
--- a/clang/test/ClangScanDeps/link-libraries.c
+++ b/clang/test/ClangScanDeps/link-libraries.c
@@ -39,14 +39,13 @@ module transitive {
// CHECK-NEXT: "modules": [
// CHECK-NEXT: {
// CHECK-NEXT: "clang-module-deps": [],
-// CHECK-NEXT: "clang-modulemap-file": "{{.*}}/__inferred_module.map",
+// CHECK-NEXT: "clang-modulemap-file": "[[PREFIX]]/Inputs/frameworks/module.modulemap",
// CHECK-NEXT: "command-line": [
// CHECK: ],
// CHECK-NEXT: "context-hash": "{{.*}}",
// CHECK-NEXT: "file-deps": [
-// CHECK-NEXT: "{{.*}}/Framework.h"
-// CHECK-NEXT: "{{.*}}/__inferred_module.map"
-// CHECK-NEXT: "{{.*}}/module.modulemap"
+// CHECK-NEXT: "[[PREFIX]]/Inputs/frameworks/Framework.framework/Headers/Framework.h"
+// CHECK-NEXT: "[[PREFIX]]/Inputs/frameworks/module.modulemap"
// CHECK-NEXT: ],
// CHECK-NEXT: "link-libraries": [
// CHECK-NEXT: {
More information about the cfe-commits
mailing list