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

Enable -Zshare-generics for inline(never) functions #123244

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 3 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
1 change: 1 addition & 0 deletions Cargo.lock
Original file line number Diff line number Diff line change
Expand Up @@ -4493,6 +4493,7 @@ dependencies = [
name = "rustc_monomorphize"
version = "0.0.0"
dependencies = [
"rustc_attr",
"rustc_data_structures",
"rustc_errors",
"rustc_fluent_macro",
Expand Down
4 changes: 3 additions & 1 deletion compiler/rustc_codegen_llvm/src/callee.rs
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,9 @@ pub fn get_fn<'ll, 'tcx>(cx: &CodegenCx<'ll, 'tcx>, instance: Instance<'tcx>) ->
// This is a monomorphization. Its expected visibility depends
// on whether we are in share-generics mode.

if cx.tcx.sess.opts.share_generics() {
if cx.tcx.sess.opts.share_generics()
|| tcx.codegen_fn_attrs(instance_def_id).inline == rustc_attr::InlineAttr::Never
{
// We are in share_generics mode.

if let Some(instance_def_id) = instance_def_id.as_local() {
Expand Down
12 changes: 11 additions & 1 deletion compiler/rustc_codegen_ssa/src/back/symbol_export.rs
Original file line number Diff line number Diff line change
Expand Up @@ -308,7 +308,7 @@ fn exported_symbols_provider_local(
));
}

if tcx.sess.opts.share_generics() && tcx.local_crate_exports_generics() {
if tcx.local_crate_exports_generics() {
use rustc_middle::mir::mono::{Linkage, MonoItem, Visibility};
use rustc_middle::ty::InstanceDef;

Expand Down Expand Up @@ -336,6 +336,16 @@ fn exported_symbols_provider_local(
continue;
}

if !tcx.sess.opts.share_generics() {
if tcx.codegen_fn_attrs(mono_item.def_id()).inline == rustc_attr::InlineAttr::Never
{
// this is OK, we explicitly allow sharing inline(never) across crates even
// without share-generics.
} else {
continue;
}
}

match *mono_item {
MonoItem::Fn(Instance { def: InstanceDef::Item(def), args }) => {
if args.non_erasable_generics(tcx, def).next().is_some() {
Expand Down
7 changes: 7 additions & 0 deletions compiler/rustc_middle/src/mir/mono.rs
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,13 @@ impl<'tcx> MonoItem<'tcx> {
return InstantiationMode::GloballyShared { may_conflict: false };
}

if let InlineAttr::Never = tcx.codegen_fn_attrs(instance.def_id()).inline
&& self.is_generic_fn(tcx)
{
// Upgrade inline(never) to a globally shared instance.
return InstantiationMode::GloballyShared { may_conflict: true };
}

// At this point we don't have explicit linkage and we're an
// inlined function. If we're inlining into all CGUs then we'll
// be creating a local copy per CGU.
Expand Down
2 changes: 0 additions & 2 deletions compiler/rustc_middle/src/ty/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1503,8 +1503,6 @@ impl<'tcx> TyCtxt<'tcx> {

#[inline]
pub fn local_crate_exports_generics(self) -> bool {
debug_assert!(self.sess.opts.share_generics());

self.crate_types().iter().any(|crate_type| {
match crate_type {
CrateType::Executable
Expand Down
18 changes: 11 additions & 7 deletions compiler/rustc_middle/src/ty/instance.rs
Original file line number Diff line number Diff line change
Expand Up @@ -194,19 +194,23 @@ impl<'tcx> Instance<'tcx> {
/// This method already takes into account the global `-Zshare-generics`
/// setting, always returning `None` if `share-generics` is off.
pub fn upstream_monomorphization(&self, tcx: TyCtxt<'tcx>) -> Option<CrateNum> {
// If we are not in share generics mode, we don't link to upstream
// monomorphizations but always instantiate our own internal versions
// instead.
if !tcx.sess.opts.share_generics() {
return None;
}

// If this is an item that is defined in the local crate, no upstream
// crate can know about it/provide a monomorphization.
if self.def_id().is_local() {
return None;
}

// If we are not in share generics mode, we don't link to upstream
// monomorphizations but always instantiate our own internal versions
// instead.
if !tcx.sess.opts.share_generics()
// However, if the def_id is marked inline(never), then it's fine to just reuse the
// upstream monomorphization.
&& tcx.codegen_fn_attrs(self.def_id()).inline != rustc_attr::InlineAttr::Never
{
return None;
}

// If this a non-generic instance, it cannot be a shared monomorphization.
self.args.non_erasable_generics(tcx, self.def_id()).next()?;

Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_monomorphize/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ edition = "2021"

[dependencies]
# tidy-alphabetical-start
rustc_attr = { path = "../rustc_attr" }
rustc_data_structures = { path = "../rustc_data_structures" }
rustc_errors = { path = "../rustc_errors" }
rustc_fluent_macro = { path = "../rustc_fluent_macro" }
Expand Down
34 changes: 26 additions & 8 deletions compiler/rustc_monomorphize/src/partitioning.rs
Original file line number Diff line number Diff line change
Expand Up @@ -206,8 +206,8 @@ where
// available to downstream crates. This depends on whether we are in
// share-generics mode and whether the current crate can even have
// downstream crates.
let export_generics =
cx.tcx.sess.opts.share_generics() && cx.tcx.local_crate_exports_generics();
let can_export_generics = cx.tcx.local_crate_exports_generics();
let always_export_generics = can_export_generics && cx.tcx.sess.opts.share_generics();

let cgu_name_builder = &mut CodegenUnitNameBuilder::new(cx.tcx);
let cgu_name_cache = &mut UnordMap::default();
Expand Down Expand Up @@ -247,7 +247,8 @@ where
cx.tcx,
&mono_item,
&mut can_be_internalized,
export_generics,
can_export_generics,
always_export_generics,
);
if visibility == Visibility::Hidden && can_be_internalized {
internalization_candidates.insert(mono_item);
Expand Down Expand Up @@ -734,12 +735,19 @@ fn mono_item_linkage_and_visibility<'tcx>(
tcx: TyCtxt<'tcx>,
mono_item: &MonoItem<'tcx>,
can_be_internalized: &mut bool,
export_generics: bool,
can_export_generics: bool,
always_export_generics: bool,
) -> (Linkage, Visibility) {
if let Some(explicit_linkage) = mono_item.explicit_linkage(tcx) {
return (explicit_linkage, Visibility::Default);
}
let vis = mono_item_visibility(tcx, mono_item, can_be_internalized, export_generics);
let vis = mono_item_visibility(
tcx,
mono_item,
can_be_internalized,
can_export_generics,
always_export_generics,
);
(Linkage::External, vis)
}

Expand All @@ -762,7 +770,8 @@ fn mono_item_visibility<'tcx>(
tcx: TyCtxt<'tcx>,
mono_item: &MonoItem<'tcx>,
can_be_internalized: &mut bool,
export_generics: bool,
can_export_generics: bool,
always_export_generics: bool,
) -> Visibility {
let instance = match mono_item {
// This is pretty complicated; see below.
Expand Down Expand Up @@ -822,7 +831,11 @@ fn mono_item_visibility<'tcx>(

// Upstream `DefId` instances get different handling than local ones.
let Some(def_id) = def_id.as_local() else {
return if export_generics && is_generic {
return if is_generic
&& (always_export_generics
|| (can_export_generics
&& tcx.codegen_fn_attrs(def_id).inline == rustc_attr::InlineAttr::Never))
{
// If it is an upstream monomorphization and we export generics, we must make
// it available to downstream crates.
*can_be_internalized = false;
Expand All @@ -833,7 +846,12 @@ fn mono_item_visibility<'tcx>(
};

if is_generic {
if export_generics {
// An inline(never) function won't get inlined in upstream crates anyway, so we might as
// well export it.
if always_export_generics
|| (can_export_generics
&& tcx.codegen_fn_attrs(def_id).inline == rustc_attr::InlineAttr::Never)
{
if tcx.is_unreachable_local_definition(def_id) {
// This instance cannot be used from another crate.
Visibility::Hidden
Expand Down
1 change: 0 additions & 1 deletion library/alloc/src/raw_vec.rs
Original file line number Diff line number Diff line change
Expand Up @@ -548,7 +548,6 @@ impl<T, A: Allocator> RawVec<T, A> {
// above `RawVec::grow_amortized` for details. (The `A` parameter isn't
// significant, because the number of different `A` types seen in practice is
// much smaller than the number of `T` types.)
#[inline(never)]
fn finish_grow<A>(
Mark-Simulacrum marked this conversation as resolved.
Show resolved Hide resolved
new_layout: Result<Layout, LayoutError>,
current_memory: Option<(NonNull<u8>, Layout)>,
Expand Down
8 changes: 7 additions & 1 deletion library/alloc/src/vec/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1036,7 +1036,13 @@ impl<T, A: Allocator> Vec<T, A> {
/// ```
#[stable(feature = "try_reserve", since = "1.57.0")]
pub fn try_reserve(&mut self, additional: usize) -> Result<(), TryReserveError> {
self.buf.try_reserve(self.len, additional)
let len = self.len;
let ret = self.buf.try_reserve(len, additional);
unsafe {
// Inform the optimier that growing did not change the length.
core::hint::assert_unchecked(self.len == len);
}
ret
}

/// Tries to reserve the minimum capacity for at least `additional`
Expand Down
16 changes: 16 additions & 0 deletions library/std/src/panicking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -733,6 +733,22 @@ pub const fn begin_panic<M: Any + Send>(msg: M) -> ! {
}
}

// This forces codegen of the function called by panic!() inside the std crate, rather than in
// downstream crates. Primarily this is useful for rustc's codegen tests, which rely on noticing
// complete removal of panic from generated IR. Since begin_panic is inline(never), it's only
// codegen'd once per crate-graph so this pushes that to std rather than our codegen test crates.
//
// (See https://github.com/rust-lang/rust/pull/123244 for more info on why).
//
// If this is causing problems we can also modify those codegen tests to use a crate type like
// cdylib which doesn't export "Rust" symbols to downstream linkage units.
#[unstable(feature = "libstd_sys_internals", reason = "used by the panic! macro", issue = "none")]
#[doc(hidden)]
#[allow(dead_code)]
#[used]
pub static EMPTY_PANIC: fn(&'static str) -> ! =
begin_panic::<&'static str> as fn(&'static str) -> !;

/// Central point for dispatching panics.
///
/// Executes the primary logic for a panic, including checking for recursive
Expand Down
11 changes: 11 additions & 0 deletions tests/codegen-units/partitioning/auxiliary/cgu_generic_function.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,21 @@ pub fn foo<T>(x: T) -> (T, u32, i8) {
#[inline(never)]
fn bar<T>(x: T) -> (T, Struct) {
let _ = not_exported_and_not_generic(0);
exported_and_generic::<u32>(0);
(x, Struct(1))
}

pub static F: fn(u32) -> u32 = exported_and_generic::<u32>;

// These should not contribute to the codegen items of other crates.

// This is generic, but it's only instantiated with a u32 argument and that instantiation is present
// in the local crate (see F above).
Comment on lines +22 to +23
Copy link
Member

Choose a reason for hiding this comment

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

Is this testing what we want? I was expecting to see a cross-crate call to a #[inline(never)] generic function in a test, because I think the point of this PR is to change the behavior for such calls, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this is indeed checking that behavior, though it's rather obscure. As per the comment just above ("These should not contribute...") we are implicitly checking that these functions are absent in the mono-items of the downstream crate (tests/codegen-units/partitioning/extern-generic.rs) since they're not listed as MONO-ITEM declarations in that file. They are referenced though via the foo function above which has to get codegen'd in the downstream crate since it is generic and never called in this one.

I suppose we could try to write a more direct test case? But this also tests that the property holds transitively (i.e., we don't codegen the called function twice with inline(never) even if it's not directly being called).

IIRC, before my changes, this would fail.

Copy link
Member

Choose a reason for hiding this comment

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

Oh I see! This is very interconnected, and I should really be used to that by now.

Copy link
Member

Choose a reason for hiding this comment

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

Also I checked and yes the test does fail without the rest of this PR.

#[inline(never)]
pub fn exported_and_generic<T>(x: T) -> T {
x
}

#[inline(never)]
pub fn exported_but_not_generic(x: i32) -> i64 {
x as i64
Expand Down
2 changes: 1 addition & 1 deletion tests/codegen/avr/avr-func-addrspace.rs
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ pub extern "C" fn test() {

// A call through the Fn trait must use address space 1.
//
// CHECK: call{{.+}}addrspace(1) void @call_through_fn_trait()
// CHECK: call{{.+}}addrspace(1) void @call_through_fn_trait(ptr {{.*}} poison)
call_through_fn_trait(&mut update_bar_value);

// A call through a global variable must use address space 1.
Expand Down
1 change: 1 addition & 0 deletions tests/codegen/issues/issue-77812.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ extern "C" {
fn exf2();
}

#[no_mangle]
pub static mut GLOBAL: Variant = Variant::Zero;

// CHECK-LABEL: @issue_77812
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
thread 'main' panicked at $DIR/issue-47429-short-backtraces.rs:23:5:
explicit panic
stack backtrace:
0: std::panicking::begin_panic
0: std::panicking::begin_panic::<&str>
1: issue_47429_short_backtraces::main
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
2 changes: 1 addition & 1 deletion tests/ui/panics/runtime-switch.legacy.run.stderr
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
thread 'main' panicked at $DIR/runtime-switch.rs:26:5:
explicit panic
stack backtrace:
0: std::panicking::begin_panic
0: std::panicking::begin_panic::<&str>
1: runtime_switch::main
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
thread 'main' panicked at $DIR/short-ice-remove-middle-frames-2.rs:56:5:
debug!!!
stack backtrace:
0: std::panicking::begin_panic
0: std::panicking::begin_panic::<&str>
1: short_ice_remove_middle_frames_2::eight
2: short_ice_remove_middle_frames_2::seven::{{closure}}
[... omitted 3 frames ...]
Expand Down
2 changes: 1 addition & 1 deletion tests/ui/panics/short-ice-remove-middle-frames.run.stderr
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
thread 'main' panicked at $DIR/short-ice-remove-middle-frames.rs:52:5:
debug!!!
stack backtrace:
0: std::panicking::begin_panic
0: std::panicking::begin_panic::<&str>
1: short_ice_remove_middle_frames::seven
2: short_ice_remove_middle_frames::sixth
3: short_ice_remove_middle_frames::fifth::{{closure}}
Expand Down
Loading