Fix inconsistent offset of locals declared after a dynamic array - #4
Open
dgkimura wants to merge 1 commit into
Open
Fix inconsistent offset of locals declared after a dynamic array#4dgkimura wants to merge 1 commit into
dgkimura wants to merge 1 commit into
Conversation
identifier_offset() did not look an identifier up in a symbol table. It recomputed the frame layout at runtime on every single reference by re-walking the declaration list and summing sizes into rcx. When it reached a declarator with a count it re-entered visit_expression() on the count expression to get the array's size in bytes. The prologue evaluated that same count expression independently when it reserved stack space, and nothing was memoized, so a dynamically sized array's size expression was evaluated once in the prologue plus once more for every reference to any local declared at or after the array. Two defects followed. The offsets disagreed whenever the size expression was not pure, so a local declared after the array had a different address at each reference. And re-evaluating the size expression is wrong on its own; C99 6.7.5.2p5 requires it to be evaluated exactly once when the declaration is reached. Split the frame into a static area and a dynamic area, as gcc and clang do. build_frame() runs before any code is emitted and assigns every parameter and local a compile-time-constant offset from rbp. Scalars and fixed size arrays live in the static area. A dynamically sized array gets only an 8 byte pointer slot there; its data is carved out of the dynamic area below and the slot holds the address. The size expression is evaluated once, in the prologue, where the declaration is reached. identifier_offset() collapses to a single instruction and takes only the identifier, so the parameters and declarations arguments are dropped from its 14 call sites. Nothing about a variable's address depends on a runtime value any more, which makes this bug class structurally impossible rather than patching one instance of it. Incidental effects of removing the runtime walk: - Every variable reference drops from O(number of declarations) instructions to one, and the prologue drops from one subq per local to a single subq for the static area. examples/primes.s shrinks from 346 to 213 lines with identical output. - identifier_offset() no longer emits a call or clobbers rax mid expression, which fixes miscompiles of nested calls such as 'a[i] = sq(i + 2)'. - Multi-declarator declarations resolve correctly. The old inner loop accumulated size for every declarator but only compared against declarators[0], so 'int a, b;' returned b's address for a. - The 32/64-bit mismatch between the two imul sites is gone; only the prologue's 'imul $N, %eax' remains. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
identifier_offset() did not look an identifier up in a symbol table. It recomputed the frame layout at runtime on every single reference by re-walking the declaration list and summing sizes into rcx. When it reached a declarator with a count it re-entered visit_expression() on the count expression to get the array's size in bytes.
The prologue evaluated that same count expression independently when it reserved stack space, and nothing was memoized, so a dynamically sized array's size expression was evaluated once in the prologue plus once more for every reference to any local declared at or after the array.
Two defects followed. The offsets disagreed whenever the size expression was not pure, so a local declared after the array had a different address at each reference. And re-evaluating the size expression is wrong on its own; C99 6.7.5.2p5 requires it to be evaluated exactly once when the declaration is reached.
Split the frame into a static area and a dynamic area, as gcc and clang do. build_frame() runs before any code is emitted and assigns every parameter and local a compile-time-constant offset from rbp. Scalars and fixed size arrays live in the static area. A dynamically sized array gets only an 8 byte pointer slot there; its data is carved out of the dynamic area below and the slot holds the address. The size expression is evaluated once, in the prologue, where the declaration is reached.
identifier_offset() collapses to a single instruction and takes only the identifier, so the parameters and declarations arguments are dropped from its 14 call sites. Nothing about a variable's address depends on a runtime value any more, which makes this bug class structurally impossible rather than patching one instance of it.
Incidental effects of removing the runtime walk: