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

Clarify guarantees provided by repr(packed) #1163

Merged
merged 3 commits into from
May 2, 2022
Merged

Conversation

joshlf
Copy link
Contributor

@joshlf joshlf commented Feb 9, 2022

Clarify that repr(packed) minimizes inter-field padding and, in the special case of repr(packed(1))/repr(packed), guarantees no inter-field padding.

Clarify that `repr(packed)` minimizes inter-field padding and, in the special case of `repr(packed(1))`/`repr(packed)`, guarantees no inter-field padding.
@Gankra
Copy link
Contributor

Gankra commented Feb 9, 2022

See also #1152 which is similarly trying to clarify repr details which are seemingly-obvious but worth stating explicitly.

@joshlf
Copy link
Contributor Author

joshlf commented Feb 9, 2022

See also #1152 which is similarly trying to clarify repr details which are seemingly-obvious but worth stating explicitly.

Thanks, that's awesome to see!

@joshlf
Copy link
Contributor Author

joshlf commented Feb 15, 2022

What are the next steps to get this approved/merged?

@ehuss
Copy link
Contributor

ehuss commented Feb 15, 2022

I'm not entirely certain what this is trying to clarify. If the fields are aligned in a certain way, the size of the padding falls out as a consequence of the field layout. In what way would there be more padding than what is required? I also don't see packed(1) as being much of a special case, or at least not different in the layout method.

Can you say more about why it seems like these guarantees need to be explicitly called out?

@djkoloski
Copy link

repr(Rust) structs explicitly do not make any guarantees about data layout. Under the previous wording, a repr(Rust, packed) struct could have inter-field padding as packed only "may also alter the padding between fields". This updated wording guarantees that repr(Rust, packed) structs have no inter-field padding and other packed(N) reprs have the minimum amount of padding to satisfy the repr.

@ehuss
Copy link
Contributor

ehuss commented Feb 15, 2022

Perhaps the "no guarantees" is worded too specifically, and #1152 is trying to soften that. Using packed with the default representation still follows the rules set out in the packed section.

The note about "may also alter padding between fields" is more of a heads-up warning that packed(N) may change the offsets of the fields and as a consequence changes to padding may occur. The real rules are specified in the paragraph below. It is not a statement that padding may be arbitrarily altered. Perhaps that sentence could be reworded to make that clearer? Like say `packed` may also alter the alignment of each field as specified below. and just not mention "padding" if it is misleading?

@djkoloski
Copy link

Even with that clarification, I think the problem still remains. Even though a struct/union and all of its fields are lowered to 1-alignment, that does not guarantee that it will not have padding. The layout could add arbitrary padding (for example, to reach some more desirable total size) unless it's specifically guaranteed that repr(packed(1)) structs/unions do not have any padding. At least, that's my understanding of how alignment interacts with type layout.

@joshlf
Copy link
Contributor Author

joshlf commented Feb 15, 2022

Even with that clarification, I think the problem still remains. Even though a struct/union and all of its fields are lowered to 1-alignment, that does not guarantee that it will not have padding. The layout could add arbitrary padding (for example, to reach some more desirable total size) unless it's specifically guaranteed that repr(packed(1)) structs/unions do not have any padding. At least, that's my understanding of how alignment interacts with type layout.

This is my understanding as well - the reference, as currently written, does not guarantee that a packed struct will have the minimum possible padding. It allows that that could happen, but it doesn't guarantee that it will. This PR is intended to change that so that it can be relied upon.

I also don't see packed(1) as being much of a special case, or at least not different in the layout method.

It's a natural consequence of the guarantee I'd like to add, and doesn't technically need to be there, agreed. However, it's a very common use case, and so I figured it was useful to call it out specifically. I'd be happy to rephrase it as "an important consequence of these rules" or something like that.

@ehuss
Copy link
Contributor

ehuss commented Mar 21, 2022

I'd be happy to rephrase it as "an important consequence of these rules" or something like that.

That might be better.
There also appears to be a CI error.

@rust-lang/lang Nominating this for discussion/approval. This seems reasonable to me, though I don't know if there is something I haven't considered.

@joshlf
Copy link
Contributor Author

joshlf commented Mar 21, 2022

I'd be happy to rephrase it as "an important consequence of these rules" or something like that.

That might be better.

Done.

@nikomatsakis
Copy link
Contributor

These changes look good to me -- I like the new wording.

@nikomatsakis
Copy link
Contributor

(We'll cover it in the lang team meeting tomorrow, presumably.)

@nikomatsakis
Copy link
Contributor

We discussed this in today's @rust-lang/lang meeting and the general consensus was that this change made sense, but that it also made sense to do an FCP since this was a change to the lang spec, if only a small one. Therefore...

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Mar 22, 2022

Team member @nikomatsakis 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!

See this document for info about what commands tagged team members can give me.

@rfcbot
Copy link

rfcbot commented Apr 21, 2022

🔔 This is now entering its final comment period, as per the review above. 🔔

psst @nikomatsakis, I wasn't able to add the final-comment-period label, please do so.

@rfcbot
Copy link

rfcbot commented May 1, 2022

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

psst @nikomatsakis, I wasn't able to add the finished-final-comment-period label, please do so.

Copy link
Contributor

@ehuss ehuss left a comment

Choose a reason for hiding this comment

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

Thanks!

@ehuss ehuss merged commit d029b86 into rust-lang:master May 2, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request May 11, 2022
Update books

## nomicon

9 commits in c7d8467ca9158da58ef295ae65dbf00a308752d9..10d40c59a581c66d8ecd29ad18d410bf97ed524d
2022-04-06 14:26:54 +0900 to 2022-05-07 10:45:07 +0900
- Introducing init/uninit before its use (rust-lang/nomicon#355)
- Change will to would to discuss what don't occur (rust-lang/nomicon#361)
- State that pop for length 1 is an example (rust-lang/nomicon#360)
- Correct a sentence that didn't seem to be proper (rust-lang/nomicon#358)
- Indicate that C reference are C reference (rust-lang/nomicon#357)
- Introduce and avoid dropck (rust-lang/nomicon#353)
- Rephrase improperly reduced borrows introduction (rust-lang/nomicon#352)
- Two lifetime clarification (rust-lang/nomicon#350)
- "UB" vs "Undefined Behavior" (rust-lang/nomicon#349)

## reference

9 commits in b5f6c2362baf932db9440fbfcb509b309237ee85..8e36971959ff238b5aa2575fbc7a2e09e1313e82
2022-04-10 19:19:51 -0700 to 2022-05-09 17:20:59 -0700
- Stop saying that const functions cannot use 'extern' (rust-lang/reference#1207)
- Moved the option variant imports (rust-lang/reference#1208)
- #[must_use] on traits also affects trait objects (rust-lang/reference#1203)
- Don't use PathPattern in RangePattern bounds (rust-lang/reference#1204)
- Inline assembly: Add kreg0 register class (rust-lang/reference#1205)
- Fix crate_type attribute examples (rust-lang/reference#1201)
- Say that that the default function return type is the unit type (rust-lang/reference#1199)
- Clarify guarantees provided by repr(packed) (rust-lang/reference#1163)
- Document the Termination trait for main() and test functions (rust-lang/reference#1194)

## book

43 commits in de0dbffc5812fd885700874e8d258dd334733ac4..d9415b7cbfcb4b24062683f429bd0ff535396362
2022-04-18 19:29:45 -0400 to 2022-05-09 09:10:44 -0400
- Update ch09-02-recoverable-errors-with-result.md
- Added missing be 2
- Added missing be
- Move hardcoded string into status_line to be consistent
- Fix trailing space
- Propagate tech review edits back to src
- Change "semantics" to "mechanics"; when referring to compiler behavior, rather than syntax.
- Propagate some edits to ch4 snapshot
- Suggestions from tech review
- Propagate edits to src
- Propagate edits back to nostarch version
- Clarify sentences about lock types. Fixes rust-lang/book#2937.
- Edits to edits to chapter 16
- Edits from nostarch for chapter 16
- Propagate nostarch edits back to src
- Add words to dictionary
- Propagating edits back to the nostarch snapshot
- Small wording change. Fixes rust-lang/book#3112.
- Clarify the kind of manual cleanup meant here
- Edits to edits to chapter 15
- Edits from nostarch
- Add missing word
- Improve sentence structure
- fix unidiomatic new functions in chapter 15
- Propagate nostarch ch14 to src
- Update a link and the -p publishing instructions
- Actually, I don't think we need to show the command output here
- Edits to edits to chapter 14
- Update manual regeneration instructions
- Reflect the addition of the -p flag in Cargo 1.56 in chapter 14
- Change polarity and names of variables in env var section
- Propagate nostarch edits back to ch 12
- Change environment variable and field name to perhaps be less confusing
- Responses to nostarch edits
- Merge remote-tracking branch 'origin/ch13'
- Fix rust-lang/book#3002 use noplayground with common.rs
- Propagate ch3 edits to src
- Updating chapter 3 to use new println style
- Specify loop label format. Fixes rust-lang/book#3105.
- Clarify function definition must be in an accessible scope. Fixes rust-lang/book#3003
- Addressing tech review comments, propagating other changes
- Comments from tech review
- Chapter 3, section 2 - Add explicit type annotation to example of scalar type char.

## rust-by-example

6 commits in 44a80e8d8bfc5881c9bd69a2cb3a570776ee4181..e9f93cfcf410bc092c9107b8a41a82f144c761f2
2022-04-19 07:46:28 -0300 to 2022-05-08 18:24:06 -0300
- Add empty slice example (rust-lang/rust-by-example#1538)
- Enhancement/print (rust-lang/rust-by-example#1536)
- Update cast.md (rust-lang/rust-by-example#1521)
- Update iter_any.md (rust-lang/rust-by-example#1522)
- Update tuples.md (rust-lang/rust-by-example#1524)
- fix indent in fs.md (rust-lang/rust-by-example#1535)

## rustc-dev-guide

8 commits in 043e60f..0c02acd
2022-04-20 18:57:49 +0900 to 2022-05-10 09:45:31 -0300
- Update overview.md (rust-lang/rustc-dev-guide#1351)
- Update date references on parallel-rustc (rust-lang/rustc-dev-guide#1348)
- mention `WithOptConstParam` (rust-lang/rustc-dev-guide#1346)
- Fix format (rust-lang/rustc-dev-guide#1349)
- correct type of SubstsRef (rust-lang/rustc-dev-guide#1347)
- Document ErrorGuaranteed (rust-lang/rustc-dev-guide#1316)
- Edit "What the compiler does to your code" (rust-lang/rustc-dev-guide#1306)
- Update some date refs
@joshlf joshlf deleted the patch-1 branch October 2, 2024 21:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants