Skip to content

Commit

Permalink
Rollup merge of rust-lang#24472 - dotdash:23431, r=nikomatsakis
Browse files Browse the repository at this point in the history
 Loading from and storing to small aggregates happens by casting the
aggregate pointer to an appropriately sized integer pointer to avoid
the usage of first class aggregates which would lead to less optimized
code.

But this means that, for example, a tuple of type (i16, i16) will be
loading through an i32 pointer and because we currently don't provide
alignment information LLVM assumes that the load should use the ABI
alignment for i32 which would usually be 4 byte alignment. But the
alignment requirement for the (i16, i16) tuple will usually be just 2
bytes, so we're overestimating alignment, which invokes undefined
behaviour.

Therefore we must emit appropriate alignment information for
stores/loads through such casted pointers.

Fixes rust-lang#23431
  • Loading branch information
Manishearth committed Apr 17, 2015
2 parents f4ab2b2 + e153dea commit 3741bb5
Show file tree
Hide file tree
Showing 6 changed files with 38 additions and 16 deletions.
8 changes: 4 additions & 4 deletions src/librustc_trans/trans/adt.rs
Original file line number Diff line number Diff line change
Expand Up @@ -880,7 +880,7 @@ pub fn trans_set_discr<'blk, 'tcx>(bcx: Block<'blk, 'tcx>, r: &Repr<'tcx>,
CEnum(ity, min, max) => {
assert_discr_in_range(ity, min, max, discr);
Store(bcx, C_integral(ll_inttype(bcx.ccx(), ity), discr as u64, true),
val)
val);
}
General(ity, ref cases, dtor) => {
if dtor_active(dtor) {
Expand All @@ -889,7 +889,7 @@ pub fn trans_set_discr<'blk, 'tcx>(bcx: Block<'blk, 'tcx>, r: &Repr<'tcx>,
Store(bcx, C_u8(bcx.ccx(), DTOR_NEEDED as usize), ptr);
}
Store(bcx, C_integral(ll_inttype(bcx.ccx(), ity), discr as u64, true),
GEPi(bcx, val, &[0, 0]))
GEPi(bcx, val, &[0, 0]));
}
Univariant(ref st, dtor) => {
assert_eq!(discr, 0);
Expand All @@ -901,14 +901,14 @@ pub fn trans_set_discr<'blk, 'tcx>(bcx: Block<'blk, 'tcx>, r: &Repr<'tcx>,
RawNullablePointer { nndiscr, nnty, ..} => {
if discr != nndiscr {
let llptrty = type_of::sizing_type_of(bcx.ccx(), nnty);
Store(bcx, C_null(llptrty), val)
Store(bcx, C_null(llptrty), val);
}
}
StructWrappedNullablePointer { nndiscr, ref discrfield, .. } => {
if discr != nndiscr {
let llptrptr = GEPi(bcx, val, &discrfield[..]);
let llptrty = val_ty(llptrptr).element_type();
Store(bcx, C_null(llptrty), llptrptr)
Store(bcx, C_null(llptrty), llptrptr);
}
}
}
Expand Down
16 changes: 14 additions & 2 deletions src/librustc_trans/trans/base.rs
Original file line number Diff line number Diff line change
Expand Up @@ -760,9 +760,14 @@ pub fn load_ty<'blk, 'tcx>(cx: Block<'blk, 'tcx>,
}

let ptr = to_arg_ty_ptr(cx, ptr, t);
let align = type_of::align_of(cx.ccx(), t);

if type_is_immediate(cx.ccx(), t) && type_of::type_of(cx.ccx(), t).is_aggregate() {
return Load(cx, ptr);
let load = Load(cx, ptr);
unsafe {
llvm::LLVMSetAlignment(load, align);
}
return load;
}

unsafe {
Expand All @@ -788,13 +793,20 @@ pub fn load_ty<'blk, 'tcx>(cx: Block<'blk, 'tcx>,
Load(cx, ptr)
};

unsafe {
llvm::LLVMSetAlignment(val, align);
}

from_arg_ty(cx, val, t)
}

/// Helper for storing values in memory. Does the necessary conversion if the in-memory type
/// differs from the type used for SSA values.
pub fn store_ty<'blk, 'tcx>(cx: Block<'blk, 'tcx>, v: ValueRef, dst: ValueRef, t: Ty<'tcx>) {
Store(cx, to_arg_ty(cx, v, t), to_arg_ty_ptr(cx, dst, t));
let store = Store(cx, to_arg_ty(cx, v, t), to_arg_ty_ptr(cx, dst, t));
unsafe {
llvm::LLVMSetAlignment(store, type_of::align_of(cx.ccx(), t));
}
}

pub fn to_arg_ty(bcx: Block, val: ValueRef, ty: Ty) -> ValueRef {
Expand Down
8 changes: 4 additions & 4 deletions src/librustc_trans/trans/build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -646,13 +646,13 @@ pub fn LoadNonNull(cx: Block, ptr: ValueRef) -> ValueRef {
}
}

pub fn Store(cx: Block, val: ValueRef, ptr: ValueRef) {
if cx.unreachable.get() { return; }
pub fn Store(cx: Block, val: ValueRef, ptr: ValueRef) -> ValueRef {
if cx.unreachable.get() { return C_nil(cx.ccx()); }
B(cx).store(val, ptr)
}

pub fn VolatileStore(cx: Block, val: ValueRef, ptr: ValueRef) {
if cx.unreachable.get() { return; }
pub fn VolatileStore(cx: Block, val: ValueRef, ptr: ValueRef) -> ValueRef {
if cx.unreachable.get() { return C_nil(cx.ccx()); }
B(cx).volatile_store(val, ptr)
}

Expand Down
7 changes: 4 additions & 3 deletions src/librustc_trans/trans/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -509,18 +509,18 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
value
}

pub fn store(&self, val: ValueRef, ptr: ValueRef) {
pub fn store(&self, val: ValueRef, ptr: ValueRef) -> ValueRef {
debug!("Store {} -> {}",
self.ccx.tn().val_to_string(val),
self.ccx.tn().val_to_string(ptr));
assert!(!self.llbuilder.is_null());
self.count_insn("store");
unsafe {
llvm::LLVMBuildStore(self.llbuilder, val, ptr);
llvm::LLVMBuildStore(self.llbuilder, val, ptr)
}
}

pub fn volatile_store(&self, val: ValueRef, ptr: ValueRef) {
pub fn volatile_store(&self, val: ValueRef, ptr: ValueRef) -> ValueRef {
debug!("Store {} -> {}",
self.ccx.tn().val_to_string(val),
self.ccx.tn().val_to_string(ptr));
Expand All @@ -529,6 +529,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
unsafe {
let insn = llvm::LLVMBuildStore(self.llbuilder, val, ptr);
llvm::LLVMSetVolatile(insn, llvm::True);
insn
}
}

Expand Down
4 changes: 3 additions & 1 deletion src/librustc_trans/trans/foreign.rs
Original file line number Diff line number Diff line change
Expand Up @@ -802,7 +802,9 @@ pub fn trans_rust_fn_with_foreign_abi<'a, 'tcx>(ccx: &CrateContext<'a, 'tcx>,
// appropriately sized integer and we have to convert it
let tmp = builder.bitcast(llforeign_arg,
type_of::arg_type_of(ccx, rust_ty).ptr_to());
builder.load(tmp)
let load = builder.load(tmp);
llvm::LLVMSetAlignment(load, type_of::align_of(ccx, rust_ty));
load
} else {
builder.load(llforeign_arg)
}
Expand Down
11 changes: 9 additions & 2 deletions src/librustc_trans/trans/intrinsic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -456,13 +456,20 @@ pub fn trans_intrinsic_call<'a, 'blk, 'tcx>(mut bcx: Block<'blk, 'tcx>,
(_, "volatile_load") => {
let tp_ty = *substs.types.get(FnSpace, 0);
let ptr = to_arg_ty_ptr(bcx, llargs[0], tp_ty);
from_arg_ty(bcx, VolatileLoad(bcx, ptr), tp_ty)
let load = VolatileLoad(bcx, ptr);
unsafe {
llvm::LLVMSetAlignment(load, type_of::align_of(ccx, tp_ty));
}
from_arg_ty(bcx, load, tp_ty)
},
(_, "volatile_store") => {
let tp_ty = *substs.types.get(FnSpace, 0);
let ptr = to_arg_ty_ptr(bcx, llargs[0], tp_ty);
let val = to_arg_ty(bcx, llargs[1], tp_ty);
VolatileStore(bcx, val, ptr);
let store = VolatileStore(bcx, val, ptr);
unsafe {
llvm::LLVMSetAlignment(store, type_of::align_of(ccx, tp_ty));
}
C_nil(ccx)
},

Expand Down

0 comments on commit 3741bb5

Please sign in to comment.