Skip to content

sema: analyze struct field bodies in a second pass, to allow them to use the layout of the struct itself #17692

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

Merged
merged 3 commits into from
Nov 7, 2023

Conversation

kcbanner
Copy link
Contributor

@kcbanner kcbanner commented Oct 24, 2023

This PR is based off of #17658.

The main goal of this PR was to resolve the circular dependency error you used to get when doing something like this:

const StructInner = extern struct {
    outer: StructOuter = std.mem.zeroes(StructOuter),
};

const StructMiddle = extern struct {
    outer: ?*StructInner,
    inner: ?*StructOuter,
};

const StructOuter = extern struct {
    middle: StructMiddle = std.mem.zeroes(StructMiddle),
};

test "circular dependency through pointer field of a struct" {
    var outer: StructOuter = .{};
    try expect(outer.middle.outer == null);
    try expect(outer.middle.inner == null);
}
test\behavior\container_circular_dependency.zig:28:29: error: struct 'container_circular_dependency.StructMiddle' depends on itself
const StructMiddle = extern struct {
                     ~~~~~~~^~~~~~
test\behavior\container_circular_dependency.zig:34:5: note: while checking this field
    middle: StructMiddle = std.mem.zeroes(StructMiddle),
    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

This is accomplished by breaking out the struct field init body resolution into a separate pass, so it can happen after the struct's layout has been resolved.

semaStructFields now caches the ranges of the init bodies in the StructType trailing data. The init bodies are then resolved by resolveStructFieldInits, which is called before the inits are actually required. Within the init bodies, the struct decl's instruction is repurposed to refer to the field type itself. This is to allow us to easily rebuild the inst_map mapping required for the init body instructions to refer to the field type. Thanks to @mlugg for the guidance on this one!

An additional benefit of this change is that we no longer need to make incorrect guesses about struct alignment when a field init wants to use its container's alignment. Previously, this test would fail because the struct alignment would be guessed to be pointer-aligned:

test "initializer uses own alignment" {
    const S = struct {
        x: u32 = @alignOf(@This()) + 1,
    };

    var s: S = .{};
    try expectEqual(4, @alignOf(S));
    try expectEqual(@as(usize, 5), s.x);
}

I updated the alignment guessing logic to mark if the guess was made, and it's checked in resolveStructLayout. I added a similar check for unions as well.

I checked to see if the guessing logic could be removed entirely, but there are some cases where it's still required:

test "struct field has a pointer to an aligned version of itself" {
    const E = struct {
        next: *align(1) @This(),
    };
    var e: E = undefined;
    e = .{ .next = &e };

    try expect(&e == e.next);
}

I may look into how to resolve that in a future PR.

Additional changes:

@kcbanner kcbanner force-pushed the struct_field_init_pass branch 2 times, most recently from 6e2b8f0 to 99e04b5 Compare October 24, 2023 05:51
@Vexu
Copy link
Member

Vexu commented Oct 24, 2023

Fixes #14005

@kcbanner kcbanner marked this pull request as ready for review October 25, 2023 03:20
@kcbanner kcbanner requested a review from kristoff-it as a code owner October 25, 2023 03:20
@kcbanner
Copy link
Contributor Author

Fixes #14005

I added this as a test case and it revealed an unnecessary call I was making to resolveStructFieldInits in resolveStructFully, thanks!

@kcbanner kcbanner force-pushed the struct_field_init_pass branch from b1a7361 to 57dfc46 Compare October 25, 2023 04:07
@kcbanner kcbanner force-pushed the struct_field_init_pass branch 3 times, most recently from 405a821 to c6a0304 Compare October 27, 2023 06:38
@kcbanner
Copy link
Contributor Author

kcbanner commented Oct 27, 2023

I've added a fix for the CBE not handling union representations with an unknown tag - this was broken since #17352. One of the new tests cases in this PR depends on this working.

@mlugg
Copy link
Member

mlugg commented Oct 27, 2023

As a note, you accidentally left in both Add compile error test case for... commits from the other branch, but that can be sorted during merge no problem

EDIT: ah, you removed it in a later commit - no worries, if it's okay with you @kcbanner I'm gonna force-push to your branch in a moment to reshuffle a few commits, but no material changes

@kcbanner kcbanner force-pushed the struct_field_init_pass branch from c6a0304 to 66c0110 Compare October 28, 2023 16:13
@kcbanner
Copy link
Contributor Author

kcbanner commented Oct 28, 2023

As a note, you accidentally left in both Add compile error test case for... commits from the other branch, but that can be sorted during merge no problem

EDIT: ah, you removed it in a later commit - no worries, if it's okay with you @kcbanner I'm gonna force-push to your branch in a moment to reshuffle a few commits, but no material changes

I've addressed your feedback and made a couple fixes in the new test cases for big-endian arches.

Feel free to shuffle commits, thanks!

@kcbanner kcbanner force-pushed the struct_field_init_pass branch from 07fe641 to da195f9 Compare October 29, 2023 03:48
@kcbanner
Copy link
Contributor Author

Rebased to resolve conflicts.

@kcbanner kcbanner force-pushed the struct_field_init_pass branch from da195f9 to b999908 Compare October 31, 2023 23:33
@kcbanner kcbanner requested a review from mlugg November 4, 2023 23:51
@kcbanner kcbanner mentioned this pull request Nov 6, 2023
@kcbanner
Copy link
Contributor Author

kcbanner commented Nov 6, 2023

Fixes #17563

Copy link
Member

@mlugg mlugg left a comment

Choose a reason for hiding this comment

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

Sorry for how long it took me to get around to this - it all looks good. I'm going to do one final rebase (and shuffle a few commits around), then I'll set this to auto-merge. Thanks!

(EDIT: "shuffle a few commits around" ended up meaning "squash basically everything", but I think everything's still there!)

@mlugg mlugg force-pushed the struct_field_init_pass branch from b999908 to 59f145c Compare November 6, 2023 09:26
@mlugg mlugg enabled auto-merge November 6, 2023 09:27
@mlugg mlugg force-pushed the struct_field_init_pass branch from 59f145c to 1e6bcc5 Compare November 6, 2023 16:59
This change allows struct field inits to use layout information
of their own struct without causing a circular dependency.

`semaStructFields` caches the ranges of the init bodies in the `StructType`
trailing data. The init bodies are then resolved by `resolveStructFieldInits`,
which is called before the inits are actually required.

Within the init bodies, the struct decl's instruction is repurposed to refer
to the field type itself. This is to allow us to easily rebuild the inst_map
mapping required for the init body instructions to refer to the field type.

Thanks to @mlugg for the guidance on this one!
This was regressed in d657b6c, when
the comptime memory model for unions was changed to allow them to have
no defined tag type.
@mlugg mlugg force-pushed the struct_field_init_pass branch from 1e6bcc5 to 1acb6a5 Compare November 7, 2023 00:50
@mlugg mlugg merged commit b3462b7 into ziglang:master Nov 7, 2023
@andrewrk
Copy link
Member

andrewrk commented Nov 8, 2023

This broke #17892. I haven't tracked it down yet but I have a stack trace of optimized CBE: #17892 (comment)

Also I don't like those changes to packed struct representation because I don't think a flags field needs to be added to packed struct. I avoided it so far, and now 30 bits are going to be wasted for 2, which feels bad.

I should add though, thank you, this is good work 👍

@mlugg
Copy link
Member

mlugg commented Nov 8, 2023

Good point on the packed struct flags - I'm not sure if we can eliminate those bits entirely, but if not then I'd suggest we could perhaps repurpose a few bits of fields_len in TypeStructPacked.

@linusg
Copy link
Collaborator

linusg commented Nov 8, 2023

This caused a false struct depends on itself in kiesel - unfortunately the project is too large for me to easily reduce this, but building with zig master should reproduce:

src/types/language/Object/InternalMethods.zig:1:1: error: struct 'types.language.Object.InternalMethods' depends on itself
//! 6.1.7.2 Object Internal Methods and Internal Slots
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

mlugg added a commit to mlugg/zig that referenced this pull request Nov 8, 2023
…tion pointer field

b3462b7 caused a regression in a third-party project, since it forced
resolution of field initializers for any field call 'foo.bar()', despite
this only being necessary when 'bar' is a comptime field.

See ziglang#17692 (comment).
@kcbanner
Copy link
Contributor Author

kcbanner commented Nov 9, 2023

Good point on the packed struct flags - I'm not sure if we can eliminate those bits entirely, but if not then I'd suggest we could perhaps repurpose a few bits of fields_len in TypeStructPacked.

If we can steal those two bits from fields_len, I'm happy to PR that.

andrewrk pushed a commit that referenced this pull request Nov 9, 2023
…tion pointer field

b3462b7 caused a regression in a third-party project, since it forced
resolution of field initializers for any field call 'foo.bar()', despite
this only being necessary when 'bar' is a comptime field.

See #17692 (comment).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants