mirror of
https://codeberg.org/ziglang/zig.git
synced 2026-04-30 06:42:48 +03:00
Matthew Lugg
5d215838a7
I've realised that the cause of at least some of our weird CI flakiness was a bug in how `Nav` values were resolved. Consider this scenario: the frontend resolves the type of a `Nav`, and then sends a function to the backend, which requires the backend to lower a pointer to that `Nav`. The backend calls `InternPool.getNav` to determine the `Nav`'s type. However, this races with the frontend resolving the *value* of that `Nav`. This involves writing separately to two fields, `bits` and `type_or_value`. If only one of these changes is observed, then the backend will incorrectly interpret the type as the value or vice versa, leading to a crash or even a miscompilation. (Of course, there's also the straightforward issue that the racing loads were non-atomic, making them illegal). The only good solution to this was to make `Nav` 4 bytes bigger, giving it separate `type` and `value` fields. In theory that's a quite small change, but it ended up having a bunch of nice consequences which led to this diff being a bit bulkier than expected: * `Nav.Repr.Bits` was simplified, because it no longer has to track "resolution status": we can use `.none` for that. This frees up some bits to make things more consistent between the "type resolved" and "fully resolved" states. * This consistency allowed the `Nav.status` union to be replaced with a simpler field `Nav.resolved`, which is a bit nicer to work with. * Most of the "getter" functions were able to be removed from `Nav` because the state they were fetching had been moved to simple fields on `Nav.resolved`. * There were still a handful of free bits in `Nav.Repr.Bits`, which could be used to represent the "const" and "threadlocal" flags rather than these being stored on `Key.Extern` and `Key.Variable`. This is a bit more convenient for linkers. * With those bits gone, `Key.Variable` is a trivial wrapper around a type and an initial value, and the fact that a declaration is mutable can be represented solely through the "const" flag. Therefore, `Key.Variable` no longer served a purpose, and could be eliminated entirely in favour of storing the variable's initial value directly in the "value" field of the `Nav`. So, I'm quite pleased with this refactor! But anyway, regarding the bug fix which actually motivated this: if I've done my job correctly, this should solve some crashes, such as these (which were what tipped me off to this bug in the first place): https://codeberg.org/ziglang/zig/actions/runs/2306/jobs/7/attempt/1 https://codeberg.org/ziglang/zig/actions/runs/2173/jobs/6/attempt/1 ...and, who knows, perhaps even the random SIGSEGVs we've seen on some targets! Probably not, but one can hope.