Skip to content
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

Update CoverageMapping Format Version requirements #1235

Closed
wants to merge 1 commit into from

Conversation

Swatinem
Copy link

This is the companion to rust-lang/rust#90047.

r? @richkadel

Copy link
Contributor

@richkadel richkadel left a comment

Choose a reason for hiding this comment

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

Hey @Swatinem ! Exciting to see an upgrade here. Thank you.

I'm taking a look at the devguide first, which I see is a small change.

There's one issue, but it raises the question, why upgrade to version 5 and not 6?


[^llvm-and-covmap-versions]: The Rust compiler (as of
January 2021) supports _LLVM Coverage Mapping Format_ Version 4 (the most
October 2021) supports _LLVM Coverage Mapping Format_ Version 5 (the most
Copy link
Contributor

Choose a reason for hiding this comment

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

Version 5 is not the "most current version". The most current version is version 6.

(But I am wondering why not upgrade to version 6 and skip 5.)

@Swatinem
Copy link
Author

Good point, might as well jump to v6. I was under the impression that we should keep compatibility with lower versions as much as we can to get the features we need.

Also, I haven’t looked at it in too much detail yet, so I don’t know yet if the v6 changes need other code changes as well.

@richkadel
Copy link
Contributor

The description here makes it sound like a minor change, but please take a look and update this thread if it appears more complex than that.

@richkadel
Copy link
Contributor

cc: @tmandry @wesleywiser

@Swatinem
Copy link
Author

@richkadel I updated to v6. The format is backwards compatible if you only use absolute filenames.
See rust-lang/llvm-project@5fbd1a3#diff-d1da015180caa93a34599914b8a918220740263cec7756718f3762bd0d9a4abaR158-R174
I still pushed an empty filename as the first one.

@Swatinem Swatinem marked this pull request as draft October 20, 2021 15:58
@richkadel
Copy link
Contributor

Since I will be OOO for 3 weeks, please ask @tmandry or @wesleywiser to review, when you are ready. Thanks again for these contributions!

@camelid camelid added the S-waiting-on-author Status: this PR is waiting for additional action by the OP label Nov 2, 2021
@@ -221,15 +221,15 @@ substitution combinations), `mapgen`'s `finalize()` method queries the
and `CodeRegion`s; and calls LLVM codegen APIs to generate
properly-configured variables in LLVM IR, according to very specific
details of the [_LLVM Coverage Mapping Format_][coverage-mapping-format]
(Version 4).[^llvm-and-covmap-versions]
(Version 6).[^llvm-and-covmap-versions]

[^llvm-and-covmap-versions]: The Rust compiler (as of
Copy link
Member

@camelid camelid Nov 2, 2021

Choose a reason for hiding this comment

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

Could you add a machine-checkable date comment?

Suggested change
[^llvm-and-covmap-versions]: The Rust compiler (as of
[^llvm-and-covmap-versions]: The Rust compiler (as of <!-- date: 2021-11 -->

Copy link
Contributor

Choose a reason for hiding this comment

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

I made this change in the revised PR #1267.

@JohnTitor JohnTitor added S-blocked Status: this PR is blocked waiting for something and removed S-waiting-on-author Status: this PR is waiting for additional action by the OP labels Nov 2, 2021
@richkadel
Copy link
Contributor

@Swatinem - Feel free to close this PR in favor of #1267. I reviewed your changes in creating the revised PR (very helpful, thanks!) and applied the changes and review recommendations.

@Swatinem Swatinem closed this Dec 2, 2021
@Swatinem Swatinem deleted the coverage-bump branch December 2, 2021 22:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-blocked Status: this PR is blocked waiting for something
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants