New rule layout.repr.c.struct.align-empty#2264
Conversation
rustbot
added
the
S-waiting-on-review
traviscross
added
I-lang-nominated
DanielEScherzer
force-pushed
the
repr-c-no-fields
branch
from
cebbb09 to
65e75ff
Compare
DanielEScherzer
force-pushed
the
repr-c-no-fields
branch
from
65e75ff to
8d2c69a
Compare
| The alignment of the struct is the alignment of the most-aligned field in it. | ||
|
|
||
| r[layout.repr.c.struct.align-empty] | ||
| The alignment of a struct with no fields is 1. |
There was a problem hiding this comment.
We are moving towards repr(C) being defined entirely by the platform ABI (rust-lang/rfcs#3845). So I'm not sure we should add any such promises.
There was a problem hiding this comment.
We talked about this in today's @rust-lang/lang call. Our understanding of the migration plan is that that we'll be adding a today's-edition name for repr(C) (whatever that ends up being named), adding repr(ordered_fields) which will be the same (but indicates that the user explicitly wants that rather than being auto-migrated), and then adding a new repr(C) for the new edition.
Given that transition plan, our understanding is that all the current documentation of repr(C) will move to be documentation of the today's-edition repr(C) and repr(ordered_fields), and then we'll add new documentation for the new repr(C) that largely says we do whatever the platform C ABI does.
Given that transition plan, do you see an issue with this proposal as ongoing documentation of what will end up being the old repr(C) rather than the new one, assuming that all this documentation moves to being for the old one?
There was a problem hiding this comment.
For what might eventually be called repr(ordered_fields), this new clause is indeed correct.
It's also redundant though, it follows from the definition of the layout algorithm.
There was a problem hiding this comment.
This rule was proposed in the context of discussing #2243 on the t-lang-docs call yesterday where it was concluded that it does not actually follow from the definition of the layout algorithm - the size and offsets algorithm doesn't describe how to identify the alignment of the type, and the layout.repr.c.struct.align rule says
The alignment of the struct is the alignment of the most-aligned field in it.
but if there are no fields then there is no most-aligned field
There was a problem hiding this comment.
Our understanding of the migration plan is that that we'll be adding a today's-edition name for
repr(C)(whatever that ends up being named), addingrepr(ordered_fields)which will be the same
FWIW, I don't think that end state is set in stone. E.g. to resolve rust-lang/rust#112480, we might potentially end up at a state where all 3 of old repr(C), new repr(C), and repr(ordered_fields) are different.
There was a problem hiding this comment.
the layout.repr.c.struct.align rule says
The alignment of the struct is the alignment of the most-aligned field in it.
Then I would suggest that it makes more sense to edit that rule than add a new one. A single rule should suffice to define the alignment of the type.
Something lie:
The alignment of the struct is the maximum of the alignments of all fields, or 1 if there are no fields.
There was a problem hiding this comment.
We can combine the rules, no objection to that, just need T-lang to sign off on the new guarantee being added
traviscross
removed
the
I-lang-easy-decision
|
@rfcbot fcp merge lang |
|
Team member @traviscross has proposed to merge this. The next step is review by the rest of the tagged team members: No concerns currently listed. Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns. |
|
I'm fine with adding this as a guarantee. The suggestion to edit the existing rules makes sense, but I think we should leave that as an editorial question for the reference. @rfcbot reviewed |
rust-rfcbot
added
final-comment-period
and removed
proposed-final-comment-period
labels
|
🔔 This is now entering its final comment period, as per the review above. 🔔 |
traviscross
added
I-lang-radar
|
@rfcbot reviewed I agree with making this guarantee. I don't have an opinion about whether we add it as a separate clause or as part of another clause -- @RalfJung's suggestion here made sense to me. |
| ```rust,ignore | ||
| /// Returns the amount of padding needed after `offset` to ensure that the | ||
| /// following address will be aligned to `alignment`. | ||
| fn padding_needed_for(offset: usize, alignment: usize) -> usize { | ||
| let misalignment = offset % alignment; | ||
| if misalignment > 0 { | ||
| // round up to next multiple of `alignment` | ||
| alignment - misalignment | ||
| } else { | ||
| // already a multiple of `alignment` | ||
| 0 | ||
| } | ||
| } | ||
|
|
||
| struct.alignment = struct.fields().map(|field| field.alignment).max(); | ||
|
|
||
| let current_offset = 0; | ||
|
|
||
| for field in struct.fields_in_declaration_order() { | ||
| // Increase the current offset so that it's a multiple of the alignment | ||
| // of this field. For the first field, this will always be zero. | ||
| // The skipped bytes are called padding bytes. | ||
| current_offset += padding_needed_for(current_offset, field.alignment); | ||
|
|
||
| struct[field].offset = current_offset; | ||
|
|
||
| current_offset += field.size; | ||
| } | ||
|
|
||
| struct.size = current_offset + padding_needed_for(current_offset, struct.alignment); |
There was a problem hiding this comment.
you might also want to update this algorithm. Currently the line
struct.alignment = struct.fields().map(|field| field.alignment).max();is kinda bugged anyway, since the real max() method returns an Option. So:
struct.alignment = struct.fields().map(|field| field.alignment).max().unwrap_or(1);would be better ^^
There was a problem hiding this comment.
already working on that in #2243 but we couldn't updated the algorithm with unwrap_or(1) since that adds a new guarantee that isn't in the text, after this is approved I'll rebase that PR
10 participants
No description provided.