[clang] 96a54b2 - [clang][deps] Account for transitive spurious dependencies
Jan Svoboda via cfe-commits
cfe-commits at lists.llvm.org
Tue Jan 24 09:49:04 PST 2023
Author: Jan Svoboda
Date: 2023-01-24T09:48:58-08:00
New Revision: 96a54b2258cddcb52f8c98e760b9b114a1aa4034
URL: https://github.com/llvm/llvm-project/commit/96a54b2258cddcb52f8c98e760b9b114a1aa4034
DIFF: https://github.com/llvm/llvm-project/commit/96a54b2258cddcb52f8c98e760b9b114a1aa4034.diff
LOG: [clang][deps] Account for transitive spurious dependencies
In D106100, we started guarding against spurious dependencies on modules that ended up being textual includes and thus didn't have any AST file associated. That patch accounted only for direct dependencies. There's a way how to get spurious dependencies for modules that are transitive. This patch guards against that scenario and adds a test case.
(Note that since D142167, we don't allow `@import FW_Private` with `-fmodule-name=FW` anymore. However, that check lives in sema, which the scanner doesn't run. Being defensive in this patch therefore still makes sense.)
rdar://104324602
Reviewed By: benlangmuir
Differential Revision: https://reviews.llvm.org/D142165
Added:
clang/test/ClangScanDeps/modules-implementation-private.m
Modified:
clang/include/clang/Tooling/DependencyScanning/ModuleDepCollector.h
clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
Removed:
################################################################################
diff --git a/clang/include/clang/Tooling/DependencyScanning/ModuleDepCollector.h b/clang/include/clang/Tooling/DependencyScanning/ModuleDepCollector.h
index 8577e140cc6ca..fd6db992d42f1 100644
--- a/clang/include/clang/Tooling/DependencyScanning/ModuleDepCollector.h
+++ b/clang/include/clang/Tooling/DependencyScanning/ModuleDepCollector.h
@@ -19,6 +19,7 @@
#include "llvm/ADT/DenseMap.h"
#include "llvm/ADT/StringSet.h"
#include "llvm/Support/raw_ostream.h"
+#include <optional>
#include <string>
#include <unordered_map>
@@ -161,7 +162,8 @@ class ModuleDepCollectorPP final : public PPCallbacks {
/// Traverses the previously collected direct modular dependencies to discover
/// transitive modular dependencies and fills the parent \c ModuleDepCollector
/// with both.
- ModuleID handleTopLevelModule(const Module *M);
+ /// Returns the ID or nothing if the dependency is spurious and is ignored.
+ std::optional<ModuleID> handleTopLevelModule(const Module *M);
void addAllSubmoduleDeps(const Module *M, ModuleDeps &MD,
llvm::DenseSet<const Module *> &AddedModules);
void addModuleDep(const Module *M, ModuleDeps &MD,
diff --git a/clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp b/clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
index f9708332f4eec..9ad8b17bafebd 100644
--- a/clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
+++ b/clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
@@ -377,15 +377,8 @@ void ModuleDepCollectorPP::EndOfMainFile() {
if (!MDC.isPrebuiltModule(M))
DirectModularDeps.insert(M);
- for (const Module *M : DirectModularDeps) {
- // A top-level module might not be actually imported as a module when
- // -fmodule-name is used to compile a translation unit that imports this
- // module. In that case it can be skipped. The appropriate header
- // dependencies will still be reported as expected.
- if (!M->getASTFile())
- continue;
+ for (const Module *M : DirectModularDeps)
handleTopLevelModule(M);
- }
MDC.Consumer.handleDependencyOutputOpts(*MDC.Opts);
@@ -399,9 +392,17 @@ void ModuleDepCollectorPP::EndOfMainFile() {
MDC.Consumer.handlePrebuiltModuleDependency(I.second);
}
-ModuleID ModuleDepCollectorPP::handleTopLevelModule(const Module *M) {
+std::optional<ModuleID>
+ModuleDepCollectorPP::handleTopLevelModule(const Module *M) {
assert(M == M->getTopLevelModule() && "Expected top level module!");
+ // A top-level module might not be actually imported as a module when
+ // -fmodule-name is used to compile a translation unit that imports this
+ // module. In that case it can be skipped. The appropriate header
+ // dependencies will still be reported as expected.
+ if (!M->getASTFile())
+ return {};
+
// If this module has been handled already, just return its ID.
auto ModI = MDC.ModularDeps.insert({M, nullptr});
if (!ModI.second)
@@ -522,9 +523,9 @@ void ModuleDepCollectorPP::addModuleDep(
for (const Module *Import : M->Imports) {
if (Import->getTopLevelModule() != M->getTopLevelModule() &&
!MDC.isPrebuiltModule(Import)) {
- ModuleID ImportID = handleTopLevelModule(Import->getTopLevelModule());
- if (AddedModules.insert(Import->getTopLevelModule()).second)
- MD.ClangModuleDeps.push_back(ImportID);
+ if (auto ImportID = handleTopLevelModule(Import->getTopLevelModule()))
+ if (AddedModules.insert(Import->getTopLevelModule()).second)
+ MD.ClangModuleDeps.push_back(*ImportID);
}
}
}
@@ -546,9 +547,9 @@ void ModuleDepCollectorPP::addAffectingClangModule(
"Not quite import not top-level module");
if (Affecting != M->getTopLevelModule() &&
!MDC.isPrebuiltModule(Affecting)) {
- ModuleID ImportID = handleTopLevelModule(Affecting);
- if (AddedModules.insert(Affecting).second)
- MD.ClangModuleDeps.push_back(ImportID);
+ if (auto ImportID = handleTopLevelModule(Affecting))
+ if (AddedModules.insert(Affecting).second)
+ MD.ClangModuleDeps.push_back(*ImportID);
}
}
}
diff --git a/clang/test/ClangScanDeps/modules-implementation-private.m b/clang/test/ClangScanDeps/modules-implementation-private.m
new file mode 100644
index 0000000000000..6a9d83c22678b
--- /dev/null
+++ b/clang/test/ClangScanDeps/modules-implementation-private.m
@@ -0,0 +1,70 @@
+// This test checks that we don't crash or report spurious dependencies on
+// FW_Private when compiling the implementation of framework module FW.
+
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+
+//--- cdb.json.template
+[{
+ "directory": "DIR",
+ "file": "DIR/tu.m",
+ "command": "clang -fmodules -fmodules-cache-path=DIR/cache -fmodule-name=FW -F DIR/frameworks -c DIR/tu.m -o DIR/tu.o"
+}]
+
+//--- frameworks/FW.framework/Modules/module.modulemap
+framework module FW { umbrella header "FW.h" }
+//--- frameworks/FW.framework/Modules/module.private.modulemap
+framework module FW_Private { umbrella header "FW_Private.h" }
+//--- frameworks/FW.framework/Headers/FW.h
+//--- frameworks/FW.framework/PrivateHeaders/FW_Private.h
+//--- frameworks/FW.framework/PrivateHeaders/Missed.h
+#import <FW/FW.h> // When included from tu.m, this ends up adding (spurious) dependency on FW for FW_Private.
+
+//--- tu.m
+ at import FW_Private; // This is a direct dependency.
+#import <FW/Missed.h>
+
+// RUN: sed "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-NEXT: "clang-module-deps": [],
+// CHECK-NEXT: "clang-modulemap-file": "[[PREFIX]]/frameworks/FW.framework/Modules/module.private.modulemap",
+// CHECK-NEXT: "command-line": [
+// CHECK: ],
+// CHECK-NEXT: "context-hash": "{{.*}}",
+// CHECK-NEXT: "file-deps": [
+// CHECK-NEXT: "[[PREFIX]]/frameworks/FW.framework/Modules/module.private.modulemap",
+// CHECK-NEXT: "[[PREFIX]]/frameworks/FW.framework/PrivateHeaders/FW_Private.h"
+// CHECK-NEXT: ],
+// CHECK-NEXT: "name": "FW_Private"
+// CHECK-NEXT: }
+// CHECK-NEXT: ],
+// CHECK-NEXT: "translation-units": [
+// CHECK-NEXT: {
+// CHECK-NEXT: "commands": [
+// CHECK-NEXT: {
+// CHECK-NEXT: "clang-context-hash": "{{.*}}",
+// CHECK-NEXT: "clang-module-deps": [
+// CHECK-NEXT: {
+// CHECK-NEXT: "context-hash": "{{.*}}",
+// CHECK-NEXT: "module-name": "FW_Private"
+// CHECK-NEXT: }
+// CHECK-NEXT: ],
+// CHECK-NEXT: "command-line": [
+// CHECK: ],
+// CHECK-NEXT: "executable": "clang",
+// CHECK-NEXT: "file-deps": [
+// CHECK-NEXT: "[[PREFIX]]/tu.m",
+// CHECK-NEXT: "[[PREFIX]]/frameworks/FW.framework/PrivateHeaders/Missed.h",
+// CHECK-NEXT: "[[PREFIX]]/frameworks/FW.framework/Headers/FW.h"
+// CHECK-NEXT: ],
+// CHECK-NEXT: "input-file": "[[PREFIX]]/tu.m"
+// CHECK-NEXT: }
+// CHECK: ]
+// CHECK: }
+// CHECK: ]
+// CHECK: }
More information about the cfe-commits
mailing list