coverage: Never emit improperly-ordered coverage regions #119460

Zalathar · 2023-12-31T02:09:17Z

If we emit a coverage region that is improperly ordered (end < start), llvm-cov will fail with coveragemap_error::malformed, which is inconvenient for users and also very hard to debug.

Ideally we would fix the root causes of these situations, but they tend to occur in very obscure edge-case scenarios (often involving nested macros), and we don't always have a good MCVE to work from. So it makes sense to also have a catch-all check that will prevent improperly-ordered regions from ever being emitted.

This is mainly aimed at resolving #119453. We don't have a specific way to reproduce it, which is why I haven't been able to add a test case in this PR. But based on the information provided in that issue, this change seems likely to avoid the error in llvm-cov.

@rustbot label +A-code-coverage

rustbot · 2023-12-31T02:09:25Z

r? @wesleywiser

(rustbot has picked a reviewer for you, use r? to override)

rustbot · 2023-12-31T02:09:27Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Zalathar · 2023-12-31T05:05:21Z

I thought of some other edge-case conditions that might cause llvm-cov to see improper regions, despite our check, so I've added checks for those as well.

Each check contains a debug assertion, and also a graceful return for non-debug builds. My thinking here is that each property violation is a bug in some part of the coverage instrumentor, but because they're hard to rule out, we don't want to subject users to an ICE in release builds if their code (or a dependency) does trigger a violation.

Zalathar · 2024-01-05T02:31:49Z

I was unhappy with how the original patches added a lot of ad-hoc logic to make_code_region, so I moved it all out into a separate check_code_region function that gets called right at the end (diff).

For now I've also changed my mind and removed the debug_assert!, though we still log a debug message when dropping a bad region.

Zalathar · 2024-01-06T01:58:14Z

Harmonized the fallible make_code_region patch with #119033.

bors · 2024-01-09T03:06:11Z

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

compiler/rustc_mir_transform/src/coverage/mod.rs

…-cov`

Zalathar · 2024-01-10T00:04:21Z

Rebasing after #119033 made this simpler, since the other patches are no longer needed.

I also decided to put back in the debug assert after all, in the form of if cfg!(debug_assertions) { bug!(...) }.

compiler/rustc_mir_transform/src/coverage/mod.rs

wesleywiser · 2024-01-22T16:41:55Z

Thanks @Zalathar! I think ICE'ing in debug builds makes a lot of sense and will hopefully let us track down some more of those long standing code coverage issues.

@bors r+

bors · 2024-01-22T16:41:58Z

📌 Commit 21e5bea has been approved by wesleywiser

It is now in the queue for this repository.

…wiser coverage: Never emit improperly-ordered coverage regions If we emit a coverage region that is improperly ordered (end < start), `llvm-cov` will fail with `coveragemap_error::malformed`, which is inconvenient for users and also very hard to debug. Ideally we would fix the root causes of these situations, but they tend to occur in very obscure edge-case scenarios (often involving nested macros), and we don't always have a good MCVE to work from. So it makes sense to also have a catch-all check that will prevent improperly-ordered regions from ever being emitted. --- This is mainly aimed at resolving rust-lang#119453. We don't have a specific way to reproduce it, which is why I haven't been able to add a test case in this PR. But based on the information provided in that issue, this change seems likely to avoid the error in `llvm-cov`. `@rustbot` label +A-code-coverage

…iaskrgr Rollup of 9 pull requests Successful merges: - rust-lang#119433 (rc,sync: Do not create references to uninitialized values) - rust-lang#119460 (coverage: Never emit improperly-ordered coverage regions) - rust-lang#119766 (Split tait and impl trait in assoc items logic) - rust-lang#120160 (Manually implement derived `NonZero` traits.) - rust-lang#120177 (Remove duplicate dependencies for rustc) - rust-lang#120183 (Add `#[coverage(off)]` to closures introduced by `#[test]` and `#[bench]`) - rust-lang#120185 (coverage: Don't instrument `#[automatically_derived]` functions) - rust-lang#120201 (Bump some deps with syn 1.0 dependencies) - rust-lang#120246 (Re-add estebank to review rotation) r? `@ghost` `@rustbot` modify labels: rollup

…wiser coverage: Never emit improperly-ordered coverage regions If we emit a coverage region that is improperly ordered (end < start), `llvm-cov` will fail with `coveragemap_error::malformed`, which is inconvenient for users and also very hard to debug. Ideally we would fix the root causes of these situations, but they tend to occur in very obscure edge-case scenarios (often involving nested macros), and we don't always have a good MCVE to work from. So it makes sense to also have a catch-all check that will prevent improperly-ordered regions from ever being emitted. --- This is mainly aimed at resolving rust-lang#119453. We don't have a specific way to reproduce it, which is why I haven't been able to add a test case in this PR. But based on the information provided in that issue, this change seems likely to avoid the error in `llvm-cov`. ``@rustbot`` label +A-code-coverage

Rollup of 12 pull requests Successful merges: - rust-lang#112806 (Small code improvements in `collect_intra_doc_links.rs`) - rust-lang#119460 (coverage: Never emit improperly-ordered coverage regions) - rust-lang#119766 (Split tait and impl trait in assoc items logic) - rust-lang#120062 (llvm: change data layout bug to an error and make it trigger more) - rust-lang#120099 (linker: Refactor library linking methods in `trait Linker`) - rust-lang#120139 (Do not normalize closure signature when building `FnOnce` shim) - rust-lang#120160 (Manually implement derived `NonZero` traits.) - rust-lang#120171 (Fix assume and assert in jump threading) - rust-lang#120183 (Add `#[coverage(off)]` to closures introduced by `#[test]` and `#[bench]`) - rust-lang#120195 (add several resolution test cases) - rust-lang#120259 (Split Diagnostics for Uncommon Codepoints: Add List to Display Characters Involved) - rust-lang#120261 (Provide structured suggestion to use trait objects in some cases of `if` arm type divergence) r? `@ghost` `@rustbot` modify labels: rollup

…wiser coverage: Never emit improperly-ordered coverage regions If we emit a coverage region that is improperly ordered (end < start), `llvm-cov` will fail with `coveragemap_error::malformed`, which is inconvenient for users and also very hard to debug. Ideally we would fix the root causes of these situations, but they tend to occur in very obscure edge-case scenarios (often involving nested macros), and we don't always have a good MCVE to work from. So it makes sense to also have a catch-all check that will prevent improperly-ordered regions from ever being emitted. --- This is mainly aimed at resolving rust-lang#119453. We don't have a specific way to reproduce it, which is why I haven't been able to add a test case in this PR. But based on the information provided in that issue, this change seems likely to avoid the error in `llvm-cov`. ``@rustbot`` label +A-code-coverage

Rollup of 7 pull requests Successful merges: - rust-lang#119460 (coverage: Never emit improperly-ordered coverage regions) - rust-lang#120062 (llvm: change data layout bug to an error and make it trigger more) - rust-lang#120124 (Split assembly tests for ELF and MachO) - rust-lang#120185 (coverage: Don't instrument `#[automatically_derived]` functions) - rust-lang#120265 (Remove no-system-llvm) - rust-lang#120277 (Normalize field types before checking validity) - rust-lang#120285 (Remove extra # from url in suggestion) r? `@ghost` `@rustbot` modify labels: rollup

…wiser coverage: Never emit improperly-ordered coverage regions If we emit a coverage region that is improperly ordered (end < start), `llvm-cov` will fail with `coveragemap_error::malformed`, which is inconvenient for users and also very hard to debug. Ideally we would fix the root causes of these situations, but they tend to occur in very obscure edge-case scenarios (often involving nested macros), and we don't always have a good MCVE to work from. So it makes sense to also have a catch-all check that will prevent improperly-ordered regions from ever being emitted. --- This is mainly aimed at resolving rust-lang#119453. We don't have a specific way to reproduce it, which is why I haven't been able to add a test case in this PR. But based on the information provided in that issue, this change seems likely to avoid the error in `llvm-cov`. ```@rustbot``` label +A-code-coverage

Rollup of 7 pull requests Successful merges: - rust-lang#119460 (coverage: Never emit improperly-ordered coverage regions) - rust-lang#120062 (llvm: change data layout bug to an error and make it trigger more) - rust-lang#120124 (Split assembly tests for ELF and MachO) - rust-lang#120165 (Switch `NonZero` alias direction.) - rust-lang#120185 (coverage: Don't instrument `#[automatically_derived]` functions) - rust-lang#120265 (Remove no-system-llvm) - rust-lang#120285 (Remove extra # from url in suggestion) r? `@ghost` `@rustbot` modify labels: rollup

…wiser coverage: Never emit improperly-ordered coverage regions If we emit a coverage region that is improperly ordered (end < start), `llvm-cov` will fail with `coveragemap_error::malformed`, which is inconvenient for users and also very hard to debug. Ideally we would fix the root causes of these situations, but they tend to occur in very obscure edge-case scenarios (often involving nested macros), and we don't always have a good MCVE to work from. So it makes sense to also have a catch-all check that will prevent improperly-ordered regions from ever being emitted. --- This is mainly aimed at resolving rust-lang#119453. We don't have a specific way to reproduce it, which is why I haven't been able to add a test case in this PR. But based on the information provided in that issue, this change seems likely to avoid the error in `llvm-cov`. ````@rustbot```` label +A-code-coverage

Rollup of 10 pull requests Successful merges: - rust-lang#114764 ([style edition 2024] Combine all delimited exprs as last argument) - rust-lang#118326 (Add `NonZero*::count_ones`) - rust-lang#118636 (Add the unstable option to reduce the binary size of dynamic library…) - rust-lang#119460 (coverage: Never emit improperly-ordered coverage regions) - rust-lang#119616 (Add a new `wasm32-wasi-preview2` target) - rust-lang#120185 (coverage: Don't instrument `#[automatically_derived]` functions) - rust-lang#120265 (Remove no-system-llvm) - rust-lang#120284 (privacy: Refactor top-level visiting in `TypePrivacyVisitor`) - rust-lang#120285 (Remove extra # from url in suggestion) - rust-lang#120299 (Add mw to review rotation and add some owner assignments) Failed merges: - rust-lang#120124 (Split assembly tests for ELF and MachO) r? `@ghost` `@rustbot` modify labels: rollup

Rollup of 9 pull requests Successful merges: - rust-lang#114764 ([style edition 2024] Combine all delimited exprs as last argument) - rust-lang#118326 (Add `NonZero*::count_ones`) - rust-lang#119460 (coverage: Never emit improperly-ordered coverage regions) - rust-lang#119616 (Add a new `wasm32-wasi-preview2` target) - rust-lang#120185 (coverage: Don't instrument `#[automatically_derived]` functions) - rust-lang#120265 (Remove no-system-llvm) - rust-lang#120284 (privacy: Refactor top-level visiting in `TypePrivacyVisitor`) - rust-lang#120285 (Remove extra # from url in suggestion) - rust-lang#120299 (Add mw to review rotation and add some owner assignments) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#119460 - Zalathar:improper-region, r=wesleywiser coverage: Never emit improperly-ordered coverage regions If we emit a coverage region that is improperly ordered (end < start), `llvm-cov` will fail with `coveragemap_error::malformed`, which is inconvenient for users and also very hard to debug. Ideally we would fix the root causes of these situations, but they tend to occur in very obscure edge-case scenarios (often involving nested macros), and we don't always have a good MCVE to work from. So it makes sense to also have a catch-all check that will prevent improperly-ordered regions from ever being emitted. --- This is mainly aimed at resolving rust-lang#119453. We don't have a specific way to reproduce it, which is why I haven't been able to add a test case in this PR. But based on the information provided in that issue, this change seems likely to avoid the error in `llvm-cov`. `````@rustbot````` label +A-code-coverage

rustbot assigned wesleywiser Dec 31, 2023

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 Dec 31, 2023

rustbot added the A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) label Dec 31, 2023

Zalathar mentioned this pull request Dec 31, 2023

Malformed coverage data when using llvm-cov #119453

Open

Zalathar force-pushed the improper-region branch from 0fff985 to ccd6f89 Compare December 31, 2023 04:52

Swatinem approved these changes Dec 31, 2023

View reviewed changes

Zalathar force-pushed the improper-region branch 2 times, most recently from d23a97f to ff98c90 Compare January 5, 2024 02:29

Zalathar force-pushed the improper-region branch 2 times, most recently from c5a016a to bff18bd Compare January 6, 2024 01:57

wesleywiser approved these changes Jan 9, 2024

View reviewed changes

compiler/rustc_mir_transform/src/coverage/mod.rs Show resolved Hide resolved

Zalathar force-pushed the improper-region branch from bff18bd to 678183e Compare January 9, 2024 23:57

coverage: Discard code regions that might cause fatal errors in `llvm…

b152de2

…-cov`

Zalathar force-pushed the improper-region branch from 678183e to b152de2 Compare January 10, 2024 00:02

wesleywiser approved these changes Jan 22, 2024

View reviewed changes

compiler/rustc_mir_transform/src/coverage/mod.rs Outdated Show resolved Hide resolved

Use debug_assert instead of expanded equivalent

21e5bea

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-review Status: Awaiting review from the assignee but also interested parties. labels Jan 22, 2024

matthiaskrgr mentioned this pull request Jan 22, 2024

Rollup of 9 pull requests #120249

Closed

fmease mentioned this pull request Jan 23, 2024

Rollup of 12 pull requests #120279

Closed

fmease mentioned this pull request Jan 24, 2024

Rollup of 7 pull requests #120289

Closed

fmease mentioned this pull request Jan 24, 2024

Rollup of 7 pull requests #120290

Closed

fmease mentioned this pull request Jan 24, 2024

Rollup of 10 pull requests #120303

Closed

fmease mentioned this pull request Jan 24, 2024

Rollup of 9 pull requests #120309

Merged

bors merged commit 5a38754 into rust-lang:master Jan 24, 2024
11 checks passed

rustbot added this to the 1.77.0 milestone Jan 24, 2024

Zalathar deleted the improper-region branch January 24, 2024 23:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

coverage: Never emit improperly-ordered coverage regions #119460

coverage: Never emit improperly-ordered coverage regions #119460

Zalathar commented Dec 31, 2023

rustbot commented Dec 31, 2023

rustbot commented Dec 31, 2023

Zalathar commented Dec 31, 2023 •

edited

Zalathar commented Jan 5, 2024

Zalathar commented Jan 6, 2024

bors commented Jan 9, 2024

Zalathar commented Jan 10, 2024

wesleywiser commented Jan 22, 2024

bors commented Jan 22, 2024

coverage: Never emit improperly-ordered coverage regions #119460

coverage: Never emit improperly-ordered coverage regions #119460

Conversation

Zalathar commented Dec 31, 2023

rustbot commented Dec 31, 2023

rustbot commented Dec 31, 2023

Zalathar commented Dec 31, 2023 • edited

Zalathar commented Jan 5, 2024

Zalathar commented Jan 6, 2024

bors commented Jan 9, 2024

Zalathar commented Jan 10, 2024

wesleywiser commented Jan 22, 2024

bors commented Jan 22, 2024

Zalathar commented Dec 31, 2023 •

edited