From 8a3842b417429d49e0e62d9daa13c476ea5a6783 Mon Sep 17 00:00:00 2001 From: Cyndy Ishida Date: Mon, 14 Apr 2025 14:55:44 -0700 Subject: [PATCH 1/3] [clang][depscan] Support prefix mappings when deciding modules that come from "stable" directories To support compiler caching, clang may strip out common prefixes from paths like the sysroot. When this happens, include both the initial input paths that would get mapped away and the resulting mapped prefix for determining module dependencies that come from stable directories. This is to handle all of the ways paths may come streaming. For example, input paths that represented included headers will contain an absolute paths for handling indirections like vfsoverlays but sysroot's from `CompilerInstance`'s will be mapped. --- .../DependencyScanningWorker.cpp | 19 ++++ .../DependencyScanning/ModuleDepCollector.cpp | 9 +- .../ClangScanDeps/include-tree-with-pch.c | 2 +- .../modules-cas-trees-input-files.c | 4 +- .../modules-cas-trees-with-pch.c | 4 +- .../modules-in-stable-dirs-prefix-mapping.c | 94 +++++++++++++++++++ 6 files changed, 122 insertions(+), 10 deletions(-) create mode 100644 clang/test/ClangScanDeps/modules-in-stable-dirs-prefix-mapping.c diff --git a/clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp b/clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp index 5a5d217183bec..7f8e4221f895c 100644 --- a/clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp +++ b/clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp @@ -586,6 +586,11 @@ class DependencyScanningAction : public tooling::ToolAction { auto *FileMgr = ScanInstance.createFileManager(FS); ScanInstance.createSourceManager(*FileMgr); + // Initialize PrefixMapper incase mappings exist. + DepscanPrefixMapping::configurePrefixMapper( + ScanInstance.getFrontendOpts().PathPrefixMappings, + ScanInstance.getPrefixMapper()); + // Create a collection of stable directories derived from the ScanInstance // for determining whether module dependencies would fully resolve from // those directories. @@ -595,6 +600,20 @@ class DependencyScanningAction : public tooling::ToolAction { (llvm::sys::path::root_directory(Sysroot) != Sysroot)) StableDirs = {Sysroot, ScanInstance.getHeaderSearchOpts().ResourceDir}; + // When remapping is disabled, the stable directory paths are references to + // strings tied to the CompilerInstance. Otherwise, these potentially mapped + // strings need to be allocated. To reduce the number of downstream changes + // required to this support allocation, tie the lifetime of these strings to + // visiting all dependencies of a prebuilt module. + llvm::PrefixMapper &Mapper = ScanInstance.getPrefixMapper(); + llvm::BumpPtrAllocator Allocator; + llvm::StringSaver StableDirStorage(Allocator); + if (!StableDirs.empty() && !Mapper.empty()) { + StableDirs.push_back(StableDirStorage.save(Mapper.mapToString(Sysroot))); + StableDirs.push_back(StableDirStorage.save( + Mapper.mapToString(ScanInstance.getHeaderSearchOpts().ResourceDir))); + } + // Store a mapping of prebuilt module files and their properties like header // search options. This will prevent the implicit build to create duplicate // modules and will force reuse of the existing prebuilt module files diff --git a/clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp b/clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp index 955b6d0be4ca6..8e236b77a1ecc 100644 --- a/clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp +++ b/clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp @@ -255,12 +255,11 @@ bool dependencies::isPathInStableDir(const ArrayRef Directories, bool dependencies::areOptionsInStableDir(const ArrayRef Directories, const HeaderSearchOptions &HSOpts) { - // FIXME: This breaks CAS prefix mapping tests. - // assert(isPathInStableDir(Directories, HSOpts.Sysroot) && - // "Sysroots differ between module dependencies and current TU"); + assert(isPathInStableDir(Directories, HSOpts.Sysroot) && + "Sysroots differ between module dependencies and current TU"); - // assert(isPathInStableDir(Directories, HSOpts.ResourceDir) && - // "ResourceDirs differ between module dependencies and current TU"); + assert(isPathInStableDir(Directories, HSOpts.ResourceDir) && + "ResourceDirs differ between module dependencies and current TU"); for (const auto &Entry : HSOpts.UserEntries) { if (!Entry.IgnoreSysRoot) diff --git a/clang/test/ClangScanDeps/include-tree-with-pch.c b/clang/test/ClangScanDeps/include-tree-with-pch.c index 1c6d0e2791b7e..f6590c89f2754 100644 --- a/clang/test/ClangScanDeps/include-tree-with-pch.c +++ b/clang/test/ClangScanDeps/include-tree-with-pch.c @@ -3,7 +3,7 @@ // RUN: split-file %s %t // RUN: sed -e "s|DIR|%/t|g" %t/cdb.json.template > %t/cdb.json -// RUN: %clang -x c-header %t/prefix.h -target x86_64-apple-macos12 -o %t/prefix.pch -fdepscan=inline -fdepscan-include-tree -Xclang -fcas-path -Xclang %t/cas +// RUN: %clang -x c-header %t/prefix.h -target x86_64-apple-macos12 -isysroot %t -o %t/prefix.pch -fdepscan=inline -fdepscan-include-tree -Xclang -fcas-path -Xclang %t/cas // RUN: clang-scan-deps -compilation-database %t/cdb.json -format experimental-include-tree -cas-path %t/cas > %t/result.txt // RUN: FileCheck %s -input-file %t/result.txt -DPREFIX=%/t diff --git a/clang/test/ClangScanDeps/modules-cas-trees-input-files.c b/clang/test/ClangScanDeps/modules-cas-trees-input-files.c index 89f4f14b9909e..4384bff2ed106 100644 --- a/clang/test/ClangScanDeps/modules-cas-trees-input-files.c +++ b/clang/test/ClangScanDeps/modules-cas-trees-input-files.c @@ -37,7 +37,7 @@ [ { "directory" : "DIR", - "command" : "clang_tool -I DIR -x c-header DIR/prefix.h -o DIR/prefix.h.pch -fmodules -fimplicit-modules -fimplicit-module-maps -fmodules-cache-path=DIR/module-cache -Rcompile-job-cache", + "command" : "clang -I DIR -x c-header DIR/prefix.h -o DIR/prefix.h.pch -fmodules -fimplicit-modules -fimplicit-module-maps -fmodules-cache-path=DIR/module-cache -Rcompile-job-cache", "file" : "DIR/prefix.h" }, ] @@ -46,7 +46,7 @@ [ { "directory" : "DIR", - "command" : "clang_tool -I DIR -fsyntax-only DIR/tu.c -include DIR/prefix.h -fmodules -fimplicit-modules -fimplicit-module-maps -fmodules-cache-path=DIR/module-cache -Rcompile-job-cache", + "command" : "clang -I DIR -fsyntax-only DIR/tu.c -include DIR/prefix.h -fmodules -fimplicit-modules -fimplicit-module-maps -fmodules-cache-path=DIR/module-cache -Rcompile-job-cache", "file" : "DIR/tu.c" }, ] diff --git a/clang/test/ClangScanDeps/modules-cas-trees-with-pch.c b/clang/test/ClangScanDeps/modules-cas-trees-with-pch.c index d936e618ba770..bff312bced27a 100644 --- a/clang/test/ClangScanDeps/modules-cas-trees-with-pch.c +++ b/clang/test/ClangScanDeps/modules-cas-trees-with-pch.c @@ -191,7 +191,7 @@ [ { "directory" : "DIR", - "command" : "clang_tool -x c-header DIR/prefix.h -o DIR/prefix.h.pch -fmodules -fimplicit-modules -fimplicit-module-maps -fmodules-cache-path=DIR/module-cache -Rcompile-job-cache", + "command" : "clang -x c-header DIR/prefix.h -o DIR/prefix.h.pch -fmodules -fimplicit-modules -fimplicit-module-maps -fmodules-cache-path=DIR/module-cache -Rcompile-job-cache", "file" : "DIR/prefix.h" }, ] @@ -200,7 +200,7 @@ [ { "directory" : "DIR", - "command" : "clang_tool -fsyntax-only DIR/tu.c -include DIR/prefix.h -fmodules -fimplicit-modules -fimplicit-module-maps -fmodules-cache-path=DIR/module-cache -Rcompile-job-cache", + "command" : "clang -fsyntax-only DIR/tu.c -include DIR/prefix.h -fmodules -fimplicit-modules -fimplicit-module-maps -fmodules-cache-path=DIR/module-cache -Rcompile-job-cache", "file" : "DIR/tu.c" }, ] diff --git a/clang/test/ClangScanDeps/modules-in-stable-dirs-prefix-mapping.c b/clang/test/ClangScanDeps/modules-in-stable-dirs-prefix-mapping.c new file mode 100644 index 0000000000000..965e30e65f8da --- /dev/null +++ b/clang/test/ClangScanDeps/modules-in-stable-dirs-prefix-mapping.c @@ -0,0 +1,94 @@ +// This test verifies modules that are entirely comprised from stable directory inputs are captured in +// dependency information when paths are prefix mapped. + +// REQUIRES: shell,ondisk_cas + +// RUN: rm -rf %t +// RUN: split-file %s %t +// RUN: sed -e "s|DIR|%/t|g" %t/overlay.json.template > %t/overlay.json +// RUN: sed -e "s|DIR|%/t|g" %t/compile.json.in > %t/compile.json + +// RUN: clang-scan-deps -compilation-database %t/compile.json \ +// RUN: -prefix-map=%t/modules=/^modules -prefix-map=%t=/^src -prefix-map-sdk=/^sdk -prefix-map-toolchain=/^tc \ +// RUN: -cas-path %t/cas -module-files-dir %t/modules \ +// RUN: -j 1 -format experimental-full > %t/deps.db + +// RUN: cat %t/deps.db | sed 's:\\\\\?:/:g' | FileCheck %s -DPREFIX=%/t --check-prefix DEP + +// DEP: "is-in-stable-directories": true +// DEP: "name": "A" + +// CHECK-NOT: "is-in-stable-directories": true +// CHECK-NOT: warning: +// CHECK-NOT: error: + +//--- compile.json.in +[ +{ + "directory": "DIR", + "command": "clang -x c -c DIR/client.c -isysroot DIR/MacOSX.sdk -IDIR/BuildDir -ivfsoverlay DIR/overlay.json -IDIR/MacOSX.sdk/usr/include -fmodules -fmodules-cache-path=DIR/module-cache -fimplicit-module-maps -o DIR/client.c.o", + "file": "DIR/client.c" +} +] + +//--- overlay.json.template +{ + "version": 0, + "case-sensitive": "false", + "roots": [ + { + "external-contents": "DIR/BuildDir/B_vfs.h", + "name": "DIR/MacOSX.sdk/usr/include/B/B_vfs.h", + "type": "file" + } + ] +} + +//--- MacOSX.sdk/usr/include/A/module.modulemap +module A [system] { + umbrella "." +} + +//--- MacOSX.sdk/usr/include/A/A.h +typedef int A_type; + +//--- MacOSX.sdk/usr/include/B/module.modulemap +module B [system] { + umbrella "." +} + +//--- MacOSX.sdk/usr/include/B/B.h +#include + +//--- BuildDir/B_vfs.h +typedef int local_t; + +//--- MacOSX.sdk/usr/include/sys/sys.h +#include +typedef int sys_t_m; + +//--- MacOSX.sdk/usr/include/sys/module.modulemap +module sys [system] { + umbrella "." +} + +//--- MacOSX.sdk/usr/include/B_transitive/B.h +#include + +//--- MacOSX.sdk/usr/include/B_transitive/module.modulemap +module B_transitive [system] { + umbrella "." +} + +//--- MacOSX.sdk/usr/include/C/module.modulemap +module C [system] { + umbrella "." +} + +//--- MacOSX.sdk/usr/include/C/C.h +#include + + +//--- client.c +#include +#include // This dependency transitively depends on a local header. From 81ecd51590f99216f2eef760bc1f4248f6dfbef9 Mon Sep 17 00:00:00 2001 From: Cyndy Ishida Date: Thu, 17 Apr 2025 08:48:37 -0700 Subject: [PATCH 2/3] Update comment --- .../lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp b/clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp index 7f8e4221f895c..16034ef853320 100644 --- a/clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp +++ b/clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp @@ -604,7 +604,7 @@ class DependencyScanningAction : public tooling::ToolAction { // strings tied to the CompilerInstance. Otherwise, these potentially mapped // strings need to be allocated. To reduce the number of downstream changes // required to this support allocation, tie the lifetime of these strings to - // visiting all dependencies of a prebuilt module. + // the dependency scanning action. llvm::PrefixMapper &Mapper = ScanInstance.getPrefixMapper(); llvm::BumpPtrAllocator Allocator; llvm::StringSaver StableDirStorage(Allocator); From 0948a5eb5edefe9f27825e2d987559fa1b4c84ef Mon Sep 17 00:00:00 2001 From: Cyndy Ishida Date: Fri, 18 Apr 2025 14:58:24 -0700 Subject: [PATCH 3/3] Address review comments * Assign the CompilerInstance's PrefixMapper during the ActionController's for compiler caching * Add PCH test --- .../CASFSActionController.cpp | 1 + .../DependencyScanningWorker.cpp | 21 +-- .../IncludeTreeActionController.cpp | 1 + ...dules-in-stable-dirs-with-prefix-mapping.c | 139 ++++++++++++++++++ 4 files changed, 149 insertions(+), 13 deletions(-) create mode 100644 clang/test/ClangScanDeps/prebuilt-modules-in-stable-dirs-with-prefix-mapping.c diff --git a/clang/lib/Tooling/DependencyScanning/CASFSActionController.cpp b/clang/lib/Tooling/DependencyScanning/CASFSActionController.cpp index f1681522df659..7eee2bb1c5ff6 100644 --- a/clang/lib/Tooling/DependencyScanning/CASFSActionController.cpp +++ b/clang/lib/Tooling/DependencyScanning/CASFSActionController.cpp @@ -56,6 +56,7 @@ Error CASFSActionController::initialize(CompilerInstance &ScanInstance, // Setup prefix mapping. Mapper.emplace(&CacheFS); DepscanPrefixMapping::configurePrefixMapper(NewInvocation, *Mapper); + ScanInstance.setPrefixMapper(*Mapper); const PreprocessorOptions &PPOpts = ScanInstance.getPreprocessorOpts(); if (!PPOpts.Includes.empty() || !PPOpts.ImplicitPCHInclude.empty()) diff --git a/clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp b/clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp index 16034ef853320..78a728535a621 100644 --- a/clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp +++ b/clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp @@ -586,10 +586,14 @@ class DependencyScanningAction : public tooling::ToolAction { auto *FileMgr = ScanInstance.createFileManager(FS); ScanInstance.createSourceManager(*FileMgr); - // Initialize PrefixMapper incase mappings exist. - DepscanPrefixMapping::configurePrefixMapper( - ScanInstance.getFrontendOpts().PathPrefixMappings, - ScanInstance.getPrefixMapper()); + auto reportError = [&ScanInstance](Error &&E) -> bool { + ScanInstance.getDiagnostics().Report(diag::err_cas_depscan_failed) + << std::move(E); + return false; + }; + + if (Error E = Controller.initialize(ScanInstance, OriginalInvocation)) + return reportError(std::move(E)); // Create a collection of stable directories derived from the ScanInstance // for determining whether module dependencies would fully resolve from @@ -649,12 +653,6 @@ class DependencyScanningAction : public tooling::ToolAction { Opts->IncludeSystemHeaders = true; } - auto reportError = [&ScanInstance](Error &&E) -> bool { - ScanInstance.getDiagnostics().Report(diag::err_cas_depscan_failed) - << std::move(E); - return false; - }; - // FIXME: The caller APIs in \p DependencyScanningTool expect a specific // DependencyCollector to get attached to the preprocessor in order to // function properly (e.g. \p FullDependencyConsumer needs \p @@ -724,9 +722,6 @@ class DependencyScanningAction : public tooling::ToolAction { if (ScanInstance.getFrontendOpts().ProgramAction == frontend::GeneratePCH) ScanInstance.getLangOpts().CompilingPCH = true; - if (Error E = Controller.initialize(ScanInstance, OriginalInvocation)) - return reportError(std::move(E)); - if (ScanInstance.getDiagnostics().hasErrorOccurred()) return false; diff --git a/clang/lib/Tooling/DependencyScanning/IncludeTreeActionController.cpp b/clang/lib/Tooling/DependencyScanning/IncludeTreeActionController.cpp index afee5dbf93aa0..27908e052f26e 100644 --- a/clang/lib/Tooling/DependencyScanning/IncludeTreeActionController.cpp +++ b/clang/lib/Tooling/DependencyScanning/IncludeTreeActionController.cpp @@ -299,6 +299,7 @@ Expected IncludeTreeActionController::getIncludeTree() { Error IncludeTreeActionController::initialize( CompilerInstance &ScanInstance, CompilerInvocation &NewInvocation) { DepscanPrefixMapping::configurePrefixMapper(NewInvocation, PrefixMapper); + ScanInstance.setPrefixMapper(PrefixMapper); auto ensurePathRemapping = [&]() { if (PrefixMapper.empty()) diff --git a/clang/test/ClangScanDeps/prebuilt-modules-in-stable-dirs-with-prefix-mapping.c b/clang/test/ClangScanDeps/prebuilt-modules-in-stable-dirs-with-prefix-mapping.c new file mode 100644 index 0000000000000..ecff36210b3af --- /dev/null +++ b/clang/test/ClangScanDeps/prebuilt-modules-in-stable-dirs-with-prefix-mapping.c @@ -0,0 +1,139 @@ +/// This test validates that modules that depend on prebuilt modules +/// resolve `is-in-stable-directories` correctly. + +// REQUIRES: shell,ondisk_cas + +/// The steps are: +/// 1. Scan dependencies to build the PCH. One of the module's depend on header +/// that is seemingly from the sysroot. However, it depends on a local header. +/// 2. Build the PCH & dependency PCMs. +/// 3. Scan a source file that transitively depends on the same modules as the pcm. + +// REQUIRES: shell +// RUN: rm -rf %t +// RUN: split-file %s %t +// RUN: sed -e "s|DIR|%/t|g" %t/compile-pch.json.in > %t/compile-pch.json +// RUN: sed -e "s|DIR|%/t|g" %t/compile-commands.json.in > %t/compile-commands.json + +// == Scan PCH +// RUN: clang-scan-deps -compilation-database %t/compile-pch.json -format experimental-full \ +// RUN: -cas-path %t/cas -module-files-dir %t/modules \ +// RUN: -prefix-map=%t/modules=/^modules -prefix-map=%t=/^src -prefix-map-sdk=/^sdk -prefix-map-toolchain=/^tc \ +// RUN: > %t/deps_pch.db + +// == Build Deps & PCH +// RUN: %deps-to-rsp %t/deps_pch.db --module-name=A > %t/A.cc1.rsp +// RUN: %deps-to-rsp %t/deps_pch.db --module-name=B > %t/B.cc1.rsp +// RUN: %deps-to-rsp %t/deps_pch.db --module-name=B_transitive > %t/B_transitive.cc1.rsp +// RUN: %deps-to-rsp %t/deps_pch.db --module-name=C > %t/C.cc1.rsp +// RUN: %deps-to-rsp %t/deps_pch.db --tu-index 0 > %t/pch.cc1.rsp +// RUN: %clang @%t/A.cc1.rsp +// RUN: %clang @%t/B.cc1.rsp +// RUN: %clang @%t/B_transitive.cc1.rsp +// RUN: %clang @%t/C.cc1.rsp +// Ensure we load pcms from action cache +// RUN: rm -rf %t/modules +// RUN: %clang @%t/pch.cc1.rsp + +// == Scan TU +// RUN: clang-scan-deps -compilation-database %t/compile-commands.json -format experimental-full \ +// RUN: -cas-path %t/cas -module-files-dir %t/modules \ +// RUN: -prefix-map=%t/modules=/^modules -prefix-map=%t=/^src -prefix-map-sdk=/^sdk -prefix-map-toolchain=/^tc \ +// RUN: > %t/deps.db + +// == Check Deps +// RUN: cat %t/deps_pch.db | sed 's:\\\\\?:/:g' | FileCheck %s -DPREFIX=%/t --check-prefix PCH_DEP +// RUN: cat %t/deps.db | sed 's:\\\\\?:/:g' | FileCheck %s -DPREFIX=%/t --check-prefix CLIENT + +// PCH_DEP: "is-in-stable-directories": true +// PCH_DEP: "name": "A" + +// PCH_DEP-NOT: "is-in-stable-directories": true + +// Verify is-in-stable-directories is only assigned to the module that only depends on A. +// CLIENT-NOT: "is-in-stable-directories": true + +// CLIENT: "name": "D" +// CLIENT: "is-in-stable-directories": true +// CLIENT: "name": "sys" + +// CLIENT-NOT: "is-in-stable-directories": true + +//--- compile-pch.json.in +[ +{ + "directory": "DIR", + "command": "clang -x c-header -c DIR/prebuild.h -isysroot DIR/MacOSX.sdk -IDIR/BuildDir -IDIR/MacOSX.sdk/usr/include -fmodules -fmodules-cache-path=DIR/module-cache -fimplicit-module-maps -o DIR/prebuild.pch", + "file": "DIR/prebuild.h" +} +] + +//--- compile-commands.json.in +[ +{ + "directory": "DIR", + "command": "clang -c DIR/client.c -isysroot DIR/MacOSX.sdk -IDIR/BuildDir -IDIR/MacOSX.sdk/usr/include -fmodules -fmodules-cache-path=DIR/module-cache -fimplicit-module-maps -include-pch DIR/prebuild.pch", + "file": "DIR/client.c" +} +] + +//--- MacOSX.sdk/usr/include/A/module.modulemap +module A [system] { + umbrella "." +} + +//--- MacOSX.sdk/usr/include/A/A.h +typedef int A_type; + +//--- MacOSX.sdk/usr/include/B/module.modulemap +module B [system] { + umbrella "." +} + +//--- MacOSX.sdk/usr/include/B/B.h +#include + +//--- BuildDir/Local/Local.h +typedef int local_t; + +//--- MacOSX.sdk/usr/include/sys/sys.h +#include +typedef int sys_t_m; + +//--- MacOSX.sdk/usr/include/sys/module.modulemap +module sys [system] { + umbrella "." +} + +//--- MacOSX.sdk/usr/include/B_transitive/B.h +#include + +//--- MacOSX.sdk/usr/include/B_transitive/module.modulemap +module B_transitive [system] { + umbrella "." +} + +//--- MacOSX.sdk/usr/include/C/module.modulemap +module C [system] { + umbrella "." +} + +//--- MacOSX.sdk/usr/include/C/C.h +#include + + +//--- MacOSX.sdk/usr/include/D/module.modulemap +module D [system] { + umbrella "." +} + +//--- MacOSX.sdk/usr/include/D/D.h +#include + +//--- prebuild.h +#include +#include // This dependency transitively depends on a local header. + +//--- client.c +#include +#include // This dependency transitively depends on a local header.