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

Our last ICE before we get coverage working #78286

Closed
gilescope opened this issue Oct 23, 2020 · 10 comments
Closed

Our last ICE before we get coverage working #78286

gilescope opened this issue Oct 23, 2020 · 10 comments
Labels
A-codegen Area: Code generation C-bug Category: This is a bug. E-needs-mcve Call for participation: This issue has a repro, but needs a Minimal Complete and Verifiable Example I-ICE Issue: The compiler panicked, giving an Internal Compilation Error (ICE) ❄️ T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@gilescope
Copy link
Contributor

gilescope commented Oct 23, 2020

Thanks for all the work you've recently been doing to coverage - we seem to have one last hurdle before all our tests (closed source) pass with coverage enabled. (yes we use proc macros)

RUSTCFLAGS = -Zinstrument-coverage -Zprofile -Ccodegen-units=1 -Copt-level=0 -Clink-dead-code -Coverflow-checks=off
CARGO_INCREMENTAL=0
RUSTDOCFLAGS=-Cpanic=abort

cargo test --message-format=json -Ztimings=json,html,info -- -Zunstable-options --format json

test failed
thread 'rustc' panicked at 'called `Option::unwrap()` on a `None` value', compiler/rustc_codegen_llvm/src/context.rs:328:35
stack backtrace:
0: rust_begin_unwind
at /rustc/043eca7f0b34d12e61c44206beca740628647080/library/std/src/panicking.rs:483:5
1: core::panicking::panic_fmt
at /rustc/043eca7f0b34d12e61c44206beca740628647080/library/core/src/panicking.rs:85:14
2: core::panicking::panic
at /rustc/043eca7f0b34d12e61c44206beca740628647080/library/core/src/panicking.rs:50:5
3: rustc_codegen_llvm::coverageinfo::<impl rustc_codegen_ssa::traits::coverageinfo::CoverageInfoBuilderMethods for rustc_codegen_llvm::builder::Builder>::add_counter_region
4: rustc_codegen_ssa::mir::block::<impl rustc_codegen_ssa::mir::FunctionCx<Bx>>::codegen_block
5: rustc_codegen_ssa::mir::codegen_mir
6: rustc_codegen_ssa::base::codegen_instance
7: <rustc_middle::mir::mono::MonoItem as rustc_codegen_ssa::mono_item::MonoItemExt>::define
8: rustc_codegen_llvm::base::compile_codegen_unit::module_codegen
9: rustc_query_system::dep_graph::graph::DepGraph<K>::with_task
10: rustc_codegen_llvm::base::compile_codegen_unit
11: rustc_codegen_ssa::base::codegen_crate
12: <rustc_codegen_llvm::LlvmCodegenBackend as rustc_codegen_ssa::traits::backend::CodegenBackend>::codegen_crate
13: rustc_session::utils::<impl rustc_session::session::Session>::time
14: rustc_interface::queries::Queries::ongoing_codegen
15: rustc_interface::queries::<impl rustc_interface::interface::Compiler>::enter
16: rustc_span::with_source_map
17: scoped_tls::ScopedKey<T>::set
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
error: internal compiler error: unexpected panic
note: the compiler unexpectedly panicked. this is a bug.
note: rustc 1.49.0-nightly (043eca7f0 2020-10-17) running on x86_64-unknown-linux-gnu
note: compiler flags: -C codegen-units=1 -C embed-bitcode=no -C panic=abort --crate-type bin
query stack during panic:
end of query stack
Couldn't compile the test.
@gilescope gilescope added C-bug Category: This is a bug. I-ICE Issue: The compiler panicked, giving an Internal Compilation Error (ICE) ❄️ T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 23, 2020
@gilescope
Copy link
Contributor Author

@richkadel might be interested in this?

self.coverage_cx.as_ref().unwrap()

I don't know how much help this report is... but I appreciate all the work you've been doing.

@jyn514 jyn514 added the E-needs-mcve Call for participation: This issue has a repro, but needs a Minimal Complete and Verifiable Example label Oct 23, 2020
@jyn514
Copy link
Member

jyn514 commented Oct 23, 2020

@gilescope you said the original code is private - could you put together a minimal reproduction?

@jyn514 jyn514 added the A-codegen Area: Code generation label Oct 23, 2020
@gilescope
Copy link
Contributor Author

That's a very good question. Not sure - it's not that minimal at the moment.. on the plus side this issue is affecting 1% of the tests, so we're really very close to everything working...

tcx.sess.opts.debugging_opts.instrument_coverage - it looks like the code thought coverage was not turned on but then later it tried to log some coverage.

@richkadel
Copy link
Contributor

My best guess is you have a library compiled with coverage and are compiling a binary without the -Zinstrument-coverage flag. The coverage statements in the library are encountered during codegen, which assumes coverage is enabled.

Assuming this is the scenario, this should be an easy fix.

I'll put a PR together.

FYI @wesleywiser @tmandry

richkadel added a commit to richkadel/rust that referenced this issue Oct 23, 2020
Addresses Issue rust-lang#78286

Libraries compiled with coverage and linked with out enabling coverage
would fail when attempting to add the library's coverage statements to
the codegen coverage context (None).

Now, if coverage statements are encountered while compiling / linking
with `-Z instrument-coverage` disabled, codegen will *not* attempt to
add code regions to a coverage map, and it will not inject the LLVM
instrprof_increment intrinsic calls.
@richkadel
Copy link
Contributor

@gilescope I think the PR I just uploaded should fix this issue.

@wesleywiser - I hope you don't mind, I set you as the reviewer. OK?

@wesleywiser
Copy link
Member

@richkadel Yep, totally fine!

@richkadel
Copy link
Contributor

@gilescope The proposed fix has been merged. Can you rebase and retest and verify this resolves the issue?

@gilescope
Copy link
Contributor Author

I'd love to be in a position to tell you if it works or not but because of reasons I can't give you a definitive answer one way or another at the moment. The changes look good to me though - much appreciated and apologies I can't give you a definite answer yet.

@richkadel
Copy link
Contributor

Ok, I understand. Should we close this issue and reopen a new issue if the problem returns?

@gilescope
Copy link
Contributor Author

Yes I think best close this - thanks very much for the fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-codegen Area: Code generation C-bug Category: This is a bug. E-needs-mcve Call for participation: This issue has a repro, but needs a Minimal Complete and Verifiable Example I-ICE Issue: The compiler panicked, giving an Internal Compilation Error (ICE) ❄️ T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

4 participants