add spans to injected coverage counters, extract with CoverageData query

[richkadel]

This is the next iteration on the Rust Coverage implementation, and follows PR #73488

@tmandry @wesleywiser

I came up with an approach for coverage spans, pushing them through the Call terminator as additional args so they can be extracted by the CoverageData query.

I'm using an IndexVec to store them in CoverageData such that there can be only one per index (even if parts of the MIR get duplicated during optimization).

If this approach works for you, I can quickly expand on this to build a separate IndexVec for counter expressions, using a separate call that will be ignored during code generation, but from which I can extract the counter expression values.

Let me know your thoughts. Thanks!

r? @tmandry

Rust compiler MCP rust-lang/compiler-team#278
Relevant issue: #34701 - Implement support for LLVMs code coverage instrumentation

[wesleywiser]

Sorry, this notification fell off my radar. I will schedule some time tonight and tomorrow to review this.

[richkadel]

no worries @wesleywiser ... it actually works out because I'm about to push an update to this PR today

[richkadel]

@wesleywiser @tmandry - I just uploaded an iteration on this PR that shows how I may pull out all of the injected counterinfo (renamed from CoverageData, to be more consistent with debuginfo) in order to generate the coverage map in LLVM IR.

BUT as I was looking at the debug output, I realized there's another way to do this that might be simpler and faster. Please chime in if you agree or disagree with this:

When generating the LLVM intrinsic calls for each counter (src/librustc_codegen_llvm/intrinsic.rs), I now have the caller_instance, counter index, and span right here, and I also have the CodegenCx (via self.cx(), as the Builder).

Instead of expanding the query to extract the spans from the MIR, I could call a mutable function on the Builder to insert the counter info into the CodegenCx (in a coverageinfo-specific field). Then, on coverageinfo::finalize(cx), I can generate the coverage map directly from the cx.

This has the added benefit of ensuring a direct correlation between counters injected and their inclusion in the coverage map.

It also means the query for coverageinfo can be reverted back to holding only the hash and num_counters again, and returned by copy, rather than by reference.

Thoughts?

[tmandry]

Instead of expanding the query to extract the spans from the MIR, I could call a mutable function on the Builder to insert the counter info into the CodegenCx (in a coverageinfo-specific field). Then, on coverageinfo::finalize(cx), I can generate the coverage map directly from the cx.

Makes sense to me. That would sidestep the issue of figuring out which functions we need to generate a coverage map for (which is the set of functions we codegen). The analogy to debuginfo gives it precedent as well.

[bors]

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

[wesleywiser]

Thoughts?

That seems reasonable to me as well so I would be interested to see a version of the code that uses that approach!

[richkadel]

That seems reasonable to me as well so I would be interested to see a version of the code that uses that approach!

Awesome! I just finished making the changes. I'm updating to the current baseline and will push a new patch momentarily.

[richkadel]

@wesleywiser - @tmandry is out of office for the next week. (He may have told you.) Unless you're doing the same, I hope you don't mind being the primary reviewer, and pushing this to bors when ready.

I'm happy with the current version of this PR, so please feel free to review when you have time.

Thanks!

[richkadel]

@wesleywiser - I was curious why I didn't need to add the new Rust intrinsics added in this PR to https://github.com/richkadel/rust/blob/cf6208643a7def50bc2cdd9a6539604afa555737/src/librustc_mir/interpret/intrinsics.rs#L413

I had to add the case for count_code_region here:

            }
            // FIXME(#73156): Handle source code coverage in const eval
            sym::count_code_region => (),
            _ => return Ok(false),

because this function caused the compiler to break without it.

But since it's working for the new intrinsics, without this change, I tried commenting out this change, and found that the current implementation still works without it (now).

I suspect that the problem occurred because of where the pass was executing at one time, and the current location seems to avoid the check and failure.

So I should be consistent here for all of the intrinsics, and it looks like I have the option of a) removing the check for count_code_region, or adding the new intrinsics to the case along side it.

Let me know what you think.

[richkadel]

So I should be consistent here for all of the intrinsics, and it looks like I have the option of a) removing the check for count_code_region, or (b) adding the new intrinsics to the case along side it.

Let me know what you think.

I'm actually going to ahead with option (a), adding the new intrinsics, since they are "known". If emulate_intrinsic() is ever called with these intrinsics, we know it won't break (and the FIXME reference is there for future support for const evaluations of coverage calls, if needed).

[richkadel]

@wesleywiser Please note the additional commit I just uploaded. I realized something after thinking about it overnight. How the coverage intrinsic calls should be handled (whether to process them internally or pass them onto the backend or both) should be fully orchestrated by the backend-specific code, but I had half of it in rustc_codegen_ssa (in block.rs) and half in rustc_codegen_llvm. I fixed this with the change in the new commit.

[bjorn3]

Why not add the code to codegen_intrinsic_call? You could just codegen nothing there.

[richkadel]

I need to extract the values from the const u32 operands, and for the non-codegened intrinsics, I didn't like the unnecessary overhead of converting them to LLVM values (OperandRefs) and back again for no reason. So is_codegen_intrinsic takes the rustc_middle::mir::Operands directly, before the args are converted to the backend.

Plus, for the intrinsics that are not passed through to codegen_intrinsic_call, this also avoids all of the other steps between the call to is_codegen_intrinsic() and codegen_intrinsic_call() in block.rs, and all of the prepwork in the implementation of codegen_intrinsic_call() prior to match name {. That's a lot of code that I can avoid calling by implementing it this way.

[wesleywiser]

Heads up @rust-lang/wg-const-eval, this PR is adding some new stub intrinsics which are used by the LLVM code coverage feature @richkadel is adding.

[wesleywiser]

Also cc @rust-lang/lang. This PR adds a few new intrinsics which are being used by the LLVM code coverage feature @richkadel is adding. Currently, these intrinsics are no-ops when evaluated at compile time (see #73156 for possible future extensions where these could measure code coverage when run by the const eval machinery).

[nikomatsakis]

These intrinsics seem like they're an implementation detail at this point, is that correct?

[wesleywiser]

I think that's a fair characterization. The intrinsics are injected via a MIR pass enabled by the -Zinstrument-coverage flag.

[nikomatsakis]

OK 👍 then I think it's all fine from the lang team perspective.

[richkadel]

Thanks Niko!

[richkadel]

All feedback has been addressed to this point. Thanks!

[richkadel]

r? @wesleywiser
(re-assigning review to Wesley as agreed, since @tmandry is out of office this week)

[wesleywiser]

Would you mind squashing the commits together?

[richkadel]

@wesleywiser I think I got this working. (I needed to update to the latest baseline first, and the squash worked without conflicts this time.) But it still shows my baseline syncs as separate commits.

Is this what you need? If not, please advise. Thanks!

[wesleywiser]

@richkadel This looks mostly right to me although there's still 3 merge commits in addition to the actual changes.

What I usually do is this (assuming you have the remote origin which points to this repo and fork which points to your fork of the repo):

$ git fetch origin # grab the latest upstream changes
$ # assuming you're already on the latest commit of your branch with changes
$ git rebase -i origin/master # take the commits in your branch and replay them on top of origin/master
$ # this is the point where you can edit the git todo file and squash the commits together
$ git push fork -f # If you don't have a matching branch set up, then use `git push fork -f HEAD:{branch_name}`

[bjorn3]

git push fork -f

I prefer git push fork --force-with-lease. It prevents a push when the destination has new commits that you haven't fetched yet. This is a little safeguard.

[richkadel]

Looks like I got it this time, but it was more painful than that (mostly because it was more trial and error to figure it out). I tried your commands Wesley but they had no effect. I still ended up with the additional merge commits showing up.

Here's what I ended up having to do:

$ git rebase --rebase-merges -i HEAD~2

Without --rebase-merges none of the merge commits showed up in the todo list, which is why they were not removed when I squashed everything else last night. And note, HEAD~1, HEAD, origin/master, etc. did not give me a usable todo list.

Then in the todo list, I had to make two changes:

Starting from something like:

pick <hash> Description of last commit since my most recent pull to my remote master
label some-label

reset onto
merge -C <hash> Description of my last merge commit
pick <hash> Description of the only commit I want to keep

I changed it to:

pick <hash> Description of last commit since my most recent pull to my remote master
label some-label

reset some-label
pick <hash> Description of the only commit I want to keep

I saved this and exited vim and a few seconds later I was able to push the PR with a single commit.

[richkadel]

For future reference, how do you sync your fork with the latest version of the rust-lang/rust repo, and then update your development branch for an ongoing pull request without creating these merge commits?

What I was doing:

1. Create a PR in my fork to merge the changes from rust-lang/rust:master into richkadel/rust:master and merge it.

2. On my workstation:

$ git checkout master
$ git fetch
$ git pull
$ git checkout <dev-branch-for-current-rust-lang-PR>
$ git rebase master
$ git push -f   # will try --force-with-lease next time

This creates the merge commit in the PR, which required the steps above.

I'm guessing this is not how others do it, if this is not a common workaround for removing merge commits?

[wesleywiser]

@richkadel I have a similar workflow but I have pull.rebase set to true so that git pull always does git pull --rebase which does not introduce merge commits (see git-pull). Without the merge commits to deal with, you can simplify your workflow a bit:

$ git checkout <dev-branch-for-current-rust-lang-PR>
$ git fetch origin # make the latest upstream changes available in your local repo
$ git rebase origin/master # Rebase your changes on top of upstream. NOTE: origin/branch means use branch as it is on origin, not your local repo. Therefore, you don't have to checkout master and pull it before rebasing. 
$ git push -f   # will try --force-with-lease next time

[richkadel]

@wesleywiser The fully-squashed 1 commit version passed all checks.

Good to merge?

[wesleywiser]

Yes, this looks great!

@bors r+ rollup

[bors]

📌 Commit 5239a68 has been approved by wesleywiser