-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Conversation
r? @jackh726 (rustbot has picked a reviewer for you, use r? to override) |
Some changes occurred to the CTFE / Miri engine cc @rust-lang/miri |
let val = Immediate::ScalarPair(data, vtable); | ||
self.write_immediate(val, dest)?; | ||
let vtable_dest = self.place_field(dest, 1)?; | ||
self.write_scalar(vtable, &vtable_dest)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would be a test that covers this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test I changed from check-pass to build-pass ICEs at the read_immediate
I removed, so that exercises these changes I guess.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uh, that does not sound very confident.^^ Also that test doesn't use const
nor Miri so it cannot be a proper test for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll commit a Miri version of the test below then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can add that test in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Looking at this again now that I know a bit more about dyn-star)
Yeah the old code assumes a Scalar ABI. Probably this is true throughout codegen and Miri, so tests should be added for types with a different ABI (both codegen tests and Miri tests, the latter relying on #107728):
(i32, i32)
will give you ScalarPair ABI- a
repr(C)
type will give youAggregate
ABI
The latter cannot be loaded with read_immediate
, it needs a copy_op
. So the code here makes sense, but we are missing tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(i32, i32) will give you ScalarPair ABI
I realized this won't have the right alignment. I don't think it is possible to have a ScalarPair type with size+alignment 8.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I agree. Forgot to comment that I tried and failed to test a ScalarPair.
@rustbot author |
This is probably related to the Abi of the type. I don't know what exactly happens with all this in the codegen backend. |
484e570
to
5f7af80
Compare
The Miri subtree was changed cc @rust-lang/miri |
tcx.mk_array(tcx.types.usize, 3), | ||
), | ||
) | ||
TyMaybeWithLayout::Ty(tcx.mk_imm_ptr(tcx.mk_array(tcx.types.usize, 3))) |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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*
..
@@ -0,0 +1,191 @@ | |||
thread 'rustc' panicked at 'assertion failed: match (base.layout.abi, field_layout.abi) {/n (Abi::Scalar(..), Abi::Scalar(..)) => true,/n (Abi::ScalarPair(..), Abi::ScalarPair(..)) => true,/n _ => false,/n}', /home/ec2-user/rust/compiler/rustc_const_eval/src/interpret/projection.rs:LL:CC |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no idea what's going on here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's this assertion
rust/compiler/rustc_const_eval/src/interpret/projection.rs
Lines 109 to 113 in 3905517
assert!(match (base.layout.abi, field_layout.abi) { | |
(Abi::Scalar(..), Abi::Scalar(..)) => true, | |
(Abi::ScalarPair(..), Abi::ScalarPair(..)) => true, | |
_ => false, | |
}); |
So looks like a bad layout... strange that that would not be caught by the layout sanity check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it because we're storing something with an aggregate abi inside the dyn*
's data part? Then when we try to project to one of the data's fields, it explodes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the dyn*
layout in that case? It can't be ScalarPair -- if the data part is aggregate, the dyn*
must also be aggregate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well dyn*
is always a ScalarPair. I don't think it can be conditional based on what the data part is. Maybe it should always be represented as aggregate? I guess in that case codegen would need some tweaking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, this is nasty. Usually the solution for issues like this is to do force_allocation
, but then unpack_dyn_star
requires &mut self
and we are using it from the visitor which is supposed to work with &self
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I tried that too and ran into exactly the &mut problem you talked about. It did work, though, except for the fact that I had to comment out some validation code that only had access to &self 🥲
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be good to get some codegen expertise here. Looks like this PR adds a bunch of ptr casts to codegen to align things; no idea if that breaks some invariants somewhere.
On the Miri side this is pretty fundamental: we could start out with an Immediate(ScalarPair)
of type dyn*
, and then after resolving to the actual dynamic type we realize the layout is Abi::Aggregate
which cannot usually ever be stored in an immediate. This could happen in the middle of the immutable visitor, so we can't force a new allocation either -- we are entirely stuck. Either we somehow make Miri work with Immediate::Scalar
that do not have Abi::Scalar
(:scream:) or dyn*
has to be Aggregate
from the start (which also affects codegen so it doesn't seem great that Miri would force such a choice).
EDIT: Ah in codegen this cannot possibly break anything, since codegen could never go from a dyn*
to a value of the underlying type...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well I guess another option would be to not allow dyn*
for types with Abi::Aggregate
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy to further constrain the PointerLike
trait (enforcing dyn*
coerceability) to require that the type is scalar. In the future, we may want to allow (e.g.) repr(C)
data types to be coerced into a dyn*
, but for now I'm not convinced it requires the additional complications that Ralf elaborated above.
The job Click to see the possible cause of the failure (guessed by this bot)
|
…cjgillot Enforce that `PointerLike` requires a pointer-like ABI At least temporarily, let's ban coercing things that are pointer-sized and pointer-aligned but *not* `Abi::Scalar(..)` into `dyn*`. See: rust-lang#104694 (comment) This can be lifted in the future if we decie that we *want* to be able to coerce something `repr(C)` into a `dyn*`, but we'll have to figure out what to do with Miri and codegen... r? compiler
☔ The latest upstream changes (presumably #109413) made this pull request unmergeable. Please resolve the merge conflicts. |
Well, given that we now just don't support |
I'm not fully certain this is the right fix, but it seems to work fine.
For some reason the codegen backend doesn't want to represent the value (which is
usize
-sized and aligned) as anImmediate
, but instead as asOperandValue::Ref
, which means it'salloca
'd onto the stack. We handle that by reading it back into an immediate as ausize
.Similarly, during const eval, the
read_immediate
call in thedyn*
cast fails because the value is represented by anAggregate
ABI (even though it has one field). To fix this, instead of reading it to an immediate, just do the write as two parts -- the data and the vtable.Fixes #104631