Skip to content

Commit

Permalink
Auto merge of rust-lang#123128 - GuillaumeGomez:rollup-3l3zu6s, r=Gui…
Browse files Browse the repository at this point in the history
…llaumeGomez

Rollup of 6 pull requests

Successful merges:

 - rust-lang#121843 (Implement `-L KIND=`@RUSTC_BUILTIN/...`)`
 - rust-lang#122860 (coverage: Re-enable `UnreachablePropagation` for coverage builds)
 - rust-lang#123021 (Make `TyCtxt::coroutine_layout` take coroutine's kind parameter)
 - rust-lang#123024 (CFI: Enable KCFI testing of run-pass tests)
 - rust-lang#123083 (lib: fix some unnecessary_cast clippy lint)
 - rust-lang#123116 (rustdoc: Swap fields and variant documentations)

r? `@ghost`
`@rustbot` modify labels: rollup
  • Loading branch information
bors committed Mar 27, 2024
2 parents 0dcc130 + c0945f0 commit 10a7aa1
Show file tree
Hide file tree
Showing 28 changed files with 265 additions and 161 deletions.
164 changes: 92 additions & 72 deletions compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,8 @@ use crate::llvm;

use itertools::Itertools as _;
use rustc_codegen_ssa::traits::{BaseTypeMethods, ConstMethods};
use rustc_data_structures::fx::{FxIndexMap, FxIndexSet};
use rustc_hir::def::DefKind;
use rustc_hir::def_id::DefId;
use rustc_data_structures::fx::{FxHashSet, FxIndexMap, FxIndexSet};
use rustc_hir::def_id::{DefId, LocalDefId};
use rustc_index::IndexVec;
use rustc_middle::bug;
use rustc_middle::mir;
Expand Down Expand Up @@ -335,16 +334,9 @@ fn save_function_record(
);
}

/// When finalizing the coverage map, `FunctionCoverage` only has the `CodeRegion`s and counters for
/// the functions that went through codegen; such as public functions and "used" functions
/// (functions referenced by other "used" or public items). Any other functions considered unused,
/// or "Unreachable", were still parsed and processed through the MIR stage, but were not
/// codegenned. (Note that `-Clink-dead-code` can force some unused code to be codegenned, but
/// that flag is known to cause other errors, when combined with `-C instrument-coverage`; and
/// `-Clink-dead-code` will not generate code for unused generic functions.)
///
/// We can find the unused functions (including generic functions) by the set difference of all MIR
/// `DefId`s (`tcx` query `mir_keys`) minus the codegenned `DefId`s (`codegenned_and_inlined_items`).
/// Each CGU will normally only emit coverage metadata for the functions that it actually generates.
/// But since we don't want unused functions to disappear from coverage reports, we also scan for
/// functions that were instrumented but are not participating in codegen.
///
/// These unused functions don't need to be codegenned, but we do need to add them to the function
/// coverage map (in a single designated CGU) so that we still emit coverage mappings for them.
Expand All @@ -354,75 +346,109 @@ fn add_unused_functions(cx: &CodegenCx<'_, '_>) {
assert!(cx.codegen_unit.is_code_coverage_dead_code_cgu());

let tcx = cx.tcx;
let usage = prepare_usage_sets(tcx);

let is_unused_fn = |def_id: LocalDefId| -> bool {
let def_id = def_id.to_def_id();

// To be eligible for "unused function" mappings, a definition must:
// - Be function-like
// - Not participate directly in codegen (or have lost all its coverage statements)
// - Not have any coverage statements inlined into codegenned functions
tcx.def_kind(def_id).is_fn_like()
&& (!usage.all_mono_items.contains(&def_id)
|| usage.missing_own_coverage.contains(&def_id))
&& !usage.used_via_inlining.contains(&def_id)
};

let eligible_def_ids = tcx.mir_keys(()).iter().filter_map(|local_def_id| {
let def_id = local_def_id.to_def_id();
let kind = tcx.def_kind(def_id);
// `mir_keys` will give us `DefId`s for all kinds of things, not
// just "functions", like consts, statics, etc. Filter those out.
if !matches!(kind, DefKind::Fn | DefKind::AssocFn | DefKind::Closure) {
return None;
}
// Scan for unused functions that were instrumented for coverage.
for def_id in tcx.mir_keys(()).iter().copied().filter(|&def_id| is_unused_fn(def_id)) {
// Get the coverage info from MIR, skipping functions that were never instrumented.
let body = tcx.optimized_mir(def_id);
let Some(function_coverage_info) = body.function_coverage_info.as_deref() else { continue };

// FIXME(79651): Consider trying to filter out dummy instantiations of
// unused generic functions from library crates, because they can produce
// "unused instantiation" in coverage reports even when they are actually
// used by some downstream crate in the same binary.

Some(local_def_id.to_def_id())
});

let codegenned_def_ids = codegenned_and_inlined_items(tcx);

// For each `DefId` that should have coverage instrumentation but wasn't
// codegenned, add it to the function coverage map as an unused function.
for def_id in eligible_def_ids.filter(|id| !codegenned_def_ids.contains(id)) {
// Skip any function that didn't have coverage data added to it by the
// coverage instrumentor.
let body = tcx.instance_mir(ty::InstanceDef::Item(def_id));
let Some(function_coverage_info) = body.function_coverage_info.as_deref() else {
continue;
};

debug!("generating unused fn: {def_id:?}");
let instance = declare_unused_fn(tcx, def_id);
add_unused_function_coverage(cx, instance, function_coverage_info);
add_unused_function_coverage(cx, def_id, function_coverage_info);
}
}

/// All items participating in code generation together with (instrumented)
/// items inlined into them.
fn codegenned_and_inlined_items(tcx: TyCtxt<'_>) -> DefIdSet {
let (items, cgus) = tcx.collect_and_partition_mono_items(());
let mut visited = DefIdSet::default();
let mut result = items.clone();

for cgu in cgus {
for item in cgu.items().keys() {
if let mir::mono::MonoItem::Fn(ref instance) = item {
let did = instance.def_id();
if !visited.insert(did) {
continue;
}
let body = tcx.instance_mir(instance.def);
for block in body.basic_blocks.iter() {
for statement in &block.statements {
let mir::StatementKind::Coverage(_) = statement.kind else { continue };
let scope = statement.source_info.scope;
if let Some(inlined) = scope.inlined_instance(&body.source_scopes) {
result.insert(inlined.def_id());
}
}
}
struct UsageSets<'tcx> {
all_mono_items: &'tcx DefIdSet,
used_via_inlining: FxHashSet<DefId>,
missing_own_coverage: FxHashSet<DefId>,
}

/// Prepare sets of definitions that are relevant to deciding whether something
/// is an "unused function" for coverage purposes.
fn prepare_usage_sets<'tcx>(tcx: TyCtxt<'tcx>) -> UsageSets<'tcx> {
let (all_mono_items, cgus) = tcx.collect_and_partition_mono_items(());

// Obtain a MIR body for each function participating in codegen, via an
// arbitrary instance.
let mut def_ids_seen = FxHashSet::default();
let def_and_mir_for_all_mono_fns = cgus
.iter()
.flat_map(|cgu| cgu.items().keys())
.filter_map(|item| match item {
mir::mono::MonoItem::Fn(instance) => Some(instance),
mir::mono::MonoItem::Static(_) | mir::mono::MonoItem::GlobalAsm(_) => None,
})
// We only need one arbitrary instance per definition.
.filter(move |instance| def_ids_seen.insert(instance.def_id()))
.map(|instance| {
// We don't care about the instance, just its underlying MIR.
let body = tcx.instance_mir(instance.def);
(instance.def_id(), body)
});

// Functions whose coverage statments were found inlined into other functions.
let mut used_via_inlining = FxHashSet::default();
// Functions that were instrumented, but had all of their coverage statements
// removed by later MIR transforms (e.g. UnreachablePropagation).
let mut missing_own_coverage = FxHashSet::default();

for (def_id, body) in def_and_mir_for_all_mono_fns {
let mut saw_own_coverage = false;

// Inspect every coverage statement in the function's MIR.
for stmt in body
.basic_blocks
.iter()
.flat_map(|block| &block.statements)
.filter(|stmt| matches!(stmt.kind, mir::StatementKind::Coverage(_)))
{
if let Some(inlined) = stmt.source_info.scope.inlined_instance(&body.source_scopes) {
// This coverage statement was inlined from another function.
used_via_inlining.insert(inlined.def_id());
} else {
// Non-inlined coverage statements belong to the enclosing function.
saw_own_coverage = true;
}
}

if !saw_own_coverage && body.function_coverage_info.is_some() {
missing_own_coverage.insert(def_id);
}
}

result
UsageSets { all_mono_items, used_via_inlining, missing_own_coverage }
}

fn declare_unused_fn<'tcx>(tcx: TyCtxt<'tcx>, def_id: DefId) -> ty::Instance<'tcx> {
ty::Instance::new(
fn add_unused_function_coverage<'tcx>(
cx: &CodegenCx<'_, 'tcx>,
def_id: LocalDefId,
function_coverage_info: &'tcx mir::coverage::FunctionCoverageInfo,
) {
let tcx = cx.tcx;
let def_id = def_id.to_def_id();

// Make a dummy instance that fills in all generics with placeholders.
let instance = ty::Instance::new(
def_id,
ty::GenericArgs::for_item(tcx, def_id, |param, _| {
if let ty::GenericParamDefKind::Lifetime = param.kind {
Expand All @@ -431,14 +457,8 @@ fn declare_unused_fn<'tcx>(tcx: TyCtxt<'tcx>, def_id: DefId) -> ty::Instance<'tc
tcx.mk_param_from_def(param)
}
}),
)
}
);

fn add_unused_function_coverage<'tcx>(
cx: &CodegenCx<'_, 'tcx>,
instance: ty::Instance<'tcx>,
function_coverage_info: &'tcx mir::coverage::FunctionCoverageInfo,
) {
// An unused function's mappings will automatically be rewritten to map to
// zero, because none of its counters/expressions are marked as seen.
let function_coverage = FunctionCoverageCollector::unused(instance, function_coverage_info);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -683,7 +683,8 @@ fn build_union_fields_for_direct_tag_coroutine<'ll, 'tcx>(
_ => unreachable!(),
};

let coroutine_layout = cx.tcx.optimized_mir(coroutine_def_id).coroutine_layout().unwrap();
let coroutine_layout =
cx.tcx.coroutine_layout(coroutine_def_id, coroutine_args.kind_ty()).unwrap();

let common_upvar_names = cx.tcx.closure_saved_names_of_captured_variables(coroutine_def_id);
let variant_range = coroutine_args.variant_range(coroutine_def_id, cx.tcx);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,7 @@ pub(super) fn build_coroutine_di_node<'ll, 'tcx>(
unique_type_id: UniqueTypeId<'tcx>,
) -> DINodeCreationResult<'ll> {
let coroutine_type = unique_type_id.expect_ty();
let &ty::Coroutine(coroutine_def_id, _) = coroutine_type.kind() else {
let &ty::Coroutine(coroutine_def_id, coroutine_args) = coroutine_type.kind() else {
bug!("build_coroutine_di_node() called with non-coroutine type: `{:?}`", coroutine_type)
};

Expand All @@ -158,8 +158,10 @@ pub(super) fn build_coroutine_di_node<'ll, 'tcx>(
DIFlags::FlagZero,
),
|cx, coroutine_type_di_node| {
let coroutine_layout =
cx.tcx.optimized_mir(coroutine_def_id).coroutine_layout().unwrap();
let coroutine_layout = cx
.tcx
.coroutine_layout(coroutine_def_id, coroutine_args.as_coroutine().kind_ty())
.unwrap();

let Variants::Multiple { tag_encoding: TagEncoding::Direct, ref variants, .. } =
coroutine_type_and_layout.variants
Expand Down
22 changes: 11 additions & 11 deletions compiler/rustc_const_eval/src/transform/validate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -101,18 +101,17 @@ impl<'tcx> MirPass<'tcx> for Validator {
}

// Enforce that coroutine-closure layouts are identical.
if let Some(layout) = body.coroutine_layout()
if let Some(layout) = body.coroutine_layout_raw()
&& let Some(by_move_body) = body.coroutine_by_move_body()
&& let Some(by_move_layout) = by_move_body.coroutine_layout()
&& let Some(by_move_layout) = by_move_body.coroutine_layout_raw()
{
if layout != by_move_layout {
// If this turns out not to be true, please let compiler-errors know.
// It is possible to support, but requires some changes to the layout
// computation code.
// FIXME(async_closures): We could do other validation here?
if layout.variant_fields.len() != by_move_layout.variant_fields.len() {
cfg_checker.fail(
Location::START,
format!(
"Coroutine layout differs from by-move coroutine layout:\n\
"Coroutine layout has different number of variant fields from \
by-move coroutine layout:\n\
layout: {layout:#?}\n\
by_move_layout: {by_move_layout:#?}",
),
Expand Down Expand Up @@ -715,13 +714,14 @@ impl<'a, 'tcx> Visitor<'tcx> for TypeChecker<'a, 'tcx> {
// args of the coroutine. Otherwise, we prefer to use this body
// since we may be in the process of computing this MIR in the
// first place.
let gen_body = if def_id == self.caller_body.source.def_id() {
self.caller_body
let layout = if def_id == self.caller_body.source.def_id() {
// FIXME: This is not right for async closures.
self.caller_body.coroutine_layout_raw()
} else {
self.tcx.optimized_mir(def_id)
self.tcx.coroutine_layout(def_id, args.as_coroutine().kind_ty())
};

let Some(layout) = gen_body.coroutine_layout() else {
let Some(layout) = layout else {
self.fail(
location,
format!("No coroutine layout for {parent_ty:?}"),
Expand Down
55 changes: 32 additions & 23 deletions compiler/rustc_interface/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -315,30 +315,39 @@ fn test_search_paths_tracking_hash_different_order() {
json_rendered: HumanReadableErrorType::Default(ColorConfig::Never),
};

let push = |opts: &mut Options, search_path| {
opts.search_paths.push(SearchPath::from_cli_opt(
"not-a-sysroot".as_ref(),
&opts.target_triple,
&early_dcx,
search_path,
));
};

// Reference
v1.search_paths.push(SearchPath::from_cli_opt(&early_dcx, "native=abc"));
v1.search_paths.push(SearchPath::from_cli_opt(&early_dcx, "crate=def"));
v1.search_paths.push(SearchPath::from_cli_opt(&early_dcx, "dependency=ghi"));
v1.search_paths.push(SearchPath::from_cli_opt(&early_dcx, "framework=jkl"));
v1.search_paths.push(SearchPath::from_cli_opt(&early_dcx, "all=mno"));

v2.search_paths.push(SearchPath::from_cli_opt(&early_dcx, "native=abc"));
v2.search_paths.push(SearchPath::from_cli_opt(&early_dcx, "dependency=ghi"));
v2.search_paths.push(SearchPath::from_cli_opt(&early_dcx, "crate=def"));
v2.search_paths.push(SearchPath::from_cli_opt(&early_dcx, "framework=jkl"));
v2.search_paths.push(SearchPath::from_cli_opt(&early_dcx, "all=mno"));

v3.search_paths.push(SearchPath::from_cli_opt(&early_dcx, "crate=def"));
v3.search_paths.push(SearchPath::from_cli_opt(&early_dcx, "framework=jkl"));
v3.search_paths.push(SearchPath::from_cli_opt(&early_dcx, "native=abc"));
v3.search_paths.push(SearchPath::from_cli_opt(&early_dcx, "dependency=ghi"));
v3.search_paths.push(SearchPath::from_cli_opt(&early_dcx, "all=mno"));

v4.search_paths.push(SearchPath::from_cli_opt(&early_dcx, "all=mno"));
v4.search_paths.push(SearchPath::from_cli_opt(&early_dcx, "native=abc"));
v4.search_paths.push(SearchPath::from_cli_opt(&early_dcx, "crate=def"));
v4.search_paths.push(SearchPath::from_cli_opt(&early_dcx, "dependency=ghi"));
v4.search_paths.push(SearchPath::from_cli_opt(&early_dcx, "framework=jkl"));
push(&mut v1, "native=abc");
push(&mut v1, "crate=def");
push(&mut v1, "dependency=ghi");
push(&mut v1, "framework=jkl");
push(&mut v1, "all=mno");

push(&mut v2, "native=abc");
push(&mut v2, "dependency=ghi");
push(&mut v2, "crate=def");
push(&mut v2, "framework=jkl");
push(&mut v2, "all=mno");

push(&mut v3, "crate=def");
push(&mut v3, "framework=jkl");
push(&mut v3, "native=abc");
push(&mut v3, "dependency=ghi");
push(&mut v3, "all=mno");

push(&mut v4, "all=mno");
push(&mut v4, "native=abc");
push(&mut v4, "crate=def");
push(&mut v4, "dependency=ghi");
push(&mut v4, "framework=jkl");

assert_same_hash(&v1, &v2);
assert_same_hash(&v1, &v3);
Expand Down
3 changes: 2 additions & 1 deletion compiler/rustc_middle/src/mir/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -652,8 +652,9 @@ impl<'tcx> Body<'tcx> {
self.coroutine.as_ref().and_then(|coroutine| coroutine.resume_ty)
}

/// Prefer going through [`TyCtxt::coroutine_layout`] rather than using this directly.
#[inline]
pub fn coroutine_layout(&self) -> Option<&CoroutineLayout<'tcx>> {
pub fn coroutine_layout_raw(&self) -> Option<&CoroutineLayout<'tcx>> {
self.coroutine.as_ref().and_then(|coroutine| coroutine.coroutine_layout.as_ref())
}

Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_middle/src/mir/pretty.rs
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ fn dump_matched_mir_node<'tcx, F>(
Some(promoted) => write!(file, "::{promoted:?}`")?,
}
writeln!(file, " {disambiguator} {pass_name}")?;
if let Some(ref layout) = body.coroutine_layout() {
if let Some(ref layout) = body.coroutine_layout_raw() {
writeln!(file, "/* coroutine_layout = {layout:#?} */")?;
}
writeln!(file)?;
Expand Down
Loading

0 comments on commit 10a7aa1

Please sign in to comment.