Skip to content
Open
Show file tree
Hide file tree
Changes from all 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
2 changes: 1 addition & 1 deletion compiler/rustc_codegen_cranelift/src/abi/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -467,7 +467,7 @@ pub(crate) fn codegen_terminator_call<'tcx>(
true
} else {
instance.is_some_and(|inst| {
fx.tcx.codegen_fn_attrs(inst.def_id()).flags.contains(CodegenFnAttrFlags::COLD)
fx.tcx.codegen_instance_attrs(inst.def).flags.contains(CodegenFnAttrFlags::COLD)
})
};
if is_cold {
Expand Down
3 changes: 2 additions & 1 deletion compiler/rustc_codegen_ssa/src/mir/block.rs
Original file line number Diff line number Diff line change
Expand Up @@ -200,10 +200,11 @@ impl<'a, 'tcx> TerminatorCodegenHelper<'tcx> {
let fn_ty = bx.fn_decl_backend_type(fn_abi);

let fn_attrs = if bx.tcx().def_kind(fx.instance.def_id()).has_codegen_attrs() {
Some(bx.tcx().codegen_fn_attrs(fx.instance.def_id()))
Some(bx.tcx().codegen_instance_attrs(fx.instance.def))
} else {
None
};
let fn_attrs = fn_attrs.as_deref();

if !fn_abi.can_unwind {
unwind = mir::UnwindAction::Unreachable;
Expand Down
24 changes: 24 additions & 0 deletions compiler/rustc_middle/src/middle/codegen_fn_attrs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ impl<'tcx> TyCtxt<'tcx> {
self,
instance_kind: InstanceKind<'_>,
) -> Cow<'tcx, CodegenFnAttrs> {
// NOTE: we try to not clone the `CodegenFnAttrs` when that is not needed.
// The `to_mut` method used below clones the inner value.
let mut attrs = Cow::Borrowed(self.codegen_fn_attrs(instance_kind.def_id()));

// Drop the `#[naked]` attribute on non-item `InstanceKind`s, like the shims that
Expand All @@ -23,6 +25,28 @@ impl<'tcx> TyCtxt<'tcx> {
}
}

// A shim created by `#[track_caller]` should not inherit any attributes
// that modify the symbol name. Failing to remove these attributes from
// the shim leads to errors like `symbol `foo` is already defined`.
//
// A `ClosureOnceShim` with the track_caller attribute does not have a symbol,
// and therefore can be skipped here.
if let InstanceKind::ReifyShim(_, _) = instance_kind
&& attrs.flags.contains(CodegenFnAttrFlags::TRACK_CALLER)
{
if attrs.flags.contains(CodegenFnAttrFlags::NO_MANGLE) {
attrs.to_mut().flags.remove(CodegenFnAttrFlags::NO_MANGLE);
}

if attrs.flags.contains(CodegenFnAttrFlags::RUSTC_STD_INTERNAL_SYMBOL) {
attrs.to_mut().flags.remove(CodegenFnAttrFlags::RUSTC_STD_INTERNAL_SYMBOL);
}

if attrs.symbol_name.is_some() {
attrs.to_mut().symbol_name = None;
}
Comment on lines +37 to +47
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't look like the ifs are necessary? They don't seem to be communicating anything either.

Suggested change
if attrs.flags.contains(CodegenFnAttrFlags::NO_MANGLE) {
attrs.to_mut().flags.remove(CodegenFnAttrFlags::NO_MANGLE);
}
if attrs.flags.contains(CodegenFnAttrFlags::RUSTC_STD_INTERNAL_SYMBOL) {
attrs.to_mut().flags.remove(CodegenFnAttrFlags::RUSTC_STD_INTERNAL_SYMBOL);
}
if attrs.symbol_name.is_some() {
attrs.to_mut().symbol_name = None;
}
attrs.to_mut().flags.remove(CodegenFnAttrFlags::NO_MANGLE);
attrs.to_mut().flags.remove(CodegenFnAttrFlags::RUSTC_STD_INTERNAL_SYMBOL);
attrs.to_mut().symbol_name = None;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The ifs prevent a clone (done by to_mut) if nothing needs to be updated. the vast majority of shims are not #[track_caller] functions with #[no_mangle], so unconditionally cloning the full attrs struct seemed expensive when we first introduced this logic.

Copy link
Member

Choose a reason for hiding this comment

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

hmm. what if we combined the ifs? given the explanation the current version is okay, but I feel like come comments would be beneficial (just to explain that we want to avoid expensive cloning)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a comment on the Cow::Borrowed up top. I don't think combining the if conditions helps?

Copy link
Member

Choose a reason for hiding this comment

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

I was wondering whether something like if attrs.flags.intersects(CodegenFnAttrFlags::NO_MANGLE | CodegenFnAttrFlags::RUSTC_STD_INTERNAL_SYMBOL) || attrs.symbol_name.is_some() {} would make it look nicer

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think, given the formatting, that just makes it messier. the current code may feel repetitive, but it is straightforward.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

like, this is shorter, but kind of too dense for my liking

        // A shim created by `#[track_caller]` should not inherit any attributes
        // that modify the symbol name. Failing to remove these attributes from
        // the shim leads to errors like `symbol `foo` is already defined`.
        //
        // A `ClosureOnceShim` with the track_caller attribute does not have a symbol,
        // and therefore can be skipped here.
        if let InstanceKind::ReifyShim(_, _) = instance_kind
            && attrs.flags.contains(CodegenFnAttrFlags::TRACK_CALLER)
            && (attrs.flags.intersects(
                CodegenFnAttrFlags::NO_MANGLE | CodegenFnAttrFlags::RUSTC_STD_INTERNAL_SYMBOL,
            ) || attrs.symbol_name.is_some())
        {
            attrs.to_mut().flags.remove(CodegenFnAttrFlags::NO_MANGLE);
            attrs.to_mut().flags.remove(CodegenFnAttrFlags::RUSTC_STD_INTERNAL_SYMBOL);
            attrs.to_mut().symbol_name = None;
        }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rustbot ready

lmk if you do strongly feel that the above is preferable.

}

attrs
}
}
Expand Down
5 changes: 3 additions & 2 deletions compiler/rustc_mir_transform/src/inline.rs
Original file line number Diff line number Diff line change
Expand Up @@ -248,7 +248,7 @@ impl<'tcx> Inliner<'tcx> for ForceInliner<'tcx> {
fn on_inline_failure(&self, callsite: &CallSite<'tcx>, reason: &'static str) {
let tcx = self.tcx();
let InlineAttr::Force { attr_span, reason: justification } =
tcx.codegen_fn_attrs(callsite.callee.def_id()).inline
tcx.codegen_instance_attrs(callsite.callee.def).inline
else {
bug!("called on item without required inlining");
};
Expand Down Expand Up @@ -606,7 +606,8 @@ fn try_inlining<'tcx, I: Inliner<'tcx>>(
let tcx = inliner.tcx();
check_mir_is_available(inliner, caller_body, callsite.callee)?;

let callee_attrs = tcx.codegen_fn_attrs(callsite.callee.def_id());
let callee_attrs = tcx.codegen_instance_attrs(callsite.callee.def);
let callee_attrs = callee_attrs.as_ref();
check_inline::is_inline_valid_on_fn(tcx, callsite.callee.def_id())?;
check_codegen_attributes(inliner, callsite, callee_attrs)?;
inliner.check_codegen_attributes_extra(callee_attrs)?;
Expand Down
26 changes: 26 additions & 0 deletions tests/ui/rfcs/rfc-2091-track-caller/shim-does-not-modify-symbol.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
//@ run-pass
#![feature(rustc_attrs)]

// The shim that is generated for a function annotated with `#[track_caller]` should not inherit
// attributes that modify its symbol name. Failing to remove these attributes from the shim
// leads to errors like `symbol `foo` is already defined`.
//
// See also https://github.com/rust-lang/rust/issues/143162.

#[unsafe(no_mangle)]
#[track_caller]
pub fn foo() {}

#[unsafe(export_name = "bar")]
#[track_caller]
pub fn bar() {}

#[rustc_std_internal_symbol]
#[track_caller]
pub fn baz() {}

fn main() {
let _a = foo as fn();
let _b = bar as fn();
let _c = baz as fn();
}
21 changes: 13 additions & 8 deletions tests/ui/rfcs/rfc-2091-track-caller/track-caller-ffi.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,14 @@

use std::panic::Location;

extern "Rust" {
unsafe extern "Rust" {
#[track_caller]
fn rust_track_caller_ffi_test_tracked() -> &'static Location<'static>;
fn rust_track_caller_ffi_test_untracked() -> &'static Location<'static>;
safe fn rust_track_caller_ffi_test_tracked() -> &'static Location<'static>;
safe fn rust_track_caller_ffi_test_untracked() -> &'static Location<'static>;
}

fn rust_track_caller_ffi_test_nested_tracked() -> &'static Location<'static> {
unsafe { rust_track_caller_ffi_test_tracked() }
rust_track_caller_ffi_test_tracked()
}

mod provides {
Expand All @@ -31,18 +31,23 @@ fn main() {
assert_eq!(location.line(), 29);
assert_eq!(location.column(), 20);

let tracked = unsafe { rust_track_caller_ffi_test_tracked() };
let tracked = rust_track_caller_ffi_test_tracked();
assert_eq!(tracked.file(), file!());
assert_eq!(tracked.line(), 34);
assert_eq!(tracked.column(), 28);
assert_eq!(tracked.column(), 19);

let untracked = unsafe { rust_track_caller_ffi_test_untracked() };
let untracked = rust_track_caller_ffi_test_untracked();
assert_eq!(untracked.file(), file!());
assert_eq!(untracked.line(), 24);
assert_eq!(untracked.column(), 9);

let contained = rust_track_caller_ffi_test_nested_tracked();
assert_eq!(contained.file(), file!());
assert_eq!(contained.line(), 12);
assert_eq!(contained.column(), 14);
assert_eq!(contained.column(), 5);

let indirect = (rust_track_caller_ffi_test_tracked as fn() -> &'static Location<'static>)();
assert_eq!(indirect.file(), file!());
assert_eq!(indirect.line(), 7);
assert_eq!(indirect.column(), 5);
}
Loading