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

ability to set alignment on struct fields #1512

Closed
andrewrk opened this issue Sep 13, 2018 · 8 comments
Closed

ability to set alignment on struct fields #1512

andrewrk opened this issue Sep 13, 2018 · 8 comments
Labels
accepted contributor friendly docs proposal
Milestone

Comments

@andrewrk
Copy link
Member

@andrewrk andrewrk commented Sep 13, 2018

Just like global variables and local variables support alignment, struct fields should support alignment.

The alignment of a struct should be the alignment of its most aligned member. As Zig iterates over the fields of a struct, it knows the current alignment at that position. When a field requires more alignment, padding bytes are inserted until the desired alignment is achieved. Zig currently does this incorrectly - See #1248

Related is reordering fields for better performance and memory usage (see #168)

You should also be able to set the alignment of fields in packed structs. This does 2 things:

  • acts as assertions - compiler will give an error if the alignment is impossible given that the struct is packed
  • determines the overall alignment of the packed struct - it will be the alignment of the most aligned field. if no field is specified to be aligned, the alignment of the packed struct will be 1.

A packed struct in an array should give a compile error if its size is not byte-aligned, or if its size is not aligned to its own alignment.

@andrewrk andrewrk added the proposal label Sep 13, 2018
@andrewrk andrewrk added this to the 0.4.0 milestone Sep 13, 2018
@ghost
Copy link

@ghost ghost commented Sep 18, 2018

Here we can see a use case for specifying the alignment of struct fields: https://github.com/facebook/folly/blob/master/folly/ProducerConsumerQueue.h (search for 'alignas').

They specify the alignment to be the minimum offset between two objects to avoid false sharing.

@andrewrk
Copy link
Member Author

@andrewrk andrewrk commented Dec 20, 2018

Note this workaround that is now in master branch, and also described in #1845 (comment):

zig/src/analyze.cpp

Lines 2698 to 2707 in 459045a

// TODO If we have no type_entry for the field, we've already failed to
// compile the program correctly. This stage1 compiler needs a deeper
// reworking to make this correct, or we can ignore the problem
// and make sure it is fixed in stage2. This workaround is for when
// there is a false positive of a dependency loop, of alignment depending
// on itself. When this false positive happens we assume a pointer-aligned
// field, which is usually fine but could be incorrectly over-aligned or
// even under-aligned. See https://github.com/ziglang/zig/issues/1512
if (field->type_entry == nullptr) {
this_field_align = LLVMABIAlignmentOfType(g->target_data_ref, LLVMPointerType(LLVMInt8Type(), 0));

I think we have to solve the underlying issue before this issue can be resolved.

@andrewrk andrewrk added the docs label Feb 22, 2019
@andrewrk andrewrk removed this from the 0.4.0 milestone Mar 20, 2019
@andrewrk andrewrk added this to the 0.5.0 milestone Mar 20, 2019
@andrewrk
Copy link
Member Author

@andrewrk andrewrk commented Mar 27, 2019

One more thing that should be done with this issue, is to make packed structs divide based on ABI size, not store size. So for this example:

const std = @import("std");
const assert = std.debug.assert;

comptime {
    assert(@sizeOf(u24) == 4);
}
const P = packed struct {
    a: u24,
    b: u8,
};

Currently the struct is generated as

%P = type <{ i24, i8 }>

This is problematic because based on @sizeOf(u24) one expects to be able to write the 4th byte without overwriting the next field.

Instead Zig should generate the struct like this:

%P = type <{ i32 }>

and then do the normal bit shifting and masking that happens for bit fields. This will enable packed structs to work effectively when specifying alignment. Otherwise, aligned u24 fields of packed structs would incorrectly be missing a padding byte.

andrewrk added a commit that referenced this issue Mar 29, 2019
Not tested yet, but it builds.

This closes #761, and lays the groundwork for fixing the remaining
false positive "foo depends on itself" bugs, such as #624.

It also lays the groundwork for implementing ability to specify
alignment of fields (#1512).
andrewrk added a commit that referenced this issue Apr 2, 2019
Not tested yet, but it builds.

This closes #761, and lays the groundwork for fixing the remaining
false positive "foo depends on itself" bugs, such as #624.

It also lays the groundwork for implementing ability to specify
alignment of fields (#1512).
@andrewrk
Copy link
Member Author

@andrewrk andrewrk commented Apr 5, 2019

After implementing this, audit @bitOffsetOf, and @bitSizeOf if it is implemented (See #2177)

@daurnimator
Copy link
Collaborator

@daurnimator daurnimator commented Aug 5, 2019

@nrdmn notes that this is required for certain UEFI structures.

EFI_GUID is specified in https://uefi.org/sites/default/files/resources/UEFI_Spec_2_8_final.pdf on page 21 as "128-bit buffer containing a unique identifier value. Unless otherwise specified, aligned on a 64-bit boundary.", with some details in appendix A on page 2201

@andrewrk
Copy link
Member Author

@andrewrk andrewrk commented Aug 22, 2019

I'm in the middle of fixing the bugs regarding struct alignment (#2174). Once that's done, the main blocker to this will be updating the grammar, parser, zig fmt, etc. Anyone want to take a crack at that? The syntax/parser updating work can be merged even if the analysis/codegen isn't done yet.

Here's the current syntax proposal:

struct {
    x: i32 align(expression),
}

@andrewrk andrewrk added the contributor friendly label Aug 22, 2019
@andrewrk
Copy link
Member Author

@andrewrk andrewrk commented Aug 23, 2019

Thanks @Tetralux for doing the stage1 and stage2 parser/renderer for this in #3114!

@andrewrk
Copy link
Member Author

@andrewrk andrewrk commented Aug 27, 2019

Done with the merge of #3115. See #3125 for unions.

Improving the semantics and alignment considerations of packed structs will be a separate issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted contributor friendly docs proposal
Projects
None yet
Development

No branches or pull requests

2 participants