sema: Rework Decl.value_arena to fix another memory corruption issue

[kcbanner]

Fixes #15150.

This fixes a bug where resolveStructLayout was promoting from stale value_arena state which was then overwritten when another ArenaAllocator higher in the call stack saved its state back. This resulted in the memory for struct_obj.optmized_order overlapping existing allocations.

My initial fix in c7067ef wasn't sufficient, as it only checked if the struct being resolved had the same owner as the current sema instance. However, it's possible for resolveStructLayout to be called when the sema instance has a different owner, but the struct decl's value_arena is currently in use higher up in the callstack.

This change introduces ValueArena, which holds the arena state as well as tracks if an arena has already been promoted from it. This allows callers to use the value_arena storage without needing to be aware of another user of this same storage higher up in the call stack.

Question for reviewers: Is that test case enough - or should it include more context / reference the bug issue?

An example of a callstack that exhibited the original bug (from the linked issue):

zig.exe;;7FF7F224A614;2CDA614
zig.exe;allocAdvancedWithRetAddr__anon_52271();7FF7EF8B9201;349201
zig.exe;alloc__anon_6268();7FF7EF6FD1A6;18D1A6
zig.exe;resolveStructLayout();7FF7EFBBAEE0;64AEE0  < struct_obj.owner_decl != sema.owner_decl_index, so it promotes from struct_obj, but this state is already "acquired" by analyzeFnBody above
zig.exe;resolveStructFully();7FF7EFBE2DB1;672DB1
zig.exe;resolveTypeFully();7FF7EF9A0088;430088
zig.exe;resolveTypeFully();7FF7EF99FF9A;42FF9A
zig.exe;resolveTypeFully();7FF7EF9A0677;430677
zig.exe;resolveTypeFully();7FF7EF99FF9A;42FF9A
zig.exe;resolveStructFully();7FF7EFBE2FC7;672FC7
zig.exe;resolveTypeFully();7FF7EF9A0088;430088
zig.exe;semaDecl();7FF7EF995C9F;425C9F             < new Sema instance created
zig.exe;ensureDeclAnalyzed();7FF7EF7C9F86;259F86
zig.exe;ensureDeclAnalyzed();7FF7F048D8B3;F1D8B3
zig.exe;analyzeDeclRef();7FF7EFF5178F;9E178F
zig.exe;analyzeDeclVal();7FF7F0454CCC;EE4CCC
zig.exe;zirDeclVal();7FF7EFE675A2;8F75A2
zig.exe;analyzeBodyInner();7FF7EFBA3B0C;633B0C
zig.exe;analyzeBodyBreak();7FF7EF996F11;426F11
zig.exe;resolveBody();7FF7F043F65D;ECF65D
zig.exe;semaStructFields();7FF7F04F3B11;F83B11
zig.exe;resolveTypeFieldsStruct();7FF7EFF3BCD7;9CBCD7
zig.exe;resolveTypeFields();7FF7EFBCA349;65A349
zig.exe;validateRunTimeType();7FF7F04E2CBD;F72CBD
zig.exe;validateVarType();7FF7F042AF58;EBAF58
zig.exe;zirAllocMut();7FF7EFE50FB6;8E0FB6
zig.exe;analyzeBodyInner();7FF7EFBA1DD2;631DD2
zig.exe;resolveBlockBody();7FF7F0488145;F18145
zig.exe;zirBlock();7FF7EFF2C514;9BC514
zig.exe;analyzeBodyInner();7FF7EFBB84C8;6484C8
zig.exe;analyzeBody();7FF7EFE073EF;8973EF
zig.exe;analyzeFnBody();7FF7EFB89F9D;619F9D
zig.exe;ensureFuncBodyAnalyzed();7FF7EF975C96;405C96
zig.exe;ensureFuncBodyAnalyzed();7FF7F0513E4A;FA3E4A
zig.exe;resolveInferredErrorSet();7FF7EFF66050;9F6050
zig.exe;analyzeIsNonErrComptimeOnly();7FF7EFF28F10;9B8F10
zig.exe;analyzeIsNonErr();7FF7F04639CB;EF39CB
zig.exe;zirIsNonErr();7FF7EFE7297B;90297B
zig.exe;analyzeBodyInner();7FF7EFBA599E;63599E
zig.exe;resolveBlockBody();7FF7F0488145;F18145
zig.exe;zirBlock();7FF7EFF2C514;9BC514
zig.exe;analyzeBodyInner();7FF7EFBB84C8;6484C8
zig.exe;resolveBlockBody();7FF7F0488145;F18145
zig.exe;zirBlock();7FF7EFF2C514;9BC514
zig.exe;analyzeBodyInner();7FF7EFBB84C8;6484C8
zig.exe;resolveBlockBody();7FF7F0488145;F18145
zig.exe;zirSwitchBlock();7FF7EFE82F91;912F91
zig.exe;analyzeBodyInner();7FF7EFBA6E12;636E12
zig.exe;resolveBlockBody();7FF7F0488145;F18145
zig.exe;zirBlock();7FF7EFF2C514;9BC514
zig.exe;analyzeBodyInner();7FF7EFBB84C8;6484C8
zig.exe;analyzeBody();7FF7EFE073EF;8973EF
zig.exe;analyzeCall();7FF7F044A94D;EDA94D
zig.exe;zirBuiltinCall();7FF7EFEC387D;95387D
zig.exe;analyzeBodyInner();7FF7EFBAA262;63A262
zig.exe;resolveBlockBody();7FF7F0488145;F18145
zig.exe;zirBlock();7FF7EFF2C514;9BC514
zig.exe;analyzeBodyInner();7FF7EFBB84C8;6484C8
zig.exe;analyzeBody();7FF7EFE073EF;8973EF
zig.exe;analyzeCall();7FF7F044A94D;EDA94D
zig.exe;zirCall();7FF7EFE62F1F;8F2F1F
zig.exe;analyzeBodyInner();7FF7EFBA3151;633151
zig.exe;analyzeBodyBreak();7FF7EF996F11;426F11
zig.exe;resolveBody();7FF7F043F65D;ECF65D
zig.exe;zirCall();7FF7EFE62A6D;8F2A6D
zig.exe;analyzeBodyInner();7FF7EFBA3151;633151
zig.exe;resolveBlockBody();7FF7F0488145;F18145
zig.exe;zirBlock();7FF7EFF2C514;9BC514
zig.exe;analyzeBodyInner();7FF7EFBB84C8;6484C8
zig.exe;analyzeBody();7FF7EFE073EF;8973EF
zig.exe;analyzeFnBody();7FF7EFB89F9D;619F9D       < state first promoted here
zig.exe;ensureFuncBodyAnalyzed();7FF7EF975C96;405C96
zig.exe;processOneJob();7FF7EF973C2E;403C2E
zig.exe;performAllTheWork();7FF7EF824905;2B4905
zig.exe;update();7FF7EF820DB5;2B0DB5
zig.exe;updateModule();7FF7EF84EB22;2DEB22
zig.exe;buildOutputType();7FF7EF6C8499;158499
zig.exe;mainArgs();7FF7EF69C246;12C246
zig.exe;main();7FF7EF69BDEF;12BDEF
zig.exe;main();7FF7EF69DC22;12DC22
zig.exe;;7FF7F151AF06;1FAAF06
zig.exe;;7FF7F151AF6C;1FAAF6C
kernel32.dll;;7FFB45507604;17604
ntdll.dll;;7FFB45BE26A1;526A1

[Vexu]

Doesn't this just achieve the same as storing the entire arena allocator rather than just the state?

[kcbanner]

Ah - you're right. My idea was to save memory by only storing the state pointer, but once I added the ref_count, ValueArena is the same size as just storing the ArenaAllocator. I can change Decl.value_arena to just be an ArenaAllocator pointer?

[Vexu]

I can change Decl.value_arena to just be an ArenaAllocator pointer?

As far as I can see yes.

[kcbanner]

I did a bit more digging, and I found that it wasn't possible to just replace ValueArena with just a pointer to an ArenaAllocator.

The main issue there was the allocation pattern in semaDecl. Since clearValues can be called later in the function, new allocations made before that point can't be put into the previous arena if one exists - they will just get freed by clearValues. Essentially we don't know if we are going to call clearValues until after we've already made allocations into a new arena.

Upon inspection of this pattern, I also noticed when the new state was being written here:

 defer {
        decl_arena_state.* = decl_arena.state;
        decl.value_arena = decl_arena_state;
}

... that it was possible for decl.value_arena to be non-null and have allocations. So, instead of the ref_count (which was made redundant by adding the allocator parameter to release), I now store a pointer to the previous arena, if one exists. This way, all the memory can be freed recursively instead of just the arena from the most recent analysis.

[andrewrk]

Thanks for digging into this. What a mess! I'm looking forward to replacing this arena memory management strategy with an intern pool as soon as possible.