Skip to content

sema: Improvements to union and struct layout resolution to solve a false-positive circular dependency error#17490

Closed
kcbanner wants to merge 1 commit intoziglang:masterfrom
kcbanner:container_circular_dependency
Closed

sema: Improvements to union and struct layout resolution to solve a false-positive circular dependency error#17490
kcbanner wants to merge 1 commit intoziglang:masterfrom
kcbanner:container_circular_dependency

Conversation

@kcbanner
Copy link
Contributor

@kcbanner kcbanner commented Oct 12, 2023

Fixes #17287
Fixes #17533

These changes bring unions up to speed with structs in terms of alignment and size resolution. The main goal of this PR was to resolve the circular dependency error you used to get when doing something like this:

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

const Union = extern union {
    outer: ?*UnionOuter,
    inner: ?*UnionInner,
};

const UnionOuter = extern struct {
    u: Union = std.mem.zeroes(Union),
};
17287.zig:8:22: error: union '17287.Union' depends on itself
const Union = extern union {
              ~~~~~~~^~~~~
17287.zig:14:5: note: while checking this field
    u: Union = std.mem.zeroes(Union),
    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Since the fields are pointer types, there isn't actually a circular dependency here.

Changes:

  • Add resolveUnionAlignment, to resolve a union's alignment only, without triggering layout resolution.
  • Update resolveUnionLayout to cache size, alignment, and padding. abiSizeAdvanced and abiAlignmentAdvanced now use this information instead of computing it each time.
  • Resolve the false-positive circular dependency error when a union or struct uses @typeInfo on itself (such as std.mem.zeroes), when it has a field who's type refers to the containing type through a pointer.

@mlugg
Copy link
Member

mlugg commented Oct 12, 2023

I don't understand what the titular feature of this PR is doing. Clearly, in order to determine the ABI size of a union, we must know the size of all of its fields. Determining ABI size is a subset of layout resolution, so to resolve the former without resolving the latter (as is in the title of this PR) would imply that there exists some type whose size we can determine without fully resolving its in-memory layout. This seems contradictory to the whole idea of layout resolution. Do you have an example of such a case? (That is, a case where resolving a type's size requires less information than resolving its layout.)

@kcbanner kcbanner force-pushed the container_circular_dependency branch from 93d503c to 428f970 Compare October 12, 2023 22:54
@kcbanner kcbanner changed the title sema: rework union layout resolution to resolve only the union's layout, instead of recursively resolving all fields sema: Improvements to union and struct layout resolution to solve a false-positive circular dependency error Oct 12, 2023
@kcbanner
Copy link
Contributor Author

I don't understand what the titular feature of this PR is doing. Clearly, in order to determine the ABI size of a union, we must know the size of all of its fields. Determining ABI size is a subset of layout resolution, so to resolve the former without resolving the latter (as is in the title of this PR) would imply that there exists some type whose size we can determine without fully resolving its in-memory layout. This seems contradictory to the whole idea of layout resolution. Do you have an example of such a case? (That is, a case where resolving a type's size requires less information than resolving its layout.)

@mlugg Sorry, the original title was poor - it was the commit message of my initial changes before I really understood what was going on. I've updated the title and description now.

@kcbanner kcbanner force-pushed the container_circular_dependency branch 3 times, most recently from 88489bb to 981e001 Compare October 13, 2023 05:32
- Add resolveUnionAlignment, to resolve a union's alignment only, without triggering layout resolution
- Update resolveUnionLayout to cache size, alignment, and padding
- Resolve a false-positive circular dependency error when a union or struct uses @typeinfo on itself (such as std.mem.zeroes)
@kcbanner kcbanner force-pushed the container_circular_dependency branch from 981e001 to 3a89324 Compare October 13, 2023 05:32
@kcbanner kcbanner marked this pull request as ready for review October 13, 2023 05:33
@kcbanner
Copy link
Contributor Author

For reviewer's reference (and as a justification for CircularComptimeRequirementCheck or something similar) this is stack of the error: union 'container_circular_dependency.Union' depends on itself error if you comment out all the return error.CircularComptimeRequirementCheck; that I've added.

zig.exe;resolveUnionLayout();7FF60D07BB90;6FBB90
zig.exe;resolveTypeLayout();7FF60CE08456;488456
zig.exe;typeAbiSize();7FF60D4BBC94;B3BC94
zig.exe;resolveStructLayout();7FF60D07A6D5;6FA6D5
zig.exe;resolveTypeLayout();7FF60CE083D2;4883D2
zig.exe;zirTypeInfo();7FF60D42D3E8;AAD3E8
zig.exe;analyzeBodyInner();7FF60D06A4F2;6EA4F2
zig.exe;analyzeBodyBreak();7FF60CE0F581;48F581
zig.exe;analyzeBodyInner();7FF60D07904C;6F904C
zig.exe;analyzeBody();7FF60D38DD7F;A0DD7F
zig.exe;analyzeCall();7FF60D930E41;FB0E41
zig.exe;zirCall__anon_161264();7FF60D3F2E77;A72E77
zig.exe;analyzeBodyInner();7FF60D066F53;6E6F53
zig.exe;analyzeBodyBreak();7FF60CE0F581;48F581
zig.exe;resolveBody();7FF60D9278CD;FA78CD
zig.exe;semaStructFields();7FF60D9F1257;1071257
zig.exe;resolveTypeFieldsStruct();7FF60D4C65E2;B465E2
zig.exe;comptimeOnlyAdvanced();7FF60D07EC26;6FEC26
zig.exe;comptimeOnlyAdvanced();7FF60D07E329;6FE329
zig.exe;typeRequiresComptime();7FF60CDF77DD;4777DD
zig.exe;hasRuntimeBitsAdvanced();7FF60D080C24;700C24
zig.exe;typeHasRuntimeBits();7FF60CDFAC6C;47AC6C
zig.exe;unionFieldHasRuntimeBits();7FF60D4C0344;B40344
zig.exe;resolveUnionLayout();7FF60D07BDB1;6FBDB1
zig.exe;resolveTypeLayout();7FF60CE08456;488456
zig.exe;zirTypeInfo();7FF60D42A75E;AAA75E
zig.exe;analyzeBodyInner();7FF60D06A4F2;6EA4F2
zig.exe;analyzeBodyBreak();7FF60CE0F581;48F581
zig.exe;analyzeBodyInner();7FF60D07904C;6F904C
zig.exe;analyzeBody();7FF60D38DD7F;A0DD7F
zig.exe;analyzeCall();7FF60D930E41;FB0E41
zig.exe;zirCall__anon_161264();7FF60D3F2E77;A72E77
zig.exe;analyzeBodyInner();7FF60D066F53;6E6F53
zig.exe;analyzeBodyBreak();7FF60CE0F581;48F581
zig.exe;resolveBody();7FF60D9278CD;FA78CD
zig.exe;semaStructFields();7FF60D9F1257;1071257
zig.exe;resolveTypeFieldsStruct();7FF60D4C65E2;B465E2
zig.exe;comptimeOnlyAdvanced();7FF60D07EC26;6FEC26
zig.exe;typeRequiresComptime();7FF60CDF77DD;4777DD
zig.exe;validateVarType();7FF60D901A07;F81A07
zig.exe;zirAllocMut();7FF60D3E1FFB;A61FFB
zig.exe;analyzeBodyInner();7FF60D065DD4;6E5DD4
zig.exe;resolveBlockBody();7FF60D9E509A;106509A
zig.exe;zirBlock();7FF60D4BB201;B3B201
zig.exe;analyzeBodyInner();7FF60D078DFE;6F8DFE
zig.exe;analyzeBody();7FF60D38DD7F;A0DD7F
zig.exe;analyzeFnBody();7FF60D045507;6C5507
zig.exe;ensureFuncBodyAnalyzed();7FF60CDF0FA4;470FA4
zig.exe;processOneJob();7FF60CDEEFBD;46EFBD
zig.exe;performAllTheWork();7FF60CC34098;2B4098
zig.exe;update();7FF60CC2F1F9;2AF1F9
zig.exe;updateModule();7FF60CC5DA65;2DDA65
zig.exe;buildOutputType();7FF60CC809E6;3009E6
zig.exe;mainArgs();7FF60CA9D8F1;11D8F1
zig.exe;main();7FF60CA9B16E;11B16E
zig.exe;main();7FF60CA9AE6A;11AE6A
zig.exe;__tmainCRTStartup();7FF60EFB56A6;26356A6
zig.exe;mainCRTStartup();7FF60EFB570C;263570C

@kcbanner
Copy link
Contributor Author

kcbanner commented Oct 21, 2023

After talking with @mlugg, I've decided to extra just the union alignment portion into #17658, and work on a different way to solve this circular dependency.

mlugg: So I think the best option is to introduce yet another resolution stage, where we resolve default inits after field types
mlugg: Here's a simple example of a dodgy case:

const S = struct { x: u32 = @alignOf(S) + 1 };

(The addition here is just to force us to resolve the lazy value)
This in theory is fine, right? We know from the field types that S has alignment 4, so x should have default value 5. However, because we try to resolve the inits at the same time as the field types, we can't know the alignment of S before we attempt to resolve the field init. In this case, it triggers the "guess pointer aligned" code, so S gets alignment 8

@kcbanner kcbanner closed this Oct 21, 2023
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.

Setting member default values causes false detection of a dependency loop @cImport "union depends on itself" regression

2 participants