[clang] 20fa87c - [clang][modules][deps] Preserve module map load order
Jan Svoboda via cfe-commits
cfe-commits at lists.llvm.org
Thu Sep 22 12:55:01 PDT 2022
Author: Jan Svoboda
Date: 2022-09-22T12:54:51-07:00
New Revision: 20fa87c7e87fcb2fca403c7a73f8acc7e4dd5fa1
URL: https://github.com/llvm/llvm-project/commit/20fa87c7e87fcb2fca403c7a73f8acc7e4dd5fa1
DIFF: https://github.com/llvm/llvm-project/commit/20fa87c7e87fcb2fca403c7a73f8acc7e4dd5fa1.diff
LOG: [clang][modules][deps] Preserve module map load order
In `ASTWriter`, input files are sorted based on whether they are system or user. The current implementation used single `std::queue` with `push_back` and `push_front`. This resulted in the user files being reversed.
This patch fixes that by keeping the system/user distinction, but otherwise serializing files in the order they were loaded by the `SourceManager`. This is then used in the dependency scanner to report module map dependencies in the correct order.
Depends on D134224.
Reviewed By: Bigcheese
Differential Revision: https://reviews.llvm.org/D134248
Added:
clang/test/ClangScanDeps/modules-module-map-order.m
Modified:
clang/include/clang/Tooling/DependencyScanning/ModuleDepCollector.h
clang/lib/Serialization/ASTWriter.cpp
clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
clang/test/Index/Core/index-with-module.m
clang/test/Modules/module-file-home-is-cwd.m
clang/test/Modules/module_file_info.m
Removed:
################################################################################
diff --git a/clang/include/clang/Tooling/DependencyScanning/ModuleDepCollector.h b/clang/include/clang/Tooling/DependencyScanning/ModuleDepCollector.h
index c9fa763cad29c..e495f33701246 100644
--- a/clang/include/clang/Tooling/DependencyScanning/ModuleDepCollector.h
+++ b/clang/include/clang/Tooling/DependencyScanning/ModuleDepCollector.h
@@ -95,7 +95,7 @@ struct ModuleDeps {
llvm::StringSet<> FileDeps;
/// A collection of absolute paths to module map files that this module needs
- /// to know about.
+ /// to know about. The ordering is significant.
std::vector<std::string> ModuleMapFileDeps;
/// A collection of prebuilt modular dependencies this module directly depends
@@ -236,6 +236,10 @@ class ModuleDepCollector final : public DependencyCollector {
const ModuleDeps &Deps,
llvm::function_ref<void(CompilerInvocation &)> Optimize) const;
+ /// Collect module map files for given modules.
+ llvm::StringSet<>
+ collectModuleMapFiles(ArrayRef<ModuleID> ClangModuleDeps) const;
+
/// Add module map files to the invocation, if needed.
void addModuleMapFiles(CompilerInvocation &CI,
ArrayRef<ModuleID> ClangModuleDeps) const;
diff --git a/clang/lib/Serialization/ASTWriter.cpp b/clang/lib/Serialization/ASTWriter.cpp
index 05e957c26ced0..16d0bc74b9bfc 100644
--- a/clang/lib/Serialization/ASTWriter.cpp
+++ b/clang/lib/Serialization/ASTWriter.cpp
@@ -108,7 +108,6 @@
#include <cstdlib>
#include <cstring>
#include <ctime>
-#include <deque>
#include <limits>
#include <memory>
#include <queue>
@@ -1528,9 +1527,9 @@ void ASTWriter::WriteInputFiles(
IFHAbbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 32));
unsigned IFHAbbrevCode = Stream.EmitAbbrev(std::move(IFHAbbrev));
- // Get all ContentCache objects for files, sorted by whether the file is a
- // system one or not. System files go at the back, users files at the front.
- std::deque<InputFileEntry> SortedFiles;
+ // Get all ContentCache objects for files.
+ std::vector<InputFileEntry> UserFiles;
+ std::vector<InputFileEntry> SystemFiles;
for (unsigned I = 1, N = SourceMgr.local_sloc_entry_size(); I != N; ++I) {
// Get this source location entry.
const SrcMgr::SLocEntry *SLoc = &SourceMgr.getLocalSLocEntry(I);
@@ -1581,11 +1580,15 @@ void ASTWriter::WriteInputFiles(
static_cast<uint32_t>(CH.getHiBits(32).getZExtValue());
if (Entry.IsSystemFile)
- SortedFiles.push_back(Entry);
+ SystemFiles.push_back(Entry);
else
- SortedFiles.push_front(Entry);
+ UserFiles.push_back(Entry);
}
+ // User files go at the front, system files at the back.
+ auto SortedFiles = llvm::concat<InputFileEntry>(std::move(UserFiles),
+ std::move(SystemFiles));
+
unsigned UserFilesNum = 0;
// Write out all of the input files.
std::vector<uint64_t> InputFileOffsets;
diff --git a/clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp b/clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
index 905b45d07a37f..2da149c0afcce 100644
--- a/clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
+++ b/clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
@@ -116,8 +116,33 @@ ModuleDepCollector::makeInvocationForModuleBuildWithoutOutputs(
InputKind::Format::ModuleMap);
CI.getFrontendOpts().Inputs.emplace_back(Deps.ClangModuleMapFile,
ModuleMapInputKind);
- CI.getFrontendOpts().ModuleMapFiles = Deps.ModuleMapFileDeps;
- addModuleMapFiles(CI, Deps.ClangModuleDeps);
+
+ 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) {
+ // 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))
+ 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))
+ continue;
+
+ CI.getFrontendOpts().ModuleMapFiles.emplace_back(ModuleMapFile);
+ }
// Report the prebuilt modules this module uses.
for (const auto &PrebuiltModule : Deps.PrebuiltModuleDeps)
@@ -149,6 +174,17 @@ ModuleDepCollector::makeInvocationForModuleBuildWithoutOutputs(
return CI;
}
+llvm::StringSet<> ModuleDepCollector::collectModuleMapFiles(
+ ArrayRef<ModuleID> ClangModuleDeps) const {
+ llvm::StringSet<> ModuleMapFiles;
+ for (const ModuleID &MID : ClangModuleDeps) {
+ ModuleDeps *MD = ModuleDepsByID.lookup(MID);
+ assert(MD && "Inconsistent dependency info");
+ ModuleMapFiles.insert(MD->ClangModuleMapFile);
+ }
+ return ModuleMapFiles;
+}
+
void ModuleDepCollector::addModuleMapFiles(
CompilerInvocation &CI, ArrayRef<ModuleID> ClangModuleDeps) const {
if (EagerLoadModules)
@@ -203,7 +239,9 @@ void ModuleDepCollector::applyDiscoveredDependencies(CompilerInvocation &CI) {
if (KV.second->ImportedByMainFile)
DirectDeps.push_back(KV.second->ID);
+ // TODO: Report module maps the same way it's done for modular dependencies.
addModuleMapFiles(CI, DirectDeps);
+
addModuleFiles(CI, DirectDeps);
for (const auto &KV : DirectPrebuiltModularDeps)
@@ -402,27 +440,10 @@ ModuleID ModuleDepCollectorPP::handleTopLevelModule(const Module *M) {
addAllSubmoduleDeps(M, MD, SeenDeps);
addAllAffectingModules(M, MD, SeenDeps);
- llvm::DenseSet<const FileEntry *> SeenModuleMaps;
- for (const Module *SM : SeenDeps)
- if (const FileEntry *SMM = MDC.ScanInstance.getPreprocessor()
- .getHeaderSearchInfo()
- .getModuleMap()
- .getModuleMapFileForUniquing(SM))
- SeenModuleMaps.insert(SMM);
-
MDC.ScanInstance.getASTReader()->visitTopLevelModuleMaps(
*MF, [&](const FileEntry *FE) {
if (FE->getName().endswith("__inferred_module.map"))
return;
- // The top-level modulemap of this module will be the input file. We
- // don't need to specify it via "-fmodule-map-file=".
- if (FE == ModuleMap)
- return;
- // The top-level modulemap of dependencies will be serialized in the PCM
- // file (eager loading mode) or passed on the command-line through a
- //
diff erent mechanism (lazy loading mode).
- if (SeenModuleMaps.contains(FE))
- return;
MD.ModuleMapFileDeps.emplace_back(FE->getName());
});
diff --git a/clang/test/ClangScanDeps/modules-module-map-order.m b/clang/test/ClangScanDeps/modules-module-map-order.m
new file mode 100644
index 0000000000000..67070fa7d9913
--- /dev/null
+++ b/clang/test/ClangScanDeps/modules-module-map-order.m
@@ -0,0 +1,55 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+
+//--- this/module.modulemap
+module This { header "This.h" }
+//--- this/This.h
+#include "Foo.h"
+#include "Foo_Private_Excluded.h"
+
+//--- modules/module.modulemap
+module Foo { header "Foo.h" }
+//--- modules/module.private.modulemap
+explicit module Foo.Private {
+ header "Foo_Private.h"
+ exclude header "Foo_Private_Excluded.h"
+}
+//--- modules/Foo.h
+//--- modules/Foo_Private.h
+//--- modules/Foo_Private_Excluded.h
+
+//--- cdb.json.template
+[{
+ "file": "DIR/tu.m",
+ "directory": "DIR",
+ "command": "clang -fmodules -fmodules-cache-path=DIR/cache -I DIR/this -I DIR/modules -c DIR/tu.m -o DIR/tu.o"
+}]
+
+//--- tu.m
+ at import This;
+
+// 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: cat %t/result.json | sed 's:\\\\\?:/:g' | FileCheck %s -DPREFIX=%/t
+// CHECK: {
+// CHECK-NEXT: "modules": [
+// CHECK-NEXT: {
+// CHECK: },
+// CHECK-NEXT: {
+// CHECK: "command-line": [
+// CHECK: "-fmodule-map-file=[[PREFIX]]/modules/module.modulemap",
+// CHECK-NEXT: "-fmodule-map-file=[[PREFIX]]/modules/module.private.modulemap",
+// CHECK: ],
+// CHECK: "name": "This"
+// CHECK-NEXT: }
+// CHECK-NEXT: ]
+// CHECK: }
+
+// RUN: %deps-to-rsp %t/result.json --module-name=Foo > %t/Foo.cc1.rsp
+// RUN: %deps-to-rsp %t/result.json --module-name=This > %t/This.cc1.rsp
+// RUN: %deps-to-rsp %t/result.json --tu-index=0 > %t/tu.rsp
+
+// RUN: %clang @%t/Foo.cc1.rsp
+// RUN: %clang @%t/This.cc1.rsp
+// RUN: %clang @%t/tu.rsp
diff --git a/clang/test/Index/Core/index-with-module.m b/clang/test/Index/Core/index-with-module.m
index 7279229423aa2..b116768fdb557 100644
--- a/clang/test/Index/Core/index-with-module.m
+++ b/clang/test/Index/Core/index-with-module.m
@@ -23,5 +23,5 @@ void foo() {
// CHECK-DAG: 2:6 | function/C | ModA_func | c:@F at ModA_func | {{.*}} | Decl | rel: 0
// CHECK-DAG: 2:6 | function/C | SubModA_func | c:@F at SubModA_func | {{.*}} | Decl | rel: 0
// CHECK: ---- Module Inputs ----
-// CHECK: user | {{.*}}ModA.h
// CHECK: user | {{.*}}module.modulemap
+// CHECK: user | {{.*}}ModA.h
diff --git a/clang/test/Modules/module-file-home-is-cwd.m b/clang/test/Modules/module-file-home-is-cwd.m
index c455d647da3f1..4f79c5abd9ccd 100644
--- a/clang/test/Modules/module-file-home-is-cwd.m
+++ b/clang/test/Modules/module-file-home-is-cwd.m
@@ -2,7 +2,7 @@
// RUN: %clang_cc1 -fmodules -fno-implicit-modules -fmodule-file-home-is-cwd -fmodule-name=libA -emit-module Inputs/normal-module-map/module.map -o %t/mod.pcm
// RUN: llvm-bcanalyzer --dump --disable-histogram %t/mod.pcm | FileCheck %s
-// CHECK: <INPUT_FILE {{.*}}/> blob data = 'Inputs{{/|\\}}normal-module-map{{/|\\}}a1.h'
-// CHECK: <INPUT_FILE {{.*}}/> blob data = 'Inputs{{/|\\}}normal-module-map{{/|\\}}a2.h'
// CHECK: <INPUT_FILE {{.*}}/> blob data = 'Inputs{{/|\\}}normal-module-map{{/|\\}}module.map'
+// CHECK: <INPUT_FILE {{.*}}/> blob data = 'Inputs{{/|\\}}normal-module-map{{/|\\}}a2.h'
+// CHECK: <INPUT_FILE {{.*}}/> blob data = 'Inputs{{/|\\}}normal-module-map{{/|\\}}a1.h'
// CHECK-NOT: MODULE_DIRECTORY
diff --git a/clang/test/Modules/module_file_info.m b/clang/test/Modules/module_file_info.m
index 413a01a4e49ff..4cd30cf177bd2 100644
--- a/clang/test/Modules/module_file_info.m
+++ b/clang/test/Modules/module_file_info.m
@@ -45,16 +45,16 @@
// CHECK: Predefined macros:
// CHECK: -DBLARG
// CHECK: -DWIBBLE=WOBBLE
-// CHECK: Input file: {{.*}}DependsOnModulePrivate.h
-// CHECK-NEXT: Input file: {{.*}}Other.h
-// CHECK-NEXT: Input file: {{.*}}SubFramework.h
-// CHECK-NEXT: Input file: {{.*}}not_coroutines.h
-// CHECK-NEXT: Input file: {{.*}}not_cxx.h
-// CHECK-NEXT: Input file: {{.*}}other.h
-// CHECK-NEXT: Input file: {{.*}}module.map
-// CHECK-NEXT: Input file: {{.*}}DependsOnModule.h
+// CHECK: Input file: {{.*}}module.map
// CHECK-NEXT: Input file: {{.*}}module_private.map
+// CHECK-NEXT: Input file: {{.*}}DependsOnModule.h
// CHECK-NEXT: Input file: {{.*}}module.map
+// CHECK-NEXT: Input file: {{.*}}other.h
+// CHECK-NEXT: Input file: {{.*}}not_cxx.h
+// CHECK-NEXT: Input file: {{.*}}not_coroutines.h
+// CHECK-NEXT: Input file: {{.*}}SubFramework.h
+// CHECK-NEXT: Input file: {{.*}}Other.h
+// CHECK-NEXT: Input file: {{.*}}DependsOnModulePrivate.h
// CHECK: Diagnostic options:
// CHECK: IgnoreWarnings: Yes
More information about the cfe-commits
mailing list