Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Invalid LLVM coverage data produced with crate brotli_decompressor #82875

Closed
catenacyber opened this issue Mar 7, 2021 · 2 comments · Fixed by #83307
Closed

Invalid LLVM coverage data produced with crate brotli_decompressor #82875

catenacyber opened this issue Mar 7, 2021 · 2 comments · Fixed by #83307
Labels
A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage)

Comments

@catenacyber
Copy link

I tried this code:

use std::io;
use brotli_decompressor;

fn main() {
    match brotli_decompressor::BrotliDecompress(&mut io::stdin(), &mut io::stdout()) {
        Ok(_) => {},
        Err(e) => panic!("Error {:?}", e),
    }
}

compiled with RUSTFLAGS="-Zinstrument-coverage"
and Cargo.toml having

[dependencies]
brotli-decompressor = "2.3.1"

I expected to see this happen:
Running echo 123 | ./target/debug/rustcov then 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

Meta

rustc --version --verbose:

rustc 1.52.0-nightly (476acbf1e 2021-03-03)
binary: rustc
commit-hash: 476acbf1e9965b5e95c90f0d7d658709812b7003
commit-date: 2021-03-03
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 4918a3d138b907a571f496661b5367e090e1e8bb)

Using this patch in llvm

diff --git a/llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp b/llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp
index 1acdcb4bebb9..b985ea11138d 100644
--- a/llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp
+++ b/llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp
@@ -544,8 +544,11 @@ class VersionedCovMapFuncRecordReader : public CovMapFuncRecordReader {
       StringRef FuncName;
       if (Error Err = CFR->template getFuncName<Endian>(ProfileNames, FuncName))
         return Err;
-      if (FuncName.empty())
-        return make_error<InstrProfError>(instrprof_error::malformed);
+      if (FuncName.empty()) {
+       FuncName = "ossfuzz_llvm_rust_coverage_warning";
+       printf("function with hash %lx has no name", FuncHash);
+        //return make_error<InstrProfError>(instrprof_error::eof);
+      }
       ++CovMapNumUsedRecords;
       Records.emplace_back(Version, FuncName, FuncHash, Mapping,
                            FileRange.StartingIndex, FileRange.Length);

I get more information with, as llvm-cov prints
function with hash 66eebda9c13062cc has no name

cc @richkadel
Might end up as a duplicate of #82144
But I do not see any optimization in cargo build -vv

@jonas-schievink jonas-schievink added the A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) label Mar 7, 2021
@richkadel
Copy link
Contributor

I believe I have a working fix for this. It is apparently not a dup of #82144 but may be related.

The fix I applied is a change in rustc_typeck/src/collect.rs function codegen_fn_attrs():

                } else if list_contains_name(&items[..], sym::always) {
                    if tcx.sess.opts.debugging_opts.instrument_coverage {
                        InlineAttr::Hint
                    } else {
                        InlineAttr::Always
                    }

I want to make a note of this, but I'm also investigating an idea that may (possibly) allow LLVM inlining.

If that fix works, I may be able to enable LLVM optimization, and this workaround would not be necessary.

What's happening is, #[inline(always)] causes LLVM codegen to force inlining of the function, regardless of optimization settings (apparently).

I'm not sure if there is an LLVM flag we can apply today, to turn that off, or if that's even a good idea. But this fix would seem to disable inlining of all Rust functions, as long as LLVM optimization is NOT applied.

The sample code works with this fix.

I'll be submitting a PR in the near future with either this fix or a better one, if I confirm I can enable LLVM inlining after all.

cc: @tmandry @wesleywiser

@catenacyber
Copy link
Author

Thanks, looking forward to the fix

bors added a commit to rust-lang-ci/rust that referenced this issue Mar 25, 2021
…=tmandry

coverage bug fixes and optimization support

Adjusted LLVM codegen for code compiled with `-Zinstrument-coverage` to
address multiple, somewhat related issues.

Fixed a significant flaw in prior coverage solution: Every counter
generated a new counter variable, but there should have only been one
counter variable per function. This appears to have bloated .profraw
files significantly. (For a small program, it increased the size by
about 40%. I have not tested large programs, but there is anecdotal
evidence that profraw files were way too large. This is a good fix,
regardless, but hopefully it also addresses related issues.

Fixes: rust-lang#82144

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

Existing tests now work up to at least `opt-level=3`. This required a
detailed analysis of the LLVM IR, comparisons with Clang C++ LLVM IR
when compiled with coverage, and a lot of trial and error with codegen
adjustments.

The biggest hurdle was figuring out how to continue to support coverage
results for unused functions and generics. Rust's coverage results have
three advantages over Clang's coverage results:

1. Rust's coverage map does not include any overlapping code regions,
   making coverage counting unambiguous.
2. Rust generates coverage results (showing zero counts) for all unused
   functions, including generics. (Clang does not generate coverage for
   uninstantiated template functions.)
3. Rust's unused functions produce minimal stubbed functions in LLVM IR,
   sufficient for including in the coverage results; while Clang must
   generate the complete LLVM IR for each unused function, even though
   it will never be called.

This PR removes the previous hack of attempting to inject coverage into
some other existing function instance, and generates dedicated instances
for each unused function. This change, and a few other adjustments
(similar to what is required for `-C link-dead-code`, but with lower
impact), makes it possible to support LLVM optimizations.

Fixes: rust-lang#79651

Coverage report: "Unexecuted instantiation:..." for a generic function
from multiple crates

Fixed by removing the aforementioned hack. Some "Unexecuted
instantiation" notices are unavoidable, as explained in the
`used_crate.rs` test, but `-Zinstrument-coverage` has new options to
back off support for either unused generics, or all unused functions,
which avoids the notice, at the cost of less coverage of unused
functions.

Fixes: rust-lang#82875

Invalid LLVM coverage data produced with crate brotli_decompressor

Fixed by disabling the LLVM function attribute that forces inlining, if
`-Z instrument-coverage` is enabled. This attribute is applied to
Rust functions with `#[inline(always)], and in some cases, the forced
inlining breaks coverage instrumentation and reports.

FYI: `@wesleywiser`

r? `@tmandry`
@bors bors closed this as completed in bcf7555 Mar 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants