Fixes reported bugs in Rust Coverage

[richkadel]

Fixes: #79569

Fixes: #79566
Fixes: #79565

For the first issue (#79569), I got hit a debug_assert!() before
encountering the reported error message (because I have debug = true
enabled in my config.toml).

The assertion showed me that some SwitchInts can have more than one
target pointing to the same BasicBlock.

I had thought that was invalid, but since it seems to be possible, I'm
allowing this now.

I added a new test for this.

---

In the last two cases above, both tests (intentionally) fail to compile,
but the InstrumentCoverage pass is invoked anyway.

The MIR starts with an Unreachable BasicBlock, which I hadn't
encountered before. (I had assumed the InstrumentCoverage pass
would only be invoked with MIRs from successful compilations.)

I don't have test infrastructure set up to test coverage on files that
fail to compile, so I didn't add a new test.

r? @tmandry
FYI: @wesleywiser

[richkadel]

Note that generating MIR with a SwitchInt with more than one target to the same Basic Block may require #![feature(or_patterns)]. I have not seen this issue otherwise.

[richkadel]

The changes in the counters file in async.txt are just another example of the non-deterministic ordering of output from llvm-cov.

These 4 lines (from 31-34) moved to line 50 when I added support for multiple SwitchInt targets to the same BasicBlock.

The other lines moved after I updated mod.rs to ignore MIR that starts with Unreachable.

I don't think these changes should have had any real effect on the existing test results, and they don't except in this one test sample, and only in the debug counters.

The Makefile based test does not fail based on differences in counter files. So it's not a problem, but it would be nice to make these more deterministic in the future, perhaps by sorting the entire file before comparing.

[richkadel]

UPDATED AND SUMMARIZED TO BE EASIER TO DIGEST:

Many errors seem to be in tests that declare `no_core`, and I think adding `-Zinstrument-coverage`
may force loading `core` (or `std`) anyway. See `creader.rs`:

   fn inject_profiler_runtime(&mut self) {
       if (self.sess.opts.debugging_opts.instrument_coverage
           || self.sess.opts.debugging_opts.profile
           || self.sess.opts.cg.profile_generate.enabled())
           && !self.sess.opts.debugging_opts.no_profiler_runtime
       {
           info!("loading profiler");

           let name = sym::profiler_builtins;
           let cnum = self.resolve_crate(name, DUMMY_SP, CrateDepKind::Implicit, None);
           let data = self.cstore.get_crate_data(cnum);

           // Sanity check the loaded crate to ensure it is indeed a profiler runtime
           if !data.is_profiler_runtime() {
               self.sess.err("the crate `profiler_builtins` is not a profiler runtime");
           }
       }
   }

===================================================================================================

Failures due to:
    error[E0152]: found duplicate lang item `sized`

All of the 'duplicate lang item' errors include `#![no_core]` and then define at least one lang item
already defined in core.

As shown above, `-Zinstrument-coverage` force-loads `profiler_builtins`, and that
may force-load `std` or `core` anyway?

src/test/ui/associated-types/associated-types-ICE-when-projecting-out-of-err.rs
src/test/ui/feature-gates/feature-gate-cfg-target-has-atomic.rs
src/test/ui/issues/issue-19660.rs
src/test/ui/issues/issue-31076.rs
src/test/ui/lang-item-missing-generator.rs
src/test/ui/panic-handler/panic-handler-requires-panic-info.rs
src/test/ui/privacy/privacy1.rs
src/test/ui/privacy/privacy4.rs
src/test/ui/required-lang-item.rs
src/test/ui/static_sized_requirement.rs

Note that src/test/ui/asm/bad-arch.rs also redefines lang item `sized`, but fails earlier (no "profiler_builtins")

===================================================================================================

Failure due to:
   error: `#[panic_handler]` function required, but not found ...
   error: language item required, but not found: `eh_personality`
This test actually expects a missing lang item, because "no_core", but the test expects a specific missing lang item.
With -Zinstrument-coverage, it finds different missing lang items

src/test/ui/lang-item-missing.rs

For all of the above, a new check in CrateLoader issues an error if any option (-Z instrument-coverage or -Z profile, for example) requires loading profiler_builtins, for crates that specify no_core. This should provide clearer feedback on the actual incompatibility, and in some cases may prevent ICE.

===================================================================================================

Failures due to:
    error[E0463]: can't find crate for `profiler_builtins`
The tests' `compiler-flags` for these tests specify targets, and
the targets' rustc build `config.toml`s must not have included `profiler = true`

src/test/ui/asm/bad-arch.rs
    # --target sparc-unknown-linux-gnu
src/test/ui/issues/issue-50993.rs
    # --target thumbv7em-none-eabihf
src/test/ui/sanitize/unsupported-target.rs
    # --target i686-unknown-linux-gnu

For the above, I think the current error is good enough.

But should we enable profile = true by default (and update config.toml.example to reflect that too)?

That would prevent some confusion.

I don't know if there are any platform-specific issues with profiler_builtins for platforms that don't currently enable this (for example, in CI).

===================================================================================================

Failure due to:
    LLVM Profile Error: Failed to write file "default.profraw": Permission denied

The test must be compiling in some kind of sandbox. I was able to work around this problem by
prefixing the `./x.py test` command with `LLVM_PROFILE_FILE=/tmp/command-pre-exec2.profraw`

src/test/ui/command/command-pre-exec.rs

This error is fine.

===================================================================================================

These tests fail with `-Z symbol-mangling-version=v0` (without `-Z instrument-coverage`).
`-Z instrument-coverage` forces `v0` and requires it.
Some of these tests are testing `legacy` symbol mangling, so they will never be compatible with
`-Z instrument-coverage`.

src/test/ui/const-generics/issues/issue-62579-no-match.rs
src/test/ui/const-generics/slice-const-param.rs
src/test/ui/const-generics/type-dependent/issue-71348.rs
src/test/ui/panics/issue-47429-short-backtraces.rs
src/test/ui/privacy/privacy2.rs
src/test/ui/privacy/privacy3.rs
src/test/ui/symbol-names/basic.rs#legacy
src/test/ui/symbol-names/impl1.rs#legacy
src/test/ui/symbol-names/issue-60925.rs#legacy
src/test/ui/symbol-names/issue-75326.rs#legacy
src/test/ui/type_length_limit.rs

I now allow someone to run coverage AND override its default symbol-mangling-version to force legacy.

If they do that, they will get a new warning.

===================================================================================================

Failure due to:
   'found env value "__LLVM_PROFILE_RT_INIT_ONCE" "__LLVM_PROFILE_RT_INIT_ONCE"'
I assume LLVM's compiler or profiler runtime sets this, and the test would simply need to be
updated to expect it.

src/test/ui/env-funky-keys.rs

See:
src/llvm-project/compiler-rt/lib/profile/InstrProfilingFile.c:381:30:
    #define LPROF_INIT_ONCE_ENV "__LLVM_PROFILE_RT_INIT_ONCE"
src/llvm-project/compiler-rt/lib/profile/InstrProfilingFile.c:402 and 404
    _putenv(LPROF_INIT_ONCE_ENV "=" LPROF_INIT_ONCE_ENV);
    setenv(LPROF_INIT_ONCE_ENV, LPROF_INIT_ONCE_ENV, 1);

This error is fine. I overrode the test options, and without that flag, it does not fail of course. If we ever want a test like this with coverage enabled, we would simply bless the new test result.

===================================================================================================

Failures due to:
   assertion failed: `(left == right)`
     left: `80`,
    right: `72`',

The tests are setting:
    -Z thinlto -C codegen-units=8 -O
    -C codegen-units=8 -O -C lto=thin

I think both variants may have the same effect.

`-Zinstrument-coverage` causes some change that appears to effect (perhaps disable) then intended inlining by LTO.

src/test/ui/thinlto/thin-lto-inlines.rs
src/test/ui/thinlto/thin-lto-inlines2.rs

Given the nature of the test, I think the error is appropriate. The test does not enable -Zinstrument-coverage by default, and if someone wanted to test for the effect of that option combination, they may agree this is expected, or otherwise could investigate if or how to change things, but I don't think it's a problem per se.

I don't know if we should issue any warnings about combining -Z thinlto (and variants) with -Z instrument-coverage. I think the behavior tested here does not indicate the compile itself failed. Only that the result was not as optimized as it would have been without coverage. So it may not be bad to combine them.

Therefore, I'm not going to do anything differently here.

===================================================================================================

Failure due to:
    The program compiles successfully, but it is supposed to fail to compile,
    with a compile-time error because the compiler recognizes an overflow error.
With `-Z instrument-coverage`, the compiler fails to recognize the error at compile time,
but overflows at runtime.

Prior to this PR, -Zinstrument-coverage disables MIR pass `Inline`:

       if tcx.sess.opts.debugging_opts.instrument_coverage {
           // The current implementation of source code coverage injects code region counters
           // into the MIR, and assumes a 1-to-1 correspondence between MIR and source-code-
           // based function.
           debug!("function inlining is disabled when compiling with `instrument_coverage`");
           return;
       }

But this test was attempting to enable it, with `-Z mir-opt-level=2`:

src/test/ui/const_prop/inline_spans.rs

Now, I no longer forcibly skip the Inline pass, but I do issue a warning if the user tries to enable inlining via -Z mir-opt-level (>= 2). The default has always been 1 (to skip the Inline pass), which is compatible with coverage.

===================================================================================================

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)

src/test/ui/consts/unstable-precise-live-drops-in-libcore.rs

I don't know why this test produces different results with -Zinstrument-coverage

===================================================================================================

Failure due to:
   stderr:
   ------------------------------------------
   error: this operation will panic at runtime
     --> /usr/local/google/home/richkadel/rust/src/test/ui/issues/issue-33287.rs:7:17
      |
   LL |     let range = A[1]..;
      |                 ^^^^ index out of bounds: the length is 1 but the index is 1
      |
      = note: `#[deny(unconditional_panic)]` on by default

src/test/ui/issues/issue-33287.rs

I don't know why this test produces different results with -Zinstrument-coverage

===================================================================================================

Failure due to additional lines in the compiler output:
   +       print-type-size type: `std::option::Option<std::num::NonZeroU32>`: 4 bytes, alignment: 4 bytes
   +       print-type-size     variant `Some`: 4 bytes
   +       print-type-size         field `.0`: 4 bytes
   +       print-type-size     variant `None`: 0 bytes

src/test/ui/print_type_sizes/niche-filling.rs

I don't know why this test produces different results with -Zinstrument-coverage

===================================================================================================

Failure due to:
   error: internal compiler error:
   compiler/rustc_traits/src/normalize_erasing_regions.rs:43:32:
   could not fully normalize `std::option::Option<<i32 as std::iter::Iterator>::Item>`

