Skip to content

Commit

Permalink
declare imported functions with a nondescript signature
Browse files Browse the repository at this point in the history
This means when the function is imported with multiple different signatures, they don't clash
  • Loading branch information
RalfJung committed Aug 18, 2024
1 parent f04f6ca commit d4a7a52
Show file tree
Hide file tree
Showing 15 changed files with 124 additions and 48 deletions.
2 changes: 1 addition & 1 deletion compiler/rustc_ast_passes/src/feature_gate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -324,7 +324,7 @@ impl<'a> Visitor<'a> for PostExpansionVisitor<'a> {
&self,
link_llvm_intrinsics,
i.span,
"linking to LLVM intrinsics is experimental"
"linking to LLVM intrinsics is an internal feature reserved for the standard library"
);
}
}
Expand Down
25 changes: 22 additions & 3 deletions compiler/rustc_codegen_llvm/src/abi.rs
Original file line number Diff line number Diff line change
Expand Up @@ -321,7 +321,12 @@ pub trait FnAbiLlvmExt<'ll, 'tcx> {
);

/// Apply attributes to a function call.
fn apply_attrs_callsite(&self, bx: &mut Builder<'_, 'll, 'tcx>, callsite: &'ll Value);
fn apply_attrs_callsite(
&self,
bx: &mut Builder<'_, 'll, 'tcx>,
callsite: &'ll Value,
instance: Option<ty::Instance<'tcx>>,
);
}

impl<'ll, 'tcx> FnAbiLlvmExt<'ll, 'tcx> for FnAbi<'tcx, Ty<'tcx>> {
Expand Down Expand Up @@ -527,11 +532,18 @@ impl<'ll, 'tcx> FnAbiLlvmExt<'ll, 'tcx> for FnAbi<'tcx, Ty<'tcx>> {

// If the declaration has an associated instance, compute extra attributes based on that.
if let Some(instance) = instance {
llfn_attrs_from_instance(cx, llfn, instance);
llfn_attrs_from_instance(cx, instance, Some(llfn), |place, attrs| {
attributes::apply_to_llfn(llfn, place, attrs)
});
}
}

fn apply_attrs_callsite(&self, bx: &mut Builder<'_, 'll, 'tcx>, callsite: &'ll Value) {
fn apply_attrs_callsite(
&self,
bx: &mut Builder<'_, 'll, 'tcx>,
callsite: &'ll Value,
instance: Option<ty::Instance<'tcx>>,
) {
let mut func_attrs = SmallVec::<[_; 2]>::new();
if self.ret.layout.abi.is_uninhabited() {
func_attrs.push(llvm::AttributeKind::NoReturn.create_attr(bx.cx.llcx));
Expand Down Expand Up @@ -649,6 +661,13 @@ impl<'ll, 'tcx> FnAbiLlvmExt<'ll, 'tcx> for FnAbi<'tcx, Ty<'tcx>> {
&[element_type_attr],
);
}

// If the call site has an associated instance, compute extra attributes based on that.
if let Some(instance) = instance {
llfn_attrs_from_instance(bx.cx, instance, None, |place, attrs| {
attributes::apply_to_callsite(callsite, place, attrs)
});
}
}
}

Expand Down
29 changes: 18 additions & 11 deletions compiler/rustc_codegen_llvm/src/attributes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@ use crate::context::CodegenCx;
use crate::errors::{MissingFeatures, SanitizerMemtagRequiresMte, TargetFeatureDisableOrEnable};
use crate::llvm::AttributePlace::Function;
use crate::llvm::{self, AllocKindFlags, Attribute, AttributeKind, AttributePlace, MemoryEffects};
use crate::llvm_util;
use crate::value::Value;
use crate::{attributes, llvm_util};

pub fn apply_to_llfn(llfn: &Value, idx: AttributePlace, attrs: &[&Attribute]) {
if !attrs.is_empty() {
Expand Down Expand Up @@ -324,13 +324,18 @@ fn create_alloc_family_attr(llcx: &llvm::Context) -> &llvm::Attribute {
llvm::CreateAttrStringValue(llcx, "alloc-family", "__rust_alloc")
}

/// Helper for `FnAbi::apply_attrs_llfn`:
/// Helper for `FnAbi::apply_attrs_*`:
/// Composite function which sets LLVM attributes for function depending on its AST (`#[attribute]`)
/// attributes.
///
/// `apply_attrs` is called to apply the attributes, so this can be used both for declarations and
/// calls. However, some things are not represented as attributes and can only be set on
/// declarations, so `declare_llfn` should be `Some` if this is a declaration.
pub fn llfn_attrs_from_instance<'ll, 'tcx>(
cx: &CodegenCx<'ll, 'tcx>,
llfn: &'ll Value,
instance: ty::Instance<'tcx>,
declare_llfn: Option<&'ll Value>,
apply_attrs: impl Fn(AttributePlace, &[&Attribute]),
) {
let codegen_fn_attrs = cx.tcx.codegen_fn_attrs(instance.def_id());

Expand Down Expand Up @@ -440,7 +445,7 @@ pub fn llfn_attrs_from_instance<'ll, 'tcx>(
to_add.push(create_alloc_family_attr(cx.llcx));
// apply to argument place instead of function
let alloc_align = AttributeKind::AllocAlign.create_attr(cx.llcx);
attributes::apply_to_llfn(llfn, AttributePlace::Argument(1), &[alloc_align]);
apply_attrs(AttributePlace::Argument(1), &[alloc_align]);
to_add.push(llvm::CreateAllocSizeAttr(cx.llcx, 0));
let mut flags = AllocKindFlags::Alloc | AllocKindFlags::Aligned;
if codegen_fn_attrs.flags.contains(CodegenFnAttrFlags::ALLOCATOR) {
Expand All @@ -451,7 +456,7 @@ pub fn llfn_attrs_from_instance<'ll, 'tcx>(
to_add.push(llvm::CreateAllocKindAttr(cx.llcx, flags));
// apply to return place instead of function (unlike all other attributes applied in this function)
let no_alias = AttributeKind::NoAlias.create_attr(cx.llcx);
attributes::apply_to_llfn(llfn, AttributePlace::ReturnValue, &[no_alias]);
apply_attrs(AttributePlace::ReturnValue, &[no_alias]);
}
if codegen_fn_attrs.flags.contains(CodegenFnAttrFlags::REALLOCATOR) {
to_add.push(create_alloc_family_attr(cx.llcx));
Expand All @@ -461,26 +466,28 @@ pub fn llfn_attrs_from_instance<'ll, 'tcx>(
));
// applies to argument place instead of function place
let allocated_pointer = AttributeKind::AllocatedPointer.create_attr(cx.llcx);
attributes::apply_to_llfn(llfn, AttributePlace::Argument(0), &[allocated_pointer]);
apply_attrs(AttributePlace::Argument(0), &[allocated_pointer]);
// apply to argument place instead of function
let alloc_align = AttributeKind::AllocAlign.create_attr(cx.llcx);
attributes::apply_to_llfn(llfn, AttributePlace::Argument(2), &[alloc_align]);
apply_attrs(AttributePlace::Argument(2), &[alloc_align]);
to_add.push(llvm::CreateAllocSizeAttr(cx.llcx, 3));
let no_alias = AttributeKind::NoAlias.create_attr(cx.llcx);
attributes::apply_to_llfn(llfn, AttributePlace::ReturnValue, &[no_alias]);
apply_attrs(AttributePlace::ReturnValue, &[no_alias]);
}
if codegen_fn_attrs.flags.contains(CodegenFnAttrFlags::DEALLOCATOR) {
to_add.push(create_alloc_family_attr(cx.llcx));
to_add.push(llvm::CreateAllocKindAttr(cx.llcx, AllocKindFlags::Free));
// applies to argument place instead of function place
let allocated_pointer = AttributeKind::AllocatedPointer.create_attr(cx.llcx);
attributes::apply_to_llfn(llfn, AttributePlace::Argument(0), &[allocated_pointer]);
apply_attrs(AttributePlace::Argument(0), &[allocated_pointer]);
}
if codegen_fn_attrs.flags.contains(CodegenFnAttrFlags::CMSE_NONSECURE_ENTRY) {
to_add.push(llvm::CreateAttrString(cx.llcx, "cmse_nonsecure_entry"));
}
if let Some(align) = codegen_fn_attrs.alignment {
llvm::set_alignment(llfn, align);
if let Some(llfn) = declare_llfn {
llvm::set_alignment(llfn, align);
}
}
if let Some(backchain) = backchain_attr(cx) {
to_add.push(backchain);
Expand Down Expand Up @@ -551,7 +558,7 @@ pub fn llfn_attrs_from_instance<'ll, 'tcx>(
to_add.push(llvm::CreateAttrStringValue(cx.llcx, "target-features", &target_features));
}

attributes::apply_to_llfn(llfn, Function, &to_add);
apply_attrs(Function, &to_add);
}

fn wasm_import_module(tcx: TyCtxt<'_>, id: DefId) -> Option<&String> {
Expand Down
6 changes: 3 additions & 3 deletions compiler/rustc_codegen_llvm/src/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -262,7 +262,7 @@ impl<'a, 'll, 'tcx> BuilderMethods<'a, 'tcx> for Builder<'a, 'll, 'tcx> {
)
};
if let Some(fn_abi) = fn_abi {
fn_abi.apply_attrs_callsite(self, invoke);
fn_abi.apply_attrs_callsite(self, invoke, instance);
}
invoke
}
Expand Down Expand Up @@ -1307,7 +1307,7 @@ impl<'a, 'll, 'tcx> BuilderMethods<'a, 'tcx> for Builder<'a, 'll, 'tcx> {
)
};
if let Some(fn_abi) = fn_abi {
fn_abi.apply_attrs_callsite(self, call);
fn_abi.apply_attrs_callsite(self, call, instance);
}
call
}
Expand Down Expand Up @@ -1645,7 +1645,7 @@ impl<'a, 'll, 'tcx> Builder<'a, 'll, 'tcx> {
)
};
if let Some(fn_abi) = fn_abi {
fn_abi.apply_attrs_callsite(self, callbr);
fn_abi.apply_attrs_callsite(self, callbr, instance);
}
callbr
}
Expand Down
40 changes: 39 additions & 1 deletion compiler/rustc_codegen_llvm/src/declare.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,9 @@
//! * When in doubt, define.

use itertools::Itertools;
use rustc_codegen_ssa::traits::TypeMembershipMethods;
use rustc_codegen_ssa::traits::{BaseTypeMethods, TypeMembershipMethods};
use rustc_data_structures::fx::FxIndexSet;
use rustc_middle::middle::codegen_fn_attrs::CodegenFnAttrFlags;
use rustc_middle::ty::{Instance, Ty};
use rustc_sanitizers::{cfi, kcfi};
use smallvec::SmallVec;
Expand Down Expand Up @@ -127,6 +128,43 @@ impl<'ll, 'tcx> CodegenCx<'ll, 'tcx> {
) -> &'ll Value {
debug!("declare_rust_fn(name={:?}, fn_abi={:?})", name, fn_abi);

// FFI imports need to be treated specially: in Rust one can import the same function
// with different signatures, but LLVM can only import each function once. Within a
// codegen unit, whatever declaration we set up last will "win"; across codegen units,
// LLVM is doing some sort of unspecified merging as part of LTO.
// This is partially unsound due to LLVM bugs (https://github.com/llvm/llvm-project/issues/58976),
// but on the Rust side we try to ensure soundness by making the least amount of promises
// for these imports: no matter the signatures, we declare all functions as `void fn()`.
// If the same symbol is declared both as a function and a static, we just hope that
// doesn't lead to unsoundness (or LLVM crashes)...
let is_foreign_item = instance.is_some_and(|i| self.tcx.is_foreign_item(i.def_id()));
// If this is a Rust native allocator function, we want its attributes, so we do not
// treat it like the other FFI imports. If a user declares `__rust_alloc` we're going to have
// one import with the attributes and one with the default signature; that should hopefully be fine
// even if LLVM only sees the default one.
let is_rust_alloc_fn = instance.is_some_and(|i| {
let codegen_fn_flags = self.tcx.codegen_fn_attrs(i.def_id()).flags;
codegen_fn_flags.contains(CodegenFnAttrFlags::ALLOCATOR)
|| codegen_fn_flags.contains(CodegenFnAttrFlags::ALLOCATOR_ZEROED)
|| codegen_fn_flags.contains(CodegenFnAttrFlags::REALLOCATOR)
|| codegen_fn_flags.contains(CodegenFnAttrFlags::DEALLOCATOR)
});
// If this is calling an LLVM intrinsic, we must set the right signature or LLVM complains.
// These are also unstable to call so nobody but us can screw up their signature.
let is_llvm_builtin = name.starts_with("llvm.");
if is_foreign_item && !is_rust_alloc_fn && !is_llvm_builtin {
// This signatures is essentially irrelevant since we set all the attributes at the call
// site.
return declare_raw_fn(
self,
name,
llvm::CallConv::CCallConv,
llvm::UnnamedAddr::Global,
llvm::Visibility::Default,
self.type_func(&[], self.type_void()),
);
}

// Function addresses in Rust are never significant, allowing functions to
// be merged.
let llfn = declare_raw_fn(
Expand Down
6 changes: 3 additions & 3 deletions tests/codegen/box-uninit-bytes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ use std::mem::MaybeUninit;
pub fn box_uninitialized() -> Box<MaybeUninit<usize>> {
// CHECK-LABEL: @box_uninitialized
// CHECK-NOT: store
// CHECK-NOT: alloca
// CHECK-NOT: "alloca "
// CHECK-NOT: memcpy
// CHECK-NOT: memset
Box::new(MaybeUninit::uninit())
Expand All @@ -19,7 +19,7 @@ pub fn box_uninitialized() -> Box<MaybeUninit<usize>> {
pub fn box_uninitialized2() -> Box<MaybeUninit<[usize; 1024 * 1024]>> {
// CHECK-LABEL: @box_uninitialized2
// CHECK-NOT: store
// CHECK-NOT: alloca
// CHECK-NOT: "alloca ""
// CHECK-NOT: memcpy
// CHECK-NOT: memset
Box::new(MaybeUninit::uninit())
Expand All @@ -32,7 +32,7 @@ pub struct LotsaPadding(usize);
#[no_mangle]
pub fn box_lotsa_padding() -> Box<LotsaPadding> {
// CHECK-LABEL: @box_lotsa_padding
// CHECK-NOT: alloca
// CHECK-NOT: "alloca "
// CHECK-NOT: getelementptr
// CHECK-NOT: memcpy
// CHECK-NOT: memset
Expand Down
11 changes: 7 additions & 4 deletions tests/codegen/cffi/ffi-const.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,18 @@
#![crate_type = "lib"]
#![feature(ffi_const)]

#[no_mangle]
pub fn bar() {
// CHECK-LABEL: @bar
// CHECK: @foo
// CHECK-SAME: [[ATTRS:#[0-9]+]]
unsafe { foo() }
}

extern "C" {
// CHECK-LABEL: declare{{.*}}void @foo()
// CHECK-SAME: [[ATTRS:#[0-9]+]]
// The attribute changed from `readnone` to `memory(none)` with LLVM 16.0.
// CHECK-DAG: attributes [[ATTRS]] = { {{.*}}{{readnone|memory\(none\)}}{{.*}} }
#[ffi_const]
pub fn foo();
}

// The attribute changed from `readnone` to `memory(none)` with LLVM 16.0.
// CHECK: attributes [[ATTRS]] = { {{.*}}{{readnone|memory\(none\)}}{{.*}} }
11 changes: 7 additions & 4 deletions tests/codegen/cffi/ffi-pure.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,18 @@
#![crate_type = "lib"]
#![feature(ffi_pure)]

#[no_mangle]
pub fn bar() {
// CHECK-LABEL: @bar
// CHECK: @foo
// CHECK-SAME: [[ATTRS:#[0-9]+]]
unsafe { foo() }
}

extern "C" {
// CHECK-LABEL: declare{{.*}}void @foo()
// CHECK-SAME: [[ATTRS:#[0-9]+]]
// The attribute changed from `readonly` to `memory(read)` with LLVM 16.0.
// CHECK-DAG: attributes [[ATTRS]] = { {{.*}}{{readonly|memory\(read\)}}{{.*}} }
#[ffi_pure]
pub fn foo();
}

// The attribute changed from `readonly` to `memory(read)` with LLVM 16.0.
// CHECK: attributes [[ATTRS]] = { {{.*}}{{readonly|memory\(read\)}}{{.*}} }
2 changes: 1 addition & 1 deletion tests/codegen/issues/issue-111603.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ pub fn new_from_array(x: u64) -> Arc<[u64]> {

// CHECK: alloca
// CHECK-SAME: [8000 x i8]
// CHECK-NOT: alloca
// CHECK-NOT: "alloca "
let array = [x; 1000];
Arc::new(array)
}
Expand Down
2 changes: 1 addition & 1 deletion tests/codegen/iter-repeat-n-trivial-drop.rs
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ pub fn iter_repeat_n_next(it: &mut std::iter::RepeatN<NotCopy>) -> Option<NotCop
#[no_mangle]
// CHECK-LABEL: @vec_extend_via_iter_repeat_n
pub fn vec_extend_via_iter_repeat_n() -> Vec<u8> {
// CHECK: %[[ADDR:.+]] = tail call {{(noalias )?}}noundef dereferenceable_or_null(1234) ptr @__rust_alloc(i64 noundef 1234, i64 noundef 1)
// CHECK: %[[ADDR:.+]] = tail call {{(noalias )?}}noundef dereferenceable_or_null(1234) ptr @__rust_alloc(i64 noundef 1234, i64 {{(allocalign )?}}noundef 1)
// CHECK: tail call void @llvm.memset.p0.i64(ptr noundef nonnull align 1 dereferenceable(1234) %[[ADDR]], i8 42, i64 1234,

let n = 1234_usize;
Expand Down
6 changes: 2 additions & 4 deletions tests/codegen/no_builtins-at-crate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,15 +10,13 @@
pub unsafe extern "C" fn __aeabi_memcpy(dest: *mut u8, src: *const u8, size: usize) {
// CHECK: call
// CHECK-SAME: @memcpy(
// CHECK-SAME: #2
memcpy(dest, src, size);
}

// CHECK: declare
// CHECK-SAME: @memcpy
// CHECK-SAME: #0
extern "C" {
pub fn memcpy(dest: *mut u8, src: *const u8, n: usize) -> *mut u8;
}

// CHECK: attributes #0
// CHECK: attributes #2
// CHECK-SAME: "no-builtins"
5 changes: 1 addition & 4 deletions tests/codegen/pic-relocation-model.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,7 @@ pub fn call_foreign_fn() -> u8 {
unsafe { foreign_fn() }
}

// (Allow but do not require `zeroext` here, because it is not worth effort to
// spell out which targets have it and which ones do not; see rust#97800.)

// CHECK: declare{{( zeroext)?}} i8 @foreign_fn()
// CHECK: declare void @foreign_fn()
extern "C" {
fn foreign_fn() -> u8;
}
Expand Down
2 changes: 1 addition & 1 deletion tests/codegen/pie-relocation-model.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ pub fn call_foreign_fn() -> u8 {

// External functions are still marked as non-dso_local, since we don't know if the symbol
// is defined in the binary or in the shared library.
// CHECK: declare zeroext i8 @foreign_fn()
// CHECK: declare void @foreign_fn()
extern "C" {
fn foreign_fn() -> u8;
}
Expand Down
22 changes: 16 additions & 6 deletions tests/codegen/unwind-extern-imports.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,22 @@

#![crate_type = "lib"]

// The flag are only at the call sites, not at the declarations.
// CHECK-LABEL: @force_declare
#[no_mangle]
pub unsafe fn force_declare() {
// CHECK: call void @extern_fn() [[NOUNWIND_ATTR:#[0-9]+]]
extern_fn();
// Call without attributes.
// FIXME: Make sure that there truly are no attributes!
// How can we match "end of line" with FileCheck?
// CHECK: call void @c_unwind_extern_fn()
c_unwind_extern_fn();
}

extern "C" {
// CHECK: Function Attrs:{{.*}}nounwind
// CHECK-NEXT: declare{{.*}}void @extern_fn
// CHECK-NOT: nounwind
// CHECK: declare{{.*}}void @extern_fn
fn extern_fn();
}

Expand All @@ -15,7 +28,4 @@ extern "C-unwind" {
fn c_unwind_extern_fn();
}

pub unsafe fn force_declare() {
extern_fn();
c_unwind_extern_fn();
}
// CHECK: attributes [[NOUNWIND_ATTR]] = { {{.*}}nounwind{{.*}} }
Loading

0 comments on commit d4a7a52

Please sign in to comment.