coverage: Clean up `mcdc_bitmap_bytes` and `conditions_num` #124571

Zalathar · 2024-05-01T01:19:02Z

This is a combination of two mostly-separate MC/DC coverage cleanups that would conflict with each other, plus some extra tests that appeared along the way.

The first change is to stop recomputing mcdc_bitmap_bytes in the query that produces CoverageIdsInfo. This appears to have been inspired by how we were already computing max_counter_id, but there's an important difference between the two cases.

When computing max_counter_id, the highest counter ID seen might be less than the highest ID used during MIR instrumentation, because some counter-increment statements might have been removed by MIR optimizations.

But for the recomputation used for mcdc_bitmap_bytes, that's impossible, because both computations are based on pre-optimization info. So there's no need to recompute the exact same value, when it can't have changed

The second change is to rename conditions_num to num_conditions, to make it clearer that this refers to a number of conditions, not some kind of ID number.

Because this change touched the compiler warning for a decision containing too many conditions, I also noticed that we didn't have any tests for that warning.

(It now seems a bit strange to me that this is a compiler warning, not a lint, because it can't be silenced or denied by the usual mechanisms for controlling lints. But I consider that change to be beyond the scope of this PR.)

rustbot · 2024-05-01T01:19:09Z

r? @compiler-errors

rustbot has assigned @compiler-errors.
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-05-01T01:19:11Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Zalathar · 2024-05-01T01:34:07Z

It might be possible to adjust the calculation of MC/DC bitmap bytes so that it takes into account which MC/DC coverage statements have survived MIR optimizations, to potentially use fewer bytes.

But in the current state of the MC/DC implementation, I don't think the extra complexity is worth it.

(The main reason we do this sort of thing for the existing coverage statements is that things already worked that way before I started modifying it, and I didn't want to regress that particular property of the instrumented output.)

coverage: Split out MC/DC mappings from `BcbMappingKind` These variants were added to `BcbMappingKind` as part of the [MC/DC coverage](https://en.wikipedia.org/wiki/Modified_Condition/Decision_Coverage) implementation in rust-lang#123409, because that was the path-of-least-resistance for integrating them into the existing code. However, they ultimately represent complex concepts that the enum was not intended to handle, leading to more complexity in the code that processes them. This PR therefore follows in the footsteps of rust-lang#124545, and splits the MC/DC mappings out into their own dedicated vectors of structs. After that, `BcbMappingKind` itself ends up having only one variant (`Code`), so this PR also flattens that enum into its enclosing struct, renamed to `mapping::CodeMapping`. --- No functional changes. This will conflict slightly with rust-lang#124571, but hopefully that should be easy to resolve either way. `@rustbot` label +A-code-coverage

bors · 2024-05-05T22:18:57Z

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

This code for recalculating `mcdc_bitmap_bytes` doesn't provide any benefit, because its result won't have changed from the value in `FunctionCoverageInfo` that was computed during the MIR instrumentation pass.

These comments appear to be inspired by the similar comments on `CounterIncrement` and `ExpressionUsed`. But those comments refer to specific simplification steps performed during coverage codegen, and there is no corresponding step for the MC/DC coverage statements.

This field counts the number of conditions that contribute to a particular decision, but the name "conditions num" sounds like an ID instead of a count, so "num conditions" is clearer.

compiler-errors · 2024-05-07T16:44:21Z

@bors r+

bors · 2024-05-07T16:44:24Z

📌 Commit 1a701d4 has been approved by compiler-errors

It is now in the queue for this repository.

…r-errors coverage: Clean up `mcdc_bitmap_bytes` and `conditions_num` This is a combination of two mostly-separate MC/DC coverage cleanups that would conflict with each other, plus some extra tests that appeared along the way. The first change is to stop recomputing `mcdc_bitmap_bytes` in the query that produces `CoverageIdsInfo`. This appears to have been inspired by how we were already computing `max_counter_id`, but there's an important difference between the two cases. When computing `max_counter_id`, the highest counter ID seen might be less than the highest ID used during MIR instrumentation, because some counter-increment statements might have been removed by MIR optimizations. But for the recomputation used for `mcdc_bitmap_bytes`, that's impossible, because both computations are based on pre-optimization info. So there's no need to recompute the exact same value, when it can't have changed --- The second change is to rename `conditions_num` to `num_conditions`, to make it clearer that this refers to a *number of conditions*, not some kind of ID number. Because this change touched the compiler warning for a decision containing too many conditions, I also noticed that we didn't have any tests for that warning. (It now seems a bit strange to me that this is a compiler warning, not a lint, because it can't be silenced or denied by the usual mechanisms for controlling lints. But I consider that change to be beyond the scope of this PR.)

…iaskrgr Rollup of 3 pull requests Successful merges: - rust-lang#124223 (coverage: Branch coverage support for let-else and if-let) - rust-lang#124571 (coverage: Clean up `mcdc_bitmap_bytes` and `conditions_num`) - rust-lang#124838 (next_power_of_two: add a doctest to show what happens on 0) r? `@ghost` `@rustbot` modify labels: rollup

…errors coverage: Clean up `mcdc_bitmap_bytes` and `conditions_num` This is a combination of two mostly-separate MC/DC coverage cleanups that would conflict with each other, plus some extra tests that appeared along the way. The first change is to stop recomputing `mcdc_bitmap_bytes` in the query that produces `CoverageIdsInfo`. This appears to have been inspired by how we were already computing `max_counter_id`, but there's an important difference between the two cases. When computing `max_counter_id`, the highest counter ID seen might be less than the highest ID used during MIR instrumentation, because some counter-increment statements might have been removed by MIR optimizations. But for the recomputation used for `mcdc_bitmap_bytes`, that's impossible, because both computations are based on pre-optimization info. So there's no need to recompute the exact same value, when it can't have changed --- The second change is to rename `conditions_num` to `num_conditions`, to make it clearer that this refers to a *number of conditions*, not some kind of ID number. Because this change touched the compiler warning for a decision containing too many conditions, I also noticed that we didn't have any tests for that warning. (It now seems a bit strange to me that this is a compiler warning, not a lint, because it can't be silenced or denied by the usual mechanisms for controlling lints. But I consider that change to be beyond the scope of this PR.)

bors · 2024-05-08T00:35:59Z

⌛ Testing commit 1a701d4 with merge 337db83...

rust-log-analyzer · 2024-05-08T00:52:53Z

The job x86_64-gnu-llvm-18 failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

failures:

---- [ui] tests/ui/instrument-coverage/mcdc-condition-limit.rs#bad stdout ----

error in revision `bad`: test compilation failed although it shouldn't!
status: exit status: 101
command: env -u RUSTC_LOG_COLOR RUSTC_ICE="0" RUST_BACKTRACE="short" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage1/bin/rustc" "/checkout/tests/ui/instrument-coverage/mcdc-condition-limit.rs" "-Zthreads=1" "-Zsimulate-remapped-rust-src-base=/rustc/FAKE_PREFIX" "-Ztranslate-remapped-path-to-local-path=no" "-Z" "ignore-directory-in-diagnostics-source-blocks=/cargo" "-Z" "ignore-directory-in-diagnostics-source-blocks=/checkout/vendor" "--sysroot" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage1" "--target=x86_64-unknown-linux-gnu" "--cfg" "bad" "--check-cfg" "cfg(FALSE,good,bad)" "--error-format" "json" "--json" "future-incompat" "-Ccodegen-units=1" "-Zui-testing" "-Zdeduplicate-diagnostics=no" "-Zwrite-long-types-to-disk=no" "-Cstrip=debuginfo" "--emit" "metadata" "-C" "prefer-dynamic" "--out-dir" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/instrument-coverage/mcdc-condition-limit.bad" "-A" "unused" "-A" "internal_features" "-Crpath" "-Cdebuginfo=0" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/instrument-coverage/mcdc-condition-limit.bad/auxiliary" "--edition=2021" "-Cinstrument-coverage" "-Zcoverage-options=mcdc" "-Zno-profiler-runtime"
--- stderr -------------------------------
thread 'rustc' panicked at compiler/rustc_mir_build/src/build/coverageinfo/mcdc.rs:219:38:
attempt to subtract with overflow
stack backtrace:
stack backtrace:
   0: rust_begin_unwind
   1: core::panicking::panic_fmt
   2: core::panicking::panic_const::panic_const_sub_overflow
   3: <rustc_mir_build::build::coverageinfo::mcdc::MCDCInfoBuilder>::visit_evaluated_condition::<<rustc_mir_build::build::Builder>::visit_coverage_branch_condition::{closure#0}>
   4: <rustc_mir_build::build::Builder>::then_else_break_inner
   5: <rustc_mir_build::build::Builder>::then_else_break_inner
   6: <rustc_mir_build::build::Builder>::then_else_break_inner
   7: <rustc_mir_build::build::Builder>::then_else_break_inner
   9: <rustc_mir_build::build::Builder>::in_scope::<<rustc_mir_build::build::Builder>::expr_into_dest::{closure#0}::{closure#0}, ()>
  10: <rustc_mir_build::build::Builder>::expr_into_dest::{closure#0}
  11: <rustc_mir_build::build::Builder>::expr_into_dest
  12: <rustc_mir_build::build::Builder>::ast_block_stmts
  12: <rustc_mir_build::build::Builder>::ast_block_stmts
  13: <rustc_mir_build::build::Builder>::in_scope::<<rustc_mir_build::build::Builder>::ast_block::{closure#0}, ()>
  14: <rustc_mir_build::build::Builder>::expr_into_dest
  15: <rustc_mir_build::build::Builder>::in_scope::<<rustc_mir_build::build::Builder>::expr_into_dest::{closure#0}::{closure#0}, ()>
  16: <rustc_mir_build::build::Builder>::expr_into_dest::{closure#0}
  17: <rustc_mir_build::build::Builder>::expr_into_dest
  18: rustc_mir_build::build::mir_build
  19: rustc_mir_transform::mir_built
      [... omitted 2 frames ...]
  20: rustc_mir_build::check_unsafety::check_unsafety
      [... omitted 2 frames ...]
  21: <rustc_middle::hir::map::Map>::par_body_owners::<rustc_interface::passes::run_required_analyses::{closure#1}::{closure#0}>::{closure#0}
  22: std::panicking::try::<(), core::panic::unwind_safe::AssertUnwindSafe<rustc_data_structures::sync::parallel::enabled::par_for_each_in<&rustc_span::def_id::LocalDefId, &[rustc_span::def_id::LocalDefId], <rustc_middle::hir::map::Map>::par_body_owners<rustc_interface::passes::run_required_analyses::{closure#1}::{closure#0}>::{closure#0}>::{closure#0}::{closure#0}::{closure#0}>>
  23: <rustc_data_structures::sync::parallel::ParallelGuard>::run::<(), rustc_data_structures::sync::parallel::enabled::par_for_each_in<&rustc_span::def_id::LocalDefId, &[rustc_span::def_id::LocalDefId], <rustc_middle::hir::map::Map>::par_body_owners<rustc_interface::passes::run_required_analyses::{closure#1}::{closure#0}>::{closure#0}>::{closure#0}::{closure#1}::{closure#0}>
  24: <rustc_session::session::Session>::time::<(), rustc_interface::passes::run_required_analyses::{closure#1}>
  25: rustc_interface::passes::analysis
      [... omitted 2 frames ...]
  27: <rustc_interface::interface::Compiler>::enter::<rustc_driver_impl::run_compiler::{closure#0}::{closure#1}, core::result::Result<core::option::Option<rustc_interface::queries::Linker>, rustc_span::ErrorGuaranteed>>
  28: <scoped_tls::ScopedKey<rustc_span::SessionGlobals>>::set::<rustc_interface::util::run_in_thread_with_globals<rustc_interface::util::run_in_thread_pool_with_globals<rustc_interface::interface::run_compiler<core::result::Result<(), rustc_span::ErrorGuaranteed>, rustc_driver_impl::run_compiler::{closure#0}>::{closure#1}, core::result::Result<(), rustc_span::ErrorGuaranteed>>::{closure#0}, core::result::Result<(), rustc_span::ErrorGuaranteed>>::{closure#0}::{closure#0}::{closure#0}, core::result::Result<(), rustc_span::ErrorGuaranteed>>
  29: rustc_span::create_session_globals_then::<core::result::Result<(), rustc_span::ErrorGuaranteed>, rustc_interface::util::run_in_thread_with_globals<rustc_interface::util::run_in_thread_pool_with_globals<rustc_interface::interface::run_compiler<core::result::Result<(), rustc_span::ErrorGuaranteed>, rustc_driver_impl::run_compiler::{closure#0}>::{closure#1}, core::result::Result<(), rustc_span::ErrorGuaranteed>>::{closure#0}, core::result::Result<(), rustc_span::ErrorGuaranteed>>::{closure#0}::{closure#0}::{closure#0}>
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
---
note: please make sure that you have updated to the latest nightly

note: rustc 1.80.0-nightly (337db83d1 2024-05-08) running on x86_64-unknown-linux-gnu

note: compiler flags: -Z threads=1 -Z simulate-remapped-rust-src-base=/rustc/FAKE_PREFIX -Z translate-remapped-path-to-local-path=no -Z ignore-directory-in-diagnostics-source-blocks=/cargo -Z ignore-directory-in-diagnostics-source-blocks=/checkout/vendor -C codegen-units=1 -Z ui-testing -Z deduplicate-diagnostics=no -Z write-long-types-to-disk=no -C strip=debuginfo -C prefer-dynamic -C rpath -C debuginfo=0 -C instrument-coverage -Z coverage-options=mcdc -Z no-profiler-runtime
query stack during panic:
query stack during panic:
#0 [mir_built] building MIR for `main`
#1 [check_unsafety] unsafety-checking `main`
end of query stack
------------------------------------------

bors · 2024-05-08T00:53:31Z

💔 Test failed - checks-actions

Zalathar · 2024-05-08T02:32:07Z

Looks like the overflow bug already existed before this PR; it just now gets exercised by the newly-added test (but only in compiler builds with overflow checks).

The overflow itself is pretty benign in known cases, since we overflow by -1 and then immediately add 1 again to get back to 0. But it also prevents us from detecting more serious overflow bugs.

bors · 2024-05-10T19:01:05Z

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

RenjiSann · 2024-05-13T09:34:27Z

Looks like the overflow bug already existed before this PR; it just now gets exercised by the newly-added test (but only in compiler builds with overflow checks).

The overflow itself is pretty benign in known cases, since we overflow by -1 and then immediately add 1 again to get back to 0. But it also prevents us from detecting more serious overflow bugs.

Maybe rewrite the operation + 1 - num_conditions to avoid the underflow. It looks weird, but it's probably the easiest and most readable fix, I guess ?

…hercote coverage: `CoverageIdsInfo::mcdc_bitmap_bytes` is never needed This code for recalculating `mcdc_bitmap_bytes` in a query doesn't provide any benefit, because its result won't have changed from the value in `FunctionCoverageInfo` that was computed during the MIR instrumentation pass. Extracted from rust-lang#124571, to avoid having this held up by unrelated issues with condition count checks. `@rustbot` label +A-code-coverage

Rollup merge of rust-lang#125108 - Zalathar:info-bitmap-bytes, r=nnethercote coverage: `CoverageIdsInfo::mcdc_bitmap_bytes` is never needed This code for recalculating `mcdc_bitmap_bytes` in a query doesn't provide any benefit, because its result won't have changed from the value in `FunctionCoverageInfo` that was computed during the MIR instrumentation pass. Extracted from rust-lang#124571, to avoid having this held up by unrelated issues with condition count checks. `@rustbot` label +A-code-coverage

compiler-errors · 2024-05-22T14:51:17Z

@rustbot author

…cote coverage: Avoid overflow when the MC/DC condition limit is exceeded Fix for the test failure seen in rust-lang#124571 (comment). If we perform this subtraction first, it can sometimes overflow to -1 before the addition can bring its value back to 0. That behaviour seems to be benign, but it nevertheless causes test failures in compiler configurations that check for overflow. `@rustbot` label +A-code-coverage

…cote coverage: Avoid overflow when the MC/DC condition limit is exceeded Fix for the test failure seen in rust-lang#124571 (comment). If we perform this subtraction first, it can sometimes overflow to -1 before the addition can bring its value back to 0. That behaviour seems to be benign, but it nevertheless causes test failures in compiler configurations that check for overflow. ``@rustbot`` label +A-code-coverage

Rollup merge of rust-lang#125700 - Zalathar:limit-overflow, r=nnethercote coverage: Avoid overflow when the MC/DC condition limit is exceeded Fix for the test failure seen in rust-lang#124571 (comment). If we perform this subtraction first, it can sometimes overflow to -1 before the addition can bring its value back to 0. That behaviour seems to be benign, but it nevertheless causes test failures in compiler configurations that check for overflow. ``@rustbot`` label +A-code-coverage

Zalathar · 2024-05-30T03:23:09Z

Closing this in favour of #125754.

coverage: Rename MC/DC `conditions_num` to `num_conditions` Updated version of rust-lang#124571, without the other changes that were split out into rust-lang#125108 and rust-lang#125700. This value represents a quantity of conditions, not an ID, so the new spelling is more appropriate. Some of the code touched by this PR could perhaps use some other changes, but I would prefer to keep this PR as a simple renaming and avoid scope creep. `@rustbot` label +A-code-coverage

Rollup merge of rust-lang#125754 - Zalathar:conditions-num, r=lqd coverage: Rename MC/DC `conditions_num` to `num_conditions` Updated version of rust-lang#124571, without the other changes that were split out into rust-lang#125108 and rust-lang#125700. This value represents a quantity of conditions, not an ID, so the new spelling is more appropriate. Some of the code touched by this PR could perhaps use some other changes, but I would prefer to keep this PR as a simple renaming and avoid scope creep. `@rustbot` label +A-code-coverage

rustbot assigned compiler-errors May 1, 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 May 1, 2024

rustbot added the A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) label May 1, 2024

Zalathar force-pushed the num-conditions branch from 4a9c55c to 135c739 Compare May 1, 2024 01:22

This comment has been minimized.

Sign in to view

Zalathar force-pushed the num-conditions branch 2 times, most recently from 15ca582 to 316f7d6 Compare May 2, 2024 01:13

Zalathar mentioned this pull request May 2, 2024

coverage: Split out MC/DC mappings from BcbMappingKind #124603

Merged

Zalathar added 4 commits May 6, 2024 11:59

coverage: CoverageIdsInfo::mcdc_bitmap_bytes is never needed

a9b2f1b

This code for recalculating `mcdc_bitmap_bytes` doesn't provide any benefit, because its result won't have changed from the value in `FunctionCoverageInfo` that was computed during the MIR instrumentation pass.

coverage: Add tests for the MC/DC condition limit

eb1b9e0

coverage: Rename conditions_num to num_conditions

1a701d4

This field counts the number of conditions that contribute to a particular decision, but the name "conditions num" sounds like an ID instead of a count, so "num conditions" is clearer.

Zalathar force-pushed the num-conditions branch from 316f7d6 to 1a701d4 Compare May 6, 2024 02:11

Zalathar mentioned this pull request May 6, 2024

coverage: Further simplify extraction of mapping info from MIR #124615

Merged

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

matthiaskrgr mentioned this pull request May 7, 2024

Rollup of 7 pull requests #124855

Closed

matthiaskrgr mentioned this pull request May 7, 2024

Rollup of 3 pull requests #124856

Closed

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 May 8, 2024

Zalathar mentioned this pull request May 14, 2024

coverage: CoverageIdsInfo::mcdc_bitmap_bytes is never needed #125108

Merged

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 May 22, 2024

Zalathar mentioned this pull request May 29, 2024

coverage: Avoid overflow when the MC/DC condition limit is exceeded #125700

Merged

Zalathar mentioned this pull request May 30, 2024

coverage: Rename MC/DC conditions_num to num_conditions #125754

Merged

Zalathar closed this May 30, 2024

Zalathar deleted the num-conditions branch May 30, 2024 03:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

coverage: Clean up `mcdc_bitmap_bytes` and `conditions_num` #124571

coverage: Clean up `mcdc_bitmap_bytes` and `conditions_num` #124571

Zalathar commented May 1, 2024 •

edited

Loading

rustbot commented May 1, 2024

rustbot commented May 1, 2024

Zalathar commented May 1, 2024

This comment has been minimized.

bors commented May 5, 2024

compiler-errors commented May 7, 2024

bors commented May 7, 2024

bors commented May 8, 2024

rust-log-analyzer commented May 8, 2024

bors commented May 8, 2024

Zalathar commented May 8, 2024

bors commented May 10, 2024

RenjiSann commented May 13, 2024

compiler-errors commented May 22, 2024

Zalathar commented May 30, 2024

coverage: Clean up mcdc_bitmap_bytes and conditions_num #124571

coverage: Clean up mcdc_bitmap_bytes and conditions_num #124571

Conversation

Zalathar commented May 1, 2024 • edited Loading

rustbot commented May 1, 2024

rustbot commented May 1, 2024

Zalathar commented May 1, 2024

This comment has been minimized.

bors commented May 5, 2024

compiler-errors commented May 7, 2024

bors commented May 7, 2024

bors commented May 8, 2024

rust-log-analyzer commented May 8, 2024

bors commented May 8, 2024

Zalathar commented May 8, 2024

bors commented May 10, 2024

RenjiSann commented May 13, 2024

compiler-errors commented May 22, 2024

Zalathar commented May 30, 2024

coverage: Clean up `mcdc_bitmap_bytes` and `conditions_num` #124571

coverage: Clean up `mcdc_bitmap_bytes` and `conditions_num` #124571

Zalathar commented May 1, 2024 •

edited

Loading