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

coverage: Clean up creation of MC/DC condition bitmaps #124555

Merged
merged 3 commits into from
May 3, 2024

Conversation

Zalathar
Copy link
Contributor

@Zalathar Zalathar commented Apr 30, 2024

This PR improves the code for creating and initializing MC/DC condition bitmap variables, as introduced by #123409 and modified by #124255.

  • The condition bitmap variables are now created eagerly at the start of per-function codegen, via a new init_coverage method in CoverageInfoBuilderMethods. This avoids having to retroactively create the bitmaps while doing codegen for an individual coverage statement.
  • As a result, we can now create and initialize those bitmaps using existing safe APIs, instead of having to perform our own unsafe call to llvm::LLVMBuildAlloca.
  • This PR also tweaks the way we count the number of condition bitmaps needed, by tracking the total number of bitmaps needed (max depth + 1), instead of only tracking the maximum depth. This reduces the potential for subtle off-by-one confusion.

@rustbot
Copy link
Collaborator

rustbot commented Apr 30, 2024

r? @nnethercote

rustbot has assigned @nnethercote.
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 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 Apr 30, 2024
@rustbot
Copy link
Collaborator

rustbot commented Apr 30, 2024

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

@rustbot rustbot added the A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) label Apr 30, 2024
@RenjiSann
Copy link
Contributor

LGTM 👍

@erikdesjardins
Copy link
Contributor

I've changed the memory alignment of the u32 accesses to a hardcoded 4, instead of tcx.data_layout.i32_align.abi, because I believe that value is related to parameter-passing, not local variables.

That value is used for both local variable and parameter alignment.

However, this coverage code doesn't actually generate local variables (in the Rust sense of let x = ...), it's just a codegen implementation detail that isn't exposed to Rust code, right? So it doesn't have to obey that alignment, it can use any arbitrary alignment, as long as it's internally consistent.

I see some stores changed in this PR to match the hardcoded align 4, but I don't see any loads--where are these allocas loaded? If they're loaded by LLVM coverage intrinsics, you need to make sure that align 4 satisfies the alignment requirements of those intrinsics.

@Zalathar
Copy link
Contributor Author

I see some stores changed in this PR to match the hardcoded align 4, but I don't see any loads--where are these allocas loaded? If they're loaded by LLVM coverage intrinsics, you need to make sure that align 4 satisfies the alignment requirements of those intrinsics.

Yeah, I'll need to look inside InstrProfiling.cpp and make sure our alignment matches how that code is interacting with the variable.

It looks like they're using Type::getInt32Ty, so I probably need to specify whatever alignment LLVM thinks that type has.

@Zalathar
Copy link
Contributor Author

OK, it looks like the intrinsics are producing stores that don't specify an alignment, which means it defaults to the ABI alignment of the type.

So I should revert my alignment changes, and keep using the ABI alignment for i32, as before.

Thanks for the correction.

@Zalathar
Copy link
Contributor Author

Zalathar commented Apr 30, 2024

Back to using tcx.data_layout.i32_align.abi for alignment (diff).

I don't need to add an Align::FOUR constant any more, but I've kept the tests for the other alignment constants, because they're perfectly fine tests.

Because this now always takes place at the start of the function, we can just
use the normal `alloca` method and then initialize each bitmap immediately.

This patch also moves bitmap setup out of the `mcdc_parameters` method, because
there is no longer any particular reason for it to be there.
This clearly distinguishes individual decision-depth indices from the total
number of condition bitmaps to allocate.
@nnethercote
Copy link
Contributor

nnethercote commented May 2, 2024

I don't know anything about this code, but

  • You have a good PR description
  • You have written nice clear comments
  • @erikdesjardins obviously gave it a close look
  • You have added a test
  • The changes aren't that complicated

So, seems ok to me. My only suggestion is to update the PR description to remove the bit about the memory alignment. Because the PR description is used as the merge commit, so it's best for that to not contain out-of-date information. With that change made, r=me.

@bors delegate=Zalathar

@bors
Copy link
Contributor

bors commented May 2, 2024

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

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

@Zalathar
Copy link
Contributor Author

Zalathar commented May 3, 2024

Good catch; I've removed the old information from the PR description.

@bors r=@nnethercote

@bors
Copy link
Contributor

bors commented May 3, 2024

📌 Commit de972b7 has been approved by nnethercote

It is now in the queue for this repository.

@bors 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 3, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request May 3, 2024
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#122492 (Implement ptr_as_ref_unchecked)
 - rust-lang#123815 (Fix cannot usage in time.rs)
 - rust-lang#124059 (default_alloc_error_hook: explain difference to default __rdl_oom in alloc)
 - rust-lang#124510 (Add raw identifier in a typo suggestion)
 - rust-lang#124555 (coverage: Clean up creation of MC/DC condition bitmaps)
 - rust-lang#124593 (Describe and use CStr literals in CStr and CString docs)
 - rust-lang#124630 (CI: remove `env-x86_64-apple-tests` YAML anchor)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 613bccc into rust-lang:master May 3, 2024
6 checks passed
@rustbot rustbot added this to the 1.80.0 milestone May 3, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request May 3, 2024
Rollup merge of rust-lang#124555 - Zalathar:init-coverage, r=nnethercote

coverage: Clean up creation of MC/DC condition bitmaps

This PR improves the code for creating and initializing [MC/DC](https://en.wikipedia.org/wiki/Modified_condition/decision_coverage) condition bitmap variables, as introduced by rust-lang#123409 and modified by rust-lang#124255.

- The condition bitmap variables are now created eagerly at the start of per-function codegen, via a new `init_coverage` method in `CoverageInfoBuilderMethods`. This avoids having to retroactively create the bitmaps while doing codegen for an individual coverage statement.
- As a result, we can now create and initialize those bitmaps using existing safe APIs, instead of having to perform our own unsafe call to `llvm::LLVMBuildAlloca`.
- This PR also tweaks the way we count the number of condition bitmaps needed, by tracking the total number of bitmaps needed (max depth + 1), instead of only tracking the maximum depth. This reduces the potential for subtle off-by-one confusion.
@Zalathar Zalathar deleted the init-coverage branch May 3, 2024 22:24
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) 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