Skip to content

[clang][depscan] Support prefix mappings when deciding modules that come from "stable" directories #10493

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: next
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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())
Expand Down
32 changes: 23 additions & 9 deletions clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -586,6 +586,15 @@ class DependencyScanningAction : public tooling::ToolAction {
auto *FileMgr = ScanInstance.createFileManager(FS);
ScanInstance.createSourceManager(*FileMgr);

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
// those directories.
Expand All @@ -595,6 +604,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
// the dependency scanning action.
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
Expand Down Expand Up @@ -630,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
Expand Down Expand Up @@ -705,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;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -299,6 +299,7 @@ Expected<cas::IncludeTreeRoot> IncludeTreeActionController::getIncludeTree() {
Error IncludeTreeActionController::initialize(
CompilerInstance &ScanInstance, CompilerInvocation &NewInvocation) {
DepscanPrefixMapping::configurePrefixMapper(NewInvocation, PrefixMapper);
ScanInstance.setPrefixMapper(PrefixMapper);

auto ensurePathRemapping = [&]() {
if (PrefixMapper.empty())
Expand Down
9 changes: 4 additions & 5 deletions clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -255,12 +255,11 @@ bool dependencies::isPathInStableDir(const ArrayRef<StringRef> Directories,

bool dependencies::areOptionsInStableDir(const ArrayRef<StringRef> 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)
Expand Down
2 changes: 1 addition & 1 deletion clang/test/ClangScanDeps/include-tree-with-pch.c
Original file line number Diff line number Diff line change
Expand Up @@ -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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this change necessary?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, a sysroot is always passed implicitly (through the lit testing configuration) and that doesn't match with the one passed in https://github.com/swiftlang/llvm-project/pull/10493/files#diff-aaa36845ad74ad6019adf78071e73c17d9e5db7080eb49de2605959a742d98faR100

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is that a problem now but wasn't before?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The assertion was firing. I think the assertion should always hold unless a build is misconfigured, but I may be missing something.

// 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

Expand Down
4 changes: 2 additions & 2 deletions clang/test/ClangScanDeps/modules-cas-trees-input-files.c
Original file line number Diff line number Diff line change
Expand Up @@ -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"
},
]
Expand All @@ -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"
},
]
Expand Down
4 changes: 2 additions & 2 deletions clang/test/ClangScanDeps/modules-cas-trees-with-pch.c
Original file line number Diff line number Diff line change
Expand Up @@ -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"
},
]
Expand All @@ -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"
},
]
Expand Down
94 changes: 94 additions & 0 deletions clang/test/ClangScanDeps/modules-in-stable-dirs-prefix-mapping.c
Original file line number Diff line number Diff line change
@@ -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 <B/B_vfs.h>

//--- BuildDir/B_vfs.h
typedef int local_t;

//--- MacOSX.sdk/usr/include/sys/sys.h
#include <A/A.h>
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 <B/B.h>

//--- 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 <B_transitive/B.h>


//--- client.c
#include <A/A.h>
#include <C/C.h> // This dependency transitively depends on a local header.
Original file line number Diff line number Diff line change
@@ -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 <Local/Local.h>

//--- BuildDir/Local/Local.h
typedef int local_t;

//--- MacOSX.sdk/usr/include/sys/sys.h
#include <A/A.h>
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 <B/B.h>

//--- 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 <B_transitive/B.h>


//--- MacOSX.sdk/usr/include/D/module.modulemap
module D [system] {
umbrella "."
}

//--- MacOSX.sdk/usr/include/D/D.h
#include <C/C.h>

//--- prebuild.h
#include <A/A.h>
#include <C/C.h> // This dependency transitively depends on a local header.

//--- client.c
#include <sys/sys.h>
#include <D/D.h> // This dependency transitively depends on a local header.