Possible solution to allow MIR inlining with -Zinstrument-coverage

[richkadel]

Today, the MIR Inline pass is skipped if -Zinstrument-coverage is enabled.

I have an idea for a workaround that will allow MIR inlining.

I believe the problem is, when generating the LLVM coverage counters, and related coverage info used to generate the coverage map, each Coverage MIR statement assumes the coverage data should be associated with the current function being codegenned.

See

rust/compiler/rustc_codegen_ssa/src/mir/coverageinfo.rs

Line 23 in 1973f84

let coverageinfo = bx.tcx().coverageinfo(self.instance.def_id());

The self.instance is the function instance being generated. But if MIR inlining is performed, the original coverage counters for the inlined function will conflict with coverage counters of the current instance. For example, in both functions, the first counter is 1. But there will be two occurrences of that counter, for different functions in the original source. (That's essentially the problem.)

To fix this, I believe we can just add the original function's def_id to the MIR Coverage statement data.

Everywhere self.instance is used in the codegen_coverage() function should (probably) be replaced with an instance derived from the original def_id. This will apply those counters to the correct FunctionCoverage, and there will be no conflicts.

I may not be able to work on this very soon, and disabling MIR inlining is an OK workaround for now, but I wanted to capture the idea for me or someone else to try in the future.

[richkadel]

cc: @jyn514 @wesleywiser @tmandry

[richkadel]

One other note I forgot. Since an inlined function can be generated more than once, we need to make sure the coverage data isn't re-added. I suspect this is already checked, but there should be a test for this, once it's implemented.

[jyn514]

I'm not sure why you cc-ed me, I'm not very familiar with either MIR inlining or instrument-coverage (or codegen in general for that matter).

[richkadel]

No important reason Josh. Sorry. I know you typically add the coverage label, so that was the only reason

[richkadel]

Addendum:

I glossed over some details. One thing I just remembered, which is actually quite helpful, is we will need an actual Instance, not just a MIR def_id, for generics. The Inline pass will get that Instance (in order to inline a function). That means that the def_id for the Instance won't be available until the Inline pass. So there is no need to add a def_id to the Coverage statement when first constructed, but when inlining, the Coverage statement will need to be updated or replaced, adding the inlined function's Instance def_id.

Then in codegen_coverage() will use self.instance if the Coveage statement doesn't have it's own inline_instance.

This allows different parameterized versions of an inlined function to get different coverage results, just like non-inlined generics do now.

[tmiasko]

It is possible to derive an inlined instance from a scope information associated with a statement, and use that when creating a counter. Coverage itself could remain as it is now.

The general approach seems to work just fine, although I did only very limited amount of testing (I also relaxed checks for duplicate ids, allowing them as long as code regions & expressions are the same).