From 8fa59f9d45656f2d8d5460c37d3bd02f3af221e4 Mon Sep 17 00:00:00 2001 From: Hojun-Cho Date: Sun, 28 Jun 2026 01:59:21 +0900 Subject: [PATCH] cgen: store a 2-byte struct-literal field with MOVW, not an over-wide MOVQ (#15) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit cg_structlit_fill/cgstructlitfill dispatched scalar field stores as {1->MOVB, 4->MOVL, else->MOVQ} with no fsz==2 case, so a 2-byte field was stored with an 8-byte MOVQ. Interior over-stores were harmlessly overwritten by the next field, but the LAST field at the frame edge corrupted the saved base pointer: an (S|e) union success variant places the struct payload after the 8B tag, landing the last field at -4(BP), so MOVQ AX,-4(BP) wrote into saved [BP] and POPQ BP restored garbage — a silent both-stage caller-frame clobber. Route the scalar store through the existing fldstoreop/fieldstoreop helper ({1->MOVB,2->MOVW,4->MOVL,else->MOVQ}), both stages; the #13 graduation comments already pre-documented this resolution. Pure width fix, no loud-stop (scalar widths are always {1,2,4,8} and narrowing is always correct). Value-asserting pin: an i64 sentinel live across the union-maker call (detects the clobber directly) + all members, with a non-union control. --- cmd/w6c/cgen.c | 13 +++---- selfhost/cmd/wcc/cgenutil.ww | 26 ++++++-------- test/lang/union_subtail_bp_test.ww | 54 ++++++++++++++++++++++++++++++ 3 files changed, 72 insertions(+), 21 deletions(-) create mode 100644 test/lang/union_subtail_bp_test.ww diff --git a/cmd/w6c/cgen.c b/cmd/w6c/cgen.c index 4c4f085f..9ba00b73 100644 --- a/cmd/w6c/cgen.c +++ b/cmd/w6c/cgen.c @@ -3362,9 +3362,12 @@ cg_widen_tagged_push(Cg *c, Local **locals_p, Type *dst, Node *src, int sz) * because the existing inline code's reload-before-each-store pattern * matches the helper's per-store reload exactly. * - * The scalar store dispatch stays at the explicit {1->MOVB, 4->MOVL, - * else MOVQ} shape (not fieldstoreop, which emits MOVW for fsz==2) to - * stay byte-identical with cstage pending task #13. */ + * The scalar store routes through fldstoreop {1->MOVB, 2->MOVW, + * 4->MOVL, else MOVQ}: the missing MOVW for fsz==2 over-stored a + * 2-byte tail field past its slot into saved BP on a union-return + * success variant (#15). Natural field offsets aren't 8-aligned, so + * MOVQ-rely-on-ceil-8-pad (sound for #10's 8-aligned scratch) is + * wrong here; the store must be width-sized. */ static void cg_structlit_fill(Cg *c, Local **locals_p, Type *lu, Node *lit, int mode, int srcoff, const char *name, int disp) @@ -3666,9 +3669,7 @@ cg_structlit_fill(Cg *c, Local **locals_p, Type *lu, Node *lit, amem(base_reg, disp + (int)foff)); continue; } - int op = A_MOVQ; - if (fsz == 1) op = A_MOVB; - else if (fsz == 4) op = A_MOVL; + int op = fldstoreop(ft, fsz); ins2(c, op, areg(D_AX), amem(base_reg, disp + (int)foff)); } diff --git a/selfhost/cmd/wcc/cgenutil.ww b/selfhost/cmd/wcc/cgenutil.ww index a28cd715..f628f84e 100644 --- a/selfhost/cmd/wcc/cgenutil.ww +++ b/selfhost/cmd/wcc/cgenutil.ww @@ -5189,11 +5189,10 @@ export fn dotchainresolve(c: *cgen, n: *syntax.node, // preserved because the existing inline code's reload-before-each- // store pattern matches the helper's per-store reload exactly. // -// Graduation note (task #13): the scalar store currently uses the -// explicit {1→MOVB, 4→MOVL, else MOVQ} dispatch to match cstage -// byte-identically — cstage hasn't yet learned MOVW for fsz==2. Once -// #13 aligns both stages, the dispatch can switch to fieldstoreop -// which already returns MOVW where appropriate. +// The scalar store routes through fieldstoreop (full field-width +// {1→MOVB, 2→MOVW, 4→MOVL, else MOVQ}); the missing MOVW for fsz==2 +// over-stored a 2-byte tail field past its slot into saved BP on a +// union-return success variant (#15). Symmetric with cstage fldstoreop. fn cgstructlitfill(c: *cgen, si: *structinfo, lit: *syntax.node, mode: i32, srcoff: i32, srcname: str, disp: i32) void = { @@ -5704,16 +5703,13 @@ fn cgstructlitfill(c: *cgen, si: *structinfo, lit: *syntax.node, }; fi = nil; } else { - // Explicit {1→MOVB, 4→MOVL, else MOVQ} - // dispatch (not fieldstoreop) to match - // cstage byte-identically. wwstage's - // fieldstoreop would return MOVW for - // fsz==2 which cstage doesn't emit — - // tracked as task #13. - let fsz: i32 = fi.fsz; - let op: str = "MOVQ"; - if (fsz == 1) { op = "MOVB"; }; - if (fsz == 4) { op = "MOVL"; }; + // Full field-width store via fieldstoreop + // {1→MOVB, 2→MOVW, 4→MOVL, else MOVQ}: the + // missing MOVW for fsz==2 over-stored a + // 2-byte tail field past its slot into + // saved BP on a union-return success + // variant (#15). Symmetric with cstage. + let op: str = fieldstoreop(c, fi); emitline("\t"); emitline(op); emitline("\tAX, "); diff --git a/test/lang/union_subtail_bp_test.ww b/test/lang/union_subtail_bp_test.ww new file mode 100644 index 00000000..90e0ba56 --- /dev/null +++ b/test/lang/union_subtail_bp_test.ww @@ -0,0 +1,54 @@ +// union_subtail_bp_test — #15: a union-return struct-lit fill stored every +// scalar field with MOVQ except the explicit {1→MOVB, 4→MOVL} arms, so a +// 2-byte (i16/u16) field fell through to an 8-byte MOVQ. For the LAST field at +// the frame edge this over-store ran past the slot into saved [BP]: a +// `(s14|e)` success variant places the s14 payload at -16(BP) after the 8B +// tag, so the tail field g (off 12) lands at -4(BP) and `MOVQ AX,-4(BP)` writes +// bytes -4..+3 — clobbering the low 4 bytes of saved BP. POPQ BP then restores +// a corrupted BP and the CALLER runs on a garbage frame. SILENT both-stage and +// byte-id-BLIND (both stages emit the same wrong MOVQ), so these VALUE asserts +// are the sole tooth. Fix: route the scalar store through fldstoreop / +// fieldstoreop (adds the missing MOVW for fsz==2). Reverting either stage to +// the MOVQ-default reverts the clobber and reddens. +// +// Two teeth: (1) a sentinel i64 live ACROSS the maker call detects the BP +// clobber directly — on the buggy build the corrupted POPQ BP moves the +// caller frame so the sentinel read mismatches. (2) assert all s14 members. +// union_maker (the bug) + plain_maker control (same struct, NON-union maker: +// g lands at -12(BP), in-frame dead space, no live neighbour — passes even on +// master). The control isolates the union-return path as the buggy one. + +package union_subtail_bp_test; + +type e = !i32; +type s14 = struct { a: i16, b: i16, c: i16, d: i16, e: i16, f: i16, g: i16 }; + +fn mk14() (s14 | e) = { + return s14 { a = 1i16, b = 2i16, c = 3i16, d = 4i16, e = 5i16, f = 6i16, g = 7i16 }; +}; +fn mk14p() s14 = { + return s14 { a = 1i16, b = 2i16, c = 3i16, d = 4i16, e = 5i16, f = 6i16, g = 7i16 }; +}; + +@test fn union_maker() void = { + let sentinel: i64 = 0x5151515151515151i64; // lives across the mk14 call + let arr: [2]s14; + arr[1] = mk14()!; // maker over-stores g past its slot into saved BP + assert(arr[1].a == 1i16); + assert(arr[1].b == 2i16); + assert(arr[1].c == 3i16); + assert(arr[1].d == 4i16); + assert(arr[1].e == 5i16); + assert(arr[1].f == 6i16); + assert(arr[1].g == 7i16); // the over-stored tail field + assert(sentinel == 0x5151515151515151i64); // BP-clobber tooth +}; + +@test fn plain_maker() void = { // CONTROL: same struct, NON-union maker + let sentinel: i64 = 0x5252525252525252i64; + let arr: [2]s14; + arr[1] = mk14p(); + assert(arr[1].a == 1i16); + assert(arr[1].g == 7i16); + assert(sentinel == 0x5252525252525252i64); +};