From 54d83beb3897fcfbc79e26fce467792ae5857ffb Mon Sep 17 00:00:00 2001 From: Oli Scherer Date: Thu, 14 Mar 2024 13:54:00 +0000 Subject: [PATCH 1/3] Add test --- ...codegen_private_const_fn_only_used_in_const_eval.rs | 10 ++++++++++ 1 file changed, 10 insertions(+) create mode 100644 tests/codegen/dont_codegen_private_const_fn_only_used_in_const_eval.rs diff --git a/tests/codegen/dont_codegen_private_const_fn_only_used_in_const_eval.rs b/tests/codegen/dont_codegen_private_const_fn_only_used_in_const_eval.rs new file mode 100644 index 0000000000000..723ac015cdc14 --- /dev/null +++ b/tests/codegen/dont_codegen_private_const_fn_only_used_in_const_eval.rs @@ -0,0 +1,10 @@ +//! This test checks that we do not monomorphize functions that are only +//! used to evaluate static items, but never used in runtime code. + +//@compile-flags: --crate-type=lib -Copt-level=0 + +const fn foo() {} + +pub static FOO: () = foo(); + +// CHECK: define{{.*}}foo{{.*}} From 8332b47cae210e88457ea466e4cd48265f05f113 Mon Sep 17 00:00:00 2001 From: Oli Scherer Date: Tue, 12 Mar 2024 06:40:23 +0000 Subject: [PATCH 2/3] Stop walking the bodies of statics for reachability, and evaluate them instead --- compiler/rustc_passes/src/reachable.rs | 89 +++++++++++-------- ...rivate_const_fn_only_used_in_const_eval.rs | 2 +- 2 files changed, 52 insertions(+), 39 deletions(-) diff --git a/compiler/rustc_passes/src/reachable.rs b/compiler/rustc_passes/src/reachable.rs index e86c0522b3cd4..0dd3185fc83f7 100644 --- a/compiler/rustc_passes/src/reachable.rs +++ b/compiler/rustc_passes/src/reachable.rs @@ -6,6 +6,7 @@ // reachable as well. use hir::def_id::LocalDefIdSet; +use rustc_data_structures::stack::ensure_sufficient_stack; use rustc_hir as hir; use rustc_hir::def::{DefKind, Res}; use rustc_hir::def_id::{DefId, LocalDefId}; @@ -65,23 +66,8 @@ impl<'tcx> Visitor<'tcx> for ReachableContext<'tcx> { _ => None, }; - if let Some(res) = res - && let Some(def_id) = res.opt_def_id().and_then(|el| el.as_local()) - { - if self.def_id_represents_local_inlined_item(def_id.to_def_id()) { - self.worklist.push(def_id); - } else { - match res { - // Reachable constants and reachable statics can have their contents inlined - // into other crates. Mark them as reachable and recurse into their body. - Res::Def(DefKind::Const | DefKind::AssocConst | DefKind::Static { .. }, _) => { - self.worklist.push(def_id); - } - _ => { - self.reachable_symbols.insert(def_id); - } - } - } + if let Some(res) = res { + self.propagate_item(res); } intravisit::walk_expr(self, expr) @@ -201,17 +187,9 @@ impl<'tcx> ReachableContext<'tcx> { hir::ItemKind::Const(_, _, init) => { self.visit_nested_body(init); } - - // Reachable statics are inlined if read from another constant or static - // in other crates. Additionally anonymous nested statics may be created - // when evaluating a static, so preserve those, too. - hir::ItemKind::Static(_, _, init) => { - // FIXME(oli-obk): remove this body walking and instead walk the evaluated initializer - // to find nested items that end up in the final value instead of also marking symbols - // as reachable that are only needed for evaluation. - self.visit_nested_body(init); + hir::ItemKind::Static(..) => { if let Ok(alloc) = self.tcx.eval_static_initializer(item.owner_id.def_id) { - self.propagate_statics_from_alloc(item.owner_id.def_id, alloc); + self.propagate_from_alloc(alloc); } } @@ -281,25 +259,60 @@ impl<'tcx> ReachableContext<'tcx> { } } - /// Finds anonymous nested statics created for nested allocations and adds them to `reachable_symbols`. - fn propagate_statics_from_alloc(&mut self, root: LocalDefId, alloc: ConstAllocation<'tcx>) { + /// Finds things to add to `reachable_symbols` within allocations. + /// In contrast to visit_nested_body this ignores things that were only needed to evaluate + /// the allocation. + fn propagate_from_alloc(&mut self, alloc: ConstAllocation<'tcx>) { if !self.any_library { return; } for (_, prov) in alloc.0.provenance().ptrs().iter() { match self.tcx.global_alloc(prov.alloc_id()) { GlobalAlloc::Static(def_id) => { - if let Some(def_id) = def_id.as_local() - && self.tcx.local_parent(def_id) == root - // This is the main purpose of this function: add the def_id we find - // to `reachable_symbols`. - && self.reachable_symbols.insert(def_id) - && let Ok(alloc) = self.tcx.eval_static_initializer(def_id) - { - self.propagate_statics_from_alloc(root, alloc); + self.propagate_item(Res::Def(self.tcx.def_kind(def_id), def_id)) + } + GlobalAlloc::Function(instance) => { + self.propagate_item(Res::Def( + self.tcx.def_kind(instance.def_id()), + instance.def_id(), + )) + // TODO: walk generic args + } + GlobalAlloc::VTable(ty, trait_ref) => todo!("{ty:?}, {trait_ref:?}"), + GlobalAlloc::Memory(alloc) => self.propagate_from_alloc(alloc), + } + } + } + + fn propagate_item(&mut self, res: Res) { + let Res::Def(kind, def_id) = res else { return }; + let Some(def_id) = def_id.as_local() else { return }; + match kind { + DefKind::Static { nested: true, .. } => { + // This is the main purpose of this function: add the def_id we find + // to `reachable_symbols`. + if self.reachable_symbols.insert(def_id) { + if let Ok(alloc) = self.tcx.eval_static_initializer(def_id) { + // This cannot cause infinite recursion, because we abort by inserting into the + // work list once we hit a normal static. Nested statics, even if they somehow + // become recursive, are also not infinitely recursing, because of the + // `reachable_symbols` check above. + // We still need to protect against stack overflow due to deeply nested statics. + ensure_sufficient_stack(|| self.propagate_from_alloc(alloc)); } } - GlobalAlloc::Function(_) | GlobalAlloc::VTable(_, _) | GlobalAlloc::Memory(_) => {} + } + // Reachable constants and reachable statics can have their contents inlined + // into other crates. Mark them as reachable and recurse into their body. + DefKind::Const | DefKind::AssocConst | DefKind::Static { .. } => { + self.worklist.push(def_id); + } + _ => { + if self.def_id_represents_local_inlined_item(def_id.to_def_id()) { + self.worklist.push(def_id); + } else { + self.reachable_symbols.insert(def_id); + } } } } diff --git a/tests/codegen/dont_codegen_private_const_fn_only_used_in_const_eval.rs b/tests/codegen/dont_codegen_private_const_fn_only_used_in_const_eval.rs index 723ac015cdc14..eeeaebe52dd13 100644 --- a/tests/codegen/dont_codegen_private_const_fn_only_used_in_const_eval.rs +++ b/tests/codegen/dont_codegen_private_const_fn_only_used_in_const_eval.rs @@ -7,4 +7,4 @@ const fn foo() {} pub static FOO: () = foo(); -// CHECK: define{{.*}}foo{{.*}} +// CHECK-NOT: define{{.*}}foo{{.*}} From 746e4eff263527ba8960552ea62db11ab9821644 Mon Sep 17 00:00:00 2001 From: Oli Scherer Date: Tue, 12 Mar 2024 07:00:01 +0000 Subject: [PATCH 3/3] Test and implement reachability for trait objects and generic parameters of functions --- Cargo.lock | 1 + compiler/rustc_passes/Cargo.toml | 1 + compiler/rustc_passes/src/reachable.rs | 35 ++++++++++++++++--- compiler/rustc_privacy/src/lib.rs | 4 +-- .../cross-crate/auxiliary/static_init_aux.rs | 21 +++++++++++ tests/ui/cross-crate/static-init.rs | 4 +++ 6 files changed, 60 insertions(+), 6 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 16aed3dc49ca0..94e2570ebafc2 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4400,6 +4400,7 @@ dependencies = [ "rustc_lexer", "rustc_macros", "rustc_middle", + "rustc_privacy", "rustc_session", "rustc_span", "rustc_target", diff --git a/compiler/rustc_passes/Cargo.toml b/compiler/rustc_passes/Cargo.toml index 6abfa08c53080..b45a8dae277ed 100644 --- a/compiler/rustc_passes/Cargo.toml +++ b/compiler/rustc_passes/Cargo.toml @@ -18,6 +18,7 @@ rustc_index = { path = "../rustc_index" } rustc_lexer = { path = "../rustc_lexer" } rustc_macros = { path = "../rustc_macros" } rustc_middle = { path = "../rustc_middle" } +rustc_privacy = { path = "../rustc_privacy" } rustc_session = { path = "../rustc_session" } rustc_span = { path = "../rustc_span" } rustc_target = { path = "../rustc_target" } diff --git a/compiler/rustc_passes/src/reachable.rs b/compiler/rustc_passes/src/reachable.rs index 0dd3185fc83f7..718c2345397e1 100644 --- a/compiler/rustc_passes/src/reachable.rs +++ b/compiler/rustc_passes/src/reachable.rs @@ -16,7 +16,8 @@ use rustc_middle::middle::codegen_fn_attrs::{CodegenFnAttrFlags, CodegenFnAttrs} use rustc_middle::middle::privacy::{self, Level}; use rustc_middle::mir::interpret::{ConstAllocation, GlobalAlloc}; use rustc_middle::query::Providers; -use rustc_middle::ty::{self, TyCtxt}; +use rustc_middle::ty::{self, ExistentialTraitRef, TyCtxt}; +use rustc_privacy::DefIdVisitor; use rustc_session::config::CrateType; use rustc_target::spec::abi::Abi; @@ -272,13 +273,22 @@ impl<'tcx> ReachableContext<'tcx> { self.propagate_item(Res::Def(self.tcx.def_kind(def_id), def_id)) } GlobalAlloc::Function(instance) => { + // Manually visit to actually see the instance's `DefId`. Type visitors won't see it self.propagate_item(Res::Def( self.tcx.def_kind(instance.def_id()), instance.def_id(), - )) - // TODO: walk generic args + )); + self.visit(instance.args); + } + GlobalAlloc::VTable(ty, trait_ref) => { + self.visit(ty); + // Manually visit to actually see the trait's `DefId`. Type visitors won't see it + if let Some(trait_ref) = trait_ref { + let ExistentialTraitRef { def_id, args } = trait_ref.skip_binder(); + self.visit_def_id(def_id, "", &""); + self.visit(args); + } } - GlobalAlloc::VTable(ty, trait_ref) => todo!("{ty:?}, {trait_ref:?}"), GlobalAlloc::Memory(alloc) => self.propagate_from_alloc(alloc), } } @@ -318,6 +328,23 @@ impl<'tcx> ReachableContext<'tcx> { } } +impl<'tcx> DefIdVisitor<'tcx> for ReachableContext<'tcx> { + type Result = (); + + fn tcx(&self) -> TyCtxt<'tcx> { + self.tcx + } + + fn visit_def_id( + &mut self, + def_id: DefId, + _kind: &str, + _descr: &dyn std::fmt::Display, + ) -> Self::Result { + self.propagate_item(Res::Def(self.tcx.def_kind(def_id), def_id)) + } +} + fn check_item<'tcx>( tcx: TyCtxt<'tcx>, id: hir::ItemId, diff --git a/compiler/rustc_privacy/src/lib.rs b/compiler/rustc_privacy/src/lib.rs index 9f8cb8fcb41ec..073982ca5c37d 100644 --- a/compiler/rustc_privacy/src/lib.rs +++ b/compiler/rustc_privacy/src/lib.rs @@ -67,7 +67,7 @@ impl<'tcx> fmt::Display for LazyDefPathStr<'tcx> { /// First, it doesn't have overridable `fn visit_trait_ref`, so we have to catch trait `DefId`s /// manually. Second, it doesn't visit some type components like signatures of fn types, or traits /// in `impl Trait`, see individual comments in `DefIdVisitorSkeleton::visit_ty`. -trait DefIdVisitor<'tcx> { +pub trait DefIdVisitor<'tcx> { type Result: VisitorResult = (); const SHALLOW: bool = false; const SKIP_ASSOC_TYS: bool = false; @@ -98,7 +98,7 @@ trait DefIdVisitor<'tcx> { } } -struct DefIdVisitorSkeleton<'v, 'tcx, V: ?Sized> { +pub struct DefIdVisitorSkeleton<'v, 'tcx, V: ?Sized> { def_id_visitor: &'v mut V, visited_opaque_tys: FxHashSet, dummy: PhantomData>, diff --git a/tests/ui/cross-crate/auxiliary/static_init_aux.rs b/tests/ui/cross-crate/auxiliary/static_init_aux.rs index 5e172ef3198ae..dca708733b923 100644 --- a/tests/ui/cross-crate/auxiliary/static_init_aux.rs +++ b/tests/ui/cross-crate/auxiliary/static_init_aux.rs @@ -1,6 +1,8 @@ pub static V: &u32 = &X; pub static F: fn() = f; pub static G: fn() = G0; +pub static H: &(dyn Fn() + Sync) = &h; +pub static I: fn() = Helper(j).mk(); static X: u32 = 42; static G0: fn() = g; @@ -12,3 +14,22 @@ pub fn v() -> *const u32 { fn f() {} fn g() {} + +fn h() {} + +#[derive(Copy, Clone)] +struct Helper(T); + +impl Helper { + const fn mk(self) -> fn() { + i:: + } +} + +fn i() { + assert_eq!(std::mem::size_of::(), 0); + // unsafe to work around the lack of a `Default` impl for function items + unsafe { (std::mem::transmute_copy::<(), T>(&()))() } +} + +fn j() {} diff --git a/tests/ui/cross-crate/static-init.rs b/tests/ui/cross-crate/static-init.rs index 090ad5f810e16..c4697a1d010c7 100644 --- a/tests/ui/cross-crate/static-init.rs +++ b/tests/ui/cross-crate/static-init.rs @@ -6,6 +6,8 @@ extern crate static_init_aux as aux; static V: &u32 = aux::V; static F: fn() = aux::F; static G: fn() = aux::G; +static H: &(dyn Fn() + Sync) = aux::H; +static I: fn() = aux::I; fn v() -> *const u32 { V @@ -15,4 +17,6 @@ fn main() { assert_eq!(aux::v(), crate::v()); F(); G(); + H(); + I(); }