coverage: Restrict empty-span expansion to only cover `{` and `}` by Zalathar · Pull Request #132675 · rust-lang/rust

Zalathar · 2024-11-06T05:23:39Z

Coverage instrumentation has some tricky code for converting a coverage-relevant Span into a set of start/end line/byte-column coordinates that will be embedded in the CGU's coverage metadata.

A big part of this complexity is special code for handling empty spans, which are expanded into non-empty spans (if possible) because LLVM's coverage reporter does not handle empty spans well.

This PR simplifies that code by restricting it to only apply in two specific situations: when the character after the empty span is {, or the character before the empty span is }.

(As an added benefit, this means that the expanded spans no longer extend awkwardly beyond the end of a physical line, which was common under the previous implementation.)

Along the way, this PR also removes some unhelpful code for dealing with function source code spread across multiple files. Functions currently can't have coverage spans in multiple files, and if that ever changes (e.g. to properly support expansion regions) then this code will need to be completely overhauled anyway.

rustbot · 2024-11-06T05:23:47Z

r? @fee1-dead

rustbot has assigned @fee1-dead.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

rustbot · 2024-11-06T05:23:50Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

fee1-dead · 2024-11-06T14:01:01Z

r? compiler

jieyouxu · 2024-11-07T07:42:28Z

r? jieyouxu

compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs

This isn't strictly necessary, but makes it easier to distinguish `body_span` from the specific `span` being processed.

We already had a dedicated `LocalFileId` index type, but previously we used a raw `u32` for global file IDs, because index types were harder to pass through FFI.

Functions currently can't have mappings in multiple files, and if that ever changes (e.g. to properly support expansion regions), this code will need to be completely overhauled anyway.

jieyouxu

Thanks, this looks good to me for the most part, two nits.

compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs

compiler/rustc_codegen_llvm/src/coverageinfo/ffi.rs

jieyouxu · 2024-11-09T14:19:08Z

@rustbot author

Zalathar · 2024-11-10T01:05:25Z

Pass a LocalFileId to CoverageSpan::from_source_region (diff).

@rustbot ready

jieyouxu · 2024-11-10T05:16:45Z

Thanks!

@bors r+

bors · 2024-11-10T05:16:48Z

📌 Commit 925dfc8 has been approved by jieyouxu

It is now in the queue for this repository.

coverage: Restrict empty-span expansion to only cover `{` and `}` Coverage instrumentation has some tricky code for converting a coverage-relevant `Span` into a set of start/end line/byte-column coordinates that will be embedded in the CGU's coverage metadata. A big part of this complexity is special code for handling empty spans, which are expanded into non-empty spans (if possible) because LLVM's coverage reporter does not handle empty spans well. This PR simplifies that code by restricting it to only apply in two specific situations: when the character after the empty span is `{`, or the character before the empty span is `}`. (As an added benefit, this means that the expanded spans no longer extend awkwardly beyond the end of a physical line, which was common under the previous implementation.) Along the way, this PR also removes some unhelpful code for dealing with function source code spread across multiple files. Functions currently can't have coverage spans in multiple files, and if that ever changes (e.g. to properly support expansion regions) then this code will need to be completely overhauled anyway.

jieyouxu · 2024-11-10T12:07:25Z

@bors retry r- (yielding priority to a revert to fix a bootstrap issue)

jieyouxu · 2024-11-10T12:08:01Z

@bors r+

bors · 2024-11-10T12:08:03Z

📌 Commit 925dfc8 has been approved by jieyouxu

It is now in the queue for this repository.

jieyouxu · 2024-11-10T12:08:42Z

(Yielding to #132846)

coverage: Restrict empty-span expansion to only cover `{` and `}` Coverage instrumentation has some tricky code for converting a coverage-relevant `Span` into a set of start/end line/byte-column coordinates that will be embedded in the CGU's coverage metadata. A big part of this complexity is special code for handling empty spans, which are expanded into non-empty spans (if possible) because LLVM's coverage reporter does not handle empty spans well. This PR simplifies that code by restricting it to only apply in two specific situations: when the character after the empty span is `{`, or the character before the empty span is `}`. (As an added benefit, this means that the expanded spans no longer extend awkwardly beyond the end of a physical line, which was common under the previous implementation.) Along the way, this PR also removes some unhelpful code for dealing with function source code spread across multiple files. Functions currently can't have coverage spans in multiple files, and if that ever changes (e.g. to properly support expansion regions) then this code will need to be completely overhauled anyway.

bors · 2024-11-10T12:28:41Z

⌛ Testing commit 925dfc8 with merge 772f79d...

jieyouxu · 2024-11-10T12:39:10Z

@bors r- retry

jieyouxu · 2024-11-10T12:48:46Z

@bors r+

bors · 2024-11-10T12:48:48Z

📌 Commit 925dfc8 has been approved by jieyouxu

It is now in the queue for this repository.

bors · 2024-11-10T12:48:49Z

🌲 The tree is currently closed for pull requests below priority 1000. This pull request will be tested once the tree is reopened.

…iaskrgr Rollup of 3 pull requests Successful merges: - rust-lang#132675 (coverage: Restrict empty-span expansion to only cover `{` and `}`) - rust-lang#132849 (Miri subtree update) - rust-lang#132858 (Update minifer version to `0.3.2`) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#132675 - Zalathar:empty-spans, r=jieyouxu coverage: Restrict empty-span expansion to only cover `{` and `}` Coverage instrumentation has some tricky code for converting a coverage-relevant `Span` into a set of start/end line/byte-column coordinates that will be embedded in the CGU's coverage metadata. A big part of this complexity is special code for handling empty spans, which are expanded into non-empty spans (if possible) because LLVM's coverage reporter does not handle empty spans well. This PR simplifies that code by restricting it to only apply in two specific situations: when the character after the empty span is `{`, or the character before the empty span is `}`. (As an added benefit, this means that the expanded spans no longer extend awkwardly beyond the end of a physical line, which was common under the previous implementation.) Along the way, this PR also removes some unhelpful code for dealing with function source code spread across multiple files. Functions currently can't have coverage spans in multiple files, and if that ever changes (e.g. to properly support expansion regions) then this code will need to be completely overhauled anyway.

coverage: Restrict empty-span expansion to only cover `{` and `}` Coverage instrumentation has some tricky code for converting a coverage-relevant `Span` into a set of start/end line/byte-column coordinates that will be embedded in the CGU's coverage metadata. A big part of this complexity is special code for handling empty spans, which are expanded into non-empty spans (if possible) because LLVM's coverage reporter does not handle empty spans well. This PR simplifies that code by restricting it to only apply in two specific situations: when the character after the empty span is `{`, or the character before the empty span is `}`. (As an added benefit, this means that the expanded spans no longer extend awkwardly beyond the end of a physical line, which was common under the previous implementation.) Along the way, this PR also removes some unhelpful code for dealing with function source code spread across multiple files. Functions currently can't have coverage spans in multiple files, and if that ever changes (e.g. to properly support expansion regions) then this code will need to be completely overhauled anyway.

…iaskrgr Rollup of 3 pull requests Successful merges: - rust-lang#132675 (coverage: Restrict empty-span expansion to only cover `{` and `}`) - rust-lang#132849 (Miri subtree update) - rust-lang#132858 (Update minifer version to `0.3.2`) r? `@ghost` `@rustbot` modify labels: rollup

Changes required due to - rust-lang/rust#132675 (coverage: Restrict empty-span expansion to only cover { and }) By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 and MIT licenses.

Zalathar added the A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) label Nov 6, 2024

rustbot assigned fee1-dead Nov 6, 2024

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. labels Nov 6, 2024

rustbot assigned pnkfelix and unassigned fee1-dead Nov 6, 2024

rustbot assigned jieyouxu and unassigned pnkfelix Nov 7, 2024

Zalathar commented Nov 8, 2024

View reviewed changes

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

coverage: Pass ExtractedHirInfo to make_source_region

7c76995

This isn't strictly necessary, but makes it easier to distinguish `body_span` from the specific `span` being processed.

Zalathar force-pushed the empty-spans branch from 583e460 to e25a65c Compare November 8, 2024 09:38

Zalathar added 3 commits November 8, 2024 20:43

coverage: Add GlobalFileId for stricter type-checking of file IDs

3f9c54c

We already had a dedicated `LocalFileId` index type, but previously we used a raw `u32` for global file IDs, because index types were harder to pass through FFI.

coverage: Remove unhelpful code for handling multiple files per function

996bdab

Functions currently can't have mappings in multiple files, and if that ever changes (e.g. to properly support expansion regions), this code will need to be completely overhauled anyway.

coverage: Restrict empty-span expansion to only cover { and }

3c30fe3

Zalathar force-pushed the empty-spans branch from e25a65c to 3c30fe3 Compare November 8, 2024 09:43

jieyouxu approved these changes Nov 9, 2024

View reviewed changes

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

compiler/rustc_codegen_llvm/src/coverageinfo/ffi.rs Show resolved Hide resolved

rustbot 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 9, 2024

coverage: Pass a LocalFileId to CoverageSpan::from_source_region

925dfc8

Zalathar force-pushed the empty-spans branch from 9287332 to 925dfc8 Compare November 10, 2024 00:58

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

bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 10, 2024

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 10, 2024

matthiaskrgr mentioned this pull request Nov 10, 2024

Rollup of 3 pull requests #132863

Merged

bors merged commit b95232d into rust-lang:master Nov 10, 2024

rustbot added this to the 1.84.0 milestone Nov 10, 2024

Zalathar deleted the empty-spans branch November 10, 2024 22:13

qinheping mentioned this pull request Nov 12, 2024

Upgrade toolchain to nightly-2024-11-11 model-checking/kani#3710

Merged

weihanglo mentioned this pull request May 7, 2025

[beta-1.87] Update cargo #140727

Merged

Uh oh!

Conversation

Zalathar commented Nov 6, 2024

Uh oh!

rustbot commented Nov 6, 2024

Uh oh!

rustbot commented Nov 6, 2024

Uh oh!

fee1-dead commented Nov 6, 2024

Uh oh!

jieyouxu commented Nov 7, 2024

Uh oh!

Uh oh!

jieyouxu left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jieyouxu commented Nov 9, 2024

Uh oh!

Zalathar commented Nov 10, 2024

Uh oh!

jieyouxu commented Nov 10, 2024

Uh oh!

bors commented Nov 10, 2024

Uh oh!

jieyouxu commented Nov 10, 2024

Uh oh!

jieyouxu commented Nov 10, 2024

Uh oh!

bors commented Nov 10, 2024

Uh oh!

jieyouxu commented Nov 10, 2024

Uh oh!

bors commented Nov 10, 2024

Uh oh!

jieyouxu commented Nov 10, 2024

Uh oh!

jieyouxu commented Nov 10, 2024

Uh oh!

bors commented Nov 10, 2024

Uh oh!

bors commented Nov 10, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

jieyouxu left a comment •

edited

Loading