Update CoverageMappingFormat to Version6 #90047

Swatinem · 2021-10-19T09:18:55Z

Version 5 adds Branch Regions which are a prerequisite for branch coverage (#79649) which I plan to take a look at next.
Version 6 can use the zeroth filename as prefix for other relative files.

richkadel · 2021-10-19T15:58:21Z

cc: @tmandry @wesleywiser

compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs

src/doc/unstable-book/src/compiler-flags/instrument-coverage.md

src/test/run-make-fulldeps/coverage-llvmir/Makefile

src/test/run-make-fulldeps/coverage-reports/Makefile

compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs

Swatinem · 2021-10-20T07:42:37Z

Q: should I keep backwards compat with version 4 / llvm 11?
I think that later on I will update the tests to actually use the branch coverage output, at which point the tests would require llvm 13. Do we want to have support down to 11 although that might be untested?

richkadel · 2021-10-20T15:55:41Z

Q: should I keep backwards compat with version 4 / llvm 11? I think that later on I will update the tests to actually use the branch coverage output, ...

It seems like it will be really easy to keep it compatible with LLVM 11, and as long as rustc still supports LLVM 11 (I assume it does still), there is probably no reason not to maintain compatibility as long as you can, IMO. (My opinion is based on the believe that this requires only a couple of additional lines of code, which appears to be true.)

I suspect branch coverage (if it's even needed... see my Zulip comment) will require some time to implement and test, and if so, it may be helpful to LLVM 11 users for some time to come.

Is it critical? Probably not, but I think it's worth supporting, given these assumptions.

at which point the tests would require llvm 13.

Correct me if I'm wrong, but branch coverage only requires LLVM 12, right?

Do we want to have support down to 11 although that might be untested?

Hm... I was assuming there are still tests using older versions of LLVM, for every major version supported by Rustc. Isn't this the case? If rustc doesn't officially support LLVM 11 anymore (or when they don't) then we don't need to support it any longer in coverage either. I'm just requesting we support it as long as rustc officially does.

mati865 · 2021-10-20T16:21:52Z

It seems like it will be really easy to keep it compatible with LLVM 11, and as long as rustc still supports LLVM 11 (I assume it does still),

LLVM 11 is not going away soon, support for LLVM 10 is being dropped right now: #90062

richkadel · 2021-10-21T16:38:37Z

@Swatinem - I will be OOO, mostly without a laptop, for 3 weeks, after today. I'd like to nominate @tmandry or @wesleywiser to review and approve this when you are ready. Thanks for your contributions!!

Also, since there is now a proposal to stablize the instrument-coverage flag (#90132), I definitely think we need to keep LLVM 11 support for as long as feasible, but I fully agree with adding support for Coverage Mapping Format versions 5 and 6, depending on the build-time version of LLVM.

r? @tmandry (this probably doesn't work while the PR is in Draft mode)

Swatinem · 2021-10-21T21:15:15Z

@richkadel thanks for your feedback so far!

I think I will pause this at least until LLVM 11 and stabilization of the flag happens, in the meanwhile the feedback has guided me in another direction I want to explore.

wesleywiser · 2021-10-22T15:48:34Z

Also, since there is now a proposal to stablize the instrument-coverage flag (#90132), I definitely think we need to keep LLVM 11 support for as long as feasible, but I fully agree with adding support for Coverage Mapping Format versions 5 and 6, depending on the build-time version of LLVM.

+1, supporting versions 4 & 5 sounds like it will be pretty easy so I think we should definitely do that. If that isn't true, we can consider dropping support for older versions but having the widest range of support for older LLVM versions would be best.

mati865 · 2021-10-22T18:00:26Z

FYI we might end up bumping minimal LLVM to 12: #90175, that will allow to simplify this PR.

bors · 2021-10-24T00:36:08Z

☔ The latest upstream changes (presumably #90175) made this pull request unmergeable. Please resolve the merge conflicts.

mati865 · 2021-10-29T14:58:43Z

@Swatinem LLVM 12 is the minimum now.

Swatinem · 2021-11-01T11:24:47Z

Updated the PR and rebased on latest master, should be ready for review now.

r? @tmandry

wesleywiser · 2021-11-01T14:47:58Z

compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs

-/// and published in Rust's November 2020 fork of LLVM. This version is supported by the LLVM
+/// This Coverage Map complies with Coverage Mapping Format version 6 (zero-based encoded as 5),
+/// as defined at [LLVM Code Coverage Mapping Format](https://github.com/rust-lang/llvm-project/blob/rustc/13.0-2021-09-30/llvm/docs/CoverageMappingFormat.rst#llvm-code-coverage-mapping-format)
+/// and published in Rust's September 2021 fork of LLVM. This version is supported by the LLVM


nit: We might consider removing this date from the comment. I'm not sure it really adds any value.

compiler/rustc_llvm/llvm-wrapper/CoverageMappingWrapper.cpp

tmandry · 2021-11-01T23:45:44Z

@bors r+

bors · 2021-11-01T23:45:45Z

📌 Commit 52c205a34fa958f1a5b74c8016e62c21c4d7a08d has been approved by tmandry

bors · 2021-11-02T07:00:34Z

⌛ Testing commit 52c205a34fa958f1a5b74c8016e62c21c4d7a08d with merge 6fe98430601dc3b277da8938870e10b14f132e3f...

bors · 2021-11-02T07:47:45Z

💔 Test failed - checks-actions

Swatinem · 2021-11-02T20:30:05Z

Removed the date (and corrected the comment to say that we output version 5) as suggested by @wesleywiser and rebased on recent master.

I’m a bit baffled by the CI failure. I can’t reproduce that on my Windows, will try with Linux tomorrow.
From the logs, it looks like the functions (counters) inlined into the main crate are being discarded.

tmandry · 2021-11-10T00:40:34Z

@Swatinem are you still available to investigate the CI failure?

Swatinem · 2021-11-10T18:12:01Z

Haven’t had time yet to investigate, pretty much saturated with other things right now.

compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs

Version 5 adds Branch Regions which are a prerequisite for branch coverage. Version 6 can use the zeroth filename as prefix for other relative files.

richkadel · 2021-11-25T01:34:42Z

Haven’t had time yet to investigate, pretty much saturated with other things right now.

I could not repro that CI error, neither on my local Linux machine nor on the docker image that I believe corresponds to where that error occurred. I think it was an intermittent error that may not happen again?

This commit augments Swatinem's initial commit in uncommitted PR rust-lang#90047, which was a great starting point, but did not fully support LLVM Coverage Mapping Format version 6. Version 6 requires adding the compilation directory when file paths are relative, and since Rustc coverage maps use relative paths, we should add the expected compilation directory entry. Note, however, that with the compilation directory, coverage reports from `llvm-cov show` can now report file names (when the report includes more than one file) with the full absolute path to the file. This would be a problem for test results, but the workaround (for the rust coverage tests) is to include an additional `llvm-cov show` parameter: `--compilation-dir=.`

… r=tmandry Add support for LLVM coverage mapping format versions 5 and 6 This PR cherry-pick's Swatinem's initial commit in unsubmitted PR rust-lang#90047. My additional commit augments Swatinem's great starting point, but adds full support for LLVM Coverage Mapping Format version 6, conditionally, if compiling with LLVM 13. Version 6 requires adding the compilation directory when file paths are relative, and since Rustc coverage maps use relative paths, we should add the expected compilation directory entry. Note, however, that with the compilation directory, coverage reports from `llvm-cov show` can now report file names (when the report includes more than one file) with the full absolute path to the file. This would be a problem for test results, but the workaround (for the rust coverage tests) is to include an additional `llvm-cov show` parameter: `--compilation-dir=.`

Swatinem · 2021-12-01T09:57:44Z

Closing in favor of #91207 which subsumes this commit

Swatinem mentioned this pull request Oct 19, 2021

Update CoverageMapping Format Version requirements rust-lang/rustc-dev-guide#1235

Closed

Swatinem force-pushed the bump-coverage-version branch from d0ec389 to d8544b9 Compare October 19, 2021 19:07

Swatinem changed the title ~~Update CoverageMappingFormat to Version5~~ Update CoverageMappingFormat to Version6 Oct 19, 2021

richkadel suggested changes Oct 19, 2021

View reviewed changes

Swatinem marked this pull request as draft October 20, 2021 15:58

Mark-Simulacrum mentioned this pull request Oct 21, 2021

Stabilize -Z instrument-coverage as -C instrument-coverage #90132

Merged

bors added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Oct 24, 2021

Swatinem force-pushed the bump-coverage-version branch from d8544b9 to 52c205a Compare November 1, 2021 11:20

Swatinem marked this pull request as ready for review November 1, 2021 11:22

rust-highfive assigned tmandry Nov 1, 2021

wesleywiser reviewed Nov 1, 2021

View reviewed changes

tmandry reviewed Nov 1, 2021

View reviewed changes

compiler/rustc_llvm/llvm-wrapper/CoverageMappingWrapper.cpp Show resolved Hide resolved

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 1, 2021

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 2, 2021

This comment has been minimized.

Sign in to view

Swatinem force-pushed the bump-coverage-version branch from 52c205a to 99b7e9c Compare November 2, 2021 19:53

tmandry added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 10, 2021

apiraino added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Nov 11, 2021

trinity-1686a reviewed Nov 14, 2021

View reviewed changes

compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs Outdated Show resolved Hide resolved

Update CoverageMappingFormat Support to Version6

90ee252

Version 5 adds Branch Regions which are a prerequisite for branch coverage. Version 6 can use the zeroth filename as prefix for other relative files.

Swatinem force-pushed the bump-coverage-version branch from 99b7e9c to 90ee252 Compare November 15, 2021 07:55

richkadel mentioned this pull request Nov 25, 2021

Add support for LLVM coverage mapping format versions 5 and 6 #91207

Merged

Swatinem closed this Dec 1, 2021

Swatinem deleted the bump-coverage-version branch December 2, 2021 22:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update CoverageMappingFormat to Version6 #90047

Update CoverageMappingFormat to Version6 #90047

Swatinem commented Oct 19, 2021 •

edited

Loading

richkadel commented Oct 19, 2021

Swatinem commented Oct 20, 2021

richkadel commented Oct 20, 2021

mati865 commented Oct 20, 2021 •

edited

Loading

richkadel commented Oct 21, 2021 •

edited

Loading

Swatinem commented Oct 21, 2021

wesleywiser commented Oct 22, 2021

mati865 commented Oct 22, 2021

bors commented Oct 24, 2021

mati865 commented Oct 29, 2021

Swatinem commented Nov 1, 2021

wesleywiser Nov 1, 2021

tmandry commented Nov 1, 2021

bors commented Nov 1, 2021

bors commented Nov 2, 2021

bors commented Nov 2, 2021

This comment has been minimized.

Swatinem commented Nov 2, 2021

tmandry commented Nov 10, 2021

Swatinem commented Nov 10, 2021

richkadel commented Nov 25, 2021

Swatinem commented Dec 1, 2021

Update CoverageMappingFormat to Version6 #90047

Update CoverageMappingFormat to Version6 #90047

Conversation

Swatinem commented Oct 19, 2021 • edited Loading

richkadel commented Oct 19, 2021

Swatinem commented Oct 20, 2021

richkadel commented Oct 20, 2021

mati865 commented Oct 20, 2021 • edited Loading

richkadel commented Oct 21, 2021 • edited Loading

Swatinem commented Oct 21, 2021

wesleywiser commented Oct 22, 2021

mati865 commented Oct 22, 2021

bors commented Oct 24, 2021

mati865 commented Oct 29, 2021

Swatinem commented Nov 1, 2021

wesleywiser Nov 1, 2021

Choose a reason for hiding this comment

tmandry commented Nov 1, 2021

bors commented Nov 1, 2021

bors commented Nov 2, 2021

bors commented Nov 2, 2021

This comment has been minimized.

Swatinem commented Nov 2, 2021

tmandry commented Nov 10, 2021

Swatinem commented Nov 10, 2021

richkadel commented Nov 25, 2021

Swatinem commented Dec 1, 2021

Swatinem commented Oct 19, 2021 •

edited

Loading

mati865 commented Oct 20, 2021 •

edited

Loading

richkadel commented Oct 21, 2021 •

edited

Loading