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 2 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
4 changes: 2 additions & 2 deletions compiler/rustc_interface/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -561,7 +561,7 @@ fn test_debugging_options_tracking_hash() {
tracked!(link_only, true);
tracked!(merge_functions, Some(MergeFunctions::Disabled));
tracked!(mir_emit_retag, true);
tracked!(mir_opt_level, 3);
tracked!(mir_opt_level, Some(3));
tracked!(mutable_noalias, true);
tracked!(new_llvm_pass_manager, true);
tracked!(no_codegen, true);
Expand All @@ -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
7 changes: 6 additions & 1 deletion compiler/rustc_metadata/src/rmeta/encoder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -663,7 +663,12 @@ 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
.symbol_mangling_version
.unwrap_or(SymbolManglingVersion::default()),

crate_deps,
dylib_dependency_formats,
Expand Down
8 changes: 5 additions & 3 deletions compiler/rustc_mir/src/transform/const_prop.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ use rustc_middle::ty::subst::{InternalSubsts, Subst};
use rustc_middle::ty::{
self, ConstInt, ConstKind, Instance, ParamEnv, ScalarInt, Ty, TyCtxt, TypeFoldable,
};
use rustc_session::config::MIR_OPT_LEVEL_DEFAULT;
use rustc_session::lint;
use rustc_span::{def_id::DefId, Span};
use rustc_target::abi::{HasDataLayout, LayoutOf, Size, TargetDataLayout};
Expand Down Expand Up @@ -708,7 +709,7 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> {
return None;
}

if self.tcx.sess.opts.debugging_opts.mir_opt_level >= 3 {
if self.tcx.sess.opts.debugging_opts.mir_opt_level.unwrap_or(MIR_OPT_LEVEL_DEFAULT) >= 3 {
richkadel marked this conversation as resolved.
Show resolved Hide resolved
self.eval_rvalue_with_identities(rvalue, place)
} else {
self.use_ecx(|this| this.ecx.eval_rvalue_into_place(rvalue, place))
Expand Down Expand Up @@ -886,7 +887,8 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> {

/// Returns `true` if and only if this `op` should be const-propagated into.
fn should_const_prop(&mut self, op: OpTy<'tcx>) -> bool {
let mir_opt_level = self.tcx.sess.opts.debugging_opts.mir_opt_level;
let mir_opt_level =
self.tcx.sess.opts.debugging_opts.mir_opt_level.unwrap_or(MIR_OPT_LEVEL_DEFAULT);

if mir_opt_level == 0 {
return false;
Expand Down Expand Up @@ -1056,7 +1058,7 @@ impl<'mir, 'tcx> MutVisitor<'tcx> for ConstPropagator<'mir, 'tcx> {

// Only const prop copies and moves on `mir_opt_level=2` as doing so
// currently slightly increases compile time in some cases.
if self.tcx.sess.opts.debugging_opts.mir_opt_level >= 2 {
if self.tcx.sess.opts.debugging_opts.mir_opt_level.unwrap_or(MIR_OPT_LEVEL_DEFAULT) >= 2 {
self.propagate_operand(operand)
}
}
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
3 changes: 2 additions & 1 deletion compiler/rustc_mir/src/transform/dest_prop.rs
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,7 @@ use rustc_middle::mir::{
Rvalue, Statement, StatementKind, Terminator, TerminatorKind,
};
use rustc_middle::ty::{self, Ty, TyCtxt};
use rustc_session::config::MIR_OPT_LEVEL_DEFAULT;

// Empirical measurements have resulted in some observations:
// - Running on a body with a single block and 500 locals takes barely any time
Expand All @@ -129,7 +130,7 @@ impl<'tcx> MirPass<'tcx> for DestinationPropagation {
fn run_pass(&self, tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) {
// Only run at mir-opt-level=2 or higher for now (we don't fix up debuginfo and remove
// storage statements at the moment).
if tcx.sess.opts.debugging_opts.mir_opt_level <= 1 {
if tcx.sess.opts.debugging_opts.mir_opt_level.unwrap_or(MIR_OPT_LEVEL_DEFAULT) <= 1 {
return;
}

Expand Down
3 changes: 2 additions & 1 deletion compiler/rustc_mir/src/transform/early_otherwise_branch.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
use crate::{transform::MirPass, util::patch::MirPatch};
use rustc_middle::mir::*;
use rustc_middle::ty::{Ty, TyCtxt};
use rustc_session::config::MIR_OPT_LEVEL_DEFAULT;
use std::fmt::Debug;

use super::simplify::simplify_cfg;
Expand All @@ -26,7 +27,7 @@ pub struct EarlyOtherwiseBranch;

impl<'tcx> MirPass<'tcx> for EarlyOtherwiseBranch {
fn run_pass(&self, tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) {
if tcx.sess.opts.debugging_opts.mir_opt_level < 2 {
if tcx.sess.opts.debugging_opts.mir_opt_level.unwrap_or(MIR_OPT_LEVEL_DEFAULT) < 2 {
return;
}
trace!("running EarlyOtherwiseBranch on {:?}", body.source);
Expand Down
11 changes: 2 additions & 9 deletions compiler/rustc_mir/src/transform/inline.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ use rustc_middle::mir::visit::*;
use rustc_middle::mir::*;
use rustc_middle::ty::subst::Subst;
use rustc_middle::ty::{self, ConstKind, Instance, InstanceDef, ParamEnv, Ty, TyCtxt};
use rustc_session::config::MIR_OPT_LEVEL_DEFAULT;
use rustc_span::{hygiene::ExpnKind, ExpnData, Span};
use rustc_target::spec::abi::Abi;

Expand Down Expand Up @@ -37,15 +38,7 @@ struct CallSite<'tcx> {

impl<'tcx> MirPass<'tcx> for Inline {
fn run_pass(&self, tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) {
if tcx.sess.opts.debugging_opts.mir_opt_level < 2 {
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`");
if tcx.sess.opts.debugging_opts.mir_opt_level.unwrap_or(MIR_OPT_LEVEL_DEFAULT) < 2 {
return;
}

Expand Down
3 changes: 2 additions & 1 deletion compiler/rustc_mir/src/transform/match_branches.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
use crate::transform::MirPass;
use rustc_middle::mir::*;
use rustc_middle::ty::TyCtxt;
use rustc_session::config::MIR_OPT_LEVEL_DEFAULT;

pub struct MatchBranchSimplification;

Expand Down Expand Up @@ -38,7 +39,7 @@ pub struct MatchBranchSimplification;

impl<'tcx> MirPass<'tcx> for MatchBranchSimplification {
fn run_pass(&self, tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) {
if tcx.sess.opts.debugging_opts.mir_opt_level <= 1 {
if tcx.sess.opts.debugging_opts.mir_opt_level.unwrap_or(MIR_OPT_LEVEL_DEFAULT) <= 1 {
return;
}

Expand Down
3 changes: 2 additions & 1 deletion compiler/rustc_mir/src/transform/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ use rustc_middle::mir::visit::Visitor as _;
use rustc_middle::mir::{traversal, Body, ConstQualifs, MirPhase, Promoted};
use rustc_middle::ty::query::Providers;
use rustc_middle::ty::{self, TyCtxt, TypeFoldable};
use rustc_session::config::MIR_OPT_LEVEL_DEFAULT;
use rustc_span::{Span, Symbol};
use std::borrow::Cow;

Expand Down Expand Up @@ -373,7 +374,7 @@ fn run_post_borrowck_cleanup_passes<'tcx>(tcx: TyCtxt<'tcx>, body: &mut Body<'tc
}

fn run_optimization_passes<'tcx>(tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) {
let mir_opt_level = tcx.sess.opts.debugging_opts.mir_opt_level;
let mir_opt_level = tcx.sess.opts.debugging_opts.mir_opt_level.unwrap_or(MIR_OPT_LEVEL_DEFAULT);

// Lowering generator control-flow and variables has to happen before we do anything else
// to them. We run some optimizations before that, because they may be harder to do on the state
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,13 @@ use crate::transform::{simplify, MirPass};
use rustc_index::bit_set::BitSet;
use rustc_middle::mir::*;
use rustc_middle::ty::TyCtxt;
use rustc_session::config::MIR_OPT_LEVEL_DEFAULT;

pub struct MultipleReturnTerminators;

impl<'tcx> MirPass<'tcx> for MultipleReturnTerminators {
fn run_pass(&self, tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) {
if tcx.sess.opts.debugging_opts.mir_opt_level < 3 {
if tcx.sess.opts.debugging_opts.mir_opt_level.unwrap_or(MIR_OPT_LEVEL_DEFAULT) < 3 {
return;
}

Expand Down
3 changes: 2 additions & 1 deletion compiler/rustc_mir/src/transform/nrvo.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ use rustc_index::bit_set::HybridBitSet;
use rustc_middle::mir::visit::{MutVisitor, NonUseContext, PlaceContext, Visitor};
use rustc_middle::mir::{self, BasicBlock, Local, Location};
use rustc_middle::ty::TyCtxt;
use rustc_session::config::MIR_OPT_LEVEL_DEFAULT;

use crate::transform::MirPass;

Expand Down Expand Up @@ -34,7 +35,7 @@ pub struct RenameReturnPlace;

impl<'tcx> MirPass<'tcx> for RenameReturnPlace {
fn run_pass(&self, tcx: TyCtxt<'tcx>, body: &mut mir::Body<'tcx>) {
if tcx.sess.opts.debugging_opts.mir_opt_level == 0 {
if tcx.sess.opts.debugging_opts.mir_opt_level.unwrap_or(MIR_OPT_LEVEL_DEFAULT) == 0 {
return;
}

Expand Down
3 changes: 2 additions & 1 deletion compiler/rustc_mir/src/transform/unreachable_prop.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,13 @@ use crate::transform::MirPass;
use rustc_data_structures::fx::{FxHashMap, FxHashSet};
use rustc_middle::mir::*;
use rustc_middle::ty::TyCtxt;
use rustc_session::config::MIR_OPT_LEVEL_DEFAULT;

pub struct UnreachablePropagation;

impl MirPass<'_> for UnreachablePropagation {
fn run_pass<'tcx>(&self, tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) {
if tcx.sess.opts.debugging_opts.mir_opt_level < 3 {
if tcx.sess.opts.debugging_opts.mir_opt_level.unwrap_or(MIR_OPT_LEVEL_DEFAULT) < 3 {
// Enable only under -Zmir-opt-level=3 as in some cases (check the deeply-nested-opt
// perf benchmark) LLVM may spend quite a lot of time optimizing the generated code.
return;
Expand Down
38 changes: 36 additions & 2 deletions compiler/rustc_session/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,8 @@ pub enum MirSpanview {
Block,
}

pub const MIR_OPT_LEVEL_DEFAULT: usize = 1;

#[derive(Clone, PartialEq, Hash)]
pub enum LinkerPluginLto {
LinkerPlugin(PathBuf),
Expand Down Expand Up @@ -212,6 +214,12 @@ pub enum SymbolManglingVersion {
V0,
}

impl SymbolManglingVersion {
pub fn default() -> Self {
SymbolManglingVersion::Legacy
}
}

impl_stable_hash_via_hash!(SymbolManglingVersion);

#[derive(Clone, Copy, Debug, PartialEq, Hash)]
Expand Down Expand Up @@ -1757,7 +1765,33 @@ 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) => {}
}

match debugging_opts.mir_opt_level {
Some(level) if 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`.",
level,
),
);
}
_ => {}
}
}

if let Ok(graphviz_font) = std::env::var("RUSTC_GRAPHVIZ_FONT") {
Expand Down Expand Up @@ -2162,7 +2196,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
12 changes: 6 additions & 6 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 @@ -970,7 +970,7 @@ options! {DebuggingOptions, DebuggingSetter, basic_debugging_options,
mir_emit_retag: bool = (false, parse_bool, [TRACKED],
"emit Retagging MIR statements, interpreted e.g., by miri; implies -Zmir-opt-level=0 \
(default: no)"),
mir_opt_level: usize = (1, parse_uint, [TRACKED],
mir_opt_level: Option<usize> = (None, parse_opt_uint, [TRACKED],
Copy link
Member

Choose a reason for hiding this comment

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

I don't see a reason for making this an Option, it doesn't change its default dependent on coverage like symbol mangling does.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know of any other way to know if the option was set on the command line or not.

I need to know that in config.rs when checking for conflicting command line flags.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I understand your point... Let me think about this a minute. I'm inclined to agree.

Copy link
Member

Choose a reason for hiding this comment

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

To me the value matters more than if it was overridden (which you know it was today if it's > 1).. if the default were changed to 2, we would still want this to fire to warn people who were using coverage. (Probably we'd either keep the default as 1 if coverage were enabled, or add support for inlining counters. But it's unlikely that we turn on inlining by default anytime soon.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, yes, I concur. I'll remove the option from mir_opt_level. Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if the default were changed to 2, we would still want this to fire to warn people who were using coverage.

I agree 100%. Thanks.

"MIR optimization level (0-3; default: 1)"),
mutable_noalias: bool = (false, parse_bool, [TRACKED],
"emit noalias metadata for mutable references (default: no)"),
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
Loading