Rust: implement `#[repr(align)]` (tracking issue for RFC 1358)

[nikomatsakis]

Tracking issue for rust-lang/rfcs#1358

[kennytm]

heap::EMPTY is currently defined as 1 and is used as the address of reference to zero-sized types. Would it affect the program's correctness if we change the alignment of a ZST?

#[repr(align="8")]
struct F;

println!("{:p}", Box::new(F));
// prints 0x1? 0x8?

cc #27700.

[retep998]

@kennytm That already is an issue without this RFC.

use std::mem::align_of;
struct F([u32; 0]);
println!("{:p}", Box::new(F)); //prints 0x1
println!("{}", align_of::<F>()); //prints 4

[kennytm]

@retep998 Okay thanks. So it is entirely an issue of #27700.

[whitequark]

Anyone else implementing this? I need it so I may give it a go.

[mfarrugi]

What's the relationship of this issue to simd support https://github.com/rust-lang/rust/issues/27731 (ie. the current way to accomplish this on nightly)?

Can/should this be implemented separately?

[lu-zero]

@mfarrugi the two are related but not that much. SIMD usually works better on aligned data, but you need aligned data also when you use other kind of hardware features (dma engines for crypto, video decoding/encoding, etc). @whitequark got time to implement it? From what I can see the allocator now is taking an alignment so it should be not terrible to implement (I do not know the rust internals well enough to be confident in poking around this).

[whitequark]

@lu-zero Ok let me give it another try. Rustbuild and (hopefully?) incremental compilation have made rustc hacking much less painful...

[bitshifter]

@whitequark have started on this? I was interested and got a proof of concept working, so if you haven't started on it I could probably finish off what I'm doing and make a PR. I'd probably need some mentoring from someone, I haven't added a feature before.

[whitequark]

@bitshifter not really, please go ahead!

[briansmith]

@bitshifter It would be useful to know how this would compose with #[repr(transparent)]. In particular, do you foresee any problems implementing #[repr(transparent)] on top of this, such that a type could both have its alignment specified and be (otherwise) transparent?

[bitshifter]

This has turned out to be a bit more involved than what I originally thought, what a surprise!

I keep missing comment notifications for some reason. @briansmith looking at the RFC it seems you aren't allowed to combine #[repr(transparent)] with #[repr(align)]. The same restriction applies to #[repr(packed)], my changes are in theory dealing with that too but I haven't written a proper test for it yet, it shouldn't be difficult to perform similar checks for #[repr(transparent)]. (another edit) I see some comments on that RFC saying they should be allowed to be combined. I'm not sure if there would be any problems.

I have been making progress on this but one sticking point right now is Struct alignment on Rust is not passed to LLVM, in fact as far as I can tell LLVM StuctType doesn't appear to support alignment specified by an attribute, but only returns alignment based on the struct's fields. It seems that alignment attributes in LLVM are handled some other way. The array type in LLVM is also not picking up alignment from the equivalent Rust array type either. As a result this compiler bug macro gets hit https://github.com/rust-lang/rust/blob/master/src/librustc_trans/type_of.rs#L125. If I comment out that bug! then generated code for arrays sometimes isn't aligned, individual values seem to be OK though. So still trying to understand what's going on here, but I think I'm slowly making progress.

[bitshifter]

My changes so far are here https://github.com/bitshifter/rust/tree/struct_align_wip if anyone is interested. Feedback welcome!

[retep998]

@bitshifter I believe what was figured out earlier is that you have to specify the alignment on every load and store and alloca manually.

[mark-i-m]

Woah! That sounds awful :trollface:

[bitshifter]

@retep998 ah, at the moment it seems that only store actually uses the Rust type's alignment. Hopefully it's just a case of adding alignment to librustc_trans::builder::Builder alloca and load functions.

[bitshifter]

I have covered all of the cases mentioned in the RFC but have found a couple of other things that I think I need to address.

One is enums containing aligned structs and the other is structs containing aligned structs.

In the case of a struct containing an aligned struct, I think the members probably need to be manually aligned. Unless I'm totally missing something, as far as I can tell LLVM is completely unaware of alignment attributes (please let me know if you know different!). It seems that it will align/pad things correctly for primitives types that it knows about, but I haven't seen anything on the interface for creating types/structs that supports specifying a different alignment. So I think I probably need manually add padding between fields when necessary to ensure they are aligned correctly.

From what I can tell, Rust enums are basically structs with a hidden discriminant field which specifies the type (as i32?), plus the data. If this is the case, I think what probably needs to happen is the enum itself must use the maximum alignment of the data inside it, and the data must be padded so it's also aligned correctly, so hopefully fixing structs will also fix enums.

These cases in code form:

struct Align16(i32);

enum Enum { // enum should be align 16
    A(i32),
    B(Align16) // data should be offset to align 16
}

struct Nested { // struct should be align 16
    a: i32,
    b: Align16, // data should be offset to align 16
}

Those are the two outstanding things that I'm aware of. Possibly there's other use cases I haven't though of.

[bitshifter]

Paging @eddyb :) I was looking through git history and see you added https://github.com/rust-lang/rust/blob/master/src/librustc/ty/layout.rs and the Align struct. I had a couple of questions about that.

I need to keep track of both abi alignment and alignment requested by repr align. I had added a separate Align field to the struct type, but then I realised that the Align struct stores both abi alignment and pref alignment. I thought perhaps I could use the pref alignment to store repr align requests and get rid of the extra struct field I added. However, pref seems quite specific. As far as I can tell it's only calculated for dl_aggregate on TargetDataLayout. This is the starting alignment used for ADTs like structs and unions. As far as I can tell, pref is otherwise unused in Rust code. If I did use it to store repr_align values, this would mean changing a lot of code to request the pref alignment rather than the abi alignment. I also noticed that the dl_aggregate pref alignment tends to be larger than the abi alignment, so if I was going to reuse this field, I would need to init it to the abi alignment on Struct and elsewhere. I think it would help tidy up the code elsewhere though.

Does my explanation make sense? Can you see any problem with doing this? What is the intention of the dl_aggregate pref alignment?

[eddyb]

You should not touch pref alignment, what the RFC specifies is abi alignment.
We should use pref for things like globals (making some accesses faster, potentially), but what I did was to replicate LLVM's algorithms, I didn't change how alignments are being used.

cc @camlorn who almost also got started on this.

[eddyb]

Oh and the parsing of the data layout string should set both alignments for all primitives.

[bitshifter]

OK, thanks for the info. The only reason I need to keep track of the original align is for some checks to see if the Rust field matches the LLVM field alignment in https://github.com/rust-lang/rust/blob/master/src/librustc_trans/type_of.rs#L125., LLVM doesn't track alignment attributes of fields/structs so these values end up being different. Anyway, I'll leave pref alone.

[eddyb]

What clang does and what we'll have to do too is insert padding fields in between other fields.
This is what @camlorn had figured out a while back when he was considering taking this on, I believe.

[eddyb]

Oh, since you mentioned it: there is one invariant you need to take into account, and that's pref >= abi.
That is, you should inject an Align::new(x, x) into the computation, where x is user-specified.

[bitshifter]

Yes, I'm in the process of padding fields now. Adding the padding fields should mean the sizes match what LLVM expects but unless I'm mistaken it won't help when comparing what alignment it expects for a field, since I think that's based on the type of the field, which is something we can't tell LLVM about as far as I'm aware.

[eddyb]

I'm not sure what you mean by field alignment, and the line you linked to only has a }.

[bitshifter]

Sorry, wrong line. This check https://github.com/rust-lang/rust/blob/master/src/librustc_trans/type_of.rs#L130 fails with repr align, because the alignment calculated by LLVM doesn't take into account of repr align. There's no way that I can find to to LLVM that my struct is aligned to the repr align size. So the alignment determined by LLVM is based on the alignment of the fields types in the struct that it knows about. So to keep this check I'm keeping track of the original alignment before repr align is applied and comparing that what LLVM thinks alignment is, since it still seems like a useful check.

[eddyb]

You can replace the check with a weaker one that checks that the LLVM alignment is less than or equal to the one we compute - that and giving LLVM explicit alignments on instructions should be enough.

[bitshifter]

Great, that should simplify things.

[bitshifter]

Got struct field padding working. The next problem is this code causing a segfault:

#[repr(align = "16")]
struct Align16(i32);
enum Enum {
    A(i32),
    B(Align16)
}
let e = Enum::B(Align16(0));
match e {
  Enum::B(ref a) => (),
  _ => ()
}

Probably an unaligned load/store somewhere.

[camlorn]

Things to note on this:

You need to pad structs up manually to a multiple of the alignment when they are overaligned, because LLVM doesn't.

You need to explicitly allocate all variables at the appropriate alignment yourself. This requires tagging all the allocas. Be careful with this: it isn't sufficient just to tag allocas for overaligned types. You also have to tag them for arrays and other composites, I believe. What you need to do is work out how to tell if LLVM's idea of the alignment and the alignment coming from layout differ, then tag the alloca if they do. LLVM appears to have no understanding of overaligned types. Also be careful with globals, which have the same sorts of issues.

I am working on refactoring how repr works. This may impact you if yours lands after mine. Our current design for it is not optimal. I had come to the conclusion that putting this off until after that point was a good idea, which is why I never started. This isn't so much that it's impossible, more that I have to do the refactor anyway and doing this first would have made more work for myself. The conflict shouldn't be major, but will exist.

When working with the current repr design, you need to be careful to actually iterate over the whole slice. This seems kind of obvious, but I uncovered a number of compiler bugs that were related to people not realizing that you have to. Failure to do so will cause repr(C, align="128") and repr(align="128", C) to behave differently.

Please be careful to translate GEP indices through the memory_index vec. At the moment the field reordering code is disabled, but I will shortly be turning it back on.

[camlorn]

And something that kind of deserves a separate comment: why doesn't our attribute system allow for ints? Why align = "128" and not align = 128?

[eddyb]

@camlorn It does, just not stabilized yet.

[camlorn]

@eddyb
Should we change the RFC to use it, then stabilize these together? It seems like making this use strings is just going to cause someone to duplicate the is-this-an-int error logic.

[eddyb]

@camlorn I agree, not sure if @alexcrichton does.

[alexcrichton]

Yeah nowadays since we have arbitrary tokens in attributes I think it makes more sense to avoid the quotes. I'd prefer to avoid coupling the two stabilizations together, though, so my ideal world would be for this specific feature we accept strings and ints, and when ints are stabilized we issue warnings if you pass a string.

[eddyb]

@alexcrichton We have only literals, not arbitrary tokens, AFAIK. cc @jseyfried

[jseyfried]

@alexcrichton

my ideal world would be for this specific feature we accept strings and ints

:+1:

arbitrary tokens in attributes

As @eddyb said, we only have literals now -- @Sergio generalized the MetaItem structure to support literals in #35850. Arbitrary tokens are coming soon though!

[bitshifter]

Haven't had much time to look at this in the last few days. Current status - enums are weird. I need to fix enum bitcasts and geps missing padding. So far I've been calculating padding in rustc::ty::layout::Struct::new but only adding the padded fields just before the LLVM type is created in rustc_trans::adt::struct_llfields. I was trying to kind of hide the padded fields from the rustc view of things. This was working pretty well until I ran into the enum issue. Hopefully I'll get some time to dig into this more tomorrow.

[camlorn]

@bitshifter
I have now refactored layout to use a new representation for reprs. It has merged to master. This will make your life a bit easier once you get past the merge conflict.

Handling enums is simple enough for the variants themselves: you need only treat them like individual structs. I believe the helper methods for getting alignment and such already determine it by looking to see which variant is the most aligned, so that part may just work.

And now a couple of random offhand thoughts which may be more or less helpful:

I can make a case for doing all of the calculation in trans: some backends (such as C for some compilers) may be able to do it by just tagging the types as overaligned. If I was doing this, this is what I'd try for. It may be impossible or at least incredibly difficult, however.

There may be some performance advantage to using a design where the version of the struct passed to alloca is not padded out to the alignment but the alloca is tagged. There may be some bugs and a good bit of difficulty if you try for this. Perhaps @eddyb can chime in here.

[eddyb]

You need to tag the alloca anyway because otherwise you might get a lower alignment (computed by LLVM from individual fields) than you need for the stack slot itself.

[bitshifter]

Thanks @camlorn. I started a rebase last night, the repr changes didn't cause any issues. However, https://github.com/rust-lang/rust/commit/09825de61bb4e9a339b3c6759d73ebb4c0b6c7b1 changed all the alloca, load and store functions which I'm going to have a bit of fun integrating with (though this change might make alignment easier to deal with in the long run). Prior to this, things did seem to be working OK on the codegen front including the enum issues I mentioned earlier. I think my approach is effectively tagging the type as being overaligned.

Aside from getting what I had working again, the next thing I need to look at is this from the RFC:

For now, it will be illegal for any #[repr(packed)] struct to transitively contain a struct with #[repr(align)]. Specifically, both attributes cannot be applied on the same struct, and a #[repr(packed)] struct cannot transitively contain another struct with #[repr(align)].

Checking for both align and packed on a struct is easy enough, but I'm not sure how to go about the transitive part in the front end of the compiler. That can certainly be determined in rustc::ty::layout, but that seems like the wrong place to be checking for this.

I've also been getting varying results running x.py test --stage 1, even on master I seem to get some test failures. Is this to be expected? I thought all the tests had to pass before bors submitted to master.

[eddyb]

At --stage 1 anything using compiler plugins will fail. Just remove --stage 1 when not doing selective suites - it will take longer but actually succeed. IOW:

# Quickly go through the bulk of the tests
./x.py test --stage 1 src/test/{compile-fail,run-{fail,pass}}
# If that worked, finish bootstrap and run all tests
./x.py test

[bitshifter]

I've now implemented 100% of the RFC AFAIK. Just working through a few broken unit tests, maybe an optimisation and then PR.

[sstewartgallus]

@bitshifter Can you give a status update?

[eddyb]

@sstewartgallus PR is #39999, mostly ready except for needing #40658 to land first.

[bitshifter]

@sstewartgallus in addition to @eddyb's comment. Currently working on getting rebase with latest master to compile (asserts in LLVM during stage 1 build) and I still need to add a feature gate.

[eddyb]

@bitshifter you could try rebasing on top of my PR to see if that solves whatever issues you may hit.

[bitshifter]

@eddyb yeah it might, the problem looks similar to the failing test that your PR solved.

[chriskrycho]

Flagging this up as relatively high-priority since it looks like it may land on nightly soon: it will also need documentation before it stabilizes! 😄

[bitshifter]

This is now in master https://github.com/rust-lang/rust/commit/6d841da4a0d7629f826117f99052e3d4a7997a7e!

There's some follow up things that should probably be done in the future:

• write a codegen test - the test/run-pass/align-struct.rs doesn't catch everything - alloca's missing the alignment can just work depended on how they're used (e.g. address might happen to have correct alignment, only fails if usage requires alignment like vector instruction load/stores)

• specify alignment for over aligned types to stores and loads - this is an optimisation. it's always safe to under specify alignment, it's just slower, but over specifying is undefined behaviour so care is required. I thought it would be good to see how [repr(align)] is being used before tackling this.

[retep998]

@bitshifter Now that you've gained all this experience implementing repr(align), maybe you could take a stab at https://github.com/rust-lang/rust/issues/33158.

[bitshifter]

@retep998 thanks for the suggestion. I'll probably take a little break for now but this might be a good RFC to look at next. I'm somewhat familiar with the packed implementation at this point :)

[gnzlbg]

The RFC says:

#[repr(align = "16")]
struct MoreAligned(i32);

has size 16. IIUC it then follows that:

struct S {
  f0: MoreAligned,
  f1: u8,
}

has size 32 since f0 has size 16, the minimum size of S would be 17 (16 + 1 byte from the u8), but since f0 must be aligned to a 16 bytes boundary the alignment of S must be increased to 32, which then again means that its size is increased to 32 the size of S must be increased to 32.

However f0 only uses 4 bytes so the u8 can be placed in any of the 12 bytes remaining from the 16 required to align f0 properly. That is, S actually should have size 16.

Which part of the RFC guarantees that S will have size 16 ?

[eddyb]

since f0 must be aligned to a 16 bytes boundary the alignment of S must be increased to 32

Where does the second "must" come from? The alignment of S is max(field[i].align) so 16.
The size of S has to be a multiple of its alignment (since it's not packed), and the unaligned size is 17, so it gets the next multiple of 16, i.e. 32.

the u8 can be placed in any of those remaining 12 bytes

No, it can't. At least, not in today's Rust. We have to remove trailing padding from MoreAligned so that it actually has 4 bytes to do this - otherwise a write of f0: MoreAligned overwrites f1.

[retep998]

@gnzlbg Rust currently does not place fields into the padding of sibling fields preceding them, and we'd never be able to do that for #[repr(C)] because C compilers don't do that. In fact, because your S does not have #[repr(C)] there are no guarantees about its size. For #[repr(C)] however, the size of S would be 32.

[gnzlbg]

@eddyb

Where does the second "must" come from? The alignment of S is max(field[i].align) so 16.
The size of S has to be a multiple of its alignment (since it's not packed), and the unaligned size is 17, so it gets the next multiple of 16, i.e. 32.

Indeed, my alignment computation of S was wrong. I've striked it since it is not really relevant to the question.

@eddyb

No, it can't. At least, not in today's Rust. We have to remove trailing padding from MoreAligned so that it actually has 4 bytes to do this - otherwise a write of f0: MoreAligned overwrites f1.

@retep998

@gnzlbg Rust currently does not place fields into the padding of sibling fields preceding them, and we'd never be able to do that for #[repr(C)] because C compilers don't do that. In fact, because your S does not have #[repr(C)] there are no guarantees about its size. For #[repr(C)] however, the size of S would be 32.

I guess I understood the RFC correctly then. Is there a discussion about _why_ it was proposed this way anywhere? (I cannot find it in the RFC, the PR to the rfc repo, nor here).

IMO if the size of S is 32 and not 16 then #[repr(align)] is not a zero-cost abstraction because if I were to align f0 by hand everywhere, e.g. by manually adding padding as optimal everywhere, I would be able to do a 2x better job than #[repr(align)] in this case.

I do not have a better idea, but I'd like to read the discussions about this and the alternatives that were considered.

[retep998]

@gnzlbg One of the biggest motivations for this RFC is to allow libraries to match the layout of types from C libraries for FFI purposes. As a result, #[repr(align(N))] has to match the behavior of the equivalent in C (__declspec(align(N)) and the gcc equivalent), which is why it behaves the way it does.

[gnzlbg]

@retep998 Wouldn't I need to use #[repr(C, align = ...)] to get that guarantee?

[retep998]

@gnzlbg Yes. Without #[repr(C)] you don't get any layout guarantees other than the alignment of the type will be at least N so discussions of how big the type is don't really matter.

[joshlf]

Does #[repr(C)] guarantee that the size of a type is a multiple of its alignment (as is guaranteed w/o #[repr(C)])? By a similar token, alignments are normally required to be powers of two - is this required for #[repr(C)] as well?

While I'm at it, I should ask more generally: are there _any_ conditions under which those two rules (that alignments are powers of two, are at least 1, and that sizes are multiples of alignments) don't hold? I'm implementing an allocator so this would be bad news :)

[gnzlbg]

@retep998 So IIUC #[repr(align = "...")] implies #[repr(C)], right? (I cannot find this in the RFC but if this is the case, then it definitely should be written very clearly there since its kind of important since it would clarify my case with S 100%).

EDIT: I still think this is a bit weird, because my use case would fit perfectly for a #[repr(align = "...")] feature that does not imply #[repr(C)] and thus would allow S to have size 16, but I guess that while weird, one could specify that with #[and(repr(align = "..."), not(repr(C)))] or similar. Then again if that would be possible, I would rather have that as the default.

[eddyb]

@gnzlbg What you're talking about has nothing to do with #[repr(align)] and everything to do with removing trailing padding. Which we haven't done yet and there are questions of backwards compatibility. Removing trailing padding just for cases where it's caused by #[repr(align)] has all the same problems: you need writes of one element to not write that padding (think the size you pass to memcpy) and sequences (arrays, slices, vectors, ring buffers, etc.) of that type to include the padding (otherwise alignment isn't obeyed). That means you need two size_of which we don't have, not to mention you will break unsafe code one way or another.

[retep998]

@joshlf Every platform that Rust supports guarantees that alignments are powers of 2 and sizes are multiples of alignments.

[gnzlbg]

@eddyb I see. I somehow was coming to this from a different angle, thinking that it might be possible to allow the alignment of a type to be greater than its size, separating the object from its padding [0]. This gets messy real fast though [1] but has nothing to do with removing trailing padding. I was just kind of wondering if somebody considered anything like this.

[0] E.g. such that memseting the memory doesn't memset the padding.

[1] E.g. to make pointers works one would then need to keep the padding at the type level for properly offsetting pointers which if I think more about it probably cannot be made to work anyways. In such a world, a pointer to S::MoreAligned wouldn't have the same offset as a pointer to a MoreAligned inside a &[MoreAligned]. Removing trailing padding looks way easier than going down this route.

[eddyb]

If you change just the .offset method you will still break code that relies on any other methods.
E.g. anything allocating space for n T values does size_of::<T>().checked_mul(n) or some such.
If the size isn't aligned, the allocated space might be insufficient.

[gnzlbg]

Indeed that wouldn't be enough. Code calling memset on a pointer might pass size_of::<T> as well which right now works. Even if one were to update the offset for the pointer, that size_of::<T> would still need to return the size of a single element (which includes padding). Doing something like this would break e.g. unsafe code that uses memset to change a struct field.

This probably applies to the removing trailing padding optimization as well.

[joshlf]

@gnzlbg

I think a concrete example might help illuminate:

Let's say that we use your example from above:

#[repr(align = "16")]
struct MoreAligned(i32);

struct S {
  f0: MoreAligned,
  f1: u8,
}

And let's say that S is constructed as you suggested, with the trailing padding truncated so that S itself only consumes 16 bytes.

Then let's say that somebody has provided the following (correct, in current Rust) function to zero a value:

fn zero<T>(ptr: *mut T) {
  use std::mem::size_of;
  let addr = ptr as usize;
  for i in 0..size_of::<T>() {
    *((addr + i) as *mut u8) = 0;
  }
}

Then let's also say that somebody has provided the following (correct, in current rust) function to retrieve the nth element of an array:

fn get_nth<T>(base: *mut T, n: usize) -> *mut T {
  use std::mem::size_of;
  ((base as usize) + (n * size_of::<T>())) as *mut T
}

So here's the question: what should mem::size_of::<MoreAligned>() return? If it returns 16, then if you were to call zero with a pointer to the f0 field of an instance of S, it would incorrectly overwrite the f1 field. If it returns 15 (assuming f1 is placed at the very end of S; it could also return anything else greater than or equal to 4 so long as sufficient padding was added after f1), then get_nth is incorrect.

This is meant to illustrate what @eddyb meant by "you need two size_of". Hopefully that helps to clarify.

EDIT: Looks like I posted too soon :) Hopefully this is illuminating for other people reading this thread anyway.

