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

Fixes reported bugs in Rust Coverage #79958

Merged
merged 3 commits into from
Dec 15, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion compiler/rustc_interface/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -587,7 +587,7 @@ fn test_debugging_options_tracking_hash() {
tracked!(share_generics, Some(true));
tracked!(show_span, Some(String::from("abc")));
tracked!(src_hash_algorithm, Some(SourceFileHashAlgorithm::Sha1));
tracked!(symbol_mangling_version, SymbolManglingVersion::V0);
tracked!(symbol_mangling_version, Some(SymbolManglingVersion::V0));
tracked!(teach, true);
tracked!(thinlto, Some(true));
tracked!(tune_cpu, Some(String::from("abc")));
Expand Down
11 changes: 9 additions & 2 deletions compiler/rustc_metadata/src/creader.rs
Original file line number Diff line number Diff line change
Expand Up @@ -706,14 +706,21 @@ impl<'a> CrateLoader<'a> {
self.inject_dependency_if(cnum, "a panic runtime", &|data| data.needs_panic_runtime());
}

fn inject_profiler_runtime(&mut self) {
fn inject_profiler_runtime(&mut self, krate: &ast::Crate) {
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");

if self.sess.contains_name(&krate.attrs, sym::no_core) {
self.sess.err(
"`profiler_builtins` crate (required by compiler options) \
is not compatible with crate attribute `#![no_core]`",
);
}

let name = sym::profiler_builtins;
let cnum = self.resolve_crate(name, DUMMY_SP, CrateDepKind::Implicit, None);
let data = self.cstore.get_crate_data(cnum);
Expand Down Expand Up @@ -879,7 +886,7 @@ impl<'a> CrateLoader<'a> {
}

pub fn postprocess(&mut self, krate: &ast::Crate) {
self.inject_profiler_runtime();
self.inject_profiler_runtime(krate);
self.inject_allocator_crate(krate);
self.inject_panic_runtime(krate);

Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_metadata/src/rmeta/encoder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -663,7 +663,7 @@ impl<'a, 'tcx> EncodeContext<'a, 'tcx> {
no_builtins: tcx.sess.contains_name(&attrs, sym::no_builtins),
panic_runtime: tcx.sess.contains_name(&attrs, sym::panic_runtime),
profiler_runtime: tcx.sess.contains_name(&attrs, sym::profiler_runtime),
symbol_mangling_version: tcx.sess.opts.debugging_opts.symbol_mangling_version,
symbol_mangling_version: tcx.sess.opts.debugging_opts.get_symbol_mangling_version(),

crate_deps,
dylib_dependency_formats,
Expand Down
28 changes: 16 additions & 12 deletions compiler/rustc_mir/src/transform/coverage/graph.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,24 +32,28 @@ impl CoverageGraph {

// Pre-transform MIR `BasicBlock` successors and predecessors into the BasicCoverageBlock
// equivalents. Note that since the BasicCoverageBlock graph has been fully simplified, the
// each predecessor of a BCB leader_bb should be in a unique BCB, and each successor of a
// BCB last_bb should be in its own unique BCB. Therefore, collecting the BCBs using
// `bb_to_bcb` should work without requiring a deduplication step.
// each predecessor of a BCB leader_bb should be in a unique BCB. It is possible for a
// `SwitchInt` to have multiple targets to the same destination `BasicBlock`, so
// de-duplication is required. This is done without reordering the successors.

let bcbs_len = bcbs.len();
let mut seen = IndexVec::from_elem_n(false, bcbs_len);
let successors = IndexVec::from_fn_n(
|bcb| {
for b in seen.iter_mut() {
*b = false;
}
let bcb_data = &bcbs[bcb];
let bcb_successors =
let mut bcb_successors = Vec::new();
for successor in
bcb_filtered_successors(&mir_body, &bcb_data.terminator(mir_body).kind)
.filter_map(|&successor_bb| bb_to_bcb[successor_bb])
.collect::<Vec<_>>();
debug_assert!({
let mut sorted = bcb_successors.clone();
sorted.sort_unstable();
let initial_len = sorted.len();
sorted.dedup();
sorted.len() == initial_len
});
{
if !seen[successor] {
seen[successor] = true;
bcb_successors.push(successor);
}
}
bcb_successors
},
bcbs.len(),
Expand Down
8 changes: 8 additions & 0 deletions compiler/rustc_mir/src/transform/coverage/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,14 @@ impl<'tcx> MirPass<'tcx> for InstrumentCoverage {
return;
}

match mir_body.basic_blocks()[mir::START_BLOCK].terminator().kind {
TerminatorKind::Unreachable => {
trace!("InstrumentCoverage skipped for unreachable `START_BLOCK`");
return;
}
_ => {}
}

trace!("InstrumentCoverage starting for {:?}", mir_source.def_id());
Instrumentor::new(&self.name(), tcx, mir_body).inject_counters();
trace!("InstrumentCoverage starting for {:?}", mir_source.def_id());
Expand Down
8 changes: 0 additions & 8 deletions compiler/rustc_mir/src/transform/inline.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,14 +41,6 @@ impl<'tcx> MirPass<'tcx> for Inline {
return;
}

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;
}

if inline(tcx, body) {
debug!("running simplify cfg on {:?}", body.source);
CfgSimplifier::new(body).simplify();
Expand Down
31 changes: 29 additions & 2 deletions compiler/rustc_session/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -692,6 +692,10 @@ impl DebuggingOptions {
deduplicate_diagnostics: self.deduplicate_diagnostics,
}
}

pub fn get_symbol_mangling_version(&self) -> SymbolManglingVersion {
self.symbol_mangling_version.unwrap_or(SymbolManglingVersion::Legacy)
}
}

// The type of entry function, so users can have their own entry functions
Expand Down Expand Up @@ -1757,7 +1761,30 @@ pub fn build_session_options(matches: &getopts::Matches) -> Options {
// and reversible name mangling. Note, LLVM coverage tools can analyze coverage over
// multiple runs, including some changes to source code; so mangled names must be consistent
// across compilations.
debugging_opts.symbol_mangling_version = SymbolManglingVersion::V0;
match debugging_opts.symbol_mangling_version {
None => {
debugging_opts.symbol_mangling_version = Some(SymbolManglingVersion::V0);
}
Some(SymbolManglingVersion::Legacy) => {
early_warn(
error_format,
"-Z instrument-coverage requires symbol mangling version `v0`, \
but `-Z symbol-mangling-version=legacy` was specified",
);
}
Some(SymbolManglingVersion::V0) => {}
}

if debugging_opts.mir_opt_level > 1 {
early_warn(
error_format,
&format!(
"`-Z mir-opt-level={}` (any level > 1) enables function inlining, which \
limits the effectiveness of `-Z instrument-coverage`.",
debugging_opts.mir_opt_level,
),
);
}
}

if let Ok(graphviz_font) = std::env::var("RUSTC_GRAPHVIZ_FONT") {
Expand Down Expand Up @@ -2162,7 +2189,7 @@ crate mod dep_tracking {
impl_dep_tracking_hash_via_hash!(Edition);
impl_dep_tracking_hash_via_hash!(LinkerPluginLto);
impl_dep_tracking_hash_via_hash!(SwitchWithOptPath);
impl_dep_tracking_hash_via_hash!(SymbolManglingVersion);
impl_dep_tracking_hash_via_hash!(Option<SymbolManglingVersion>);
impl_dep_tracking_hash_via_hash!(Option<SourceFileHashAlgorithm>);
impl_dep_tracking_hash_via_hash!(TrimmedDefPaths);

Expand Down
10 changes: 5 additions & 5 deletions compiler/rustc_session/src/options.rs
Original file line number Diff line number Diff line change
Expand Up @@ -677,12 +677,12 @@ macro_rules! options {
}

fn parse_symbol_mangling_version(
slot: &mut SymbolManglingVersion,
slot: &mut Option<SymbolManglingVersion>,
v: Option<&str>,
) -> bool {
*slot = match v {
Some("legacy") => SymbolManglingVersion::Legacy,
Some("v0") => SymbolManglingVersion::V0,
Some("legacy") => Some(SymbolManglingVersion::Legacy),
Some("v0") => Some(SymbolManglingVersion::V0),
_ => return false,
};
true
Expand Down Expand Up @@ -1088,9 +1088,9 @@ options! {DebuggingOptions, DebuggingSetter, basic_debugging_options,
"hash algorithm of source files in debug info (`md5`, `sha1`, or `sha256`)"),
strip: Strip = (Strip::None, parse_strip, [UNTRACKED],
"tell the linker which information to strip (`none` (default), `debuginfo` or `symbols`)"),
symbol_mangling_version: SymbolManglingVersion = (SymbolManglingVersion::Legacy,
symbol_mangling_version: Option<SymbolManglingVersion> = (None,
parse_symbol_mangling_version, [TRACKED],
"which mangling version to use for symbol names"),
"which mangling version to use for symbol names ('legacy' (default) or 'v0')"),
teach: bool = (false, parse_bool, [TRACKED],
"show extended diagnostic help (default: no)"),
terminal_width: Option<usize> = (None, parse_opt_uint, [UNTRACKED],
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_symbol_mangling/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -245,7 +245,7 @@ fn compute_symbol_name(
// 2. we favor `instantiating_crate` where possible (i.e. when `Some`)
let mangling_version_crate = instantiating_crate.unwrap_or(def_id.krate);
let mangling_version = if mangling_version_crate == LOCAL_CRATE {
tcx.sess.opts.debugging_opts.symbol_mangling_version
tcx.sess.opts.debugging_opts.get_symbol_mangling_version()
} else {
tcx.symbol_mangling_version(mangling_version_crate)
};
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
{
"data": [
{
"files": [
{
"filename": "../coverage/match_or_pattern.rs",
"summary": {
"functions": {
"count": 1,
"covered": 1,
"percent": 100
},
"instantiations": {
"count": 1,
"covered": 1,
"percent": 100
},
"lines": {
"count": 37,
"covered": 33,
"percent": 89.1891891891892
},
"regions": {
"count": 25,
"covered": 17,
"notcovered": 8,
"percent": 68
}
}
}
],
"totals": {
"functions": {
"count": 1,
"covered": 1,
"percent": 100
},
"instantiations": {
"count": 1,
"covered": 1,
"percent": 100
},
"lines": {
"count": 37,
"covered": 33,
"percent": 89.1891891891892
},
"regions": {
"count": 25,
"covered": 17,
"notcovered": 8,
"percent": 68
}
}
}
],
"type": "llvm.coverage.json.export",
"version": "2.0.1"
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
1| |#![feature(or_patterns)]
2| |
3| 1|fn main() {
4| 1| // Initialize test constants in a way that cannot be determined at compile time, to ensure
5| 1| // rustc and LLVM cannot optimize out statements (or coverage counters) downstream from
6| 1| // dependent conditions.
7| 1| let is_true = std::env::args().len() == 1;
8| 1|
9| 1| let mut a: u8 = 0;
10| 1| let mut b: u8 = 0;
11| 1| if is_true {
12| 1| a = 2;
13| 1| b = 0;
14| 1| }
^0
15| 1| match (a, b) {
16| | // Or patterns generate MIR `SwitchInt` with multiple targets to the same `BasicBlock`.
17| | // This test confirms a fix for Issue #79569.
18| 0| (0 | 1, 2 | 3) => {}
19| 1| _ => {}
20| | }
21| 1| if is_true {
22| 1| a = 0;
23| 1| b = 0;
24| 1| }
^0
25| 1| match (a, b) {
26| 0| (0 | 1, 2 | 3) => {}
27| 1| _ => {}
28| | }
29| 1| if is_true {
30| 1| a = 2;
31| 1| b = 2;
32| 1| }
^0
33| 1| match (a, b) {
34| 0| (0 | 1, 2 | 3) => {}
35| 1| _ => {}
36| | }
37| 1| if is_true {
38| 1| a = 0;
39| 1| b = 2;
40| 1| }
^0
41| 1| match (a, b) {
42| 1| (0 | 1, 2 | 3) => {}
43| 0| _ => {}
44| | }
45| 1|}

Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,8 @@ Counter in file 0 79:14 -> 79:16, 0
Counter in file 0 81:1 -> 81:2, 0
Counter in file 0 91:25 -> 91:34, 0
Counter in file 0 5:1 -> 5:25, #1
Counter in file 0 5:25 -> 6:14, #1
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Counter in file 0 7:9 -> 7:10, #2
Counter in file 0 9:9 -> 9:10, (#1 - #2)
Counter in file 0 11:1 -> 11:2, (#2 + (#1 - #2))
Counter in file 0 21:1 -> 21:23, #1
Counter in file 0 17:20 -> 17:21, #1
Counter in file 0 67:5 -> 67:23, #1
Counter in file 0 38:1 -> 38:19, #1
Counter in file 0 38:19 -> 42:12, #1
Expand All @@ -46,14 +43,18 @@ Counter in file 0 44:27 -> 44:32, #8
Counter in file 0 44:36 -> 44:38, (#6 + 0)
Counter in file 0 45:14 -> 45:16, #7
Counter in file 0 47:1 -> 47:2, (#5 + (#6 + #7))
Counter in file 0 13:20 -> 13:21, #1
Counter in file 0 29:1 -> 29:22, #1
Counter in file 0 93:1 -> 101:2, #1
Counter in file 0 91:1 -> 91:25, #1
Counter in file 0 5:25 -> 6:14, #1
Counter in file 0 7:9 -> 7:10, #2
Counter in file 0 9:9 -> 9:10, (#1 - #2)
Counter in file 0 11:1 -> 11:2, (#2 + (#1 - #2))
Counter in file 0 51:5 -> 52:18, #1
Counter in file 0 53:13 -> 53:14, #2
Counter in file 0 63:13 -> 63:14, (#1 - #2)
Counter in file 0 65:5 -> 65:6, (#2 + (#1 - #2))
Counter in file 0 17:20 -> 17:21, #1
Counter in file 0 49:1 -> 68:12, #1
Counter in file 0 69:9 -> 69:10, #2
Counter in file 0 69:14 -> 69:27, (#1 + 0)
Expand All @@ -70,7 +71,6 @@ Counter in file 0 87:14 -> 87:16, #3
Counter in file 0 89:1 -> 89:2, (#3 + (#2 + (#1 - (#3 + #2))))
Counter in file 0 17:1 -> 17:20, #1
Counter in file 0 66:5 -> 66:23, #1
Counter in file 0 13:20 -> 13:21, #1
Counter in file 0 17:9 -> 17:10, #1
Counter in file 0 17:9 -> 17:10, #1
Counter in file 0 117:17 -> 117:19, #1
Expand Down
Loading