coverage: Avoid creating malformed macro name spans

[Zalathar]

This is a workaround for #117788. It detects a particular scenario where we would create malformed coverage spans that might cause llvm-cov to immediately exit with an error, preventing the user from processing coverage reports.

The patch has been kept as simple as possible so that it's trivial to backport to beta (or stable) if desired.

---

The maybe_push_macro_name_span method is trying to detect macro invocations, so that it can split a span into two parts just after the ! of the invocation.

Under some circumstances (probably involving nested macros), it gets confused and produces a span that is larger than the original span, and possibly extends outside its enclosing function and even into an adjacent file.

In extreme cases, that can result in malformed coverage mappings that cause llvm-cov to fail. For now, we at least want to detect these egregious cases and avoid them, so that coverage reports can still be produced.

[rustbot]

r? @davidtwco

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

[rustbot]

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

[Zalathar]

@rustbot label +A-code-coverage +beta-nominated

This works around a reported stable-to-beta regression.

[Zalathar]

Even though curr is already available as a local variable, the change that extracted that variable isn't part of the current (1.74) beta branch. So instead I'm calling self.curr() explicitly here, so that the same lines of code can simply be copied over to beta if desired.

[Swatinem]

it would be nice to have a regression test for this. I assume creating one while keeping this PR beta-backportable might be a problem though, considering you were merging/cleaning up the coverage tests in the meantime?

[Zalathar]

The main obstacle to writing a test is that currently I can only reproduce the issue via a standalone workspace that depends on abi_stable. So far I haven't been able to write a test that reproduces the issue.

If I investigate more, I might learn enough about the trigger conditions to be able to come up with a useful test.

[Zalathar]

I still haven't been able to reproduce the llvm-cov failure in a test, but I was able to write a test that demonstrates a more benign form of the problem. Here's what happens if the workaround is not applied:

   LL|       |// edition: 2021
   LL|       |
   LL|       |// Regression test for <https://github.com/rust-lang/rust/issues/117788>.
   LL|       |// Under some circumstances, the heuristics that detect macro name spans can
   LL|       |// get confused and produce incorrect spans beyond the bounds of the span
   LL|       |// being processed.
   LL|       |
   LL|      1|fn main() {
   LL|      1|    affected_function();
   LL|      1|}
   LL|       |
   LL|       |macro_rules! macro_that_defines_a_function {
   LL|       |    (fn $name:ident () $body:tt) => {
   LL|      1|        fn $name () -> () $body
   LL|      1|    }
   LL|      1|}
   LL|      1|
   LL|      1|// These comment lines are not executable and should not have counters.
   LL|       |// If they do have counters, it means that a span from the above $body has
   LL|       |// been extended beyond its proper bounds.
   LL|       |
   LL|       |macro_rules! macro_with_an_unreasonably_and_egregiously_long_name {
   LL|       |    () => {
   LL|       |        println!("hello");
   LL|       |    };
   LL|       |}
   LL|       |
   LL|       |macro_that_defines_a_function! {
   LL|       |    fn affected_function() {
   LL|       |        macro_with_an_unreasonably_and_egregiously_long_name!();
   LL|       |    }
   LL|       |}

Notice that there is a span of “executable” code that extends outside of macro_that_defines_a_function and spills over into the subsequent comment. In #117788, that span spills over into a subsequent file, so the line number for the end of the span is less than the line number of its start; that's what causes llvm-cov to exit with an error.

[davidtwco]

The main obstacle to writing a test is that currently I can only reproduce the issue via a standalone workspace that depends on abi_stable. So far I haven't been able to write a test that reproduces the issue.

If I investigate more, I might learn enough about the trigger conditions to be able to come up with a useful test.

Is there a reason you can't write a test with multiple crates containing the reproduction from #117788? Here's the compiletest docs for auxiliary crates if that's helpful - though it may just be a limitation of the coverage test suite that I'm unaware of.

[davidtwco]

This patch seems good to me, r=me if the MCVE test can't be added.

[Zalathar]

I'll try a version that puts the macro in an auxiliary crate, and see if I can get it to reproduce the full failure instead of the lesser problem.

[davidtwco]

I don't have a good sense of what the priority of the issue being resolved is, but given that the release is this week and this won't be visited by a triage meeting, I'm going to unilaterally backport approve this - it's a very simple patch which I think has little risk of introducing new bugs.

[Zalathar]

@davidtwco I tried putting the macro in a separate auxiliary crate and was able to reproduce the full llvm-cov failure; good call on that one. I've pushed an updated test.

[davidtwco]

@bors r+

[bors]

📌 Commit 514e324 has been approved by davidtwco

It is now in the queue for this repository.

[bors]

⌛ Testing commit 514e324 with merge b5cdb96...

[bors]

☀️ Test successful - checks-actions
Approved by: davidtwco
Pushing b5cdb96 to master...

[rust-timer]

Finished benchmarking commit (b5cdb96): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.8%	[0.4%, 1.5%]	3
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.7%	[-0.7%, -0.7%]	2
All ❌✅ (primary)	0.8%	[0.4%, 1.5%]	3

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.5%	[0.4%, 0.6%]	2
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.5%	[-0.5%, -0.5%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.2%	[-0.5%, 0.6%]	3

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 674.078s -> 673.401s (-0.10%)
Artifact size: 311.07 MiB -> 311.12 MiB (0.02%)

[cuviper]

I don't have a good sense of what the priority of the issue being resolved is, but given that the release is this week and this won't be visited by a triage meeting, I'm going to unilaterally backport approve this - it's a very simple patch which I think has little risk of introducing new bugs.

davidtwco added beta-accepted and removed beta-nominated labels

Procedural note: we keep both labels on to signal that it's approved, and only drop the nomination after someone (usually on the release team) has actually done the backport.

Since this PR landed in 1.76, and was backported to 1.74 during its beta->stable (#117843), then I assume we still do need it for 1.75-beta as well. If you want to reconsider the approval in a triage meeting, feel free to drop beta-accepted for now!
@rustbot label +beta-nominated

[Zalathar]

Note for whoever applies this to the 1.75 beta branch: take the patches from this PR since they should apply cleanly.

(The patches that were applied to stable needed some trivial adjustments that are not relevant to 1.75.)