From 75135aaf19f22637b60b0e29255f0000029bdbf9 Mon Sep 17 00:00:00 2001 From: Zalathar Date: Fri, 21 Mar 2025 15:07:05 +1100 Subject: [PATCH 1/8] coverage: Extract module `mapgen::unused` for handling unused functions --- .../src/coverageinfo/mapgen.rs | 129 +----------------- .../src/coverageinfo/mapgen/unused.rs | 128 +++++++++++++++++ 2 files changed, 132 insertions(+), 125 deletions(-) create mode 100644 compiler/rustc_codegen_llvm/src/coverageinfo/mapgen/unused.rs diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs index 9a2473d6cf23d..23e068fafacf1 100644 --- a/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs +++ b/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs @@ -5,15 +5,11 @@ use rustc_abi::Align; use rustc_codegen_ssa::traits::{ BaseTypeCodegenMethods, ConstCodegenMethods, StaticCodegenMethods, }; -use rustc_data_structures::fx::{FxHashSet, FxIndexMap}; -use rustc_hir::def_id::{DefId, LocalDefId}; +use rustc_data_structures::fx::FxIndexMap; use rustc_index::IndexVec; -use rustc_middle::mir; -use rustc_middle::mir::mono::MonoItemPartitions; -use rustc_middle::ty::{self, TyCtxt}; +use rustc_middle::ty::TyCtxt; use rustc_session::RemapFileNameExt; use rustc_session::config::RemapPathScopeComponents; -use rustc_span::def_id::DefIdSet; use rustc_span::{SourceFile, StableSourceFileId}; use tracing::debug; @@ -24,6 +20,7 @@ use crate::llvm; mod covfun; mod spans; +mod unused; /// Generates and exports the coverage map, which is embedded in special /// linker sections in the final binary. @@ -76,7 +73,7 @@ pub(crate) fn finalize(cx: &CodegenCx<'_, '_>) { // In a single designated CGU, also prepare covfun records for functions // in this crate that were instrumented for coverage, but are unused. if cx.codegen_unit.is_code_coverage_dead_code_cgu() { - let mut unused_instances = gather_unused_function_instances(cx); + let mut unused_instances = unused::gather_unused_function_instances(cx); // Sort the unused instances by symbol name, for the same reason as the used ones. unused_instances.sort_by_cached_key(|&instance| tcx.symbol_name(instance).name); covfun_records.extend(unused_instances.into_iter().filter_map(|instance| { @@ -249,121 +246,3 @@ fn generate_covmap_record<'ll>(cx: &CodegenCx<'ll, '_>, version: u32, filenames_ cx.add_used_global(covmap_global); } - -/// 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. -/// We also end up adding their symbol names to a special global array that LLVM will include in -/// its embedded coverage data. -fn gather_unused_function_instances<'tcx>(cx: &CodegenCx<'_, 'tcx>) -> Vec> { - 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 { - // Usage sets expect `DefId`, so convert from `LocalDefId`. - let d: DefId = LocalDefId::to_def_id(def_id); - // To be potentially eligible for "unused function" mappings, a definition must: - // - Be eligible for coverage instrumentation - // - Not participate directly in codegen (or have lost all its coverage statements) - // - Not have any coverage statements inlined into codegenned functions - tcx.is_eligible_for_coverage(def_id) - && (!usage.all_mono_items.contains(&d) || usage.missing_own_coverage.contains(&d)) - && !usage.used_via_inlining.contains(&d) - }; - - // 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. - - tcx.mir_keys(()) - .iter() - .copied() - .filter(|&def_id| is_unused_fn(def_id)) - .map(|def_id| make_dummy_instance(tcx, def_id)) - .collect::>() -} - -struct UsageSets<'tcx> { - all_mono_items: &'tcx DefIdSet, - used_via_inlining: FxHashSet, - missing_own_coverage: FxHashSet, -} - -/// 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 MonoItemPartitions { all_mono_items, codegen_units, .. } = - 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 = codegen_units - .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 statements 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); - } - } - - UsageSets { all_mono_items, used_via_inlining, missing_own_coverage } -} - -fn make_dummy_instance<'tcx>(tcx: TyCtxt<'tcx>, local_def_id: LocalDefId) -> ty::Instance<'tcx> { - let def_id = local_def_id.to_def_id(); - - // Make a dummy instance that fills in all generics with placeholders. - ty::Instance::new( - def_id, - ty::GenericArgs::for_item(tcx, def_id, |param, _| { - if let ty::GenericParamDefKind::Lifetime = param.kind { - tcx.lifetimes.re_erased.into() - } else { - tcx.mk_param_from_def(param) - } - }), - ) -} diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen/unused.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen/unused.rs new file mode 100644 index 0000000000000..e7c9075bc15d2 --- /dev/null +++ b/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen/unused.rs @@ -0,0 +1,128 @@ +use rustc_data_structures::fx::FxHashSet; +use rustc_hir::def_id::{DefId, LocalDefId}; +use rustc_middle::mir; +use rustc_middle::mir::mono::MonoItemPartitions; +use rustc_middle::ty::{self, TyCtxt}; +use rustc_span::def_id::DefIdSet; + +use crate::common::CodegenCx; + +/// 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. +/// We also end up adding their symbol names to a special global array that LLVM will include in +/// its embedded coverage data. +pub(crate) fn gather_unused_function_instances<'tcx>( + cx: &CodegenCx<'_, 'tcx>, +) -> Vec> { + 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 { + // Usage sets expect `DefId`, so convert from `LocalDefId`. + let d: DefId = LocalDefId::to_def_id(def_id); + // To be potentially eligible for "unused function" mappings, a definition must: + // - Be eligible for coverage instrumentation + // - Not participate directly in codegen (or have lost all its coverage statements) + // - Not have any coverage statements inlined into codegenned functions + tcx.is_eligible_for_coverage(def_id) + && (!usage.all_mono_items.contains(&d) || usage.missing_own_coverage.contains(&d)) + && !usage.used_via_inlining.contains(&d) + }; + + // 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. + + tcx.mir_keys(()) + .iter() + .copied() + .filter(|&def_id| is_unused_fn(def_id)) + .map(|def_id| make_dummy_instance(tcx, def_id)) + .collect::>() +} + +struct UsageSets<'tcx> { + all_mono_items: &'tcx DefIdSet, + used_via_inlining: FxHashSet, + missing_own_coverage: FxHashSet, +} + +/// 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 MonoItemPartitions { all_mono_items, codegen_units, .. } = + 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 = codegen_units + .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 statements 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); + } + } + + UsageSets { all_mono_items, used_via_inlining, missing_own_coverage } +} + +fn make_dummy_instance<'tcx>(tcx: TyCtxt<'tcx>, local_def_id: LocalDefId) -> ty::Instance<'tcx> { + let def_id = local_def_id.to_def_id(); + + // Make a dummy instance that fills in all generics with placeholders. + ty::Instance::new( + def_id, + ty::GenericArgs::for_item(tcx, def_id, |param, _| { + if let ty::GenericParamDefKind::Lifetime = param.kind { + tcx.lifetimes.re_erased.into() + } else { + tcx.mk_param_from_def(param) + } + }), + ) +} From b3c40cf374422ac8f6cbb14fa6747b2fae5762db Mon Sep 17 00:00:00 2001 From: Zalathar Date: Fri, 21 Mar 2025 15:14:22 +1100 Subject: [PATCH 2/8] coverage: Deal with unused functions and their names in one place --- .../src/coverageinfo/mapgen.rs | 34 ++----------- .../src/coverageinfo/mapgen/covfun.rs | 8 ---- .../src/coverageinfo/mapgen/unused.rs | 48 ++++++++++++++++++- 3 files changed, 51 insertions(+), 39 deletions(-) diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs index 23e068fafacf1..5e62ce285dd8c 100644 --- a/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs +++ b/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs @@ -73,12 +73,11 @@ pub(crate) fn finalize(cx: &CodegenCx<'_, '_>) { // In a single designated CGU, also prepare covfun records for functions // in this crate that were instrumented for coverage, but are unused. if cx.codegen_unit.is_code_coverage_dead_code_cgu() { - let mut unused_instances = unused::gather_unused_function_instances(cx); - // Sort the unused instances by symbol name, for the same reason as the used ones. - unused_instances.sort_by_cached_key(|&instance| tcx.symbol_name(instance).name); - covfun_records.extend(unused_instances.into_iter().filter_map(|instance| { - prepare_covfun_record(tcx, &mut global_file_table, instance, false) - })); + unused::prepare_covfun_records_for_unused_functions( + cx, + &mut global_file_table, + &mut covfun_records, + ); } // If there are no covfun records for this CGU, don't generate a covmap record. @@ -97,33 +96,10 @@ pub(crate) fn finalize(cx: &CodegenCx<'_, '_>) { // contain multiple covmap records from different compilation units. let filenames_hash = llvm_cov::hash_bytes(&filenames_buffer); - let mut unused_function_names = vec![]; - for covfun in &covfun_records { - unused_function_names.extend(covfun.mangled_function_name_if_unused()); - covfun::generate_covfun_record(cx, filenames_hash, covfun) } - // For unused functions, we need to take their mangled names and store them - // in a specially-named global array. LLVM's `InstrProfiling` pass will - // detect this global and include those names in its `__llvm_prf_names` - // section. (See `llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp`.) - if !unused_function_names.is_empty() { - assert!(cx.codegen_unit.is_code_coverage_dead_code_cgu()); - - let name_globals = unused_function_names - .into_iter() - .map(|mangled_function_name| cx.const_str(mangled_function_name).0) - .collect::>(); - let initializer = cx.const_array(cx.type_ptr(), &name_globals); - - let array = llvm::add_global(cx.llmod, cx.val_ty(initializer), c"__llvm_coverage_names"); - llvm::set_global_constant(array, true); - llvm::set_linkage(array, llvm::Linkage::InternalLinkage); - llvm::set_initializer(array, initializer); - } - // Generate the coverage map header, which contains the filenames used by // this CGU's coverage mappings, and store it in a well-known global. // (This is skipped if we returned early due to having no covfun records.) diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen/covfun.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen/covfun.rs index 048e1988c3278..93419c2caad25 100644 --- a/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen/covfun.rs +++ b/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen/covfun.rs @@ -37,14 +37,6 @@ pub(crate) struct CovfunRecord<'tcx> { regions: ffi::Regions, } -impl<'tcx> CovfunRecord<'tcx> { - /// FIXME(Zalathar): Make this the responsibility of the code that determines - /// which functions are unused. - pub(crate) fn mangled_function_name_if_unused(&self) -> Option<&'tcx str> { - (!self.is_used).then_some(self.mangled_function_name) - } -} - pub(crate) fn prepare_covfun_record<'tcx>( tcx: TyCtxt<'tcx>, global_file_table: &mut GlobalFileTable, diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen/unused.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen/unused.rs index e7c9075bc15d2..ab030f5b6152a 100644 --- a/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen/unused.rs +++ b/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen/unused.rs @@ -1,3 +1,4 @@ +use rustc_codegen_ssa::traits::{BaseTypeCodegenMethods, ConstCodegenMethods}; use rustc_data_structures::fx::FxHashSet; use rustc_hir::def_id::{DefId, LocalDefId}; use rustc_middle::mir; @@ -6,6 +7,9 @@ use rustc_middle::ty::{self, TyCtxt}; use rustc_span::def_id::DefIdSet; use crate::common::CodegenCx; +use crate::coverageinfo::mapgen::GlobalFileTable; +use crate::coverageinfo::mapgen::covfun::{CovfunRecord, prepare_covfun_record}; +use crate::llvm; /// 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 @@ -15,9 +19,48 @@ use crate::common::CodegenCx; /// coverage map (in a single designated CGU) so that we still emit coverage mappings for them. /// We also end up adding their symbol names to a special global array that LLVM will include in /// its embedded coverage data. -pub(crate) fn gather_unused_function_instances<'tcx>( +pub(crate) fn prepare_covfun_records_for_unused_functions<'tcx>( cx: &CodegenCx<'_, 'tcx>, -) -> Vec> { + global_file_table: &mut GlobalFileTable, + covfun_records: &mut Vec>, +) { + assert!(cx.codegen_unit.is_code_coverage_dead_code_cgu()); + + let mut unused_instances = gather_unused_function_instances(cx); + // Sort the unused instances by symbol name, so that their order isn't hash-sensitive. + unused_instances.sort_by_key(|instance| instance.symbol_name); + + // Try to create a covfun record for each unused function. + let mut name_globals = Vec::with_capacity(unused_instances.len()); + covfun_records.extend(unused_instances.into_iter().filter_map(|unused| try { + let record = prepare_covfun_record(cx.tcx, global_file_table, unused.instance, false)?; + // If successful, also store its symbol name in a global constant. + name_globals.push(cx.const_str(unused.symbol_name.name).0); + record + })); + + // Store the names of unused functions in a specially-named global array. + // LLVM's `InstrProfilling` pass will detect this array, and include the + // referenced names in its `__llvm_prf_names` section. + // (See `llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp`.) + if !name_globals.is_empty() { + let initializer = cx.const_array(cx.type_ptr(), &name_globals); + + let array = llvm::add_global(cx.llmod, cx.val_ty(initializer), c"__llvm_coverage_names"); + llvm::set_global_constant(array, true); + llvm::set_linkage(array, llvm::Linkage::InternalLinkage); + llvm::set_initializer(array, initializer); + } +} + +/// Holds a dummy function instance along with its symbol name, to avoid having +/// to repeatedly query for the name. +struct UnusedInstance<'tcx> { + instance: ty::Instance<'tcx>, + symbol_name: ty::SymbolName<'tcx>, +} + +fn gather_unused_function_instances<'tcx>(cx: &CodegenCx<'_, 'tcx>) -> Vec> { assert!(cx.codegen_unit.is_code_coverage_dead_code_cgu()); let tcx = cx.tcx; @@ -45,6 +88,7 @@ pub(crate) fn gather_unused_function_instances<'tcx>( .copied() .filter(|&def_id| is_unused_fn(def_id)) .map(|def_id| make_dummy_instance(tcx, def_id)) + .map(|instance| UnusedInstance { instance, symbol_name: tcx.symbol_name(instance) }) .collect::>() } From cd4453fdbadec9d9a3b0b40a069c63e01ad52ebf Mon Sep 17 00:00:00 2001 From: jackh726 Date: Sat, 5 Apr 2025 18:21:22 +0000 Subject: [PATCH 3/8] Explicitly depend on ena in rustc_type_ir and make the UnifyKey and UnifyValue imports non-nightly --- Cargo.lock | 1 + compiler/rustc_type_ir/Cargo.toml | 1 + compiler/rustc_type_ir/src/data_structures/mod.rs | 1 + compiler/rustc_type_ir/src/ty_kind.rs | 8 +------- 4 files changed, 4 insertions(+), 7 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 96526f7e9e7da..ce8efe631ce61 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4581,6 +4581,7 @@ version = "0.0.0" dependencies = [ "bitflags", "derive-where", + "ena", "indexmap", "rustc-hash 1.1.0", "rustc_ast_ir", diff --git a/compiler/rustc_type_ir/Cargo.toml b/compiler/rustc_type_ir/Cargo.toml index 4adf715792666..83d3d78298e60 100644 --- a/compiler/rustc_type_ir/Cargo.toml +++ b/compiler/rustc_type_ir/Cargo.toml @@ -7,6 +7,7 @@ edition = "2024" # tidy-alphabetical-start bitflags = "2.4.1" derive-where = "1.2.7" +ena = "0.14.3" indexmap = "2.0.0" rustc-hash = "1.1.0" rustc_ast_ir = { path = "../rustc_ast_ir", default-features = false } diff --git a/compiler/rustc_type_ir/src/data_structures/mod.rs b/compiler/rustc_type_ir/src/data_structures/mod.rs index 30c67d10d0e86..a72669cbd189b 100644 --- a/compiler/rustc_type_ir/src/data_structures/mod.rs +++ b/compiler/rustc_type_ir/src/data_structures/mod.rs @@ -1,5 +1,6 @@ use std::hash::BuildHasherDefault; +pub use ena::unify::{NoError, UnifyKey, UnifyValue}; use rustc_hash::FxHasher; pub use rustc_hash::{FxHashMap as HashMap, FxHashSet as HashSet}; diff --git a/compiler/rustc_type_ir/src/ty_kind.rs b/compiler/rustc_type_ir/src/ty_kind.rs index d35b22d517c52..753a72a051ad8 100644 --- a/compiler/rustc_type_ir/src/ty_kind.rs +++ b/compiler/rustc_type_ir/src/ty_kind.rs @@ -6,9 +6,8 @@ use rustc_ast_ir::Mutability; #[cfg(feature = "nightly")] use rustc_data_structures::stable_hasher::{HashStable, StableHasher}; #[cfg(feature = "nightly")] -use rustc_data_structures::unify::{NoError, UnifyKey, UnifyValue}; -#[cfg(feature = "nightly")] use rustc_macros::{Decodable_NoContext, Encodable_NoContext, HashStable_NoContext}; +use rustc_type_ir::data_structures::{NoError, UnifyKey, UnifyValue}; use rustc_type_ir_macros::{Lift_Generic, TypeFoldable_Generic, TypeVisitable_Generic}; use self::TyKind::*; @@ -796,7 +795,6 @@ pub enum InferTy { /// Raw `TyVid` are used as the unification key for `sub_relations`; /// they carry no values. -#[cfg(feature = "nightly")] impl UnifyKey for TyVid { type Value = (); #[inline] @@ -812,7 +810,6 @@ impl UnifyKey for TyVid { } } -#[cfg(feature = "nightly")] impl UnifyValue for IntVarValue { type Error = NoError; @@ -832,7 +829,6 @@ impl UnifyValue for IntVarValue { } } -#[cfg(feature = "nightly")] impl UnifyKey for IntVid { type Value = IntVarValue; #[inline] // make this function eligible for inlining - it is quite hot. @@ -848,7 +844,6 @@ impl UnifyKey for IntVid { } } -#[cfg(feature = "nightly")] impl UnifyValue for FloatVarValue { type Error = NoError; @@ -866,7 +861,6 @@ impl UnifyValue for FloatVarValue { } } -#[cfg(feature = "nightly")] impl UnifyKey for FloatVid { type Value = FloatVarValue; #[inline] From 13bf79cd879023ae4434ed4c6b287159bef32b48 Mon Sep 17 00:00:00 2001 From: HaeNoe Date: Sun, 9 Mar 2025 22:55:07 +0100 Subject: [PATCH 4/8] fix usage of `autodiff` macro with inner functions - fix errors caused by the move of `ast::Item::ident` (see #138740) - move the logic of getting `sig`, `vis`, and `ident` from two seperate `match` statements into one (less repetition especially with the nested `match`) --- compiler/rustc_builtin_macros/src/autodiff.rs | 108 ++++++++++++------ 1 file changed, 76 insertions(+), 32 deletions(-) diff --git a/compiler/rustc_builtin_macros/src/autodiff.rs b/compiler/rustc_builtin_macros/src/autodiff.rs index 7f99f75b2b9df..287f0fdc516b6 100644 --- a/compiler/rustc_builtin_macros/src/autodiff.rs +++ b/compiler/rustc_builtin_macros/src/autodiff.rs @@ -199,27 +199,46 @@ mod llvm_enzyme { return vec![item]; } let dcx = ecx.sess.dcx(); - // first get the annotable item: - let (primal, sig, is_impl): (Ident, FnSig, bool) = match &item { + + // first get information about the annotable item: + let (sig, vis, primal) = match &item { Annotatable::Item(iitem) => { - let (ident, sig) = match &iitem.kind { - ItemKind::Fn(box ast::Fn { ident, sig, .. }) => (ident, sig), + let (sig, ident) = match &iitem.kind { + ItemKind::Fn(box ast::Fn { sig, ident, .. }) => (sig, ident), _ => { dcx.emit_err(errors::AutoDiffInvalidApplication { span: item.span() }); return vec![item]; } }; - (*ident, sig.clone(), false) + (sig.clone(), iitem.vis.clone(), ident.clone()) } Annotatable::AssocItem(assoc_item, Impl { of_trait: false }) => { - let (ident, sig) = match &assoc_item.kind { - ast::AssocItemKind::Fn(box ast::Fn { ident, sig, .. }) => (ident, sig), + let (sig, ident) = match &assoc_item.kind { + ast::AssocItemKind::Fn(box ast::Fn { sig, ident, .. }) => (sig, ident), + _ => { + dcx.emit_err(errors::AutoDiffInvalidApplication { span: item.span() }); + return vec![item]; + } + }; + (sig.clone(), assoc_item.vis.clone(), ident.clone()) + } + Annotatable::Stmt(stmt) => { + let (sig, vis, ident) = match &stmt.kind { + ast::StmtKind::Item(iitem) => match &iitem.kind { + ast::ItemKind::Fn(box ast::Fn { sig, ident, .. }) => { + (sig.clone(), iitem.vis.clone(), ident.clone()) + } + _ => { + dcx.emit_err(errors::AutoDiffInvalidApplication { span: item.span() }); + return vec![item]; + } + }, _ => { dcx.emit_err(errors::AutoDiffInvalidApplication { span: item.span() }); return vec![item]; } }; - (*ident, sig.clone(), true) + (sig, vis, ident) } _ => { dcx.emit_err(errors::AutoDiffInvalidApplication { span: item.span() }); @@ -238,15 +257,6 @@ mod llvm_enzyme { let has_ret = has_ret(&sig.decl.output); let sig_span = ecx.with_call_site_ctxt(sig.span); - let vis = match &item { - Annotatable::Item(iitem) => iitem.vis.clone(), - Annotatable::AssocItem(assoc_item, _) => assoc_item.vis.clone(), - _ => { - dcx.emit_err(errors::AutoDiffInvalidApplication { span: item.span() }); - return vec![item]; - } - }; - // create TokenStream from vec elemtents: // meta_item doesn't have a .tokens field let mut ts: Vec = vec![]; @@ -379,6 +389,22 @@ mod llvm_enzyme { } Annotatable::AssocItem(assoc_item.clone(), i) } + Annotatable::Stmt(ref mut stmt) => { + match stmt.kind { + ast::StmtKind::Item(ref mut iitem) => { + if !iitem.attrs.iter().any(|a| same_attribute(&a.kind, &attr.kind)) { + iitem.attrs.push(attr); + } + if !iitem.attrs.iter().any(|a| same_attribute(&a.kind, &inline_never.kind)) + { + iitem.attrs.push(inline_never.clone()); + } + } + _ => unreachable!("stmt kind checked previously"), + }; + + Annotatable::Stmt(stmt.clone()) + } _ => { unreachable!("annotatable kind checked previously") } @@ -389,22 +415,40 @@ mod llvm_enzyme { delim: rustc_ast::token::Delimiter::Parenthesis, tokens: ts, }); + let d_attr = outer_normal_attr(&rustc_ad_attr, new_id, span); - let d_annotatable = if is_impl { - let assoc_item: AssocItemKind = ast::AssocItemKind::Fn(asdf); - let d_fn = P(ast::AssocItem { - attrs: thin_vec![d_attr, inline_never], - id: ast::DUMMY_NODE_ID, - span, - vis, - kind: assoc_item, - tokens: None, - }); - Annotatable::AssocItem(d_fn, Impl { of_trait: false }) - } else { - let mut d_fn = ecx.item(span, thin_vec![d_attr, inline_never], ItemKind::Fn(asdf)); - d_fn.vis = vis; - Annotatable::Item(d_fn) + let d_annotatable = match &item { + Annotatable::AssocItem(_, _) => { + let assoc_item: AssocItemKind = ast::AssocItemKind::Fn(asdf); + let d_fn = P(ast::AssocItem { + attrs: thin_vec![d_attr, inline_never], + id: ast::DUMMY_NODE_ID, + span, + vis, + kind: assoc_item, + tokens: None, + }); + Annotatable::AssocItem(d_fn, Impl { of_trait: false }) + } + Annotatable::Item(_) => { + let mut d_fn = ecx.item(span, thin_vec![d_attr, inline_never], ItemKind::Fn(asdf)); + d_fn.vis = vis; + + Annotatable::Item(d_fn) + } + Annotatable::Stmt(_) => { + let mut d_fn = ecx.item(span, thin_vec![d_attr, inline_never], ItemKind::Fn(asdf)); + d_fn.vis = vis; + + Annotatable::Stmt(P(ast::Stmt { + id: ast::DUMMY_NODE_ID, + kind: ast::StmtKind::Item(d_fn), + span, + })) + } + _ => { + unreachable!("item kind checked previously") + } }; return vec![orig_annotatable, d_annotatable]; From 72091edcc46601b0fc10c8d71349449d86718d5e Mon Sep 17 00:00:00 2001 From: HaeNoe Date: Sat, 22 Mar 2025 00:26:11 +0100 Subject: [PATCH 5/8] feat: add test to validate autodiff macro expansion --- tests/pretty/autodiff_forward.pp | 15 +++++++++++++++ tests/pretty/autodiff_forward.rs | 8 ++++++++ 2 files changed, 23 insertions(+) diff --git a/tests/pretty/autodiff_forward.pp b/tests/pretty/autodiff_forward.pp index 4b2fb6166ff7e..a62ced56dc41d 100644 --- a/tests/pretty/autodiff_forward.pp +++ b/tests/pretty/autodiff_forward.pp @@ -29,6 +29,8 @@ // Make sure, that we add the None for the default return. + // We want to make sure that we can use the macro for functions defined inside of functions + ::core::panicking::panic("not implemented") } #[rustc_autodiff(Forward, 1, Dual, Const, Dual)] @@ -158,4 +160,17 @@ ::core::hint::black_box((bx_0,)); ::core::hint::black_box(::default()) } +pub fn f9() { + #[rustc_autodiff] + #[inline(never)] + fn inner(x: f32) -> f32 { x * x } + #[rustc_autodiff(Forward, 1, Dual, DualOnly)] + #[inline(never)] + fn d_inner(x: f32, bx_0: f32) -> f32 { + unsafe { asm!("NOP", options(pure, nomem)); }; + ::core::hint::black_box(inner(x)); + ::core::hint::black_box((bx_0,)); + ::core::hint::black_box(::default()) + } +} fn main() {} diff --git a/tests/pretty/autodiff_forward.rs b/tests/pretty/autodiff_forward.rs index a765738c2a815..e61d1ec361723 100644 --- a/tests/pretty/autodiff_forward.rs +++ b/tests/pretty/autodiff_forward.rs @@ -54,4 +54,12 @@ fn f8(x: &f32) -> f32 { unimplemented!() } +// We want to make sure that we can use the macro for functions defined inside of functions +pub fn f9() { + #[autodiff(d_inner, Forward, Dual, DualOnly)] + fn inner(x: f32) -> f32 { + x * x + } +} + fn main() {} From 63e825e52a1961f1270bed5075c52f2c9921ef08 Mon Sep 17 00:00:00 2001 From: HaeNoe Date: Tue, 1 Apr 2025 07:25:04 +0200 Subject: [PATCH 6/8] feat: apply autodiff macro twice to inner function Verify that the expanded `inline` and `rustc_autodiff` macros are not duplicated. --- tests/pretty/autodiff_forward.pp | 10 +++++++++- tests/pretty/autodiff_forward.rs | 3 ++- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/tests/pretty/autodiff_forward.pp b/tests/pretty/autodiff_forward.pp index a62ced56dc41d..713b8f541ae0d 100644 --- a/tests/pretty/autodiff_forward.pp +++ b/tests/pretty/autodiff_forward.pp @@ -164,9 +164,17 @@ #[rustc_autodiff] #[inline(never)] fn inner(x: f32) -> f32 { x * x } + #[rustc_autodiff(Forward, 1, Dual, Dual)] + #[inline(never)] + fn d_inner_2(x: f32, bx_0: f32) -> (f32, f32) { + unsafe { asm!("NOP", options(pure, nomem)); }; + ::core::hint::black_box(inner(x)); + ::core::hint::black_box((bx_0,)); + ::core::hint::black_box(<(f32, f32)>::default()) + } #[rustc_autodiff(Forward, 1, Dual, DualOnly)] #[inline(never)] - fn d_inner(x: f32, bx_0: f32) -> f32 { + fn d_inner_1(x: f32, bx_0: f32) -> f32 { unsafe { asm!("NOP", options(pure, nomem)); }; ::core::hint::black_box(inner(x)); ::core::hint::black_box((bx_0,)); diff --git a/tests/pretty/autodiff_forward.rs b/tests/pretty/autodiff_forward.rs index e61d1ec361723..5a0660a08e526 100644 --- a/tests/pretty/autodiff_forward.rs +++ b/tests/pretty/autodiff_forward.rs @@ -56,7 +56,8 @@ fn f8(x: &f32) -> f32 { // We want to make sure that we can use the macro for functions defined inside of functions pub fn f9() { - #[autodiff(d_inner, Forward, Dual, DualOnly)] + #[autodiff(d_inner_1, Forward, Dual, DualOnly)] + #[autodiff(d_inner_2, Forward, Dual, Dual)] fn inner(x: f32) -> f32 { x * x } From bf69443a9f0fa9b44aaec36c1c470ad22a325c2a Mon Sep 17 00:00:00 2001 From: HaeNoe Date: Thu, 3 Apr 2025 22:47:30 +0200 Subject: [PATCH 7/8] refactor: simplify function-info gathering --- compiler/rustc_builtin_macros/src/autodiff.rs | 67 +++++++------------ 1 file changed, 26 insertions(+), 41 deletions(-) diff --git a/compiler/rustc_builtin_macros/src/autodiff.rs b/compiler/rustc_builtin_macros/src/autodiff.rs index 287f0fdc516b6..351413dea493c 100644 --- a/compiler/rustc_builtin_macros/src/autodiff.rs +++ b/compiler/rustc_builtin_macros/src/autodiff.rs @@ -17,7 +17,7 @@ mod llvm_enzyme { use rustc_ast::visit::AssocCtxt::*; use rustc_ast::{ self as ast, AssocItemKind, BindingMode, ExprKind, FnRetTy, FnSig, Generics, ItemKind, - MetaItemInner, PatKind, QSelf, TyKind, + MetaItemInner, PatKind, QSelf, TyKind, Visibility, }; use rustc_expand::base::{Annotatable, ExtCtxt}; use rustc_span::{Ident, Span, Symbol, kw, sym}; @@ -72,6 +72,16 @@ mod llvm_enzyme { } } + // Get information about the function the macro is applied to + fn extract_item_info(iitem: &P) -> Option<(Visibility, FnSig, Ident)> { + match &iitem.kind { + ItemKind::Fn(box ast::Fn { sig, ident, .. }) => { + Some((iitem.vis.clone(), sig.clone(), ident.clone())) + } + _ => None, + } + } + pub(crate) fn from_ast( ecx: &mut ExtCtxt<'_>, meta_item: &ThinVec, @@ -201,49 +211,24 @@ mod llvm_enzyme { let dcx = ecx.sess.dcx(); // first get information about the annotable item: - let (sig, vis, primal) = match &item { - Annotatable::Item(iitem) => { - let (sig, ident) = match &iitem.kind { - ItemKind::Fn(box ast::Fn { sig, ident, .. }) => (sig, ident), - _ => { - dcx.emit_err(errors::AutoDiffInvalidApplication { span: item.span() }); - return vec![item]; - } - }; - (sig.clone(), iitem.vis.clone(), ident.clone()) - } + let Some((vis, sig, primal)) = (match &item { + Annotatable::Item(iitem) => extract_item_info(iitem), + Annotatable::Stmt(stmt) => match &stmt.kind { + ast::StmtKind::Item(iitem) => extract_item_info(iitem), + _ => None, + }, Annotatable::AssocItem(assoc_item, Impl { of_trait: false }) => { - let (sig, ident) = match &assoc_item.kind { - ast::AssocItemKind::Fn(box ast::Fn { sig, ident, .. }) => (sig, ident), - _ => { - dcx.emit_err(errors::AutoDiffInvalidApplication { span: item.span() }); - return vec![item]; - } - }; - (sig.clone(), assoc_item.vis.clone(), ident.clone()) - } - Annotatable::Stmt(stmt) => { - let (sig, vis, ident) = match &stmt.kind { - ast::StmtKind::Item(iitem) => match &iitem.kind { - ast::ItemKind::Fn(box ast::Fn { sig, ident, .. }) => { - (sig.clone(), iitem.vis.clone(), ident.clone()) - } - _ => { - dcx.emit_err(errors::AutoDiffInvalidApplication { span: item.span() }); - return vec![item]; - } - }, - _ => { - dcx.emit_err(errors::AutoDiffInvalidApplication { span: item.span() }); - return vec![item]; + match &assoc_item.kind { + ast::AssocItemKind::Fn(box ast::Fn { sig, ident, .. }) => { + Some((assoc_item.vis.clone(), sig.clone(), ident.clone())) } - }; - (sig, vis, ident) - } - _ => { - dcx.emit_err(errors::AutoDiffInvalidApplication { span: item.span() }); - return vec![item]; + _ => None, + } } + _ => None, + }) else { + dcx.emit_err(errors::AutoDiffInvalidApplication { span: item.span() }); + return vec![item]; }; let meta_item_vec: ThinVec = match meta_item.kind { From fbdf9f8766c9e01c98e600c588f92564ede26726 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Thu, 3 Apr 2025 14:23:49 +1100 Subject: [PATCH 8/8] Allow for missing invisible close delim when reparsing an expression. This can happen when invalid syntax is passed to a declarative macro. We shouldn't be too strict about the token stream position once the parser has rejected the invalid syntax. Fixes #139248 and #139445. --- compiler/rustc_parse/src/parser/mod.rs | 5 ++- .../ui/macros/no-close-delim-issue-139248.rs | 14 ++++++++ .../macros/no-close-delim-issue-139248.stderr | 33 +++++++++++++++++++ 3 files changed, 51 insertions(+), 1 deletion(-) create mode 100644 tests/ui/macros/no-close-delim-issue-139248.rs create mode 100644 tests/ui/macros/no-close-delim-issue-139248.stderr diff --git a/compiler/rustc_parse/src/parser/mod.rs b/compiler/rustc_parse/src/parser/mod.rs index 3b0861a9942a6..5920f2d4daa3c 100644 --- a/compiler/rustc_parse/src/parser/mod.rs +++ b/compiler/rustc_parse/src/parser/mod.rs @@ -793,7 +793,10 @@ impl<'a> Parser<'a> { self.bump(); Some(res) } else { - panic!("no close delim when reparsing {mv_kind:?}"); + // This can occur when invalid syntax is passed to a decl macro. E.g. see #139248, + // where the reparse attempt of an invalid expr consumed the trailing invisible + // delimiter. + None } } else { None diff --git a/tests/ui/macros/no-close-delim-issue-139248.rs b/tests/ui/macros/no-close-delim-issue-139248.rs new file mode 100644 index 0000000000000..9cfa934ab8ffb --- /dev/null +++ b/tests/ui/macros/no-close-delim-issue-139248.rs @@ -0,0 +1,14 @@ +// This code caused a "no close delim when reparsing Expr" ICE in #139248. + +macro_rules! m { + (static a : () = $e:expr) => { + static a : () = $e; + //~^ ERROR macro expansion ends with an incomplete expression: expected expression + } +} + +m! { static a : () = (if b) } +//~^ error: expected `{`, found `)` +//~| error: expected `{`, found `)` + +fn main() {} diff --git a/tests/ui/macros/no-close-delim-issue-139248.stderr b/tests/ui/macros/no-close-delim-issue-139248.stderr new file mode 100644 index 0000000000000..6ed41ae9b46a6 --- /dev/null +++ b/tests/ui/macros/no-close-delim-issue-139248.stderr @@ -0,0 +1,33 @@ +error: expected `{`, found `)` + --> $DIR/no-close-delim-issue-139248.rs:10:27 + | +LL | m! { static a : () = (if b) } + | ^ expected `{` + | +note: the `if` expression is missing a block after this condition + --> $DIR/no-close-delim-issue-139248.rs:10:26 + | +LL | m! { static a : () = (if b) } + | ^ + +error: expected `{`, found `)` + --> $DIR/no-close-delim-issue-139248.rs:10:27 + | +LL | m! { static a : () = (if b) } + | ^ expected `{` + | +note: the `if` expression is missing a block after this condition + --> $DIR/no-close-delim-issue-139248.rs:10:26 + | +LL | m! { static a : () = (if b) } + | ^ + = note: duplicate diagnostic emitted due to `-Z deduplicate-diagnostics=no` + +error: macro expansion ends with an incomplete expression: expected expression + --> $DIR/no-close-delim-issue-139248.rs:5:28 + | +LL | static a : () = $e; + | ^ expected expression + +error: aborting due to 3 previous errors +