Add field offsets to layout info for struct-like types#218
Add field offsets to layout info for struct-like types#218spernsteiner wants to merge 2 commits intomasterfrom
Conversation
RyanGlScott
left a comment
There was a problem hiding this comment.
Looks reasonable at a glance.
Dumb question from someone not familiar with MIR's layout approach: do we have to care about padding in addition to offsets? For instance, the documentation for FieldsShape::Arbitrary notes that the start of an enum may have padding due to the layout of the discriminant—is this something that we have to take into account?
| "field_offsets": field_offsets | ||
| })) | ||
| } else { | ||
| // TODO: provide field offsets even for unsized types |
There was a problem hiding this comment.
Can you open a mir-json issue to track this task?
The padding is implicit in the sizes and offsets. In the One thing we'll need to be careful of on the crucible-mir side is that some operations preserve the values of padding bytes while others don't. Here's an example—running it normally should print garbage for |
2e8c03c to
bed14bb
Compare
Output looks like this:
{ "kind": "ty", "data": { "layout": { "align": 4, "field_offsets": [ // <-- new 0, 4 ], "size": 8 }, "name": "ty::Tuple::23d21b33719c9a05", "needs_drop": false, "ty": { "kind": "Tuple", "tys": [ "ty::u16", "ty::u32" ] } } }This branch adds field offsets only for sized types, which should be sufficient for implementing realistic layouts for tuples in crucible-mir. For migrating structs to
MirAggregateRepr, we'll need field offsets and possibly some additional info in order to support unsized structs; I figure we can wait to add that until it's clearer what exactly we need.I'll wait to merge this until I have a matching crucible-mir PR ready to go.