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.

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

Employ non-null metadata for loads from fat pointers #27180

Closed
wants to merge 2 commits into from
Closed
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
9 changes: 9 additions & 0 deletions src/librustc/middle/ty.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3850,6 +3850,15 @@ impl<'tcx> TyS<'tcx> {
}
}

pub fn pointee_type(&self, cx: &ctxt<'tcx>) -> Ty<'tcx> {
match self.sty {
ty::TyBox(inner) |
ty::TyRef(_, ty::TypeAndMut { ty: inner, .. }) |
ty::TyRawPtr(ty::TypeAndMut { ty: inner, .. }) => inner,
_ => cx.sess.bug(&format!("pointee_type called on non-pointer value: {}", self))
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

There's already a function Ty::builtin_deref.


pub fn sequence_element_type(&self, cx: &ctxt<'tcx>) -> Ty<'tcx> {
match self.sty {
TyArray(ty, _) | TySlice(ty) => ty,
Expand Down
35 changes: 17 additions & 18 deletions src/librustc_trans/trans/_match.rs
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,6 @@ pub use self::TransBindingMode::*;
use self::Opt::*;
use self::FailureHandler::*;

use back::abi;
use llvm::{ValueRef, BasicBlockRef};
use middle::check_match::StaticInliner;
use middle::check_match;
Expand Down Expand Up @@ -678,9 +677,8 @@ fn bind_subslice_pat(bcx: Block,
let slice_ty = bcx.tcx().mk_imm_ref(bcx.tcx().mk_region(ty::ReStatic),
bcx.tcx().mk_slice(unit_ty));
let scratch = rvalue_scratch_datum(bcx, slice_ty, "");
Store(bcx, slice_begin,
GEPi(bcx, scratch.val, &[0, abi::FAT_PTR_ADDR]));
Store(bcx, slice_len, GEPi(bcx, scratch.val, &[0, abi::FAT_PTR_EXTRA]));
store_addr(bcx, slice_begin, scratch.val);
store_extra(bcx, slice_len, scratch.val);
scratch.val
}

Expand Down Expand Up @@ -833,10 +831,11 @@ fn compare_values<'blk, 'tcx>(cx: Block<'blk, 'tcx>,
None,
&format!("comparison of `{}`", rhs_t),
StrEqFnLangItem);
let lhs_data = Load(cx, expr::get_dataptr(cx, lhs));
let lhs_len = Load(cx, expr::get_len(cx, lhs));
let rhs_data = Load(cx, expr::get_dataptr(cx, rhs));
let rhs_len = Load(cx, expr::get_len(cx, rhs));
let str_ty = cx.tcx().mk_str();
let lhs_data = load_addr(cx, lhs);
let lhs_len = load_extra(cx, lhs, str_ty);
let rhs_data = load_addr(cx, rhs);
let rhs_len = load_extra(cx, rhs, str_ty);
callee::trans_lang_call(cx, did, &[lhs_data, lhs_len, rhs_data, rhs_len], None, debug_loc)
}

Expand All @@ -857,15 +856,15 @@ fn compare_values<'blk, 'tcx>(cx: Block<'blk, 'tcx>,
let ty_str_slice = cx.tcx().mk_static_str();

let rhs_str = alloc_ty(cx, ty_str_slice, "rhs_str");
Store(cx, GEPi(cx, rhs, &[0, 0]), expr::get_dataptr(cx, rhs_str));
Store(cx, C_uint(cx.ccx(), pat_len), expr::get_len(cx, rhs_str));
store_addr(cx, GEPi(cx, rhs, &[0, 0]), rhs_str);
store_extra(cx, C_uint(cx.ccx(), pat_len), rhs_str);

let lhs_str;
if val_ty(lhs) == val_ty(rhs) {
// Both the discriminant and the pattern are thin pointers
lhs_str = alloc_ty(cx, ty_str_slice, "lhs_str");
Store(cx, GEPi(cx, lhs, &[0, 0]), expr::get_dataptr(cx, lhs_str));
Store(cx, C_uint(cx.ccx(), pat_len), expr::get_len(cx, lhs_str));
store_addr(cx, GEPi(cx, lhs, &[0, 0]), lhs_str);
store_extra(cx, C_uint(cx.ccx(), pat_len), lhs_str);
}
else {
// The discriminant is a fat pointer
Expand Down Expand Up @@ -1016,7 +1015,7 @@ fn compile_submatch<'a, 'p, 'blk, 'tcx>(bcx: Block<'blk, 'tcx>,
let binfo = *data.bindings_map.get(ident).unwrap();
call_lifetime_start(bcx, binfo.llmatch);
if binfo.trmode == TrByRef && type_is_fat_ptr(bcx.tcx(), binfo.ty) {
expr::copy_fat_ptr(bcx, *value_ptr, binfo.llmatch);
expr::copy_fat_ptr(bcx, *value_ptr, binfo.llmatch, binfo.ty);
}
else {
Store(bcx, *value_ptr, binfo.llmatch);
Expand Down Expand Up @@ -1080,7 +1079,7 @@ fn compile_submatch_continue<'a, 'p, 'blk, 'tcx>(mut bcx: Block<'blk, 'tcx>,
// the last field specially: instead of simply passing a
// ValueRef pointing to that field, as with all the others,
// we skip it and instead construct a 'fat ptr' below.
(arg_count - 1, Load(bcx, expr::get_dataptr(bcx, val)))
(arg_count - 1, load_addr(bcx, val))
};
let mut field_vals: Vec<ValueRef> = (0..arg_count).map(|ix|
adt::trans_field_ptr(bcx, &*repr, struct_val, 0, ix)
Expand All @@ -1099,9 +1098,9 @@ fn compile_submatch_continue<'a, 'p, 'blk, 'tcx>(mut bcx: Block<'blk, 'tcx>,
let llty = type_of::type_of(bcx.ccx(), unsized_ty);
let scratch = alloca_no_lifetime(bcx, llty, "__struct_field_fat_ptr");
let data = adt::trans_field_ptr(bcx, &*repr, struct_val, 0, arg_count);
let len = Load(bcx, expr::get_len(bcx, val));
Store(bcx, data, expr::get_dataptr(bcx, scratch));
Store(bcx, len, expr::get_len(bcx, scratch));
let len = load_extra(bcx, val, left_ty);
store_addr(bcx, data, scratch);
store_extra(bcx, len, scratch);
field_vals.push(scratch);
}
_ => {}
Expand Down Expand Up @@ -1681,7 +1680,7 @@ pub fn bind_irrefutable_pat<'blk, 'tcx>(bcx: Block<'blk, 'tcx>,
// By ref binding: the value of the variable
// is the pointer `val` itself or fat pointer referenced by `val`
if type_is_fat_ptr(bcx.tcx(), ty) {
expr::copy_fat_ptr(bcx, val, llval);
expr::copy_fat_ptr(bcx, val, llval, ty);
}
else {
Store(bcx, val, llval);
Expand Down
49 changes: 35 additions & 14 deletions src/librustc_trans/trans/base.rs
Original file line number Diff line number Diff line change
Expand Up @@ -408,9 +408,7 @@ pub fn iter_structural_ty<'blk, 'tcx, F>(cx: Block<'blk, 'tcx>,
let (data_ptr, info) = if common::type_is_sized(cx.tcx(), t) {
(av, None)
} else {
let data = GEPi(cx, av, &[0, abi::FAT_PTR_ADDR]);
let info = GEPi(cx, av, &[0, abi::FAT_PTR_EXTRA]);
(Load(cx, data), Some(Load(cx, info)))
(load_addr(cx, av), Some(load_extra(cx, av, t)))
};

let mut cx = cx;
Expand All @@ -426,8 +424,8 @@ pub fn iter_structural_ty<'blk, 'tcx, F>(cx: Block<'blk, 'tcx>,
llfld_a
} else {
let scratch = datum::rvalue_scratch_datum(cx, field_ty, "__fat_ptr_iter");
Store(cx, llfld_a, GEPi(cx, scratch.val, &[0, abi::FAT_PTR_ADDR]));
Store(cx, info.unwrap(), GEPi(cx, scratch.val, &[0, abi::FAT_PTR_EXTRA]));
store_addr(cx, llfld_a, scratch.val);
store_extra(cx, info.unwrap(), scratch.val);
scratch.val
};
cx = f(cx, val, field_ty);
Expand Down Expand Up @@ -777,6 +775,27 @@ pub fn load_if_immediate<'blk, 'tcx>(cx: Block<'blk, 'tcx>,
return v;
}

pub fn load_addr(bcx: Block, fatptr: ValueRef) -> ValueRef {
LoadNonNull(bcx, expr::get_dataptr(bcx, fatptr))
}

pub fn store_addr(bcx: Block, addr: ValueRef, fatptr: ValueRef) -> ValueRef {
Store(bcx, addr, expr::get_dataptr(bcx, fatptr))
}

pub fn load_extra<'blk, 'tcx>(bcx: Block<'blk, 'tcx>, fatptr: ValueRef, ty: Ty<'tcx>) -> ValueRef {
assert!(!common::type_is_sized(bcx.tcx(), ty));
if ty.is_trait() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be ty.struct_tail().is_trait()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed!

LoadNonNull(bcx, expr::get_len(bcx, fatptr))
} else {
Load(bcx, expr::get_len(bcx, fatptr))
}
}

pub fn store_extra(bcx: Block, extra: ValueRef, fatptr: ValueRef) -> ValueRef {
Store(bcx, extra, expr::get_len(bcx, fatptr))
}

/// Helper for loading values from memory. Does the necessary conversion if the in-memory type
/// differs from the type used for SSA values. Also handles various special cases where the type
/// gives us better information about what we are loading.
Expand Down Expand Up @@ -835,8 +854,8 @@ pub fn store_ty<'blk, 'tcx>(cx: Block<'blk, 'tcx>, v: ValueRef, dst: ValueRef, t
}

if common::type_is_fat_ptr(cx.tcx(), t) {
Store(cx, ExtractValue(cx, v, abi::FAT_PTR_ADDR), expr::get_dataptr(cx, dst));
Store(cx, ExtractValue(cx, v, abi::FAT_PTR_EXTRA), expr::get_len(cx, dst));
store_addr(cx, ExtractValue(cx, v, abi::FAT_PTR_ADDR), dst);
store_extra(cx, ExtractValue(cx, v, abi::FAT_PTR_EXTRA), dst);
} else {
let store = Store(cx, from_arg_ty(cx, v, t), to_arg_ty_ptr(cx, dst, t));
unsafe {
Expand Down Expand Up @@ -960,7 +979,9 @@ pub fn memcpy_ty<'blk, 'tcx>(bcx: Block<'blk, 'tcx>,
t: Ty<'tcx>) {
let _icx = push_ctxt("memcpy_ty");
let ccx = bcx.ccx();
if t.is_structural() {
if common::type_is_fat_ptr(bcx.tcx(), t) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@dotdash

Be sure you don't miscompile the likes of

pub fn foo() -> *const [u8] { unsafe { std::mem::transmute([0u64,0]) } }

Copy link
Member

Choose a reason for hiding this comment

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

@arielb1 out of curiosity, what was the red flag here that made you worry about that test case? At first blush, I would have thought that expr::copy_fat_ptr will handle this correctly.

Update: ah, I see, there is a separate note on the commit where @areilb1 states some concern about the change to copy_fat_ptr itself, (and the example here is meant to provide a case where the data is not dereferenceable).

expr::copy_fat_ptr(bcx, src, dst, t);
} else if t.is_structural() {
let llty = type_of::type_of(ccx, t);
let llsz = llsize_of(ccx, llty);
let llalign = type_of::align_of(ccx, t);
Expand Down Expand Up @@ -1348,8 +1369,8 @@ pub fn create_datums_for_fn_args<'a, 'tcx>(mut bcx: Block<'a, 'tcx>,
unpack_datum!(bcx, datum::lvalue_scratch_datum(bcx, arg_ty, "",
arg_scope_id, (data, extra),
|(data, extra), bcx, dst| {
Store(bcx, data, expr::get_dataptr(bcx, dst));
Store(bcx, extra, expr::get_len(bcx, dst));
store_addr(bcx, data, dst);
store_extra(bcx, extra, dst);
bcx
}))
} else {
Expand Down Expand Up @@ -1379,8 +1400,8 @@ pub fn create_datums_for_fn_args<'a, 'tcx>(mut bcx: Block<'a, 'tcx>,
if common::type_is_fat_ptr(bcx.tcx(), tupled_arg_ty) {
let data = get_param(bcx.fcx.llfn, idx);
let extra = get_param(bcx.fcx.llfn, idx + 1);
Store(bcx, data, expr::get_dataptr(bcx, lldest));
Store(bcx, extra, expr::get_len(bcx, lldest));
store_addr(bcx, data, lldest);
store_extra(bcx, extra, lldest);
idx += 2;
} else {
let datum = datum::Datum::new(
Expand Down Expand Up @@ -1784,8 +1805,8 @@ fn trans_enum_variant_or_tuple_like_struct<'a, 'tcx>(ccx: &CrateContext<'a, 'tcx
disr,
i);
if common::type_is_fat_ptr(bcx.tcx(), arg_ty) {
Store(bcx, get_param(fcx.llfn, llarg_idx), expr::get_dataptr(bcx, lldestptr));
Store(bcx, get_param(fcx.llfn, llarg_idx + 1), expr::get_len(bcx, lldestptr));
store_addr(bcx, get_param(fcx.llfn, llarg_idx), lldestptr);
store_extra(bcx, get_param(fcx.llfn, llarg_idx + 1), lldestptr);
llarg_idx += 2;
} else {
let arg = get_param(fcx.llfn, llarg_idx);
Expand Down
5 changes: 3 additions & 2 deletions src/librustc_trans/trans/callee.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1123,8 +1123,9 @@ pub fn trans_arg_datum<'blk, 'tcx>(bcx: Block<'blk, 'tcx>,
debug!("--- trans_arg_datum passing {}", bcx.val_to_string(val));

if common::type_is_fat_ptr(bcx.tcx(), formal_arg_ty) {
llargs.push(Load(bcx, expr::get_dataptr(bcx, val)));
llargs.push(Load(bcx, expr::get_len(bcx, val)));
let ty = formal_arg_ty.pointee_type(bcx.tcx());
llargs.push(load_addr(bcx, val));
llargs.push(load_extra(bcx, val, ty));
} else {
llargs.push(val);
}
Expand Down
3 changes: 1 addition & 2 deletions src/librustc_trans/trans/datum.rs
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,6 @@ pub use self::RvalueMode::*;

use llvm::ValueRef;
use trans::base::*;
use trans::build::Load;
use trans::common::*;
use trans::cleanup;
use trans::cleanup::CleanupMethods;
Expand Down Expand Up @@ -524,7 +523,7 @@ impl<'tcx> Datum<'tcx, Lvalue> {
let val = if type_is_sized(bcx.tcx(), self.ty) {
gep(self.val)
} else {
gep(Load(bcx, expr::get_dataptr(bcx, self.val)))
gep(load_addr(bcx, self.val))
};
Datum {
val: val,
Expand Down
26 changes: 15 additions & 11 deletions src/librustc_trans/trans/expr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -290,9 +290,13 @@ pub fn get_dataptr(bcx: Block, fat_ptr: ValueRef) -> ValueRef {
GEPi(bcx, fat_ptr, &[0, abi::FAT_PTR_ADDR])
}

pub fn copy_fat_ptr(bcx: Block, src_ptr: ValueRef, dst_ptr: ValueRef) {
Store(bcx, Load(bcx, get_dataptr(bcx, src_ptr)), get_dataptr(bcx, dst_ptr));
Store(bcx, Load(bcx, get_len(bcx, src_ptr)), get_len(bcx, dst_ptr));
pub fn copy_fat_ptr<'blk, 'tcx>(bcx: Block<'blk, 'tcx>,
src_ptr: ValueRef,
dst_ptr: ValueRef,
ty: Ty<'tcx>) {
let pointee_ty = ty.pointee_type(bcx.tcx());
store_addr(bcx, load_addr(bcx, src_ptr), dst_ptr);
store_extra(bcx, load_extra(bcx, src_ptr, pointee_ty), dst_ptr);
}

/// Retrieve the information we are losing (making dynamic) in an unsizing
Expand Down Expand Up @@ -453,8 +457,8 @@ fn coerce_unsized<'blk, 'tcx>(bcx: Block<'blk, 'tcx>,
// to use a different vtable. In that case, we want to
// load out the original data pointer so we can repackage
// it.
(Load(bcx, get_dataptr(bcx, source.val)),
Some(Load(bcx, get_len(bcx, source.val))))
(load_addr(bcx, source.val),
Some(load_extra(bcx, source.val, inner_source)))
} else {
let val = if source.kind.is_by_ref() {
load_ty(bcx, source.val, source.ty)
Expand All @@ -472,8 +476,8 @@ fn coerce_unsized<'blk, 'tcx>(bcx: Block<'blk, 'tcx>,
let ptr_ty = type_of::in_memory_type_of(bcx.ccx(), inner_target).ptr_to();
let base = PointerCast(bcx, base, ptr_ty);

Store(bcx, base, get_dataptr(bcx, target.val));
Store(bcx, info, get_len(bcx, target.val));
store_addr(bcx, base, target.val);
store_extra(bcx, info, target.val);
}

// This can be extended to enums and tuples in the future.
Expand Down Expand Up @@ -727,9 +731,9 @@ fn trans_field<'blk, 'tcx, F>(bcx: Block<'blk, 'tcx>,
DatumBlock { datum: d.to_expr_datum(), bcx: bcx }
} else {
let scratch = rvalue_scratch_datum(bcx, d.ty, "");
Store(bcx, d.val, get_dataptr(bcx, scratch.val));
let info = Load(bcx, get_len(bcx, base_datum.val));
Store(bcx, info, get_len(bcx, scratch.val));
store_addr(bcx, d.val, scratch.val);
let info = load_extra(bcx, base_datum.val, d.ty);
store_extra(bcx, info, scratch.val);

// Always generate an lvalue datum, because this pointer doesn't own
// the data and cleanup is scheduled elsewhere.
Expand Down Expand Up @@ -2083,7 +2087,7 @@ fn trans_imm_cast<'blk, 'tcx>(bcx: Block<'blk, 'tcx>,
// Return the address
return immediate_rvalue_bcx(bcx,
PointerCast(bcx,
Load(bcx, get_dataptr(bcx, datum.val)),
load_addr(bcx, datum.val),
ll_t_out),
t_out).to_expr_datumblock();
}
Expand Down
5 changes: 2 additions & 3 deletions src/librustc_trans/trans/foreign.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ use trans::cabi;
use trans::common::*;
use trans::debuginfo::DebugLoc;
use trans::declare;
use trans::expr;
use trans::machine;
use trans::monomorphize;
use trans::type_::Type;
Expand Down Expand Up @@ -298,8 +297,8 @@ pub fn trans_native_call<'blk, 'tcx>(bcx: Block<'blk, 'tcx>,
type_of::type_of(ccx, passed_arg_tys[i]),
"__arg");
if type_is_fat_ptr(ccx.tcx(), passed_arg_tys[i]) {
Store(bcx, llargs_rust[i + offset], expr::get_dataptr(bcx, scratch));
Store(bcx, llargs_rust[i + offset + 1], expr::get_len(bcx, scratch));
base::store_addr(bcx, llargs_rust[i + offset], scratch);
base::store_extra(bcx, llargs_rust[i + offset + 1], scratch);
offset += 1;
} else {
base::store_ty(bcx, llarg_rust, scratch, passed_arg_tys[i]);
Expand Down
14 changes: 5 additions & 9 deletions src/librustc_trans/trans/glue.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@
// Code relating to drop glue.


use back::abi;
use back::link::*;
use llvm;
use llvm::{ValueRef, get_param};
Expand Down Expand Up @@ -374,7 +373,7 @@ fn trans_struct_drop<'blk, 'tcx>(bcx: Block<'blk, 'tcx>,
let (_, bcx) = if type_is_sized(bcx.tcx(), t) {
invoke(bcx, dtor_addr, &[v0], dtor_ty, DebugLoc::None)
} else {
let args = [Load(bcx, expr::get_dataptr(bcx, v0)), Load(bcx, expr::get_len(bcx, v0))];
let args = [load_addr(bcx, v0), load_extra(bcx, v0, t)];
invoke(bcx, dtor_addr, &args, dtor_ty, DebugLoc::None)
};

Expand Down Expand Up @@ -465,15 +464,13 @@ fn make_drop_glue<'blk, 'tcx>(bcx: Block<'blk, 'tcx>, v0: ValueRef, g: DropGlueK
// a safe-guard, assert TyBox not used with TyContents.
assert!(!skip_dtor);
if !type_is_sized(bcx.tcx(), content_ty) {
let llval = GEPi(bcx, v0, &[0, abi::FAT_PTR_ADDR]);
let llbox = Load(bcx, llval);
let llbox = load_addr(bcx, v0);
let llbox_as_usize = PtrToInt(bcx, llbox, Type::int(bcx.ccx()));
let drop_flag_not_dropped_already =
ICmp(bcx, llvm::IntNE, llbox_as_usize, dropped_pattern, DebugLoc::None);
with_cond(bcx, drop_flag_not_dropped_already, |bcx| {
let bcx = drop_ty(bcx, v0, content_ty, DebugLoc::None);
let info = GEPi(bcx, v0, &[0, abi::FAT_PTR_EXTRA]);
let info = Load(bcx, info);
let info = load_extra(bcx, v0, content_ty);
let (llsize, llalign) = size_and_align_of_dst(bcx, content_ty, info);

// `Box<ZeroSizeType>` does not allocate.
Expand Down Expand Up @@ -531,12 +528,11 @@ fn make_drop_glue<'blk, 'tcx>(bcx: Block<'blk, 'tcx>, v0: ValueRef, g: DropGlueK
// versus without calling Drop::drop. Assert caller is
// okay with always calling the Drop impl, if any.
assert!(!skip_dtor);
let data_ptr = GEPi(bcx, v0, &[0, abi::FAT_PTR_ADDR]);
let vtable_ptr = Load(bcx, GEPi(bcx, v0, &[0, abi::FAT_PTR_EXTRA]));
let vtable_ptr = load_extra(bcx, v0, t);
let dtor = Load(bcx, vtable_ptr);
Call(bcx,
dtor,
&[PointerCast(bcx, Load(bcx, data_ptr), Type::i8p(bcx.ccx()))],
&[PointerCast(bcx, load_addr(bcx, v0), Type::i8p(bcx.ccx()))],
None,
DebugLoc::None);
bcx
Expand Down
4 changes: 2 additions & 2 deletions src/librustc_trans/trans/intrinsic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -397,8 +397,8 @@ pub fn trans_intrinsic_call<'a, 'blk, 'tcx>(mut bcx: Block<'blk, 'tcx>,
llargs[0]
} else {
let scratch = rvalue_scratch_datum(bcx, tp_ty, "tmp");
Store(bcx, llargs[0], expr::get_dataptr(bcx, scratch.val));
Store(bcx, llargs[1], expr::get_len(bcx, scratch.val));
store_addr(bcx, llargs[0], scratch.val);
store_extra(bcx, llargs[1], scratch.val);
fcx.schedule_lifetime_end(cleanup::CustomScope(cleanup_scope), scratch.val);
scratch.val
};
Expand Down
Loading