[bitshifter]

@gnzlbg Skimming the thread, I don't think anyone said explicitly yet; #[repr(align(n))] does not imply #[repr(C)]. They can be used independently or together. There are uses for alignment in Rust outside of FFI. E.g. trying to force a variable to a cache line. The main difference being that Rust will reorder fields if #[repr(C)].

So for example:

struct S {
  f0: u32,
  f1: MoreAligned,
}

would be reordered by Rust so the largest aligned field is first in memory (which often reduces the size of the struct, although not in this example).

[retep998]

#[repr(align(8))]
pub union Foo {
    bar: i32,
}

error[E0517]: attribute should be applied to struct
 --> <anon>:4:1
  |
4 | #[repr(align(8))]
  | ^^^^^^^^^^^^^^^^^ requires a struct

Please extend this feature to be usable on unions as well.

[bitshifter]

I've been a bit too busy lately, but I'll try and take a look. Note that you should be able to force alignment of a union by putting an aligned struct inside it :)

[joshlf]

Have we thought about what happens if the alignment is greater than the platform's page size? Most allocators cannot support alignment larger than the page size (since, at least for large objects, most allocators just directly call mmap, which only guarantees page alignment).

I'd think in general that doing alloc(Layout::new::<T>()) on the global allocator should always be supported for any T that was accepted by the compiler. That'd be a nice property. Is the answer here to simply say that if you specify an unreasonably large alignment, then it's on you if the allocator cannot handle allocating your type? That seems reasonable to me, but I think we should have an explicit position on the question.

[bitshifter]

Is the answer here to simply say that if you specify an unreasonably large alignment, then it's on you if the allocator cannot handle allocating your type?

I think that yes, the allocator may not be able to service all alignment types and it should be up to the caller to handle the failure. Much like an OOM. I don't think we can know the page size of the machine that any program will be run on at compile time, so handling unsupported alignments at runtime is probably the best we can do.

[retep998]

(since, at least for large objects, most allocators just directly call mmap, which only guarantees page alignment).

On some platforms the allocation granularity for allocating virtual memory is different than the page size, Windows being a notable example where VirtualAlloc reservations are always aligned to 64K even though the page size is only 4K. Also some allocators will overallocate and manually align the allocation to satisfy higher alignments, such as system_alloc on Windows which has no limitation on the maximum alignment. Really I'm not too concerned about heap allocation because we can simply fail at runtime if the allocator doesn't actually support it.

Where I am concerned however is stack allocations. Has anyone run any tests to check whether stack allocating ridiculously aligned types works correctly? If we can guarantee stack allocations behave correctly, then there shouldn't be any practical limit. As long as the heap allocator properly errors when it is unable to satisfy a high alignment request, then everything is fine.

[joshlf]

Where I am concerned however is stack allocations. Has anyone run any tests to check whether stack allocating ridiculously aligned types works correctly? If we can guarantee stack allocations behave correctly, then there shouldn't be any practical limit. As long as the heap allocator properly errors when it is unable to satisfy a high alignment request, then everything is fine.

Oh, that's a really good point, and a tricky thing to handle. In general, if you have some data which is guaranteed to have alignment a, then regardless of how large that data is, you can compute how much padding is needed after it so that the next object has alignment b so long as b <= a (in particular, the amount of padding is only a function of a and the size of the data). However, if b > a, all bets are off - you need to know more than just a and the size of the data in order to figure out how much padding to add.

[bitshifter]

Please extend this feature to be usable on unions as well.

Should the same restrictions on combining repr packed and align apply to unions as well? That is structs don't allow using both repr hints and don't allow a packed struct to contain repr align member.

[retep998]

I don't know if repr(packed) on a union even does anything. However a repr(packed) struct definitely should not be allowed to (transitively) contain any types which are repr(align) even if that type was a union.

[bitshifter]

I wondered the same thing, it does - https://github.com/rust-lang/rust/blob/master/src/test/run-pass/union/union-packed.rs#L67

[retep998]

Oh right, it does effectively lower the alignment, and can even change the size, it just doesn't change the offsets of fields the way it does with structs. But yeah, the same restrictions on structs combining those attributes should apply to unions.

[nanokatze]

Would there be a way to specify alignment based on generic type parameter's size (or alignment)? Say, we could have a generic Vec3

#[repr(align(sizeof::<T>() * 4))]
pub struct Vec3<T>(pub T, pub T, pub T);

[briansmith]

What needs to be done w.r.t. this feature before it can be put in Rust stable? I'd like to help with the stabilization effort so that it can be stabilized ASAP. Is the stabilization something that can happen in the impl period?

[eddyb]

I'm not aware of anything right now. cc @alexcrichton @retep998

[retep998]

Personally I'd like to see #[repr(packed(N))] implemented first so we can ensure layout is working correctly with both of them.

[briansmith]

Personally I'd like to see #[repr(packed(N))] implemented first so we can ensure layout is working correctly with both of them.

Do you mean that you want to make sure that #[repr(align)] and #[repr(packed)] interact correctly? If so, It's definitely reasonable to want to ensure they interact correctly but if only one is implemented then I don't think it's the best trade-off to delay the stabilization of the first one implemented just for that. In particular, I think the people like me who want #[repr(align)] often do not need #[repr(packed)] and so it would be an unreasonable burden to require work on #[repr(packed)] to get #[repr(align)] to move forward.

[mfarrugi]

What needs to be done w.r.t. this feature before it can be put in Rust stable?

I am also interested in seeing this stabilized asap, is this stalled or blocked by anything?

[bitshifter]

One thing preventing stabilisation of repr_align is it's currently dependant on attr_literals https://github.com/rust-lang/rust/issues/34981 which is also still unstable.

[mfarrugi]

Is that the only thing? We could support string arguments to speed this
along.

On Mon, Nov 27, 2017, 21:15 Cameron Hart [e-mail] wrote:

One thing preventing stabilisation of repr_align is it's currently
dependant on attr_literals #34981
https://github.com/rust-lang/rust/issues/34981 which is also still
unstable.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
https://github.com/rust-lang/rust/issues/33626#issuecomment-347391593,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ACLjePtV_vAWb2fIiSRjbOrVtemrk1cyks5s62yvgaJpZM4IedCO
.

[retep998]

Is anyone opposed to having #[repr(align = "N")]? I'm totally fine with it, even if it ends up co-existing with #[repr(align(N))], especially since I always thought the attr_literals feature was about making those two forms effectively equivalent.

[bitshifter]

I always thought the attr_literals feature was about making those two forms effectively equivalent.

Perhaps that was my misunderstanding, I removed the old code when I added support for attr_literals.

[eddyb]

What I remember was a consensus(?) that attributes using the new syntax can be stabilized independently.

OTOH, if const generics were any closer, I'd propose using AlignTo<N> fields... while not great for integer literals, you pretty much need something like that if you're using a named constant or some kind of arithmetic/trait dispatch to compute the actual value, but maybe that's rare enough.

[bitshifter]

Yes, looking at my original PR I think I misunderstood adding attr_literals support meant chucking the original way of doing things. Shame I rebased away the original implementation :'(

I'll try add it back.

As for if there's anything else preventing this from being stabilised, ¯\_(ツ)_/¯

[eddyb]

To be clear, my position is that we should just stabilize the repr(align(N)) syntax (without stabilizing attr_literals) and not add back the other one.

[bitshifter]

I've created a pull request for stabilization, I still need to update the reference. I wasn't planning on updating the book or rust by example as they don't appear to talk about repr hints.

[bitshifter]

I have not updated the reference Attribute syntax grammar documentation as the new syntax is only used by #[repr(align)] and I thought it might be pretty confusing to include a grammar which is only used by one kind of attribute for now.

[whitequark]

I think this can be closed now since the attribute is stable.

[gnzlbg]

@kennytm can this issue be closed?