Open richkadel opened 3 years ago
@tmandry @wesleywiser
I created this issue for the 4 tests that created errors or output that I can't explain.
I don't know if any of these issues are actually bugs in -Z instrument-coverage
, or acceptable incompatabilities. In case there are bugs, they might affect the tracking issue: #79121
unstable-precise-live-drops-in-libcore.rs
emits unused code warnings, but only if -Zinstrument-coverage
is not specified:
> rustc +stage1 src/test/ui/consts/unstable-precise-live-drops-in-libcore.rs
warning: enum is never used: `Either`
--> src/test/ui/consts/unstable-precise-live-drops-in-libcore.rs:7:6
|
7 | enum Either<T, S> {
| ^^^^^^
|
= note: `#[warn(dead_code)]` on by default
warning: associated function is never used: `unwrap`
--> src/test/ui/consts/unstable-precise-live-drops-in-libcore.rs:15:18
|
15 | pub const fn unwrap(self) -> T {
| ^^^^^^
warning: 2 warnings emitted
Backtrace:
The error happens during drop elaboration. I'm guessing -Zinstrument-coverage
somehow marks the code as used; perhaps the unreachable function handling is involved somehow?
cc @ecstatic-morse
issue-33287.rs
: Using -Ztreat-err-as-bug
again we can see that the covered_file_name
query is causing optimized_mir
to be evaluated for the test
function, when it wasn't before.
This indicates a possible performance issue with that query; maybe there's a way we can implement it that doesn't demand optimized MIR. EDIT: The fact that this is causing new errors might also mean that it isn't correct to rely on that query, but that is less clear to me.
Also, I feel like this function should be getting called by main
in the test to make sure we're exercising all the code paths the original regression test was meant to exercise.
cc @wesleywiser
..skipping the layout one for now..
trivial-bounds-inconsistent.rs
: This again seems to be happening during drop elaboration, but this time it's happening as a direct result of the covered_file_name
query, so sort of a combination of the last two.
Here we're generating MIR for a function which can never be called, due to its where i32: Iterator
clause. So this might indicate that it's a bug to run the optimized_mir
query on a function which couldn't be called. Or that the normalization code is too brittle somehow.
cc @matthewjasper
I haven't looked into the layout issue yet but it looks like the coverage code is causing a layout to be computed that wasn't before. Unclear to me if that's actually a problem or not; the test is relying on the side effectful outputs of a query computed on demand so that's not going to be super robust.
perhaps the unreachable function handling is involved somehow?
I can't think of a reason the "unreachable function handling" would mark anything as used. It also seems "late" in the process (well after MIR processing is done). But in any case, it doesn't (at least not intentionally) actually use the code. It just gets the spans for unused functions, and adds code regions for those spans to the coverage map for a function that is used (being codegenned).
But outside of that, if an unused function (as reported by these errors) is still going through codegen anyway, the coverage map variables (generated by LLVM) may add references to those functions' symbols that could mark functions "used" (perhaps only indirectly) in LLVM IR.
Are the errors generated based on an assessment of used or not used, from an LLVM perspective? Or just from MIR? If only from MIR, then I don't think the coverage map generation would play a role in marking them used; and I would look at the InstrumentCoverage
pass instead. It's inserting additional statements and occasionally additional BasicBlocks with Goto terminators (to count some branches).
LLVM [IR] won't affect any compiler messages emitted by rustc.
I also can't think of how the unreachable code handling would affect this, just throwing out ideas. It does seem that without coverage enabled the functions are marked unused, and with coverage they are considered used, and I'm not sure why.
The regression in the first case is legitimate and not related to unused functions. If you look at the MIR after SimplifyCfg-elaborate-drops
, you'll see that drop elaboration is no longer able to remove the frivolous drops in the presence of code coverage statements. IIRC, I looked for a very specific pattern when determining whether a SwitchInt
terminator is a switch on an enum discriminant or not:
We used to have a bespoke terminator for that operation, but it was refactored away. To fix the test, modify that function to skip over code coverage statements when looking for a discriminant read or put the code coverage statements elsewhere.
@tmandry - I currently inject the Coverage
statement in the last bb of the bcb, and I push it to the end of the statements
vec. But looking at this example, I think that looks weird, since Terminator
s are sort of a special case of a Statement
, and in this case, one logically flows into the other. Adding the Coverage
statement oddly breaks up that pair.
I think I will try injecting the statement at the front of the statements vec, and in the first (leader
) bb of the bcb. A slight performance hit with the vec insert(0, cov_stmt)
vs. the push()
, but I think it's negligible.
I'll see if that resolves it. Who knows, maybe it will help with other issues.
$ ./x.py test --rustc-args="-Zinstrument-coverage" src/test/ui/consts/unstable-precise-live-drops-in-libcore.rs
Now passes, with #80072
1 down, 3 to go.
The fix in #80072 did not fix tests 2, 3, and 4 above.
When experimentally adding
-Zinstrument-coverage
to the set ofui
tests in the Rust source tree, most tests still pass; of those that don't, almost all failures can be explained, and some improvements have been made to surface known incompatibilities between-Zinstrument-coverage
and other compiler options (see https://github.com/rust-lang/rust/pull/79958#issuecomment-743958675).However, there are ~4~ 3 tests that still fail with
-Zinstrument-coverage
that I don't understand.I'm hoping someone(s) that understand these tests and/or compiler options can take a look, explain why they are expected to fail, or clarify what the bug might be, if they should not fail.
Fixed via #80072
~$ ./x.py test --rustc-args="-Zinstrument-coverage" src/test/ui/consts/unstable-precise-live-drops-in-libcore.rs~
~Failure due to: error[E0493]: destructors cannot be evaluated at compile-time --> /usr/local/google/home/richkadel/rust/src/test/ui/consts/unstable-precise-live-drops-in-libcore.rs:15:25 | LL | pub const fn unwrap(self) -> T { | ^^^^ constant functions cannot evaluate destructors (this error is printed twice)~