Skip to content

Commit

Permalink
Rollup merge of rust-lang#122619 - erikdesjardins:cast, r=compiler-er…
Browse files Browse the repository at this point in the history 
…rors

Fix some unsoundness with PassMode::Cast ABI

Fixes rust-lang#122617

Reviewable commit-by-commit. More info in each commit message.
  • Loading branch information
@matthiaskrgr
matthiaskrgr authored Apr 3, 2024
2 parents 32c8c5c + dec81ac commit bc8415b
Show file tree
Hide file tree
Showing 8 changed files with 476 additions and 89 deletions.
120 changes: 51 additions & 69 deletions compiler/rustc_codegen_llvm/src/abi.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,15 @@ pub use rustc_middle::ty::layout::{FAT_PTR_ADDR, FAT_PTR_EXTRA};
use rustc_middle::ty::Ty;
use rustc_session::config;
pub use rustc_target::abi::call::*;
use rustc_target::abi::{self, HasDataLayout, Int};
use rustc_target::abi::{self, HasDataLayout, Int, Size};
pub use rustc_target::spec::abi::Abi;
use rustc_target::spec::SanitizerSet;

use libc::c_uint;
use smallvec::SmallVec;

use std::cmp;

pub trait ArgAttributesExt {
fn apply_attrs_to_llfn(&self, idx: AttributePlace, cx: &CodegenCx<'_, '_>, llfn: &Value);
fn apply_attrs_to_callsite(
Expand Down Expand Up @@ -130,42 +132,36 @@ impl LlvmType for Reg {
impl LlvmType for CastTarget {
fn llvm_type<'ll>(&self, cx: &CodegenCx<'ll, '_>) -> &'ll Type {
let rest_ll_unit = self.rest.unit.llvm_type(cx);
let (rest_count, rem_bytes) = if self.rest.unit.size.bytes() == 0 {
(0, 0)
let rest_count = if self.rest.total == Size::ZERO {
0
} else {
(
self.rest.total.bytes() / self.rest.unit.size.bytes(),
self.rest.total.bytes() % self.rest.unit.size.bytes(),
)
assert_ne!(
self.rest.unit.size,
Size::ZERO,
"total size {:?} cannot be divided into units of zero size",
self.rest.total
);
if self.rest.total.bytes() % self.rest.unit.size.bytes() != 0 {
assert_eq!(self.rest.unit.kind, RegKind::Integer, "only int regs can be split");
}
self.rest.total.bytes().div_ceil(self.rest.unit.size.bytes())
};

// Simplify to a single unit or an array if there's no prefix.
// This produces the same layout, but using a simpler type.
if self.prefix.iter().all(|x| x.is_none()) {
// Simplify to a single unit when there is no prefix and size <= unit size
if self.rest.total <= self.rest.unit.size {
if rest_count == 1 {
return rest_ll_unit;
}

// Simplify to array when all chunks are the same size and type
if rem_bytes == 0 {
return cx.type_array(rest_ll_unit, rest_count);
}
}

// Create list of fields in the main structure
let mut args: Vec<_> = self
.prefix
.iter()
.flat_map(|option_reg| option_reg.map(|reg| reg.llvm_type(cx)))
.chain((0..rest_count).map(|_| rest_ll_unit))
.collect();

// Append final integer
if rem_bytes != 0 {
// Only integers can be really split further.
assert_eq!(self.rest.unit.kind, RegKind::Integer);
args.push(cx.type_ix(rem_bytes * 8));
return cx.type_array(rest_ll_unit, rest_count);
}

// Generate a struct type with the prefix and the "rest" arguments.
let prefix_args =
self.prefix.iter().flat_map(|option_reg| option_reg.map(|reg| reg.llvm_type(cx)));
let rest_args = (0..rest_count).map(|_| rest_ll_unit);
let args: Vec<_> = prefix_args.chain(rest_args).collect();
cx.type_struct(&args, false)
}
}
Expand Down Expand Up @@ -215,47 +211,33 @@ impl<'ll, 'tcx> ArgAbiExt<'ll, 'tcx> for ArgAbi<'tcx, Ty<'tcx>> {
bug!("unsized `ArgAbi` must be handled through `store_fn_arg`");
}
PassMode::Cast { cast, pad_i32: _ } => {
// FIXME(eddyb): Figure out when the simpler Store is safe, clang
// uses it for i16 -> {i8, i8}, but not for i24 -> {i8, i8, i8}.
let can_store_through_cast_ptr = false;
if can_store_through_cast_ptr {
bx.store(val, dst.llval, self.layout.align.abi);
} else {
// The actual return type is a struct, but the ABI
// adaptation code has cast it into some scalar type. The
// code that follows is the only reliable way I have
// found to do a transform like i64 -> {i32,i32}.
// Basically we dump the data onto the stack then memcpy it.
//
// Other approaches I tried:
// - Casting rust ret pointer to the foreign type and using Store
// is (a) unsafe if size of foreign type > size of rust type and
// (b) runs afoul of strict aliasing rules, yielding invalid
// assembly under -O (specifically, the store gets removed).
// - Truncating foreign type to correct integral type and then
// bitcasting to the struct type yields invalid cast errors.

// We instead thus allocate some scratch space...
let scratch_size = cast.size(bx);
let scratch_align = cast.align(bx);
let llscratch = bx.alloca(cast.llvm_type(bx), scratch_align);
bx.lifetime_start(llscratch, scratch_size);

// ... where we first store the value...
bx.store(val, llscratch, scratch_align);

// ... and then memcpy it to the intended destination.
bx.memcpy(
dst.llval,
self.layout.align.abi,
llscratch,
scratch_align,
bx.const_usize(self.layout.size.bytes()),
MemFlags::empty(),
);

bx.lifetime_end(llscratch, scratch_size);
}
// The ABI mandates that the value is passed as a different struct representation.
// Spill and reload it from the stack to convert from the ABI representation to
// the Rust representation.
let scratch_size = cast.size(bx);
let scratch_align = cast.align(bx);
// Note that the ABI type may be either larger or smaller than the Rust type,
// due to the presence or absence of trailing padding. For example:
// - On some ABIs, the Rust layout { f64, f32, <f32 padding> } may omit padding
// when passed by value, making it smaller.
// - On some ABIs, the Rust layout { u16, u16, u16 } may be padded up to 8 bytes
// when passed by value, making it larger.
let copy_bytes = cmp::min(scratch_size.bytes(), self.layout.size.bytes());
// Allocate some scratch space...
let llscratch = bx.alloca(cast.llvm_type(bx), scratch_align);
bx.lifetime_start(llscratch, scratch_size);
// ...store the value...
bx.store(val, llscratch, scratch_align);
// ... and then memcpy it to the intended destination.
bx.memcpy(
dst.llval,
self.layout.align.abi,
llscratch,
scratch_align,
bx.const_usize(copy_bytes),
MemFlags::empty(),
);
bx.lifetime_end(llscratch, scratch_size);
}
_ => {
OperandRef::from_immediate_or_packed_pair(bx, val, self.layout).val.store(bx, dst);
Expand Down
32 changes: 29 additions & 3 deletions compiler/rustc_codegen_ssa/src/mir/block.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1505,9 +1505,35 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {

if by_ref && !arg.is_indirect() {
// Have to load the argument, maybe while casting it.
if let PassMode::Cast { cast: ty, .. } = &arg.mode {
let llty = bx.cast_backend_type(ty);
llval = bx.load(llty, llval, align.min(arg.layout.align.abi));
if let PassMode::Cast { cast, pad_i32: _ } = &arg.mode {
// The ABI mandates that the value is passed as a different struct representation.
// Spill and reload it from the stack to convert from the Rust representation to
// the ABI representation.
let scratch_size = cast.size(bx);
let scratch_align = cast.align(bx);
// Note that the ABI type may be either larger or smaller than the Rust type,
// due to the presence or absence of trailing padding. For example:
// - On some ABIs, the Rust layout { f64, f32, <f32 padding> } may omit padding
// when passed by value, making it smaller.
// - On some ABIs, the Rust layout { u16, u16, u16 } may be padded up to 8 bytes
// when passed by value, making it larger.
let copy_bytes = cmp::min(scratch_size.bytes(), arg.layout.size.bytes());
// Allocate some scratch space...
let llscratch = bx.alloca(bx.cast_backend_type(cast), scratch_align);
bx.lifetime_start(llscratch, scratch_size);
// ...memcpy the value...
bx.memcpy(
llscratch,
scratch_align,
llval,
align,
bx.const_usize(copy_bytes),
MemFlags::empty(),
);
// ...and then load it with the ABI type.
let cast_ty = bx.cast_backend_type(cast);
llval = bx.load(cast_ty, llscratch, scratch_align);
bx.lifetime_end(llscratch, scratch_size);
} else {
// We can't use `PlaceRef::load` here because the argument
// may have a type we don't treat as immediate, but the ABI
Expand Down
25 changes: 14 additions & 11 deletions compiler/rustc_target/src/abi/call/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -251,9 +251,9 @@ pub struct Uniform {
/// The total size of the argument, which can be:
/// * equal to `unit.size` (one scalar/vector),
/// * a multiple of `unit.size` (an array of scalar/vectors),
/// * if `unit.kind` is `Integer`, the last element
/// can be shorter, i.e., `{ i64, i64, i32 }` for
/// 64-bit integers with a total size of 20 bytes.
/// * if `unit.kind` is `Integer`, the last element can be shorter, i.e., `{ i64, i64, i32 }`
/// for 64-bit integers with a total size of 20 bytes. When the argument is actually passed,
/// this size will be rounded up to the nearest multiple of `unit.size`.
pub total: Size,
}

Expand Down Expand Up @@ -319,14 +319,17 @@ impl CastTarget {
}

pub fn size<C: HasDataLayout>(&self, _cx: &C) -> Size {
let mut size = self.rest.total;
for i in 0..self.prefix.iter().count() {
match self.prefix[i] {
Some(v) => size += v.size,
None => {}
}
}
return size;
// Prefix arguments are passed in specific designated registers
let prefix_size = self
.prefix
.iter()
.filter_map(|x| x.map(|reg| reg.size))
.fold(Size::ZERO, |acc, size| acc + size);
// Remaining arguments are passed in chunks of the unit size
let rest_size =
self.rest.unit.size * self.rest.total.bytes().div_ceil(self.rest.unit.size.bytes());

prefix_size + rest_size
}

pub fn align<C: HasDataLayout>(&self, cx: &C) -> Align {
Expand Down
24 changes: 24 additions & 0 deletions tests/auxiliary/rust_test_helpers.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,30 @@ rust_dbg_extern_identity_TwoDoubles(struct TwoDoubles u) {
return u;
}

struct FiveU16s {
uint16_t one;
uint16_t two;
uint16_t three;
uint16_t four;
uint16_t five;
};

struct FiveU16s
rust_dbg_extern_return_FiveU16s() {
struct FiveU16s s;
s.one = 10;
s.two = 20;
s.three = 30;
s.four = 40;
s.five = 50;
return s;
}

struct FiveU16s
rust_dbg_extern_identity_FiveU16s(struct FiveU16s u) {
return u;
}

struct ManyInts {
int8_t arg1;
int16_t arg2;
Expand Down
Loading

0 comments on commit bc8415b

Please sign in to comment.