Invalid LLVM coverage data produced when compiled with -C opt-level=1

[catenacyber]

I tried this code:

use num_integer::Roots;
 
fn main() {
    let a:u32 = 54321; 
    let c = a.cbrt();
    println!("Hello, world {} {}!", a, c);
}

compiled with RUSTFLAGS="-Zinstrument-coverage -C opt-level=1"
and Cargo.toml having

[dependencies]
num-integer = "0.1"

I expected to see this happen:
Running llvm-profdata merge -j=1 -sparse, then llvm-cov should give me a coverage report

Instead, this happened:
llvm-cov fails with `Failed to load coverage: Malformed instrumentation profile data

When building with -C opt-level=0, the buggy behavior does not show up with this reproducer

Meta

rustc --version --verbose:

rustc 1.52.0-nightly (07194ffcd 2021-02-10)
binary: rustc
commit-hash: 07194ffcd25b0871ce560b9f702e52db27ac9f77
commit-date: 2021-02-10
host: x86_64-unknown-linux-gnu
release: 1.52.0-nightly
LLVM version: 11.0.1

Backtrace

There is no backtrace here as the failure happens with llvm-cov
I used the following clang

clang --version
clang version 12.0.0 (https://github.com/llvm/llvm-project.git f086e85eea94a51eb42115496ac5d24f07bc8791)

Using this patch in clang

diff --git a/llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp b/llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp
index b75738bc360c..1ad5d930eafd 100644
--- a/llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp
+++ b/llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp
@@ -525,7 +525,8 @@ class VersionedCovMapFuncRecordReader : public CovMapFuncRecordReader {
       if (Error Err = CFR->template getFuncName<Endian>(ProfileNames, FuncName))
         return Err;
       if (FuncName.empty())
-        return make_error<InstrProfError>(instrprof_error::malformed);
+        FuncName = "lola";
+        //return make_error<InstrProfError>(instrprof_error::malformed);
       ++CovMapNumUsedRecords;
       Records.emplace_back(Version, FuncName, FuncHash, Mapping,
                            FileRange.StartingIndex, FileRange.Length);

I get more information with
./bin/llvm-cov show -name-regex="lol" -instr-profile=dump.profdata -object=/path/to/rustbinary
It shows the pretendedly unnamed function code which can be found here https://github.com/rust-num/num-integer/blob/master/src/roots.rs#L174

  174|      0|fn fixpoint<T, F>(mut x: T, f: F) -> T
  175|      0|where
  176|      0|    T: Integer + Copy,
  177|      0|    F: Fn(T) -> T,
  178|      0|{
...

cc @richkadel

Original discussion comes from google/oss-fuzz#4697

[catenacyber]

Another thing I just noticed is that the problematic functions (ie fixpoint in the reproducer case), in addition to be hinted inline, are also called by hinted inline functions themselves, and these caller functions are in traits implementations...

[richkadel]

@catenacyber - Thanks for filing this issue. I was able to build and test your example, and I can reproduce the problem.

Just to be clear, this is not exactly a bug. Due to certain requirements of LLVM's InstrProf coverage, some compiler (and/or linker) optimizations are not compatible. But coverage instrumentation is not expected to be used in production software, so limiting coverage instrumentation to non-optimized code is acceptable.

I may be able to issue a compiler warning when a -C opt-level greater than zero (0) is combined with -Z instrument-coverage. (As we have found with certain MIR optimization levels, making this an error could be problematic, so I hesitate to go beyond a warning.) It may also be possible to disable opt-level optimizations if coverage enabled, but again, from the MIR optimization experience, it may be hard to be sure what optimizations to disable, and where. Nevertheless, I'll look into it.

But I'm concerned that this issue does not really get to the real problem.

As I understand it from your last couple of postings on google/oss-fuzz#4697, you still have not figured out why your program is failing when coverage is enabled and you have set -Copt-level=0.

Can you provide a minimal example and steps to reproduce the problem where you have set -Copt-level=0, but still get corrupted coverage data?

[catenacyber]

Thanks @richkadel

I also think that -C opt-level is not the real problem, only a convenient way to get a reproducer where changing one byte shows the buggy behavior.
But I am not sure (there may be multiple problems)

I will work on minimizing the example and steps to reproduce the problem with -Copt-level=0 (and double check that I have no optimizations)

[richkadel]

@wesleywiser @tmandry - I've been thinking about how to reduce friction for cases like the one that prompted this issue (google/oss-fuzz#4697). Looking at that pull request, and seeing the challenges that @catenacyber has had, trying to disable optimizations, I think this must be a case where another tool is wrapping invocations of the Rust compiler, and forcing certain flags on it. It seems (in this case) possible to add -Z instrument-coverage to at least parts of the build process, but it's not clear how or even if it's possible to disable the optimization flags that conflict with -Z instrument-coverage.

We know that -O1 (aka -C opt-level=1 or higher does not work with coverage, per the trivial example shown in this issue. Sure, the program runs, but it would also run without -Z instrument-coverage; but it fails to generate coverage reports, which is the only reason for including the flag.

I think we should flag this combination as a compiler error (not a warning), both in command line option processing, and at any point downstream that an incompatible option might be enabled (for example, I believe that when LTO is enabled, the compiler also enables -O1 late in the compiler stages). (We could also issue an error when LTO options are included with -Z instrument-coverage, but I'm just trying to cover all of the bases. Maybe something else enables LTO, late in the compile process.)

This way, the problem @catenacyber is having would manifest itself at the point of actually trying to compile the program, which should help @catenacyber debug the root cause.

I don't think the error would ever be a "bad" thing, because the developer can avoid the error by not adding -Z instrument-coverage, if the optimization flags are important or unavoidable. Otherwise, if they want coverage, they will need to find a way to remove the optimization flags.

In both cases, this approach ensures the developer is in control.

The other option would be to automatically disable optimizations when -Z instrument-coverage is enabled, but I feel that this approach could hide potential side effects, for instance, if they've enabled an inlining optimization, but we magically disable it under the hood, they could be surprised by the result; and it probably would not be clear that the problem was related to including the -Z instrument-coverage flag.

WDYT?

[catenacyber]

I now think I have indeed 2 problems.
The first one is indeed that -Z instrument-coverage is incompatible with optimisations such as shipped in by cargo build --release

The second one, I can reproduce with suricata
The important settings are

export CC=clang
export CXX=clang++
export CFLAGS="-O1 -fno-omit-frame-pointer -gline-tables-only -DFUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION  -fprofile-instr-generate -fcoverage-mapping -pthread -Wl,--no-as-needed -Wl,-ldl -Wl,-lm -Wno-unused-command-line-argument"
export CXXFLAGS="-O1 -fno-omit-frame-pointer -gline-tables-only -DFUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION  -fprofile-instr-generate -fcoverage-mapping -pthread -Wl,--no-as-needed -Wl,-ldl -Wl,-lm -Wno-unused-command-line-argument -stdlib=libc++"
export RUSTFLAGS="--cfg fuzzing -Cdebuginfo=1 -Cforce-frame-pointers -Zinstrument-coverage -C link-arg=-lc++ -C debug-assertions=n"
./configure --disable-shared --enable-debug --enable-fuzztargets

Then running llvm-cov, I get Failed to load coverage: Malformed instrumentation profile data error: Could not load coverage information

So, I will investigate further the second problem (I did not manage to have a reproducer with Rust only yet) and create another issue about it
And for this issue, I think the combination of -Zinstrument-coverage with some optimisation deserves a warning or an error

[richkadel]

cargo build --release should not be a problem.

Are you trying to generate coverage from both C++ and Rust in the same binary? This has not been tried. I think it should work if you are using LLVM 11. I'm not sure about LLVM 12. Clang MUST generate Coverage Mapping Format version 4.

Note that there are multiple new version in the works, so generating both Rust and C/C++ coverage in the same binary is going to be difficult to keep working as versions progress. You may want to consider instrumenting a binary with only Rust coverage, and a separate binary with only C/C++.

This recommendation is new, based on conversations I just had yesterday (Feb 22), but it is probably good advice.

You are breaking new ground by attempting to generate coverage from the same binary from two different languages.

Note, your C/C++ flags also have coverage options and optimization options (like -O1). I don't know what optimizations are supported, but from what we've seen with LLVM optimization issues with Rust coverage, I suspect many optimizations in Clang/LLVM are also incompatible with Clang coverage.

[catenacyber]

cargo build --release should not be a problem.

It sets opt-level to 3 for some crates... So is it really not a problem ?

Are you trying to generate coverage from both C++ and Rust in the same binary?

And it works for my ecc-diff-fuzzer (where there is only a little part of Rust)
Yes, and this worked at some point for Suricata (maybe LLVM 11 indeed)

I think it should work if you are using LLVM 11

I will try that again

Clang MUST generate Coverage Mapping Format version 4.

Version 5 seems only an extension, so it should not be incompatible, or am I missing something ?

You may want to consider instrumenting a binary with only Rust coverage, and a separate binary with only C/C++.

It looks like when I try to instrument only rust for coverage (ie setting RUSTFLAGS, but not CFLAGS), running my compiled binary does not produce any default.profraw file
I get one when I set LDFLAGS, but then I still get Malformed instrumentation profile data with llvm-cov

TL;DR
I will try again LLVM 11, current rustc nightly and disabling optimizations

[catenacyber]

I will try again LLVM 11, current rustc nightly and disabling optimizations

Still getting Malformed instrumentation profile data with llvm-cov-11 clang-11 compilation...

[richkadel]

Note, your C/C++ flags also have coverage options and optimization options (like -O1). I don't know what optimizations are supported, but from what we've seen with LLVM optimization issues with Rust coverage, I suspect many optimizations in Clang/LLVM are also incompatible with Clang coverage.

I need to correct at least one thing I said here. I spoke to a colleague that is more familiar with Clang and LLVM, and was told that Clang's implementation of LLVM InstrProf coverage is compatible with at least some of Clang's optimization levels. (Clang -O1 and -O2 at least, maybe higher.)

It's not clear to me if Clang's -O1 is exactly the same as Rust's -O1. (I doubt they are required to be identical, but I'm not sure how similar they are, particularly when it comes to correlating optimization levels to LLVM passes and options.)

But it's reasonable to believe that it's possible for the Rust Compiler to support some optimization levels with coverage instrumentation at the same time.

The sample in this issue shows that is not the case, or at least, not always the case.

The current workaround is to disable optimizations when instrumenting for coverage. In most cases, coverage testing is not used in production, so some performance degradation is not going to be a major sacrifice (generally).

We can leave this issue in place, in case someone has ideas on how to expand support for coverage to work with some optimization levels. (Just setting expectations though: this is not currently a priority feature request, and AFAIK, no one is actively working on it.)

Here is something I did discover though. I compiled the sample, with and without -O1, and added --emit=llvm-ir,link to dump LLVM IR *.ll files. Then I counted the occurrences of loading the counters:

$ grep 'pgocount = load' $(find target -name '*.ll') | wc -l
199
$ grep 'pgocount = load' $(find target_with_O1 -name '*.ll') | wc -l
169

30 counters are no longer present in LLVM IR.

This lines up with what @catenacyber saw as well.

Something, either in Rust or due to Rust-specific LLVM passes or options, is removing counters and are probably corrupting the coverage counts expected in the coverage map.

[wesleywiser]

@richkadel

Looking at that pull request, and seeing the challenges that @catenacyber has had, trying to disable optimizations, I think this must be a case where another tool is wrapping invocations of the Rust compiler, and forcing certain flags on it. It seems (in this case) possible to add -Z instrument-coverage to at least parts of the build process, but it's not clear how or even if it's possible to disable the optimization flags that conflict with -Z instrument-coverage.

I think having an external tool that invokes rustc for you, providing the correct flags and then potentially runs tests and collects the coverage data for you would be great! cargo-flamegraph does something similar and packages an entire workflow into an easy to use command.

[tmandry]

FYI, linking instrumented Rust and C/C++ in the same binary is likely to fail (or it could work now but break in the future.) The reason is that the separate compilers each expect their own version of the runtime instrumentation library, but only one can be loaded in the binary. There are no compatibility guarantees between versions.

[richkadel]

@wesleywiser @tmandry - Before I put together a PR, I'd like to make sure you agree with it and will approve it.

tl;dr - I believe issuing an error when -O1 and -Z instrument-coverage are combined is by far the best mitigation for rustc. (And offering an additional flag that allows the combination, without error, is not a bad idea either.)

As I see it, we have three options:

1. Do nothing. There is already a warning in the Rust Unstable Book that some optimization flags are not compatible with -Z instrument-coverage. Users beware.
2. Issue a warning. This is nearly the same as doing nothing, especially when rustc is called via some wrapper (as I think is the case that triggered this issue:
   • The developer may never see the warning.
   • Warnings are easily ignored, and often don't mean the program will fail.
   • Since a program compiled with -O and -Z instrument-coverage may not fail in some cases, the warning will just be an annoyance.
3. Issue an error. This is my preference:
   • It forces the user to do something about it.
   • In the case of tools that wrap rustc, the developer's workflow will fail early, giving them a fighting chance to find and fix the rustc flag combination.
   • If anyone really cares about optimizing binaries with coverage enabled, this will ensure the fix is prioritized.
   • If no one really cares, then there is no reason to provide support for it, and an error is fine.

We could also add one more flag/option to override the error, so if someone really wants to try combining them, they still can, but they must proactively enable it.

Either way, the error means there are no surprises, and it won't be possible to build a program that might "just happen to work" with the combination one day, but fail to compile after some small change in their program that triggers the coverage failure.

[catenacyber]

FYI, linking instrumented Rust and C/C++ in the same binary is likely to fail (or it could work now but break in the future.) The reason is that the separate compilers each expect their own version of the runtime instrumentation library, but only one can be loaded in the binary. There are no compatibility guarantees between versions.

Thanks @tmandry
If I get it correctly, that is just adding the constraint that I use compatible LLVM/clang/rustc when instrumenting my Rust+C program, right ?

[richkadel]

@catenacyber - Just to be clear, @tmandry is explaining a case that is known to fail, but the explanation does not state what is known to work.

Until we have evidence that someone has tried generating coverage reports from both Rust and C++, we don't know for certain if there is a configuration that will work.

But known constraints are:

• Both compilers must be built using the same version of LLVM.
• The libraries and binaries must depend on and be linked with the runtime instrumentation library from the same version of LLVM.
• Both compilers must be using the same Coverage Mapping Format.

I don't know if there are other dependencies, but given the issues with optimization levels (discussed in this issue) and the observation that optimization levels (and other compiler options, probably) can change LLVM IR in ways that may affect coverage, there may also be limitations on what compiler flags are allowed, for both Rust and Clang.

If all of these things are done right (and if I haven't missed anything), then it might work; but it would be cool to see a non-trivial example that does work, to know for sure.

Sadly, even if it works today, I know there are multiple new versions of Coverage Mapping Format in the pipeline that Clang will probably support, and it may be non-trivial to add them to Rust as well. It will be difficult to keep Rust in sync with both the LLVM version and the coverage map version, and still take advantage of new releases of the Rust compiler.

That's why I'm starting to believe the best practice may have to be to generate coverage reports for each language separately, from the same binary; which means running the program or tests twice.

[richkadel]

tl;dr - I believe issuing an error when -O1 and -Z instrument-coverage are combined is by far the best mitigation for rustc. (And offering an additional flag that allows the combination, without error, is not a bad idea either.)

I spoke to both @wesleywiser and @tmandry offline, and both agreed with this plan (including the additional flag to override the error, which will be useful for rustc developers in diagnosing this kind of issue). And I see @catenacyber also gave a thumbs up.

I will move forward with a PR for this.

I plan to split out a separate issue that summarizes the feature request to support some -O level(s) with -Z instrument-coverage.

The compiler error message will reference that issue, so when someone encounters the error, they will know why the flag combination is not supported, and what can be done, if desired.

When the PR to add the error lands, we'll close this issue (#82144), and the feature request will remain open for now.

[catenacyber]

Until we have evidence that someone has tried generating coverage reports from both Rust and C++

That works for my ecc-diff-fuzzer project (which is smaller)

With Suricata, I currently get llvm-cov instrprof_error::malformed because of FuncName.empty() in llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp so I can print the function hash which is f53110d1692373a5

Any more insight on how to debug this ?

That's why I'm starting to believe the best practice may have to be to generate coverage reports for each language separately, from the same binary; which means running the program or tests twice.

I do not manage to get Rust-only coverage from my binary (ie it does not produce default.profraw file) when I set my RUSTFLAGS but no CFLAGS/CXXFLAGS

[catenacyber]

This bug does not show up with rustc 1.50.0-nightly (5be3f9f10 2020-12-03)
This bug does show up with rustc 1.50.0-nightly (3ff10e74a 2020-12-04)

So, as stated in google/oss-fuzz#4697 (comment)
I can get coverage for both C and Rust with one binary, even if I optimize my Rust (and now even if my clang 13 uses coverage map version 5)

So a regression took place between 5be3f9f and 3ff10e7 for coverage of optimized code cf this report
There seems to have been quite some changes to llvm coverage in this commit range...
Maybe adding unreachable regions to covered files ?

Furthermore, since this comment, Suricata added a new dependency with the brotli crate which confuses coverage, but this is another bug report #82875

[richkadel]

Thanks for recognizing that this was a regression and for bisecting to find the exact date of the change!

#79109 landed on 12-3-2020. As you said, that was a big feature commit to complete almost all of the planned capabilities. But I think I can narrow down the likely problem code once I get into it.

I should be able to add a new test or two, and try disabling change(s) implemented in this commit to see if the tests work without some feature. Then I'll need to devise a better solution to whatever feature had to be disabled to make it work.

I don't have the time to do this right now, so I'm glad you found a workaround for Suricata!

But I will try to find the time in the next few weeks.

[richkadel]

Note to self, notable changes in that PR:

• Implemented add_unreachable_coverage() to make sure we have counters for code that was in the MIR, even if it was never generated. Knowing there is code that is never called is an important feature of coverage analysis, and this appeared to work better than -C link-dead-code, and (as far as we knew) it didn't break things, unlike -C link-dead-code (which caused lots of problems). This seems like a likely candidate for causing the problem described in the current issue (even if it's rare). Maybe we can find a workaround, perhaps some LLVM IR annotation or declaration that helps LLVM avoid whatever is causing it to fail in some cases.
• No longer enables -C link-dead-code by default. -C link-dead-code is known to cause a lot of problems for coverage. This is probably not the problem, but the workaround that allowed me to disable -C link-dead-code is a better candidate for contributing to the regression.

[catenacyber]

I tried a quick patch to remove add_unreachable_coverage() and still got the bug

[richkadel]

OK, good to know.

[catenacyber]

Thanks very much @richkadel
Did you try to get suricata coverage without the previous hacks to check this PR ?

[richkadel]

No, I didn't test that. Fingers crossed, but I suspect it will work now.

[richkadel]

@catenacyber - Did you propose or submit a patch to upstream LLVM to change how it handles missing functions (to make it a warning but not an error)?

Reading the comments on various PRs and Issues, it looks like you did not follow through with this, but I'm considering making that change to llvm-cov.

I'm still seeing this error triggered, very infrequently, and I'm not sure under what circumstances.

The current implementation is (IMO) unnecessarily brittle and disruptive (throwing out the entire coverage report when it's only missing one function name).

If you have a patch in progress, can you send me the LLVM link? Or if not, do you still have the code for a proposed patch?

I noticed that if you return a static string, and there is only one missing function, the coverage results still show up, so you basically get a valid coverage report anyways.

But with a static string, if there are more than one missing function, only one has coverage, and the rest appear uncovered, because multiple functions share the same fallback name.

So a solutions would be to generate a new string for each missing function (which requires allocating them somewhere).

Definitely fixable, but not as simple as returning a simple static string.

If you already implemented a fix like that, I'd love to move that forward.

If not, I will start a new patch.

Let me know.

Thanks!
Rich

[catenacyber]

Did you propose or submit a patch to upstream LLVM to change how it handles missing functions (to make it a warning but not an error)?

I did not

but I'm considering making that change to llvm-cov

You are welcome to do it.
My quick and dirty patch is in the first comment of this issue.
I am not sure about this deserves warning vs error...
So I did not push it upstream

I noticed that if you return a static string, and there is only one missing function, the coverage results still show up, so you basically get a valid coverage report anyways.

Indeed, maybe the coverage about the whole static library/rust crate is wrong, but the other ones look correct.

[richkadel]

Thank you! I just wanted to make sure I wasn't duplicating effort. Thanks so much for your time and effort helping to find this root cause. It meant a lot!