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

RFC: Add #[repr(pack = "N")] #1399

Merged
merged 7 commits into from
Apr 22, 2016

Conversation

retep998
Copy link
Member

@retep998 retep998 commented Dec 7, 2015

Signed-off-by: Peter Atashian <retep998@gmail.com>
Signed-off-by: Peter Atashian <retep998@gmail.com>
Signed-off-by: Peter Atashian <retep998@gmail.com>
* Packing values must be a power of two.

By specifying this attribute, the alignment of the struct would be the smaller
of the specified packing and the default alignment of the struct otherwise. The
Copy link
Member

Choose a reason for hiding this comment

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

s/otherwise//

Signed-off-by: Peter Atashian <retep998@gmail.com>
# Drawbacks
[drawbacks]: #drawbacks

This would unfortunately make my life easier even though one of the unstated
Copy link
Member

Choose a reason for hiding this comment

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

Please remove this paragraph.

Signed-off-by: Peter Atashian <retep998@gmail.com>
# Unresolved questions
[unresolved]: #unresolved-questions

* The behavior specified here should match the behavior of MSVC at least. Does
Copy link
Member

Choose a reason for hiding this comment

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

This must be resolved. Verify against the behavior of clang (or gcc).

Copy link
Member Author

Choose a reason for hiding this comment

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

Specifically clang or gcc on a platform other than Windows. On Windows they are supposed to match the msvc ABI.

@emberian
Copy link
Member

emberian commented Dec 7, 2015

The feature is necessary. If this behavior is in line with common C compilers, :shipit:

[motivation]: #motivation

Many C/C++ compilers allow a packing to be specified for structs which
effectivally lowers the alignment for a struct and its fields (for example with
Copy link
Member

Choose a reason for hiding this comment

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

s/effectivally/effectively/

@DanielKeep
Copy link

For reference, here's an example of Clang doing this.

Signed-off-by: Peter Atashian <retep998@gmail.com>
Signed-off-by: Peter Atashian <retep998@gmail.com>
@retep998
Copy link
Member Author

retep998 commented Dec 7, 2015

See this comment for an interesting interaction edge case with the alignment RFC regarding required alignment.

@nrc nrc added T-lang Relevant to the language team, which will review and decide on the RFC. T-compiler Relevant to the compiler team, which will review and decide on the RFC. labels Dec 8, 2015
@nikomatsakis nikomatsakis self-assigned this Dec 17, 2015
@nikomatsakis
Copy link
Contributor

Hear ye, hear ye! This RFC is now entering final comment period.

@nikomatsakis nikomatsakis added the final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. label Apr 8, 2016
@strega-nil
Copy link

This seems right to have. I think the best is #[repr(packed(1))] or #[repr(packed = "1")]. So the default of #[repr(packed)] would be #[repr(packed(1))].

@solson
Copy link
Member

solson commented Apr 11, 2016

@ubsan #[repr(packed(1))] is not currently valid attribute syntax (but #[repr(packed = "1")] is). I guess the RFC mentions this, so cc @retep998 as well.

It could become valid with #1559, though, or as @nrc mentions in a comment, the procedural macros RFC.

@retep998
Copy link
Member Author

I don't mind the syntax either way, as long as I can have this attribute at all.

@starkat99
Copy link

I think it should just extend #[repr(packed)] rather than adding a new pack syntax. The duplication is confusing otherwise. Just have #[repr(packed)] be default to #[repr(packed = "1")].

@nikomatsakis
Copy link
Contributor

Hmm, I think I too prefer #[repr(packed = "N")]. Otherwise, the RFC seems unobjectionable to me. It'd also be nice to approve #1358, which covers very similar subject matter.

@nikomatsakis
Copy link
Contributor

One area the RFC did not (explicitly) address -- or I missed it -- is what to do if there are multiple packed specifiers. It did say that #[repr(pack = "2")] and #[repr(packed)] cannot be combined, but it did not say anything about #[repr(pack = "2")] and #[repr(pack = "4")]. Presumably this is an error as well? (Unlike the proposed align RFC?)

Is there any reason one might want multiple specifiers? Like, independent macros might add tighter and tighter requirements? (However, unlike with alignment, it's not obvious to me whether you would want the max or min packing that was specified.)

@retep998
Copy link
Member Author

I think if we did allow specifying the packing multiple times, that the right thing to do would be to use the smallest packing specified. The alignment of any given field is min(default, packing) so extending packing to be min(packing1, packing2) seems fairly natural.

As for the align RFC, there'd need to be a decision on how it interacts with packing. Whether it does the msvc style __declspec(align(N)) with required alignment that cannot be lowered by packing, or the gcc style alignment attribute that can be lowered by packing.

@nikomatsakis
Copy link
Contributor

Huzzah! The @rust-lang/lang team has decided to accept this RFC, with the caveat that the attribute be renamed to packed (and not pack). I will make this change when merging (I assume @retep998 you do not object.)

We discussed a few other things:

  • We should (obviously) clarify the relationship to an align attribute. We're going to bring the align RFC into FCP, discussion can happen there. But in the @rust-lang/lang meeting, I think the general feeling was that if both attributes are present, then the specified align would overrule the alignment derived from packing (in other words, we would match the MSVC style). Another option is to forbid both options from being used simultaneously, but that seems unnecessary to me.

@nikomatsakis
Copy link
Contributor

Tracking issue: rust-lang/rust#33158

If you'd like to keep following the development of this feature, please subscribe to that issue, thanks! :)

nikomatsakis added a commit to nikomatsakis/rfcs that referenced this pull request Apr 22, 2016
also adjust from `#[repr(pack = "N")]` to `#[repr(packed = "N")]`
@nikomatsakis nikomatsakis merged commit cbb18a4 into rust-lang:master Apr 22, 2016
@Centril Centril added A-repr #[repr(...)] related proposals & ideas A-machine Proposals relating to Rust's abstract machine. A-data-types RFCs about data-types A-packed Proposals relating to types with a packed representation. labels Nov 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-data-types RFCs about data-types A-machine Proposals relating to Rust's abstract machine. A-packed Proposals relating to types with a packed representation. A-repr #[repr(...)] related proposals & ideas final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. T-compiler Relevant to the compiler team, which will review and decide on the RFC. T-lang Relevant to the language team, which will review and decide on the RFC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants