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

Handle dyn* coercions for values that are represented with OperandValue::Ref #104694

Closed
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
4 changes: 4 additions & 0 deletions compiler/rustc_codegen_ssa/src/base.rs
Original file line number Diff line number Diff line change
Expand Up @@ -276,6 +276,10 @@ pub fn cast_to_dyn_star<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>>(
// FIXME(dyn-star): We can remove this when all supported LLVMs use opaque ptrs only.
let unit_ptr = bx.cx().type_ptr_to(bx.cx().type_struct(&[], false));
let src = match bx.cx().type_kind(bx.cx().backend_type(src_ty_and_layout)) {
// if `is_backend_immediate` is not true, then the value is
// represented by an `OperandValue::Ref`, and it's already
// been read into a usize.
_ if !bx.cx().is_backend_immediate(src_ty_and_layout) => src,
TypeKind::Pointer => bx.pointercast(src, unit_ptr),
TypeKind::Integer => bx.inttoptr(src, unit_ptr),
// FIXME(dyn-star): We probably have to do a bitcast first, then inttoptr.
Expand Down
12 changes: 11 additions & 1 deletion compiler/rustc_codegen_ssa/src/mir/rvalue.rs
Original file line number Diff line number Diff line change
Expand Up @@ -270,9 +270,19 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
}
mir::CastKind::DynStar => {
let (lldata, llextra) = match operand.val {
OperandValue::Ref(_, _, _) => todo!(),
OperandValue::Ref(ptr, None, align) => {
// Read the value as a `usize` -- this should be possible,
// since the value is guaranteed to be a properly aligned
// and sized.
let isize_ptr_ty = bx.cx().type_ptr_to(bx.cx().type_isize());
let ptr = bx.pointercast(ptr, isize_ptr_ty);
let v = bx.load(bx.cx().type_isize(), ptr, align);
(v, None)
}
OperandValue::Immediate(v) => (v, None),
OperandValue::Pair(v, l) => (v, Some(l)),
OperandValue::Ref(a, b, _) =>
bug!("unexpected operand cast to dyn*: lldata={a:?} llextra={b:?}"),
};
let (lldata, llextra) =
base::cast_to_dyn_star(bx, lldata, operand.layout, cast.ty, llextra);
Expand Down
13 changes: 8 additions & 5 deletions compiler/rustc_const_eval/src/interpret/cast.rs
Original file line number Diff line number Diff line change
Expand Up @@ -123,12 +123,15 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
DynStar => {
if let ty::Dynamic(data, _, ty::DynStar) = cast_ty.kind() {
// Initial cast from sized to dyn trait
// Copy the data into the first field of the destination.
compiler-errors marked this conversation as resolved.
Show resolved Hide resolved
// (Can be a non-immediate type, so we have to do a proper copy.)
let data_dest = self.place_field(dest, 0)?;
self.copy_op(src, &data_dest, true)?;

// Copy the vtable into the second field of the destination.
let vtable = self.get_vtable_ptr(src.layout.ty, data.principal())?;
let vtable = Scalar::from_maybe_pointer(vtable, self);
let data = self.read_immediate(src)?.to_scalar();
let _assert_pointer_like = data.to_pointer(self)?;
let val = Immediate::ScalarPair(data, vtable);
self.write_immediate(val, dest)?;
let vtable_dest = self.place_field(dest, 1)?;
self.write_pointer(vtable, &vtable_dest)?;
} else {
bug!()
}
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_const_eval/src/interpret/operand.rs
Original file line number Diff line number Diff line change
Expand Up @@ -404,7 +404,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
Abi::Scalar(abi::Scalar::Initialized { .. })
| Abi::ScalarPair(abi::Scalar::Initialized { .. }, abi::Scalar::Initialized { .. })
) {
span_bug!(self.cur_span(), "primitive read not possible for type: {:?}", op.layout.ty);
span_bug!(self.cur_span(), "primitive read not possible for: {:?}", op.layout);
}
let imm = self.read_immediate_raw(op)?.right().unwrap();
if matches!(*imm, Immediate::Uninit) {
Expand Down
7 changes: 1 addition & 6 deletions compiler/rustc_middle/src/ty/layout.rs
Original file line number Diff line number Diff line change
Expand Up @@ -774,12 +774,7 @@ where
TyMaybeWithLayout::Ty(tcx.mk_mut_ptr(tcx.types.unit))
} else if i == 1 {
// FIXME(dyn-star) same FIXME as above applies here too
TyMaybeWithLayout::Ty(
tcx.mk_imm_ref(
tcx.lifetimes.re_static,
tcx.mk_array(tcx.types.usize, 3),
),
)
TyMaybeWithLayout::Ty(tcx.mk_imm_ptr(tcx.mk_array(tcx.types.usize, 3)))
Copy link
Member Author

Choose a reason for hiding this comment

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

This is necessary because if we represent the vtable as a reference, then CTFE wants to validate the pointer. However, the vtable is represented by GlobalAlloc::VTable, which is treated like Size::ZERO. So it complains that we're trying to write 24 bytes into an allocation of size 0.

Alternatively, we could fix this instead I guess:

return (Size::ZERO, self.tcx.data_layout.pointer_align.abi, AllocKind::VTable);

Copy link
Member

Choose a reason for hiding this comment

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

This shouldn't happen, validity checking should stop at the dyn* and not descend into its components.

Did you rebase? #107728 fixed some issues with dyn* handling in the validity check.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, this is based off of a rebased master.

Copy link
Member

Choose a reason for hiding this comment

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

Strange. Then CTFE should never see this type.

What's the error you are getting otherwise, and for which code? Ideally with RUSTC_CTFE_BACKTRACE=1

Copy link
Member Author

Choose a reason for hiding this comment

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

Strange. Then CTFE should never see this type.

Why?

https://github.com/rust-lang/rust/pull/104694/files#diff-60d85c7ca5a99b206384a4ccb596b7e0f9824fd6606ae7241e2d4a4e5ae65025R134

Here we write the vtable to the second part of the scalar pair. So we're trying to write the vtable allocation pointer to a place of type &'static [usize; 3], which then tries to validate that vtable pointer.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, that's where this comes from. But during CTFE we don't do validation on writes... this should only affect Miri. Still a problem though.

However why wouldn't this also be a problem for vtables in wide pointers?

I'm also worried using a raw pointer type here could have bad side-effects, like losing nonnull attributes. IIRC there are two places where we define the layout/type/attributes for fields of wide ptrs and dyn* and I have no idea what happens when they start to diverge.

Copy link
Member Author

Choose a reason for hiding this comment

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

However why wouldn't this also be a problem for vtables in wide pointers?

It's probably because we don't explicitly split up the write for &dyn .. writes into two writes like we do for dyn*..

} else {
bug!("no field {i} on dyn*")
}
Expand Down
13 changes: 13 additions & 0 deletions src/tools/miri/tests/pass/dyn-star-aggregate.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
#![feature(dyn_star)]
#![allow(incomplete_features)]

use std::fmt::Debug;

#[derive(Debug)]
#[repr(C)]
pub struct Foo(usize);

fn main() {
let x = Foo(1) as dyn* Debug;
assert_eq!(format!("{x:?}"), "Foo(1)");
}
Loading