Skip to content

Commit

Permalink
Improve warnings on incompatible options involving -Zinstrument-coverage
Browse files Browse the repository at this point in the history
Adds checks for:

* `no_core` attribute
* explicitly-enabled `legacy` symbol mangling
* mir_opt_level > 1 (which enables inlining)

I removed code from the `Inline` MIR pass that forcibly disabled
inlining if `-Zinstrument-coverage` was set. The default `mir_opt_level`
does not enable inlining anyway. But if the level is explicitly set and
is greater than 1, I issue a warning.

The new warnings show up in tests, which is much better for diagnosing
potential option conflicts in these cases.
  • Loading branch information
richkadel committed Dec 14, 2020
1 parent eb963ff commit 4f550f1
Show file tree
Hide file tree
Showing 16 changed files with 103 additions and 49 deletions.
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 {
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
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],
"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
6 changes: 5 additions & 1 deletion compiler/rustc_symbol_mangling/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -245,7 +245,11 @@ 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
.symbol_mangling_version
.unwrap_or(SymbolManglingVersion::default())
} else {
tcx.symbol_mangling_version(mangling_version_crate)
};
Expand Down
Loading

0 comments on commit 4f550f1

Please sign in to comment.