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][MCDC] Group mcdc tests and fix panic when generating mcdc code for inlined expressions. #127234

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

ZhuUx
Copy link
Contributor

@ZhuUx ZhuUx commented Jul 2, 2024

Changes

  1. Group all mcdc tests to one directory.
  2. Since mcdc instruments different mappings for boolean expressions with normal branch coverage as MCDC Coverage: instrument last boolean RHS operands from condition coverage #125766 introduces, it would be better also trace branch coverage results in mcdc tests.
  3. So far rustc does not call CoverageInfoBuilderMethods::init_coverage for inlined functions. As a result, it could panic if it tries to instrument mcdc statements for inlined functions due to uninitialized cond bitmaps. We can reproduce this issue by current nightly rustc and the test with flag --release. This patch fixes it.

@rustbot
Copy link
Collaborator

rustbot commented Jul 2, 2024

r? @davidtwco

rustbot has assigned @davidtwco.
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 Jul 2, 2024
@rustbot
Copy link
Collaborator

rustbot commented Jul 2, 2024

Some changes occurred in coverage instrumentation.

cc @Zalathar

Some changes occurred in coverage instrumentation.

cc @Zalathar

Some changes occurred in coverage tests.

cc @Zalathar

@ZhuUx
Copy link
Contributor Author

ZhuUx commented Jul 2, 2024

@rustbot label +A-code-coverage
cc @RenjiSann

@rustbot rustbot added the A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) label Jul 2, 2024
@ZhuUx ZhuUx force-pushed the inlined-expr branch 2 times, most recently from 7d1d52e to 49e8e2d Compare July 2, 2024 08:40
@Zalathar
Copy link
Contributor

Zalathar commented Jul 2, 2024

Unfortunately, I'm pretty sure this fix is wrong.

We can't just call init_coverage in the middle of visiting an inlined coverage statement, because (even with these changes) that method is written around the assumption that it will be called exactly once, at the start of the relevant function.

For example, what happens if a function is inlined in multiple places, or is both inlined and also codegenned as a non-inlined function? It looks like only some of those places will end up with the local variables that they need.


For now, I think the workaround should just be to completely disable MIR inlining when MC/DC is enabled.

@Zalathar
Copy link
Contributor

Zalathar commented Jul 2, 2024

By the way, if you want to extract the first two patches (adjusting tests) to their own PR, they should be fine.

(I can't directly approve them myself, but I can at least tell the assigned reviewer that they're OK.)

@Zalathar
Copy link
Contributor

Zalathar commented Jul 2, 2024

Something like this in compiler/rustc_mir_transform/src/inline.rs should work around the problem:

impl<'tcx> MirPass<'tcx> for Inline {
    fn is_enabled(&self, sess: &rustc_session::Session) -> bool {
        // FIXME(#127234): Coverage instrumentation currently doesn't handle inlined
        // MIR correctly when Modified Condition/Decision Coverage is enabled.
        if sess.instrument_coverage_mcdc() {
            return false;
        }

@ZhuUx
Copy link
Contributor Author

ZhuUx commented Jul 2, 2024

We can't just call init_coverage in the middle of visiting an inlined coverage statement, because (even with these changes) that method is written around the assumption that it will be called exactly once, at the start of the relevant function.

How about use (RootInstance, InlinedInstance) as the key to reference the local mcdc parameters?

@rustbot
Copy link
Collaborator

rustbot commented Jul 2, 2024

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

@ZhuUx
Copy link
Contributor Author

ZhuUx commented Jul 2, 2024

Updated. Considering there might be much overhead if we created mcdc parameters for each inlined functions separately, disable inlining when mcdc is enabled seems better for now.

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.

This looks good to me. r=me but would like to confirm that @Zalathar is happy with it, knowing coverage more than I do.

@Zalathar
Copy link
Contributor

Zalathar commented Jul 5, 2024

@davidtwco Yes, r+ from me also. 👍

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-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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants