jansvoboda11 created this revision.
jansvoboda11 added reviewers: Bigcheese, benlangmuir.
Herald added a subscriber: ributzka.
Herald added a project: All.
jansvoboda11 requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
In D106100 <https://reviews.llvm.org/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.
rdar://104324602
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D142165
Files:
clang/include/clang/Tooling/DependencyScanning/ModuleDepCollector.h
clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
clang/test/ClangScanDeps/modules-implementation-private.c
Index: clang/test/ClangScanDeps/modules-implementation-private.c
===================================================================
--- /dev/null
+++ clang/test/ClangScanDeps/modules-implementation-private.c
@@ -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.c",
+ "command": "clang -fmodules -fmodules-cache-path=DIR/cache -fmodule-name=FW -F DIR/frameworks -c DIR/tu.c -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.c, this ends up adding (spurious) dependency on FW for FW_Private.
+
+//--- tu.c
+@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.c",
+// 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.c"
+// CHECK-NEXT: }
+// CHECK: ]
+// CHECK: }
+// CHECK: ]
+// CHECK: }
Index: clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
===================================================================
--- clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
+++ clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
@@ -379,15 +379,8 @@
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);
@@ -401,9 +394,17 @@
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)
@@ -524,9 +525,9 @@
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);
}
}
}
@@ -548,9 +549,9 @@
"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);
}
}
}
Index: clang/include/clang/Tooling/DependencyScanning/ModuleDepCollector.h
===================================================================
--- clang/include/clang/Tooling/DependencyScanning/ModuleDepCollector.h
+++ 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 @@
/// 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,
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
