Skip to content

Add path remapping with -coverage-prefix-map to coverage data #32175

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

Merged
merged 1 commit into from
Jun 16, 2020

Conversation

keith
Copy link
Member

@keith keith commented Jun 4, 2020

Previously the path to covered files in the __LLVM_COV / __llvm_covmap
section were absolute. This made remote builds with coverage information
difficult because all machines would have to have the same build root.
This change uses the values for -coverage-prefix-map to remap files in
the coverage info to relative paths, or any other remapping that was
passed. These paths work correctly with llvm-cov when it is run from
the same source directory as the compilation, or from a different
directory using the -path-equivalence argument.

This is analogous to this change in clang https://reviews.llvm.org/D81122

@theblixguy theblixguy requested a review from vedantk June 4, 2020 16:50
@keith keith force-pushed the ks/coverage-relative branch from 84bad2f to 9f0839b Compare June 4, 2020 21:11
@keith keith requested a review from vedantk June 4, 2020 21:11
@keith
Copy link
Member Author

keith commented Jun 8, 2020

@vedantk mind checking up on this one again when you have a chance?

@vedantk
Copy link
Contributor

vedantk commented Jun 10, 2020

I need to check up on our conversation on cfe-dev. Petr raised a point about existing users of -debug-prefix-map + -profile-coverage-mapping: if e.g. they've got an Xcode project set up this way, the IDE integration may rely on being able to produce a coverage report from anywhere on the filesystem, which this change would break. So I think we'll need to add a new flag to swift to change this behavior.

@keith
Copy link
Member Author

keith commented Jun 10, 2020

I think regardless of a new flag or not Xcode would have to be updated to support these bundles generated from anywhere (assuming it wanted to support this or not)

I'm happy to add a new flag here, but I did think this one was "more ok" since unlike clang Swift feels pretty conservative with new flags and instead tries to do the "right thing" in most of these cases, which I would argue this is.

@vedantk
Copy link
Contributor

vedantk commented Jun 10, 2020

Yep, that's the issue -- Xcode doesn't support coverage mappings with relative paths today, and I'm not sure when (or if) it will. So I think we should try not to break coverage workflows that use -debug-prefix-map, esp. considering non-xcode workflows that hand-roll llvm-cov commands can have the same issue. Introducing a new driver option isn't ideal, but if part of its contract explicitly states that all paths in compiled objects are going to be remapped, hopefully that'll be the last one..

Previously the path to covered files in the __LLVM_COV / __llvm_covmap
section were absolute. This made remote builds with coverage information
difficult because all machines would have to have the same build root.
This change uses the values for `-coverage-prefix-map` to remap files in
the coverage info to relative paths. These paths work correctly with
llvm-cov when it is run from the same source directory as the
compilation, or from a different directory using the `-path-equivalence`
argument.

This is analogous to this change in clang https://reviews.llvm.org/D81122
@keith keith changed the title Add path remapping with -debug-prefix-map to coverage data Add path remapping with -coverage-prefix-map to coverage data Jun 16, 2020
@keith keith force-pushed the ks/coverage-relative branch from 9f0839b to 1b77448 Compare June 16, 2020 00:39
@keith
Copy link
Member Author

keith commented Jun 16, 2020

@vedantk I've updated this change with -coverage-prefix-map

keith added a commit to keith/swift-driver that referenced this pull request Jun 16, 2020
This is analogous to this change in Driver.cpp swiftlang/swift#32175
keith added a commit to keith/swift-driver that referenced this pull request Jun 16, 2020
This is analogous to this change in Driver.cpp swiftlang/swift#32175
Copy link
Contributor

@vedantk vedantk left a comment

Choose a reason for hiding this comment

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

Thanks, lgtm!

@vedantk
Copy link
Contributor

vedantk commented Jun 16, 2020

@swift-ci smoke test and merge

@swift-ci swift-ci merged commit 068bba3 into swiftlang:master Jun 16, 2020
@keith keith deleted the ks/coverage-relative branch June 16, 2020 21:06
@compnerd
Copy link
Member

I think that this is responsible for a Windows regression: https://ci-external.swift.org/job/oss-swift-windows-x86_64-vs2019/1737/

keith added a commit to bazelbuild/rules_swift that referenced this pull request Sep 15, 2020
keith added a commit to bazelbuild/rules_swift that referenced this pull request Oct 13, 2020
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.

4 participants