src/test/ui/trivial-bounds/trivial-bounds-inconsistent.rs

I don't know why this test produces different results with -Zinstrument-coverage

===================================================================================================

Side note regarding one more difference when compiling with `-Zinstrument-coverage`:

These test failures don't necessarily indicate an issue linking the LLVM runtime, but note,
ONLY the `GccLinker` does something specific for coverage and profiling. This may or may not be
relevant to other issues related to -Zinstrument-coverage.

impl<'a> Linker for GccLinker {
   ...
   fn pgo_gen(&mut self) {
       if !self.sess.target.linker_is_gnu {
           return;
       }

       // If we're doing PGO generation stuff and on a GNU-like linker, use the
       // "-u" flag to properly pull in the profiler runtime bits.
       //
       // This is because LLVM otherwise won't add the needed initialization
       // for us on Linux (though the extra flag should be harmless if it
       // does).
       //
       // See https://reviews.llvm.org/D14033 and https://reviews.llvm.org/D14030.
       //
       // Though it may be worth to try to revert those changes upstream, since
       // the overhead of the initialization should be minor.
       self.cmd.arg("-u");
       self.cmd.arg("__llvm_profile_runtime");
   }

[richkadel]

FYI, I updated the previous comment after doing a bit more investigation, to provide a better summarization, grouping like-issues, and providing explanations where I could. The last 4 tests as listed in the comment above show test result differences for which I could not deduce a specific cause.

For all of the others, it's pretty clear why they don't work with instrument-coverage, and generally "work as intended".

By generally, the only thing we may want to consider (I guess) is, does it ever make sense to combine no_core and -Z instrument-coverage. If so, then I guess the profiler_builtins library would need to be loaded in a way that will not also load core.

Is that even possible?

[wesleywiser]

By generally, the only thing we may want to consider (I guess) is, does it ever make sense to combine no_core and -Z instrument-coverage. If so, then I guess the profiler_builtins library would need to be loaded in a way that will not also load core.

