miri: accept extern types in structs if they are the only field #55672
Conversation
|
r? @varkor (rust_highfive has picked a reviewer for you, use r? to override) |
|
I would prefer it if this was based on offset being |
|
Like this? |
aa34f15 to
aca76d4
Compare
|
I'm not sure the alignment condition is needed, since |
src/librustc_mir/interpret/place.rs
Outdated
| @@ -354,7 +354,8 @@ where | |||
| let (meta, offset) = if field_layout.is_unsized() { | |||
| // re-use parent metadata to determine dynamic field layout | |||
| let (_, align) = self.size_and_align_of(base.meta, field_layout)? | |||
| .expect("Fields cannot be extern types"); | |||
| // If this is an extern type, we fall back to its static size and alignment. | |||
| .unwrap_or_else(|| base.layout.size_and_align()); | |||
There was a problem hiding this comment.
Interestingly, we can make size_and_align_of error, but have a separate align_of that works even for extern type. Only the alignment is used here, anyway!
There was a problem hiding this comment.
Oh, you can also just move the Option to be around the size, but not the alignment.
There was a problem hiding this comment.
I could, but I'd rather wait for the decision to treat the size_of_val and align_of_val intrinsics asymmetrically before doing that.
extern type will likely get alignment 1 currently, which is likely not actually correct for the type they are opaquely representing, so I am a bit worried about unilaterally implementing such behavior.
There was a problem hiding this comment.
You can't access that alignment without first having a reference, so having it too high would be a problem (the assumption might not be guaranteed by what you're FFI-ing to), but having it too low wouldn't break anything (other than field offsets).
So the way I see it, the alignment is only a lower bound, and 1 is fine, unless you want to use it as an aligned field in a struct.
Anyway, my concern is also kind of a confusing code thing.
You could also add a .map(|(_, align)| align) before unwrap_or_else, which would mean the code and the comment wouldn't mention the "size" of the extern type.
There was a problem hiding this comment.
You can't access that alignment without first having a reference, so having it too high would be a problem (the assumption might not be guaranteed by what you're FFI-ing to), but having it too low wouldn't break anything (other than field offsets).
Oh that's a good point. We cannot create such references ourselves anyway as we cannot have data of that type.
Still I think miri shouldn't move ahead of the rest of the decision process here.
I will implement your map suggestion.
There was a problem hiding this comment.
I implemented what you suggested.
|
@bors r+ |
|
📌 Commit a6622c2 has been approved by |
src/librustc_mir/interpret/place.rs
Outdated
| // FIXME: Once we have made decisions for how to handle size and alignment | ||
| // of `extern type`, this should be adapted. It is just a temporary hack | ||
| // to get some code to work that probably ought to work. | ||
| .unwrap_or_else(|| base.layout.align); |
There was a problem hiding this comment.
Should this really be base.layout, not field_layout?
|
Hm actually I am not entirely sure I got this right, will this error out appropriately when there are other fields? It will error out in |
|
@RalfJung Ah, yeah, you probably want it in both. This is why decoupling alignment from size should help - size and offset computation are similar/equivalent, whereas alignment is simpler. |
|
Offset computation of a field only uses the field's alignment, though (and the |
|
I fixed that, I think. |
|
@bors r+ |
|
📌 Commit ba0bab3 has been approved by |
|
⌛ Testing commit ba0bab3 with merge b2282c6f23a0d1c0e850f6cf871e5cbe8b1985f9... |
|
💔 Test failed - status-appveyor |
|
It's that timing test again: @bors retry @rust-lang/infra maybe the permitted time difference should be increased to 500ms or so? |
miri: accept extern types in structs if they are the only field Fixes rust-lang#55541 Cc @oli-obk @eddyb rust-lang#43467
|
⌛ Testing commit ba0bab3 with merge 54966b13b4f4b6d5d168c952194b6f9e2e73551b... |
|
💔 Test failed - status-appveyor |
|
@bors retry |
|
☀️ Test successful - status-appveyor, status-travis |
Fixes #55541
Cc @oli-obk @eddyb #43467