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 +@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: } _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
