Skip to content

Conversation

@Zalathar
Copy link
Member

@Zalathar Zalathar commented Dec 17, 2023

Normally the compiler emits column numbers as a 1-based number of Unicode code points.

But when we embed coverage mappings for -Cinstrument-coverage, those mappings will ultimately be read by the llvm-cov tool. That tool assumes that column numbers are 1-based numbers of bytes, and relies on that assumption when slicing up source code to apply highlighting (in HTML reports, and in text-based reports with colour).

For the very common case of all-ASCII source code, bytes and code points are the same, so the difference isn't noticeable. But for code that contains non-ASCII characters, emitting column numbers as code points will result in llvm-cov slicing strings in the wrong places, producing mangled output or fatal errors.

(See taiki-e/cargo-llvm-cov#275 as an example of what can go wrong.)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) labels Dec 17, 2023
@bors

This comment was marked as resolved.

@Zalathar
Copy link
Member Author

Zalathar commented Jan 6, 2024

#119034 has landed, so now I can think about making this ready for review.

@Zalathar Zalathar marked this pull request as ready for review January 6, 2024 01:50
@rustbot
Copy link
Collaborator

rustbot commented Jan 6, 2024

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

@Zalathar

This comment was marked as resolved.

@rustbot
Copy link
Collaborator

rustbot commented Jan 6, 2024

Error: Parsing assign command in comment failed: ...'' | error: specify user to assign to at >| ''...

Please file an issue on GitHub at triagebot if there's a problem with this bot, or reach out on #t-infra on Zulip.

@Zalathar
Copy link
Member Author

Zalathar commented Jan 6, 2024

r? compiler

Copy link
Member

@davidtwco davidtwco left a comment

Choose a reason for hiding this comment

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

r=me when CI passes

@oli-obk
Copy link
Contributor

oli-obk commented Jan 8, 2024

@bors delegate+

@bors
Copy link
Collaborator

bors commented Jan 8, 2024

✌️ @Zalathar, you can now approve this pull request!

If @oli-obk told you to "r=me" after making some further change, please make that change, then do @bors r=@oli-obk

@Zalathar
Copy link
Member Author

Zalathar commented Jan 8, 2024

@bors r=davidtwco

@bors
Copy link
Collaborator

bors commented Jan 8, 2024

📌 Commit 6971e93 has been approved by davidtwco

It is now in the queue for this repository.

@bors bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 8, 2024
@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Jan 8, 2024
@bors bors merged commit 70e3f8d into rust-lang:master Jan 9, 2024
@rustbot rustbot added this to the 1.77.0 milestone Jan 9, 2024
@Zalathar Zalathar deleted the unicode branch January 9, 2024 02:24
@Zalathar
Copy link
Member Author

Zalathar commented Mar 1, 2024

@rustbot label +relnotes

This is a user-visible bug fix for taiki-e/cargo-llvm-cov#275.

Suggested summary: “Fix coverage instrumentation/reports for non-ASCII source code”

@rustbot rustbot added the relnotes Marks issues that should be documented in the release notes of the next release. label Mar 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants