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.

Sign up for GitHub

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

cg_llvm: Clean up FFI calls for operand bundles #132342

Merged
merged 1 commit into from
Oct 30, 2024
+103 −138
Merged

cg_llvm: Clean up FFI calls for operand bundles #132342

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 2 additions & 1 deletion compiler/rustc_codegen_llvm/src/allocator.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,14 +154,15 @@ fn create_wrapper_function(
.enumerate()
.map(|(i, _)| llvm::LLVMGetParam(llfn, i as c_uint))
.collect::<Vec<_>>();
let ret = llvm::LLVMRustBuildCall(
let ret = llvm::LLVMBuildCallWithOperandBundles(
llbuilder,
ty,
callee,
args.as_ptr(),
args.len() as c_uint,
[].as_ptr(),
0 as c_uint,
c"".as_ptr(),
);
llvm::LLVMSetTailCall(ret, True);
if output.is_some() {
Expand Down
23 changes: 9 additions & 14 deletions compiler/rustc_codegen_llvm/src/builder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -239,7 +239,6 @@ impl<'a, 'll, 'tcx> BuilderMethods<'a, 'tcx> for Builder<'a, 'll, 'tcx> {

let args = self.check_call("invoke", llty, llfn, args);
let funclet_bundle = funclet.map(|funclet| funclet.bundle());
let funclet_bundle = funclet_bundle.as_ref().map(|b| &*b.raw);
let mut bundles: SmallVec<[_; 2]> = SmallVec::new();
if let Some(funclet_bundle) = funclet_bundle {
bundles.push(funclet_bundle);
Expand All @@ -250,13 +249,12 @@ impl<'a, 'll, 'tcx> BuilderMethods<'a, 'tcx> for Builder<'a, 'll, 'tcx> {

// Emit KCFI operand bundle
let kcfi_bundle = self.kcfi_operand_bundle(fn_attrs, fn_abi, instance, llfn);
let kcfi_bundle = kcfi_bundle.as_ref().map(|b| &*b.raw);
if let Some(kcfi_bundle) = kcfi_bundle {
if let Some(kcfi_bundle) = kcfi_bundle.as_deref() {
bundles.push(kcfi_bundle);
}

let invoke = unsafe {
llvm::LLVMRustBuildInvoke(
llvm::LLVMBuildInvokeWithOperandBundles(
self.llbuilder,
llty,
llfn,
Expand Down Expand Up @@ -1179,7 +1177,6 @@ impl<'a, 'll, 'tcx> BuilderMethods<'a, 'tcx> for Builder<'a, 'll, 'tcx> {

let args = self.check_call("call", llty, llfn, args);
let funclet_bundle = funclet.map(|funclet| funclet.bundle());
let funclet_bundle = funclet_bundle.as_ref().map(|b| &*b.raw);
let mut bundles: SmallVec<[_; 2]> = SmallVec::new();
if let Some(funclet_bundle) = funclet_bundle {
bundles.push(funclet_bundle);
Expand All @@ -1190,20 +1187,20 @@ impl<'a, 'll, 'tcx> BuilderMethods<'a, 'tcx> for Builder<'a, 'll, 'tcx> {

// Emit KCFI operand bundle
let kcfi_bundle = self.kcfi_operand_bundle(fn_attrs, fn_abi, instance, llfn);
let kcfi_bundle = kcfi_bundle.as_ref().map(|b| &*b.raw);
if let Some(kcfi_bundle) = kcfi_bundle {
if let Some(kcfi_bundle) = kcfi_bundle.as_deref() {
bundles.push(kcfi_bundle);
}

let call = unsafe {
llvm::LLVMRustBuildCall(
llvm::LLVMBuildCallWithOperandBundles(
self.llbuilder,
llty,
llfn,
args.as_ptr() as *const &llvm::Value,
args.len() as c_uint,
bundles.as_ptr(),
bundles.len() as c_uint,
c"".as_ptr(),
)
};
if let Some(fn_abi) = fn_abi {
Expand Down Expand Up @@ -1509,7 +1506,6 @@ impl<'a, 'll, 'tcx> Builder<'a, 'll, 'tcx> {

let args = self.check_call("callbr", llty, llfn, args);
let funclet_bundle = funclet.map(|funclet| funclet.bundle());
let funclet_bundle = funclet_bundle.as_ref().map(|b| &*b.raw);
let mut bundles: SmallVec<[_; 2]> = SmallVec::new();
if let Some(funclet_bundle) = funclet_bundle {
bundles.push(funclet_bundle);
Expand All @@ -1520,13 +1516,12 @@ impl<'a, 'll, 'tcx> Builder<'a, 'll, 'tcx> {

// Emit KCFI operand bundle
let kcfi_bundle = self.kcfi_operand_bundle(fn_attrs, fn_abi, instance, llfn);
let kcfi_bundle = kcfi_bundle.as_ref().map(|b| &*b.raw);
if let Some(kcfi_bundle) = kcfi_bundle {
if let Some(kcfi_bundle) = kcfi_bundle.as_deref() {
bundles.push(kcfi_bundle);
}

let callbr = unsafe {
llvm::LLVMRustBuildCallBr(
llvm::LLVMBuildCallBr(
self.llbuilder,
llty,
llfn,
Expand Down Expand Up @@ -1601,7 +1596,7 @@ impl<'a, 'll, 'tcx> Builder<'a, 'll, 'tcx> {
fn_abi: Option<&FnAbi<'tcx, Ty<'tcx>>>,
instance: Option<Instance<'tcx>>,
llfn: &'ll Value,
) -> Option<llvm::OperandBundleDef<'ll>> {
) -> Option<llvm::OperandBundleOwned<'ll>> {
let is_indirect_call = unsafe { llvm::LLVMRustIsNonGVFunctionPointerTy(llfn) };
let kcfi_bundle = if self.tcx.sess.is_sanitizer_kcfi_enabled()
&& let Some(fn_abi) = fn_abi
Expand All @@ -1627,7 +1622,7 @@ impl<'a, 'll, 'tcx> Builder<'a, 'll, 'tcx> {
kcfi::typeid_for_fnabi(self.tcx, fn_abi, options)
};

Some(llvm::OperandBundleDef::new("kcfi", &[self.const_u32(kcfi_typeid)]))
Some(llvm::OperandBundleOwned::new("kcfi", &[self.const_u32(kcfi_typeid)]))
} else {
None
};
Expand Down
8 changes: 4 additions & 4 deletions compiler/rustc_codegen_llvm/src/common.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ use tracing::debug;

use crate::consts::const_alloc_to_llvm;
pub(crate) use crate::context::CodegenCx;
use crate::llvm::{self, BasicBlock, Bool, ConstantInt, False, Metadata, OperandBundleDef, True};
use crate::llvm::{self, BasicBlock, Bool, ConstantInt, False, Metadata, True};
use crate::type_::Type;
use crate::value::Value;

Expand Down Expand Up @@ -63,19 +63,19 @@ use crate::value::Value;
/// the `OperandBundleDef` value created for MSVC landing pads.
pub(crate) struct Funclet<'ll> {
cleanuppad: &'ll Value,
operand: OperandBundleDef<'ll>,
operand: llvm::OperandBundleOwned<'ll>,
}

impl<'ll> Funclet<'ll> {
pub(crate) fn new(cleanuppad: &'ll Value) -> Self {
Funclet { cleanuppad, operand: OperandBundleDef::new("funclet", &[cleanuppad]) }
Funclet { cleanuppad, operand: llvm::OperandBundleOwned::new("funclet", &[cleanuppad]) }
}

pub(crate) fn cleanuppad(&self) -> &'ll Value {
self.cleanuppad
}

pub(crate) fn bundle(&self) -> &OperandBundleDef<'ll> {
pub(crate) fn bundle(&self) -> &llvm::OperandBundle<'ll> {
&self.operand
}
}
Expand Down
91 changes: 47 additions & 44 deletions compiler/rustc_codegen_llvm/src/llvm/ffi.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

use std::fmt::Debug;
use std::marker::PhantomData;
use std::ptr;

use libc::{c_char, c_int, c_uint, c_ulonglong, c_void, size_t};
use rustc_macros::TryFromU32;
Expand Down Expand Up @@ -708,8 +709,9 @@ unsafe extern "C" {
}
#[repr(C)]
pub struct RustArchiveMember<'a>(InvariantOpaque<'a>);
/// Opaque pointee of `LLVMOperandBundleRef`.
#[repr(C)]
pub struct OperandBundleDef<'a>(InvariantOpaque<'a>);
pub(crate) struct OperandBundle<'a>(InvariantOpaque<'a>);
#[repr(C)]
pub struct Linker<'a>(InvariantOpaque<'a>);

Expand Down Expand Up @@ -1538,6 +1540,50 @@ unsafe extern "C" {

pub fn LLVMGetOrInsertComdat(M: &Module, Name: *const c_char) -> &Comdat;
pub fn LLVMSetComdat(V: &Value, C: &Comdat);

pub(crate) fn LLVMCreateOperandBundle(
Tag: *const c_char,
TagLen: size_t,
Args: *const &'_ Value,
NumArgs: c_uint,
) -> *mut OperandBundle<'_>;
pub(crate) fn LLVMDisposeOperandBundle(Bundle: ptr::NonNull<OperandBundle<'_>>);

pub(crate) fn LLVMBuildCallWithOperandBundles<'a>(
B: &Builder<'a>,
Ty: &'a Type,
Fn: &'a Value,
Args: *const &'a Value,
NumArgs: c_uint,
Bundles: *const &OperandBundle<'a>,
NumBundles: c_uint,
Name: *const c_char,
) -> &'a Value;
pub(crate) fn LLVMBuildInvokeWithOperandBundles<'a>(
B: &Builder<'a>,
Ty: &'a Type,
Fn: &'a Value,
Args: *const &'a Value,
NumArgs: c_uint,
Then: &'a BasicBlock,
Catch: &'a BasicBlock,
Bundles: *const &OperandBundle<'a>,
NumBundles: c_uint,
Name: *const c_char,
) -> &'a Value;
pub(crate) fn LLVMBuildCallBr<'a>(
B: &Builder<'a>,
Ty: &'a Type,
Fn: &'a Value,
DefaultDest: &'a BasicBlock,
IndirectDests: *const &'a BasicBlock,
NumIndirectDests: c_uint,
Args: *const &'a Value,
NumArgs: c_uint,
Bundles: *const &OperandBundle<'a>,
NumBundles: c_uint,
Name: *const c_char,
) -> &'a Value;
}

#[link(name = "llvm-wrapper", kind = "static")]
Expand Down Expand Up @@ -1623,47 +1669,11 @@ unsafe extern "C" {
AttrsLen: size_t,
);

pub fn LLVMRustBuildInvoke<'a>(
B: &Builder<'a>,
Ty: &'a Type,
Fn: &'a Value,
Args: *const &'a Value,
NumArgs: c_uint,
Then: &'a BasicBlock,
Catch: &'a BasicBlock,
OpBundles: *const &OperandBundleDef<'a>,
NumOpBundles: c_uint,
Name: *const c_char,
) -> &'a Value;

pub fn LLVMRustBuildCallBr<'a>(
B: &Builder<'a>,
Ty: &'a Type,
Fn: &'a Value,
DefaultDest: &'a BasicBlock,
IndirectDests: *const &'a BasicBlock,
NumIndirectDests: c_uint,
Args: *const &'a Value,
NumArgs: c_uint,
OpBundles: *const &OperandBundleDef<'a>,
NumOpBundles: c_uint,
Name: *const c_char,
) -> &'a Value;

pub fn LLVMRustSetFastMath(Instr: &Value);
pub fn LLVMRustSetAlgebraicMath(Instr: &Value);
pub fn LLVMRustSetAllowReassoc(Instr: &Value);

// Miscellaneous instructions
pub fn LLVMRustBuildCall<'a>(
B: &Builder<'a>,
Ty: &'a Type,
Fn: &'a Value,
Args: *const &'a Value,
NumArgs: c_uint,
OpBundles: *const &OperandBundleDef<'a>,
NumOpBundles: c_uint,
) -> &'a Value;
pub fn LLVMRustBuildMemCpy<'a>(
B: &Builder<'a>,
Dst: &'a Value,
Expand Down Expand Up @@ -2357,13 +2367,6 @@ unsafe extern "C" {

pub fn LLVMRustSetDataLayoutFromTargetMachine<'a>(M: &'a Module, TM: &'a TargetMachine);

pub fn LLVMRustBuildOperandBundleDef(
Name: *const c_char,
Inputs: *const &'_ Value,
NumInputs: c_uint,
) -> &mut OperandBundleDef<'_>;
pub fn LLVMRustFreeOperandBundleDef<'a>(Bundle: &'a mut OperandBundleDef<'a>);

pub fn LLVMRustPositionBuilderAtStart<'a>(B: &Builder<'a>, BB: &'a BasicBlock);

pub fn LLVMRustSetModulePICLevel(M: &Module);
Expand Down
38 changes: 27 additions & 11 deletions compiler/rustc_codegen_llvm/src/llvm/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,12 @@

use std::cell::RefCell;
use std::ffi::{CStr, CString};
use std::ops::Deref;
use std::ptr;
use std::str::FromStr;
use std::string::FromUtf8Error;

use libc::c_uint;
use rustc_data_structures::small_c_str::SmallCStr;
use rustc_llvm::RustString;
use rustc_target::abi::{Align, Size, WrappingRange};

Expand Down Expand Up @@ -331,28 +332,43 @@ pub fn last_error() -> Option<String> {
}
}

pub struct OperandBundleDef<'a> {
pub raw: &'a mut ffi::OperandBundleDef<'a>,
/// Owns an [`OperandBundle`], and will dispose of it when dropped.
pub(crate) struct OperandBundleOwned<'a> {
raw: ptr::NonNull<OperandBundle<'a>>,
Comment on lines +335 to +337
Copy link
Member

@workingjubilee workingjubilee Oct 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

ah, and the dependence on the &[&'a Value] is what means we can't just make OperandBundleOwned be actually this, right?

type OperandBundleOwned = OperandBundle<'static>;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, from what I can tell an operand bundle holds copies of the value pointers internally, so it would be unsound for the bundle to outlive those values.

(In practice, I think the lifetime of values is always 'll, i.e. the lifetime of the LLVM context.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh one other relevant point, OperandBundleOwned is closer to Box<OperandBundle>, since its representation is a pointer to the underlying bundle, and the owner is responsible for deallocating on drop.

So the two types would still have to be different, unless we hypothetically had &own references to handle dropping.

}

impl<'a> OperandBundleDef<'a> {
pub fn new(name: &str, vals: &[&'a Value]) -> Self {
let name = SmallCStr::new(name);
let def = unsafe {
LLVMRustBuildOperandBundleDef(name.as_ptr(), vals.as_ptr(), vals.len() as c_uint)
impl<'a> OperandBundleOwned<'a> {
pub(crate) fn new(name: &str, vals: &[&'a Value]) -> Self {
workingjubilee marked this conversation as resolved.
Show resolved Hide resolved
let raw = unsafe {
LLVMCreateOperandBundle(
name.as_c_char_ptr(),
name.len(),
vals.as_ptr(),
vals.len() as c_uint,
)
};
OperandBundleDef { raw: def }
OperandBundleOwned { raw: ptr::NonNull::new(raw).unwrap() }
}
}

impl Drop for OperandBundleDef<'_> {
impl Drop for OperandBundleOwned<'_> {
fn drop(&mut self) {
unsafe {
LLVMRustFreeOperandBundleDef(&mut *(self.raw as *mut _));
LLVMDisposeOperandBundle(self.raw);
}
}
}

impl<'a> Deref for OperandBundleOwned<'a> {
type Target = OperandBundle<'a>;

fn deref(&self) -> &Self::Target {
// SAFETY: The returned reference is opaque and can only used for FFI.
// It is valid for as long as `&self` is.
unsafe { self.raw.as_ref() }
}
}

pub(crate) fn add_module_flag_u32(
module: &Module,
merge_behavior: ModuleFlagMergeBehavior,
Expand Down
Loading
Loading