I don't think it really makes sense or, at least, without some kind of clear use-case, I don't think we should spend any effort to attempt supporting that now. If someone eventually does need that, then we can consider adding support for it at that time.

[richkadel]

@richkadel said:

does it ever make sense to combine no_core and -Z instrument-coverage

@wesleywiser replied:

I don't think we should spend any effort to attempt supporting that now

Agreed, so instead, I added a check, when loading profiler_builtins, to see if no_core is specified, and if so, I issue a warning.

Similarly, I'm checking to see if -Z symbol-mangling-version=legacy is explicitly specified, with -Z instrument-coverage.

In both cases I issue a warning now.

This improves the error handling in tests, and resolves some ICE cases. It should be a little easier to understand why things like these are failing.

[richkadel]

Comment removed because it referred to an error in an earlier comment, which I've now fixed.

[richkadel]

Agreed, so instead, I added a check, when loading profiler_builtins, to see if no_core is specified, and if so, I issue a warning.

Similarly, I'm checking to see if -Z symbol-mangling-version=legacy is explicitly specified, with -Z instrument-coverage.

Oh, the changes I made for these new checks and warnings are in the second commit of this PR.

[richkadel]

FYI, here's how I'm testing some of these, for example:

$ ./x.py test --rustc-args="-Zinstrument-coverage" src/test/ui/associated-types/associated-types-ICE-when-projecting-out-of-err.rs
$ ./x.py test --rustc-args="-Zinstrument-coverage -Zsymbol-mangling-version=legacy" src/test/ui/const-generics/issues/issue-62579-no-match.rs 

[richkadel]

I updated #79958 (comment) again, this time adding commentary on what I've improved in this PR, and/or decided was working as intended.

I still don't know what is happening with the last 4 issues. My current theory (at least maybe for 3 of the 4) is--similar to the no_core issue--maybe we are injecting profiler_builtins (and it's dependency, the current baseline version of std, and core) too early. If some of the feature options are supposed to influence how std or core are loaded, but are too late, maybe we should be catching that. But I really don't know if this is on track or a bad theory.

src/test/ui/issues/issue-33287.rs doesn't make sense at all. It looks like the compiler error produced with -Zinstrument-coverage is a valid error, so why does it not generate an error without -Zinstrument-coverage?

[richkadel]

I cleaned up a couple of things this morning, but it should be ready for review again.

Sorry for the churn. I think these changes were worth it.

[tmandry]

I don't see a reason for making this an Option, it doesn't change its default dependent on coverage like symbol mangling does.

[richkadel]

I don't know of any other way to know if the option was set on the command line or not.

I need to know that in config.rs when checking for conflicting command line flags.

[richkadel]

Ah, I understand your point... Let me think about this a minute. I'm inclined to agree.

[tmandry]

To me the value matters more than if it was overridden (which you know it was today if it's > 1).. if the default were changed to 2, we would still want this to fire to warn people who were using coverage. (Probably we'd either keep the default as 1 if coverage were enabled, or add support for inlining counters. But it's unlikely that we turn on inlining by default anytime soon.)

[richkadel]

Sorry, yes, I concur. I'll remove the option from mir_opt_level. Thanks!

[richkadel]

if the default were changed to 2, we would still want this to fire to warn people who were using coverage.

I agree 100%. Thanks.

[tmandry]

Looks good after comment (which I'd like to see addressed because of unnecessary complexity). Thanks!

@bors delegate+

[bors]

✌️ @richkadel can now approve this pull request

[richkadel]

@bors r=tmandry rollup

[bors]

📌 Commit 36c639a has been approved by tmandry

[richkadel]

I still don't know what is happening with the last 4 issues. My current theory (at least maybe for 3 of the 4) is--similar to the no_core issue--maybe we are injecting profiler_builtins (and it's dependency, the current baseline version of std, and core) too early.

FYI, I re-ran these tests with -Zprofile, which should also load the profiler_builtins library. The tests succeeded, which means my theory was wrong.

I also tried forcing -Zsymbol-mangling-version=legacy (with instrument-coverage) and that didn't help. So these failures don't seem to be effected by how -Z instrument-coverage loads the core/std libraries, or by the mangling version.

The only thing left (I think) is the changes that -Zinstrument-coverage makes to the MIR.

If that's true, then for each of these 4 tests, the MIR processing implied by the flags and/or attrs seems to be confused by the additional Coverage statements?