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 #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

[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.

[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 #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 ***@***.***> wrote: One thing preventing stabilisation of repr_align is it's currently dependant on attr_literals #34981 <#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 <#33626 (comment)>, 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?