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

Incompatibility using both -Zinstrument-coverage and -Z build-std #79401

Open
catenacyber opened this issue Nov 25, 2020 · 8 comments
Open

Incompatibility using both -Zinstrument-coverage and -Z build-std #79401

catenacyber opened this issue Nov 25, 2020 · 8 comments
Assignees
Labels
-Zbuild-std Unstable Cargo option: Compile the standard library yourself. A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@catenacyber
Copy link

catenacyber commented Nov 25, 2020

I tried this code:

git clone https://github.com/OISF/suricata.git
cd suricata
export RUSTFLAGS="-Zinstrument-coverage"
sh autogen.sh
./configure --disable-shared --enable-fuzztargets --disable-gccmarch-native --enable-debug-validation
make

This ends up running cargo build --release -Z build-std

I expected to see this happen:
Compilation succeeds

Instead, this happened:
Compilation failed with

Updating crates.io index
   Compiling core v0.0.0 (/Users/catena/.rustup/toolchains/nightly-x86_64-apple-darwin/lib/rustlib/src/rust/library/core)
error[E0152]: found duplicate lang item `f32`
   --> /Users/catena/.rustup/toolchains/nightly-x86_64-apple-darwin/lib/rustlib/src/rust/library/core/src/num/f32.rs:319:1
    |
319 | / impl f32 {
320 | |     /// The radix or base of the internal representation of `f32`.
321 | |     #[stable(feature = "assoc_int_consts", since = "1.43.0")]
322 | |     pub const RADIX: u32 = 2;
...   |
937 | |     }
938 | | }
    | |_^
    |
    = note: the lang item is first defined in crate `core`.
    = note: first definition in `core` loaded from /Users/catena/.rustup/toolchains/nightly-x86_64-apple-darwin/lib/rustlib/x86_64-apple-darwin/lib/libcore-e3cd79f6ad1173fd.rlib
    = note: second definition in the local crate (`core`)

and many more errors about other duplicate items

Meta

This is part of #34701 cc @richkadel

rustc --version --verbose:

rustc 1.50.0-nightly (603ab5bd6 2020-11-15)
binary: rustc
commit-hash: 603ab5bd6e0ffefafa7411cd8bd23a6ca82bcff0
commit-date: 2020-11-15
host: x86_64-apple-darwin
release: 1.50.0-nightly
Backtrace

    Updating crates.io index
   Compiling core v0.0.0 (/Users/catena/.rustup/toolchains/nightly-x86_64-apple-darwin/lib/rustlib/src/rust/library/core)
error[E0152]: found duplicate lang item `f32`
   --> /Users/catena/.rustup/toolchains/nightly-x86_64-apple-darwin/lib/rustlib/src/rust/library/core/src/num/f32.rs:319:1
    |
319 | / impl f32 {
320 | |     /// The radix or base of the internal representation of `f32`.
321 | |     #[stable(feature = "assoc_int_consts", since = "1.43.0")]
322 | |     pub const RADIX: u32 = 2;
...   |
937 | |     }
938 | | }
    | |_^
    |
    = note: the lang item is first defined in crate `core`.
    = note: first definition in `core` loaded from /Users/catena/.rustup/toolchains/nightly-x86_64-apple-darwin/lib/rustlib/x86_64-apple-darwin/lib/libcore-e3cd79f6ad1173fd.rlib
    = note: second definition in the local crate (`core`)

@catenacyber catenacyber added the C-bug Category: This is a bug. label Nov 25, 2020
@catenacyber
Copy link
Author

By the way, there is a current PR OISF/suricata#5582 with a fix : not using -Z build-std when -Zinstrument-coverage is in RUSTFLAGS

@richkadel
Copy link
Contributor

@catenacyber - Thanks for the bug report!

I did some experimenting (briefly), and I can repro this issue, for instance, with:

RUSTFLAGS="-Zinstrument-coverage" cargo build --release -Z build-std --target x86_64-unknown-linux-gnu

I don't fully understand how -Z build-std works, but from the error messages, it looks like the compiler may have built a new version of the std library (as intended), and then the compiler uses that new version but also (?) loads another version of std (or two versions of core).

One copy loads the standard LangItems, and the second copy also tries to load them but fails because they were already loaded.

(This is a minimually-educated guess, from the error messages.)

Anyhow, I don't think -Zinstrument-coverage would be doing anything specifically to cause this problem, so I thought, maybe it's not specific to -Zinstrument-coverage.

So I tried:

RUSTFLAGS="-Zprofile" cargo build --release -Z build-std --target x86_64-unknown-linux-gnu

Sure enough, I get the same errors.

Since cargo -Z build-std is also an "experimental" feature, I would redirect this issue to the build-std feature experts, to see if they can figure out why some compiler options (like -Zinstrument-coverage and -Zprofile) conflict in this way.

Note, both -Z instrument-coverage and -Z profile do have a dependency on the profiler_builtins library in rust/library, so one place they might look is where the profiler_builtins library gets loaded. Maybe it's forcing the default std library to load, as a direct dependency of profiler_builtins, rather than using the std that was just built.

@catenacyber
Copy link
Author

Indeed, I do not know which experimental feature should fix this.
So, please redirect this report how you see fit 👍

@richkadel
Copy link
Contributor

richkadel commented Dec 3, 2020

@alexcrichton - Is cargo -Z build-std something you would be familiar with? Or if not, do you know who might be? (Thanks!)

@alexcrichton
Copy link
Member

I believe the profiler_builtins crate is the key here. Using -Zbuild-std builds a fixed set of crates and profiler_builtins is not included in that set. This would probably be an issue for https://github.com/rust-lang/wg-cargo-std-aware/issues since I don't think there's much rust-lang/rust can do here for this.

@richkadel
Copy link
Contributor

@catenacyber - Would you mind copy/pasting your issue into that list (perhaps just linking back to here for context if helpful)?

Thanks!

@taiki-e
Copy link
Member

taiki-e commented Jul 12, 2023

@rustbot label +A-code-coverage

@Enselic
Copy link
Member

Enselic commented Nov 21, 2024

@rustbot claim

For me the fix in #133300 makes this work. See the PR for detailed instructions.

@Enselic Enselic added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Nov 22, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Nov 24, 2024
…age, r=jieyouxu

inject_panic_runtime(): Avoid double negation for 'any non rlib'

<details>

<summary>This PR originally did more things .Click to expand to see.</summary>

By not trying to inject a profiler runtime when only building an rlib. This logic already exists for the panic runtime.

This makes

    RUSTFLAGS="-Cinstrument-coverage" cargo build -Zbuild-std=std,profiler_builtins

work. Note that you probably also need
`RUST_COMPILER_RT_FOR_PROFILER=$src/llvm-project/compiler-rt` in your environment.

cc rust-lang#79401

# Demonstration

Before this fix you get these errors:

```console
$ rm -rf target ; RUST_COMPILER_RT_FOR_PROFILER=/home/martin/src/llvm-project/compiler-rt RUSTFLAGS="-Cinstrument-coverage" cargo +nightly build --release -Zbuild-std=std,profiler_builtins
error: `profiler_builtins` crate (required by compiler options) is not compatible with crate attribute `#![no_core]`
error[E0152]: found duplicate lang item `manually_drop`
    = note: first definition in `core` loaded from /home/martin/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/lib/libcore-d453bab70303062c.rlib
    = note: second definition in the local crate (`core`)
```

With the fix the build succeeds:

```console
$ rm -rf target ; RUST_COMPILER_RT_FOR_PROFILER=/home/martin/src/llvm-project/compiler-rt RUSTFLAGS="-Cinstrument-coverage" cargo +stage1 build --release -Zbuild-std=std,profiler_builtins
    Finished `release` profile [optimized] target(s) in 45.57s
```

And we can check code coverage. My example program looks like this:

```rs
fn main() {
    if std::env::args_os().nth(1) == Some("write-file".into()) {
        std::fs::write("hello.txt", "Hello, world!").unwrap();
    } else {
        println!("Hello, world!");
    }
}
```

when the program prints to stdout:

```
$ LLVM_PROFILE_FILE=stdout.profraw ./target/release/hello-world
Hello, world!
```

we can see that `fs::write()` is not being used (note the `0`'s):

```console
$ /home/martin/src/rust/build/x86_64-unknown-linux-gnu/stage1/lib/rustlib/x86_64-unknown-linux-gnu/bin/llvm-profdata merge -sparse stdout.profraw -o stdout.profdata
$ /home/martin/src/rust/build/x86_64-unknown-linux-gnu/stage1/lib/rustlib/x86_64-unknown-linux-gnu/bin/llvm-cov show target/release/hello-world --sources /home/martin/src/rust/build/x86_64-unknown-linux-gnu/stage1/lib/rustlib/src/rust/library/std/src/fs.rs --instr-profile stdout.profdata --color | grep -A 3 'pub fn write(&mut self, write: b
ool) -> &mut Self {'
 1357|      0|    pub fn write(&mut self, write: bool) -> &mut Self {
 1358|      0|        self.0.write(write);
 1359|      0|        self
 1360|      0|    }
```

but when we print to a file:

```console
$ LLVM_PROFILE_FILE=file.profraw ./target/release/hello-world write-file
```

the code coverage shows `fs::write()` as being used (note the `1`'s):

```console
$ /home/martin/src/rust/build/x86_64-unknown-linux-gnu/stage1/lib/rustlib/x86_64-unknown-linux-gnu/bin/llvm-profdata merge -sparse file.profraw -o file.profdata
$ /home/martin/src/rust/build/x86_64-unknown-linux-gnu/stage1/lib/rustlib/x86_64-unknown-linux-gnu/bin/llvm-cov show target/release/hello-world --sources /home/martin/src/rust/build/x86_64-unknown-linux-gnu/stage1/lib/rustlib/src/rust/library/std/src/fs.rs --instr-profile file.profdata --color | grep -A 3 'pub fn write(&mut self, write: bool) -> &mut Self {'
 1357|      1|    pub fn write(&mut self, write: bool) -> &mut Self {
 1358|      1|        self.0.write(write);
 1359|      1|        self
 1360|      1|    }
```
</summary>
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Nov 24, 2024
Rollup merge of rust-lang#133300 - Enselic:build-std-instrument-coverage, r=jieyouxu

inject_panic_runtime(): Avoid double negation for 'any non rlib'

<details>

<summary>This PR originally did more things .Click to expand to see.</summary>

By not trying to inject a profiler runtime when only building an rlib. This logic already exists for the panic runtime.

This makes

    RUSTFLAGS="-Cinstrument-coverage" cargo build -Zbuild-std=std,profiler_builtins

work. Note that you probably also need
`RUST_COMPILER_RT_FOR_PROFILER=$src/llvm-project/compiler-rt` in your environment.

cc rust-lang#79401

# Demonstration

Before this fix you get these errors:

```console
$ rm -rf target ; RUST_COMPILER_RT_FOR_PROFILER=/home/martin/src/llvm-project/compiler-rt RUSTFLAGS="-Cinstrument-coverage" cargo +nightly build --release -Zbuild-std=std,profiler_builtins
error: `profiler_builtins` crate (required by compiler options) is not compatible with crate attribute `#![no_core]`
error[E0152]: found duplicate lang item `manually_drop`
    = note: first definition in `core` loaded from /home/martin/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/lib/libcore-d453bab70303062c.rlib
    = note: second definition in the local crate (`core`)
```

With the fix the build succeeds:

```console
$ rm -rf target ; RUST_COMPILER_RT_FOR_PROFILER=/home/martin/src/llvm-project/compiler-rt RUSTFLAGS="-Cinstrument-coverage" cargo +stage1 build --release -Zbuild-std=std,profiler_builtins
    Finished `release` profile [optimized] target(s) in 45.57s
```

And we can check code coverage. My example program looks like this:

```rs
fn main() {
    if std::env::args_os().nth(1) == Some("write-file".into()) {
        std::fs::write("hello.txt", "Hello, world!").unwrap();
    } else {
        println!("Hello, world!");
    }
}
```

when the program prints to stdout:

```
$ LLVM_PROFILE_FILE=stdout.profraw ./target/release/hello-world
Hello, world!
```

we can see that `fs::write()` is not being used (note the `0`'s):

```console
$ /home/martin/src/rust/build/x86_64-unknown-linux-gnu/stage1/lib/rustlib/x86_64-unknown-linux-gnu/bin/llvm-profdata merge -sparse stdout.profraw -o stdout.profdata
$ /home/martin/src/rust/build/x86_64-unknown-linux-gnu/stage1/lib/rustlib/x86_64-unknown-linux-gnu/bin/llvm-cov show target/release/hello-world --sources /home/martin/src/rust/build/x86_64-unknown-linux-gnu/stage1/lib/rustlib/src/rust/library/std/src/fs.rs --instr-profile stdout.profdata --color | grep -A 3 'pub fn write(&mut self, write: b
ool) -> &mut Self {'
 1357|      0|    pub fn write(&mut self, write: bool) -> &mut Self {
 1358|      0|        self.0.write(write);
 1359|      0|        self
 1360|      0|    }
```

but when we print to a file:

```console
$ LLVM_PROFILE_FILE=file.profraw ./target/release/hello-world write-file
```

the code coverage shows `fs::write()` as being used (note the `1`'s):

```console
$ /home/martin/src/rust/build/x86_64-unknown-linux-gnu/stage1/lib/rustlib/x86_64-unknown-linux-gnu/bin/llvm-profdata merge -sparse file.profraw -o file.profdata
$ /home/martin/src/rust/build/x86_64-unknown-linux-gnu/stage1/lib/rustlib/x86_64-unknown-linux-gnu/bin/llvm-cov show target/release/hello-world --sources /home/martin/src/rust/build/x86_64-unknown-linux-gnu/stage1/lib/rustlib/src/rust/library/std/src/fs.rs --instr-profile file.profdata --color | grep -A 3 'pub fn write(&mut self, write: bool) -> &mut Self {'
 1357|      1|    pub fn write(&mut self, write: bool) -> &mut Self {
 1358|      1|        self.0.write(write);
 1359|      1|        self
 1360|      1|    }
```
</summary>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-Zbuild-std Unstable Cargo option: Compile the standard library yourself. A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants