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(align = "N")] #1358

Merged
merged 5 commits into from
May 13, 2016
Merged

Conversation

alexcrichton
Copy link
Member

@alexcrichton alexcrichton commented Nov 9, 2015

Extend the existing #[repr] attribute on structs with an align = "N" option
to specify a custom alignment for struct types.

Rendered

Extend the existing `#[repr]` attribute on structs with an `align = "N"` option
to specify a custom alignment for `struct` types.

[Rendered][link]

[link]: https://github.com/alexcrichton/rfcs/blob/repr-align/text/0000-repr-align.md
@alexcrichton alexcrichton added the T-lang Relevant to the language team, which will review and decide on the RFC. label Nov 9, 2015
@alexcrichton alexcrichton self-assigned this Nov 9, 2015
@alexcrichton
Copy link
Member Author

cc #325, an RFC issue also about this.

@eddyb
Copy link
Member

eddyb commented Nov 10, 2015

I wish we had arbitrary tokens in attributes, to make this nicer.
At the very least, I prefer #[repr(align("N"))], as the alignment is not set by the attribute, but rather extended, and I expect the following forms to have the same effect:

#[repr(align("32"))] #[repr(align("16"))]
struct Foo(u8);
#[repr(align("16"), align("32"))]
struct Foo(u8);
#[repr(align("32"))]
struct Foo(u8);

@comex
Copy link

comex commented Nov 10, 2015

Why the quotes?

It would be certainly be nicer to be able to specify any constant expression.

@alexcrichton
Copy link
Member Author

@eddyb

Yeah unfortunately the syntax for attributes doesn't allow foo("bar") today. It's true though that align = "foo" may be confusing as it doesn't set alignment, so a better name may be along the lines of min_align or something like that.


@comex

Currently #[foo(bar = 32)] isn't a valid attribute, the syntax only allows for name = "string", name, or name(<list-of-more-attributes>).

@erickt
Copy link

erickt commented Nov 10, 2015

It's probably best for this to be another rfc, but I think we should consider allowing the #[repr(align = <token tree>)]. I've certainly want that for serde, and it seems to be a more appropriate value here.

@eddyb
Copy link
Member

eddyb commented Nov 11, 2015

@alexcrichton Ah, right, if we need token trees in that case, too, I'd take #[repr(align(N))].

@briansmith
Copy link

I don't think it makes sense to use #[repr] for this, long term. #[repr] doesn't make sense for struct fields or for stack-allocated local variables, but we need alignment for both of those cases too (for crypto code, at least). We shouldn't have to make newtype boilerplate just to get alignment. Especially, the newtype mechanism would be awkward for arrays because one would have to use a macro to handle different lengths.

RE: "Working with a raw integer, however, should provide the building block for building up other abstractions and should be maximally flexible," what are those "other abstractions"?

@alexcrichton
Copy link
Member Author

@briansmith

what are those "other abstractions"?

Ah in rust-lang/rust#17537 a suggestion was made to have symbolic alignment (e.g. "aligned as much as this other type"). For example the compiler could possibly accept something like #[repr(align = "usize")] and just "do the right thing" to avoid the #[cfg_attr(target_pointer_width = "...", repr(align = "..."))] necessity otherwise.

@briansmith
Copy link

Currently #[foo(bar = 32)] isn't a valid attribute, the syntax only allows for name = "string", name, or name().

I'm having a hard time understanding this. #[align = "32"] doesn't currently work either. The compiler has to be changed to make it work. Any syntax is possible when the compiler changes are involved.

#[repr(align = "usize")]

In particular, it seems bad to put identifiers and integers in quotes for no reason. Quotes should be reserved for strings. More generally, less noise is better, and the quotes are noise, as is the use of "=" and the use of "repr". #[align(32)] and #[align(usize)] read much better.

@retep998
Copy link
Member

What I want specifically is the equivalent of #pragma pack or /Zp in msvc.

When you specify this option, each structure member after the first is stored on either the size of the member type or n-byte boundaries (where n is 1, 2, 4, 8, or 16), whichever is smaller.

https://msdn.microsoft.com/en-us/library/xh3e3fd0.aspx

This RFC does not look like it does that.

@alexcrichton
Copy link
Member Author

@briansmith currently there's a difference between "this parses" and "the compiler rejects this". The syntax for attributes simply does not allow integers or values of the form ident = ident. This is orthogonal from the compiler rejecting custom attributes which actually parse. Accepting something like align(32) would be a language grammar change, where #[repr(align = "32")] is not a grammar change (which I would personally very much like to avoid).

@retep998 sure, this may not exhaustively cover all possibilities of alignment right out the gate, but the idea is to hit as many as possible and to enable other possibilities in perhaps less ergonomic ways immediately while leaving the door open for future improvements.

@ruuda
Copy link

ruuda commented Nov 16, 2015

How does this interact with heap allocating? The documentation for alloc::heap::allocate currently mentions

The alignment must be no larger than the largest supported page size on the platform.

However, the page size can differ across systems. Should there be an upper bound on the alignment?

@nikomatsakis
Copy link
Contributor

Seems like a good starting point to me. It does seem like we want to have some sort of limit regarding page-size, but I guess that's mostly a technicality in practice.

@alexcrichton do you in fact intend to allow multiple #[repr(align)] attributes? I do think min_align is maybe a clearer name in that case, but it also seems less clear in the usual case of having only one such attribute, and that is probably what we ought to optimize for.

@nikomatsakis
Copy link
Contributor

(Incidentally, I agree we should extend attribute syntax to include token trees in some form, but as a separate RFC. After that we could deprecate the use of a string constant here (or just accept both.))

* Alignment values must be a power of two.

A custom alignment cannot *decrease* the alignment of a structure unless it is
also declared with `#[repr(packed)]` (to mirror what C does in this regard), but
Copy link
Member

Choose a reason for hiding this comment

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

What's the behaviour of something like:

#[repr(packed, align = "2")]
struct Foo {
     x: u8,
     y: u16
}

I assume it has size 4, but there's no padding between the u8 and u16 (i.e. despite the fact that u16 has alignment 2)?

Copy link
Member

Choose a reason for hiding this comment

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

Only if you include trailing padding (which we should do something about, at some point).

Copy link
Member Author

Choose a reason for hiding this comment

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

The behavior will likely just mirror what C does for now, e.g.:

#include <stddef.h>                                
#include <stdalign.h>                              
#include <stdio.h>                                 

struct foo {                                       
  char a;                                          
  short b;                                         
} __attribute__((packed, aligned(2)));             

int main() {                                       
  printf("size: %d\n", (int) sizeof(struct foo));  
  printf("align: %d\n", (int) alignof(struct foo));
  printf("a: %d\n", (int) offsetof(struct foo, a));
  printf("b: %d\n", (int) offsetof(struct foo, b));
}                                                  
$ gcc foo.c && ./a.out
size: 4
align: 2
a: 0
b: 1

@retep998
Copy link
Member

Since this only increases the alignment, I'd prefer if it wasn't named align as the align = "N" syntax can really confuse people into thinking that N is now the alignment.

@eddyb
Copy link
Member

eddyb commented Nov 17, 2015

@retep998 This is why I prefer #[repr(align(N))] or even #[repr(align_to(N))] or #[repr(aligned(N))].

@alexcrichton
Copy link
Member Author

@nikomatsakis

Yeah I'd intend to accept multiple alignment attributes here and the compiler would basically just take the max (e.g. it's correct that the alignment specified would be the minimum-ish).

@eternaleye
Copy link

@alexcrichton: Is it, though? If I have some type that I need to interoperate with a coal-burning PCI card imported from Outer Elbonia with #[repr(align = "3")], I think that logic breaks down. Specifically, picking the biggest alignment only works when they are all powers of two, or (more generally), their full list of prime factors (with duplicates included) is a superset of the smaller ones' individual lists. (i.e. it's the LCM)

@mark-i-m
Copy link
Member

I'm curious as to why multiple alignment attributes would be a useful feature. Can someone please give an example of why someone would do this?

@alexcrichton
Copy link
Member Author

@eternaleye ah yeah I guess it is, I vaguely remember coming across something where the alignment ended up being less but thinking again I don't think that's the case. Note that factorization and such don't matter as alignment is always a power of two (e.g. custom alignment has this requirement as well).

@mark-i-m In terms of composability it's often nice to just add extra restrictions later on without explicitly going back and removing the prior ones. For example if a macro generated something with #[repr(align)] and then you later increased that alignment via another attribute it's useful to not have an error and instead just gracefully handle it.

@liigo
Copy link
Contributor

liigo commented Nov 18, 2015

+1
2015年11月17日 上午3:15,"Brian Smith" notifications@github.com写道:

Currently #[foo(bar = 32)] isn't a valid attribute, the syntax only allows
for name = "string", name, or name().

I'm having a hard time understanding this. #[align = "32"] doesn't
currently work either. The compiler has to be changed to make it work. Any
syntax is possible when the compiler changes are involved.

#[repr(align = "usize")]

In particular, it seems bad to put identifiers and integers in quotes for
no reason. Quotes should be reserved for strings. More generally, less
noise is better, and the quotes are noise, as is the use of "=" and the use
of "repr". #[align(32)] and #[align(usize)] read much better.


Reply to this email directly or view it on GitHub
#1358 (comment).

@retep998
Copy link
Member

retep998 commented Dec 7, 2015

This RFC should correlate to the behavior of __declspec(align(N)) in MSVC.

I started an RFC for the #pragma pack(N) counterpart to this #1399

@Aatch
Copy link
Contributor

Aatch commented Dec 7, 2015

Could we have a too-small align be a warning, rather than an error? The RFC doesn't state it explicitly, but it's implied to be an error. Macros and generic code could legitimately end up with this case (if you need at least N alignment, but higher alignment is fine). If I'm wrong about what the RFC means, I would like it to be clarified.

Also, how about extending this to be applicable to individual fields? A #[repr(align)] on the declaration can indicate the overall alignment of the type, but being able to specify alignment for specific fields can also be useful in some cases. The main case I'm thinking of is lock/wait-free code that wants to space fields out so they don't all lie on the same cache line.

@retep998
Copy link
Member

retep998 commented Dec 7, 2015

Note that when combined with my pack RFC, this results in a very interesting edge case. Specifically the required alignment of any type is 1, unless the alignment is manually specified with __declspec(align(N)) which sets the required alignment to N. The required alignment is inherited from all base types and fields of a given type so given struct Foo { Bar x; }, Foo would inherit the required alignment from Bar. Where this comes into play is #pragma pack(N) which can only lower the alignment of the type to max(required_alignment, min(packing, default_alignment)). Note that while gcc/clang support #pragma pack, the gnu style __attribute__((aligned(N))) does not change the required alignment of a type.

So basically we need to decide whether we want the interaction semantics between #[repr(pack)] and #[repr(align)] to match the semantics of __declspec(align(N)) or __attribute((aligned(N))).

@emberian
Copy link
Member

emberian commented Dec 7, 2015

That's a false choice, I think. It really needs to be either, depending on the ABI in use.

@retep998
Copy link
Member

retep998 commented Apr 28, 2016

So you're going the gcc route, where alignment attributes on a struct raise the overall alignment of the struct but it still behaves the same as a naturally aligned struct. A struct whose alignment was raised by repr(align) can thus have its alignment lowered by repr(pack) equally the same as a struct with natural alignment. Mkay.

@matthieu-m
Copy link

matthieu-m commented Apr 29, 2016

Would this RFC allow something akin to C++11:

template <typename T>
using raw = std::aligned_storage<sizeof(T), alignof(T)>::type;

to get memory storage suitably sized and aligned to store a T without actually storing it?

I've come across the usecase while trying to build a StackAllocatedVec<T> wrapping what I wanted to be raw memory equivalent to [T; 32] and could not find a way to get it done.

Note that if this RFC is not suitable for this, this is not an issue, an intrinsic or built-in could always be created for it.

@eddyb
Copy link
Member

eddyb commented Apr 29, 2016

@matthieu-m If I'm understanding correctly, you want to use T's alignment without having values of type T? Because then, alignof([T; 0]) == alignof(T) so you can use that.

@TrianglesPCT
Copy link

Can you use this on individual instances, or only on type declarations?

Like in C++:

// every object of type sse_t will be aligned to 16-byte boundary
struct alignas(16) sse_t
{
float sse_data[4];
};

// the array "cacheline" will be aligned to 128-byte boundary
alignas(128) char cacheline[128];

Same keyword works in both cases

@mark-i-m
Copy link
Member

mark-i-m commented May 1, 2016

@TrianglesPCT I might be wrong, but I'm not aware that attributes can be used on individual instances.

@nikomatsakis
Copy link
Contributor

At the @rust-lang/lang meeting, we decided not to decide on this just yet, because we were all a little confused by the back-and-forth on alignment. =) The text that @alexcrichton added to the RFC says that

The #[repr(align)] attribute will not interact with #[repr(packed)].

But, given that the #[repr(packed)] RFC does specify an effect on the alignment of a struct:

[From pack RFC]: By specifying this attribute, the alignment of the struct would be the smaller of the specified packing and the default alignment of the struct.

this sentence seems somewhat incorrect. Reading this comment from @alexcrichton somewhat clarified the intention: I believe the idea is that the #[repr(packed)] RFC lowers the alignments of the struct fields, which then indirectly lowers the overall alignment, which is ordinarily the max of the alignment of all fields. In that sense, the #[repr(align)] attribute is just another input into the process, as specified in this text in the current RFC:

[From align RFC]: Multiple #[repr(align = "..")] directives are accepted on a struct declaration, and the actual alignment of the structure will be the maximum of all align directives and the natural alignment of the struct itself.

Here, the natural alignment of the struct refers (presumably) to the maximum alignment of any individual field. (I don't believe that is clearly defined in the RFC itself, though I may be missing something.)

So, first, @alexcrichton, do you agree with this summary?

Anyway, re-reading the other comments more carefully now, I see that @retep998 has indeed highlighted the key difference here (emphasis mine):

If the alignment of the type of a field was raised via align then the field's alignment cannot be lowered by the struct's packed value. Basically once you force something to have a higher alignment, its alignment can never ever be lowered, when used as a struct field it will always be at least that aligned, any struct it is part of will always be at least that aligned, and so on transitively.

I am not convinced that there is a "right choice" in this situation. The interaction with ABI compatibility, in particular, seems concerning. I can certainly imagine wanting the ability to "match" the behavior of the target C ABI, and yet I am not crazy about having Rust's layout be so tied to what the local C compiler does. But whether we pick "GNU" or "MSVC" style, we will be out of sync with the C conventions on some platform. This seems like a place where #[repr(C)] might come into play, as well -- that is, one could imagine the Rust compiler adopting one behavior of the other, but conforming to the target C compiler in the presence of #[repr(C)].

(As an aside, clearly we must work out the interaction of align and custom packed values, since we adopted the packed RFC, but I also agree with @retep998 that this was an issue before.)

@alexcrichton
Copy link
Member Author

So, first, @alexcrichton, do you agree with this summary?

Indeed! Apologies if the RFC is a bit confusing, I seem to have a tendency to do that lately... But yes my assumption was that #[repr(packed)] only indirectly changes struct alignment by altering field alignment, and then #[repr(align)] is just another input to raising the struct alignment. In saying #[repr(align)] does not interact with #[repr(packed)], it's a veiled way of saying that #[repr(packed)] only touches field alignment, and #[repr(align)] only touches struct alignment.

I can certainly imagine wanting the ability to "match" the behavior of the target C ABI, and yet I am not crazy about having Rust's layout be so tied to what the local C compiler does.

Yes, I'm personally a fan of having understandable Rust semantics and then we can have shims to have compatibility with C as necessary.

@nikomatsakis
Copy link
Contributor

Discussed again in the @rust-lang/lang meeting. We feel like there is no clear right answer here, as I summarized before. Given the lack of feedback and experience, we wanted to propose a compromise: just disallow a struct with an explicit alignment attribute from being used inside of a packed struct, this sidestepping the distinction. We can then revisit the interaction between the two in a specific RFC when we have more experience or specific use cases in mind. Thoughts?

@alexcrichton
Copy link
Member Author

@nikomatsakis that sounds reasonable to me, and to clarify, the rule would look like this, right?

  • A struct with #[repr(packed)] and #[repr(align)] is invalid.
  • A struct with #[repr(packed)] must not transitively include any struct with a #[repr(align)] attribute

@nikomatsakis
Copy link
Contributor

Yes.
On Sun, May 08, 2016 at 11:05:35AM -0700, Alex Crichton wrote:

@nikomatsakis that sounds reasonable to me, and to clarify, the rule would look like this, right?

  • A struct with #[repr(packed)] and #[repr(align)] is invalid.
  • A struct with #[repr(packed)] must not transitively include any struct with a #[repr(align)] attribute

You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#1358 (comment)

@alexcrichton
Copy link
Member Author

Ok, I've pushed an update indicating that it is illegal to mix the two attributes

@nikomatsakis
Copy link
Contributor

nikomatsakis commented May 11, 2016

@alexcrichton I'm fine with the edit your posted, but it may be stronger than necessary. That is, I don't think there is a problem with having an "aligned" struct containing a "packed" struct:

#[repr(align=16)]
struct Foo {
    b: Bar
}

#[repr(packed)]
struct Bar {
    x: u8,
    y: u16
}

Unless I'm missing something?

@retep998
Copy link
Member

retep998 commented May 11, 2016

The only issue is when a struct that has repr(packed) contains a field whose type (or the types of the fields of that type transitively) has repr(align). As far as I can tell there's no behavior differences to worry about when repr(packed) and repr(align) are on the same struct or if a repr(packed) type is in a repr(align) type (even transitively).

@alexcrichton
Copy link
Member Author

@nikomatsakis yeah that was intentional, but I've pushed an update now to allow it.

@nikomatsakis
Copy link
Contributor

@alexcrichton 👍

@nikomatsakis
Copy link
Contributor

Huzzah! The @rust-lang/lang team has decided to accept this RFC.

@nikomatsakis
Copy link
Contributor

Tracking issue: rust-lang/rust#33626

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

@nikomatsakis nikomatsakis merged commit a43706c into rust-lang:master May 13, 2016
@Centril Centril added A-data-types RFCs about data-types A-repr #[repr(...)] related proposals & ideas A-machine Proposals relating to Rust's abstract machine. A-alignment Proposals relating to alignment. labels Nov 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-alignment Proposals relating to alignment. A-data-types RFCs about data-types A-machine Proposals relating to Rust's abstract machine. 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-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.