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

Conversation

cyndyishida
Copy link

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 be compared. For example,
input paths that represent included headers will contain an absolute
paths for handling indirections like when vfsoverlays are used,
but also when sysroot's from CompilerInstance's are mapped.

@cyndyishida
Copy link
Author

cyndyishida commented Apr 15, 2025

0003c84 is included for context but is under review against upstream.

The benefit of special handling of modules that come from stable directories is defined away when compiler caching is enabled. So, keeping this check is mostly to ensure the checks still hold when the scanner has to deal with prefix-mappings.
Some observations I'd like to confirm:

  1. Whether compiler caching is on, include trees are generated, and prefixes are mapped, are independent options. I'm not sure of the reason why, e.g. if this is for testing purposes but otherwise these should all work together.
  2. clang_tool is some test wrapper over clang which doesn't seem relevant still. I replaced it in tests that complained about mismatching resource directories, but if there aren't concerns I can update all the tests using it.

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.
Copy link

@jansvoboda11 jansvoboda11 left a comment

Choose a reason for hiding this comment

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

This makes sense to me, but I'd like @benlangmuir to take a look too.

@@ -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.

@benlangmuir
Copy link

Whether compiler caching is on, include trees are generated, and prefixes are mapped, are independent options. I'm not sure of the reason why, e.g. if this is for testing purposes but otherwise these should all work together.

They're related, but somewhat independent concepts.

  • Caching can be done with either include-tree or cas-fs. This is mainly historical and to maintain test coverage for the cas-fs mechanism at this point, as we only support include-tree outside tests.
  • Prefix mapping is optional. If you don't prefix map you can still do caching, but the input paths need to match in order to get hits.

* Assign the CompilerInstance's PrefixMapper during the ActionController's
for compiler caching
* Add PCH test
@cyndyishida
Copy link
Author

@swift-ci please test LLVM

@cyndyishida
Copy link
Author

PR failures look unrelated and/or already known

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants