From 26c3f5ae955d01aa16341ab194600688b258f277 Mon Sep 17 00:00:00 2001 From: David Kimura Date: Fri, 31 Jul 2026 16:39:25 -0700 Subject: [PATCH] Fix inconsistent offset of locals declared after a dynamic array 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) --- examples/dynamic_array.c | 58 +++++++ src/generator.c | 335 ++++++++++++++++++++++++--------------- 2 files changed, 263 insertions(+), 130 deletions(-) create mode 100644 examples/dynamic_array.c diff --git a/examples/dynamic_array.c b/examples/dynamic_array.c new file mode 100644 index 0000000..4edca5a --- /dev/null +++ b/examples/dynamic_array.c @@ -0,0 +1,58 @@ +/* + * The size expression of a variable length array is evaluated exactly once, + * where the declaration is reached, and the address of a local declared after + * such an array does not depend on that size. + */ + +int size() +{ + printf("sizing array\n"); + return 4; +} + +void print_locals() +{ + int n = 2; + int array[n]; + int i; + + i = 7; + + /* + * Reassigning 'n' must not move 'i'. Its offset was fixed at compile time. + */ + n = 5; + + printf("i=%d\n", i); + printf("n=%d\n", n); +} + +void print_array() +{ + /* + * "sizing array" is printed once, no matter how many times 'array' and + * 'total' are referenced below. + */ + int array[size()]; + int total; + int i; + + for (i=0; i<4; i++) + { + array[i] = i * 10; + } + + total = 0; + for (i=0; i<4; i++) + { + total += array[i]; + } + + printf("total=%d\n", total); +} + +int main() +{ + print_locals(); + print_array(); +} diff --git a/src/generator.c b/src/generator.c index 9761225..aae7d20 100644 --- a/src/generator.c +++ b/src/generator.c @@ -21,9 +21,7 @@ static void visit_expression(struct astnode *ast, struct ast_parameter_type_list *parameters, struct ast_declaration_list *declarations); static void -identifier_offset(char *identifier, - struct ast_parameter_type_list *parameters, - struct ast_declaration_list *declarations); +identifier_offset(char *identifier); static char * get_32bit_register(int argnum) @@ -72,6 +70,145 @@ align8(int size) return size + ((size % 8 == 0) ? 0 : 8 - (size % 8)); } +/* + * The stack frame of a function is split into a static area and a dynamic + * area. Every parameter, scalar and fixed size array is assigned a + * compile-time-constant offset from the frame pointer (rbp) in the static + * area. A variable length array is given only an 8 byte pointer slot in the + * static area; its data is carved out of the dynamic area below and the slot + * holds the address of that data. + * + * ``` + * int f() + * { + * int i; + * int array[g()]; + * } + * ``` + * + * --------- High memory + * | ret | + * rbp -> --------- + * | i | static area - constant offsets from rbp + * rbp-16 -> --------- + * | array |--. variable length array pointer slot + * rbp-24 -> --------- | + * | | <-' dynamic area - size known only at runtime + * | array[] | + * rsp -> --------- Low memory (top of stack) + * ``` + * + * An offset is the running total *after* adding the object's own size, so + * 'rbp - offset' is the base address of the object and indexing walks upward + * from there. + */ +struct frame_slot +{ + char *identifier; + int offset; + int is_vla; +}; + +#define MAX_FRAME_SLOTS 256 +static struct frame_slot frame_slots[MAX_FRAME_SLOTS]; +static int frame_slots_size; +static int frame_static_size; + +static struct frame_slot * +frame_find(char *identifier) +{ + int i; + + for (i=0; icount != NULL && + declarator->count->elided_type != AST_INTEGER_CONSTANT; +} + +/* + * Assign a constant frame offset to every parameter and local of a function. + * This runs before any code is emitted for the function body so that a + * variable's address never depends on a value that is only known at runtime. + */ +static void +build_frame(struct ast_parameter_type_list *parameters, + struct ast_declaration_list *declarations) +{ + int i, j, size; + struct ast_declaration *declaration; + struct ast_declarator *declarator; + + frame_slots_size = 0; + + /* + * Begin at 8 as the old rbp push is on the stack. + */ + frame_static_size = 8; + + for (i=0; parameters && isize; i++) + { + declaration = parameters->items[i]; + + frame_add_slot(declaration->declarators[0]->declarator_identifier, + align8(size_of_type(declaration->type_specifiers)), + 0); + } + + for (i=0; declarations && isize; i++) + { + declaration = declarations->items[i]; + + for (j=0; jdeclarators_size; j++) + { + declarator = declaration->declarators[j]; + size = align8(size_of_type(declaration->type_specifiers)); + + if (is_variable_length(declarator)) + { + frame_add_slot(declarator->declarator_identifier, 8, 1); + } + else if (declarator->count != NULL) + { + frame_add_slot(declarator->declarator_identifier, + size * declarator->count->int_value, 0); + } + else + { + frame_add_slot(declarator->declarator_identifier, size, 0); + } + } + } +} + /* * cursor and string_literal_buffer are used as a buffer to store that will * later be appended to the end of the assembly file. @@ -242,8 +379,7 @@ visit_function_call(struct ast_expression *ast, } else if (ast->arguments[i]->kind == PTR_VALUE) { - identifier_offset(ast->arguments[i]->identifier, - parameters, declarations); + identifier_offset(ast->arguments[i]->identifier); write_assembly(" mov (%%rbx), %%rax"); write_assembly(" mov (%%rax), %%%s", get_32bit_register(i)); } @@ -331,7 +467,7 @@ visit_identifier(struct ast_expression *ast, if (strcmp(ast->identifier, parameter->declarators[0]->declarator_identifier) == 0) { - identifier_offset(ast->identifier, parameters, declarations); + identifier_offset(ast->identifier); write_assembly(" mov (%%rbx), %%eax"); goto done; } @@ -348,7 +484,7 @@ visit_identifier(struct ast_expression *ast, if (ast->kind == PTR_VALUE) { - identifier_offset(ast->identifier, parameters, declarations); + identifier_offset(ast->identifier); write_assembly(" leaq (%%rbx), %%rax"); } else @@ -357,7 +493,7 @@ visit_identifier(struct ast_expression *ast, { visit_expression((struct astnode *)ast->extra, parameters, declarations); write_assembly(" push %%rax"); - identifier_offset(ast->identifier, parameters, declarations); + identifier_offset(ast->identifier); write_assembly(" pop %%rax"); write_assembly(" mov %%rax, %%rcx"); write_assembly(" leaq (%%rbx), %%rdx"); @@ -366,7 +502,7 @@ visit_identifier(struct ast_expression *ast, } else { - identifier_offset(ast->identifier, parameters, declarations); + identifier_offset(ast->identifier); write_assembly(" mov (%%rbx), %%eax"); } } @@ -536,92 +672,31 @@ visit_equality_expression(struct ast_binary_op *ast, } } +/* + * Load the address of a parameter or local into rbx. The frame layout was + * fixed by build_frame() before any code was emitted, so this is a single + * instruction and, crucially, evaluates nothing at runtime. + */ static void -identifier_offset(char *identifier, - struct ast_parameter_type_list *parameters, - struct ast_declaration_list *declarations) +identifier_offset(char *identifier) { - int i, j, local_size; - struct ast_declaration *parameter; - struct ast_declaration *declaration; + struct frame_slot *slot; - /* - * Caculate start of local variables offset from block pointer. - * - * Local variables are offset from the frame pointer (rbp) based on - * declaration position in the function. In the following function, 'i' is - * declared first so it is immediately below the frame pointer in the - * stack. - * - * ``` - * void f() - * { - * int i; - * int a[4]; - * } - * ``` - * - * In this case 'i' would be at 8 off frame pointer and 'a' would be 24 - * off frame pointer (24 == 8 + 4 * 4). - * - * --------- High memory - * | ret | - * rbp -> --------- - * | i | - * rbp-8 -> --------- - * | | - * | a | - * | | - * rbp-24 -> --------- Low memory (top of stack) - */ - write_assembly(" mov $8, %%rcx"); - for (i=0; parameters && isize; i++) - { - parameter = parameters->items[i]; + slot = frame_find(identifier); + assert(slot != NULL); - write_assembly(" add $%d, %%rcx", - align8(size_of_type(parameter->type_specifiers))); - if (strcmp(identifier, - parameter->declarators[0]->declarator_identifier) == 0) - { - goto end; - } + if (slot->is_vla) + { + /* + * The array's address was computed once in the prologue and parked in + * its static pointer slot, so nothing is re-evaluated here. + */ + write_assembly(" movq -%d(%%rbp), %%rbx", slot->offset); } - - for (i=0; declarations && isize; i++) + else { - declaration = declarations->items[i]; - - for (j=0; jdeclarators_size; j++) - { - if (declaration->declarators[j]->count != NULL) - { - write_assembly(" push %%rcx"); - visit_expression( - (struct astnode *)declaration->declarators[j]->count, - parameters, declarations); - write_assembly(" imul $%d, %%rax", - align8(size_of_type(declaration->type_specifiers))); - write_assembly(" pop %%rcx"); - write_assembly(" add %%rax, %%rcx"); - } - else - { - write_assembly(" add $%d, %%rcx", - align8(size_of_type(declaration->type_specifiers))); - } - } - - if (strcmp(identifier, - declaration->declarators[0]->declarator_identifier) == 0) - { - goto end; - } + write_assembly(" leaq -%d(%%rbp), %%rbx", slot->offset); } - -end: - write_assembly(" movq %%rbp, %%rbx"); - write_assembly(" subq %%rcx, %%rbx"); } static void @@ -637,8 +712,7 @@ visit_assignment_expression(struct ast_binary_op *ast, { visit_expression(ast->right, parameters, declarations); write_assembly(" push %%rax"); - identifier_offset(((struct ast_expression *)ast->left)->identifier, - parameters, declarations); + identifier_offset(((struct ast_expression *)ast->left)->identifier); write_assembly(" push %%rbx"); visit_expression((struct astnode *)((struct ast_expression *)ast->left)->extra, parameters, declarations); write_assembly(" mov %%rax, %%rdi"); @@ -651,8 +725,7 @@ visit_assignment_expression(struct ast_binary_op *ast, { visit_expression(ast->right, parameters, declarations); write_assembly(" push %%rax"); - identifier_offset(((struct ast_expression *)ast->left)->identifier, - parameters, declarations); + identifier_offset(((struct ast_expression *)ast->left)->identifier); write_assembly(" pop %%rax"); write_assembly(" mov %%rax, (%%rbx)"); } @@ -664,8 +737,7 @@ visit_assignment_expression(struct ast_binary_op *ast, { visit_expression(ast->right, parameters, declarations); write_assembly(" push %%rax"); - identifier_offset(((struct ast_expression *)ast->left)->identifier, - parameters, declarations); + identifier_offset(((struct ast_expression *)ast->left)->identifier); write_assembly(" push %%rbx"); visit_expression((struct astnode *)((struct ast_expression *)ast->left)->extra, parameters, declarations); write_assembly(" mov %%rax, %%rdi"); @@ -680,8 +752,7 @@ visit_assignment_expression(struct ast_binary_op *ast, { visit_expression(ast->right, parameters, declarations); write_assembly(" push %%rax"); - identifier_offset(((struct ast_expression *)ast->left)->identifier, - parameters, declarations); + identifier_offset(((struct ast_expression *)ast->left)->identifier); write_assembly(" pop %%rax"); write_assembly(" mov %%eax, %%ecx"); write_assembly(" mov (%%rbx), %%eax"); @@ -696,8 +767,7 @@ visit_assignment_expression(struct ast_binary_op *ast, { visit_expression(ast->right, parameters, declarations); write_assembly(" push %%rax"); - identifier_offset(((struct ast_expression *)ast->left)->identifier, - parameters, declarations); + identifier_offset(((struct ast_expression *)ast->left)->identifier); write_assembly(" push %%rbx"); visit_expression((struct astnode *)((struct ast_expression *)ast->left)->extra, parameters, declarations); write_assembly(" mov %%rax, %%rdi"); @@ -712,8 +782,7 @@ visit_assignment_expression(struct ast_binary_op *ast, { visit_expression(ast->right, parameters, declarations); write_assembly(" push %%rax"); - identifier_offset(((struct ast_expression *)ast->left)->identifier, - parameters, declarations); + identifier_offset(((struct ast_expression *)ast->left)->identifier); write_assembly(" pop %%rax"); write_assembly(" mov %%eax, %%ecx"); write_assembly(" mov (%%rbx), %%eax"); @@ -728,8 +797,7 @@ visit_assignment_expression(struct ast_binary_op *ast, { visit_expression(ast->right, parameters, declarations); write_assembly(" push %%rax"); - identifier_offset(((struct ast_expression *)ast->left)->identifier, - parameters, declarations); + identifier_offset(((struct ast_expression *)ast->left)->identifier); write_assembly(" push %%rbx"); visit_expression((struct astnode *)((struct ast_expression *)ast->left)->extra, parameters, declarations); write_assembly(" mov %%rax, %%rdi"); @@ -744,8 +812,7 @@ visit_assignment_expression(struct ast_binary_op *ast, { visit_expression(ast->right, parameters, declarations); write_assembly(" push %%rax"); - identifier_offset(((struct ast_expression *)ast->left)->identifier, - parameters, declarations); + identifier_offset(((struct ast_expression *)ast->left)->identifier); write_assembly(" pop %%rax"); write_assembly(" mov %%eax, %%ecx"); write_assembly(" mov (%%rbx), %%eax"); @@ -876,11 +943,10 @@ visit_function_definition(struct ast_function *ast) { /* add to local symbol table */ - int i, j; + int i, j, slot; struct listnode *list; struct astnode *statement; struct ast_declarator *declarator; - struct ast_declaration *parameter; struct ast_declaration *declaration; struct ast_compound_statement *compound; struct ast_parameter_type_list *parameters; @@ -900,35 +966,49 @@ visit_function_definition(struct ast_function *ast) write_assembly(" push %%rbp"); write_assembly(" movq %%rsp, %%rbp"); + build_frame(parameters, compound->declarations); + /* - * Begin at 8 as old rbp push is on the stack. + * Reserve the static area in one go. Every parameter, scalar and fixed + * size array lives here at a constant offset from rbp, as does the pointer + * slot of each variable length array. Reserving it up front means the + * address of a local never depends on a value computed at runtime. * * NOTE: System-V AMD64 ABI specifies in section 3.2.3 that the 6 function * arguments are passed through registers. The remainder is pushed on the * stack. + * + * NOTE: System-V AMD64 ABI mandates in section 3.2.2 that the stack frame + * must be 16 bytes aligned. Align here because the declarations below may + * emit a call while evaluating a size expression or an initializer. */ - write_assembly(" subq $8, %%rsp"); + write_assembly(" subq $%d, %%rsp", frame_static_size); + write_assembly(" andq $0xFFFFFFFFFFFFFFF0, %%rsp"); + for (i=0; parameters && isize; i++) { - parameter = parameters->items[i]; - - write_assembly(" pushq %%%s", get_64bit_register(i)); + write_assembly(" movq %%%s, -%d(%%rbp)", get_64bit_register(i), + frame_slots[i].offset); } - /* - * Reserve stack space for local variables in this function so that if this - * function calls another function it will not clobber this functions local - * variables on the stack. - * - * NOTE: System-V AMD64 ABI mandates in section 3.2.2 that the stack frame - * must be 16 bytes aligned. - */ + slot = parameters ? parameters->size : 0; + for (i=0; compound->declarations && ideclarations->size; i++) { declaration = compound->declarations->items[i]; - for (j=0; jdeclarators_size; j++) + for (j=0; jdeclarators_size; j++, slot++) { - if (declaration->declarators[j]->count != NULL) + /* + * A variable length array's size expression is evaluated exactly + * once, here, where the declaration is reached. Its data is taken + * from the dynamic area and the resulting address is stored in the + * array's static pointer slot, so later references to *any* local + * never need to evaluate the size expression again. + * + * The andq only ever lowers rsp, so the array still receives at + * least the requested number of bytes. + */ + if (frame_slots[slot].is_vla) { visit_expression( (struct astnode *)declaration->declarators[j]->count, @@ -937,11 +1017,9 @@ visit_function_definition(struct ast_function *ast) write_assembly(" imul $%d, %%eax", align8(size_of_type(declaration->type_specifiers))); write_assembly(" subq %%rax, %%rsp"); - } - else - { - write_assembly(" subq $%d, %%rsp", - align8(size_of_type(declaration->type_specifiers))); + write_assembly(" andq $0xFFFFFFFFFFFFFFF0, %%rsp"); + write_assembly(" movq %%rsp, -%d(%%rbp)", + frame_slots[slot].offset); } if (declaration->declarators[j]->initializer) @@ -952,15 +1030,12 @@ visit_function_definition(struct ast_function *ast) compound->declarations); write_assembly(" push %%rax"); identifier_offset( - declaration->declarators[j]->declarator_identifier, - parameters, - compound->declarations); + declaration->declarators[j]->declarator_identifier); write_assembly(" pop %%rax"); write_assembly(" mov %%rax, (%%rbx)"); } } } - write_assembly(" andq $0xFFFFFFFFFFFFFFF0, %%rsp"); for (i=0; compound->statements && istatements->size; i++) {