diff --git a/packages/bugc/CHANGELOG.md b/packages/bugc/CHANGELOG.md index 50805812b8..3fc4c7d36d 100644 --- a/packages/bugc/CHANGELOG.md +++ b/packages/bugc/CHANGELOG.md @@ -87,6 +87,27 @@ support. Changes to the specification itself are tracked in the root significant byte. Before, it gave the offset from the low-order end, so a debugger read the wrong bytes: for example, an `address` at the start of a slot read as the slot's high-order 20 bytes ([#336]). +- Reading a signed integer narrower than a word from storage now + sign-extends it, so a struct field, variable or mapping value of type + `int8` that holds `-56` reads as `-56`. Before, at every optimization + level, a packed field read back as its unsigned bytes, such as `200` + ([#337]). +- A write to a storage value narrower than a slot, whether a struct + field or a variable such as `x: int8`, now writes only that value's + bytes. Before, at every optimization level, a negative signed value + such as `-2 as int16` filled every higher byte of the slot with `ff`, + so the fields packed above it read back wrong, and `x = -56` stored a + full word where Solidity stores one byte ([#337]). +- At optimization level 3, merging adjacent writes to one slot now masks + each value to its width. Before, a negative signed value written next + to other fields of a packed struct overwrote them ([#337]). +- At optimization level 3, merged writes to one slot that do not start + at the slot's first byte now land at their fields' offsets. Before, + each value was shifted by its field's offset twice, so writing `s.b` + and `s.c` but not `s.a` put them in the wrong bytes ([#337]). +- At optimization level 3, two writes in a row to the same field of a + packed struct now leave the second value. Before, read/write merging + combined the two values with `or` ([#337]). ## 0.1.0-preview.1 — 2026-10-03 @@ -134,3 +155,4 @@ First publication. [#334]: https://github.com/ethdebug/format/pull/334 [#335]: https://github.com/ethdebug/format/pull/335 [#336]: https://github.com/ethdebug/format/pull/336 +[#337]: https://github.com/ethdebug/format/pull/337 diff --git a/packages/bugc/src/evmgen/behavioral.test.ts b/packages/bugc/src/evmgen/behavioral.test.ts index d304190af3..1b934f90dd 100644 --- a/packages/bugc/src/evmgen/behavioral.test.ts +++ b/packages/bugc/src/evmgen/behavioral.test.ts @@ -1322,4 +1322,231 @@ code { size = msg.data.length; }`; } } }); + + describe("packed storage writes", () => { + // Fields pack from the low-order end of slot 0: + // a (1 byte), b (1), c (2), d (4), e (8). + const define = `define { + struct S { a: int8; b: uint8; c: int16; d: uint32; e: int64; }; +}`; + + // a = -56, b = 7, c = -2, d = 0x01020304, e = -3 + const word = BigInt( + "0x" + "00".repeat(16) + "fffffffffffffffd" + "01020304" + "fffe07c8", + ); + + const fields = [ + "s.a = -56 as int8;", + "s.b = 7 as uint8;", + "s.c = -2 as int16;", + "s.d = 16909060 as uint32;", + "s.e = -3 as int64;", + ]; + + for (const level of [0, 1, 2, 3] as const) { + it(`should mask constants in create (level ${level})`, async () => { + const source = `name PackedCreate; +${define} +storage { [0] s: S; [1] x: uint256; } +create { ${fields.join(" ")} } +code { x = 1; }`; + + const result = await executeProgram(source, { + optimizationLevel: level, + }); + expect(await result.getStorage(0n)).toBe(word); + }); + + it(`should mask constants in either order (level ${level})`, async () => { + const source = `name PackedReverse; +${define} +storage { [0] s: S; [1] x: uint256; } +create { x = 1; } +code { ${[...fields].reverse().join(" ")} }`; + + const result = await executeProgram(source, { + calldata: "", + optimizationLevel: level, + }); + expect(result.callSuccess).toBe(true); + expect(await result.getStorage(0n)).toBe(word); + }); + + it(`should mask runtime values (level ${level})`, async () => { + const source = `name PackedRuntime; +${define} +storage { + [0] s: S; + [1] na: int256; + [2] nc: int256; + [3] ne: int256; + [4] t: S; +} +create { + na = -56 as int256; + nc = -2 as int256; + ne = -3 as int256; +} +code { + let a = na as int8; + let b = 7 as uint8; + let c = nc as int16; + let d = 16909060 as uint32; + let e = ne as int64; + s.a = a; + s.b = b; + s.c = c; + s.d = d; + s.e = e; + t.e = s.e; + t.d = s.d; + t.c = s.c; + t.b = s.b; + t.a = s.a; +}`; + + const result = await executeProgram(source, { + calldata: "", + optimizationLevel: level, + }); + expect(result.callSuccess).toBe(true); + expect(await result.getStorage(0n)).toBe(word); + expect(await result.getStorage(4n)).toBe(word); + }); + + it(`should keep the fields around writes past offset 0 (level ${level})`, async () => { + const source = `name PackedMiddle; +${define} +storage { [0] s: S; [1] nc: int256; } +create { nc = -2 as int256; s.a = -56 as int8; s.e = -3 as int64; } +code { + let b = 7 as uint8; + let c = nc as int16; + let d = 16909060 as uint32; + s.b = b; + s.c = c; + s.d = d; +}`; + + const result = await executeProgram(source, { + calldata: "", + optimizationLevel: level, + }); + expect(result.callSuccess).toBe(true); + expect(await result.getStorage(0n)).toBe(word); + }); + + // Writes to one field: the later write replaces the earlier + for (const [writes, expected] of [ + ["s.a = a; s.a = ua;", 0x03n], + ["s.b = b; s.a = a; s.b = ub;", 0x0003c8n], + ] as const) { + it(`should apply \`${writes}\` in order (level ${level})`, async () => { + const source = `name PackedOverlap; +define { struct T { a: int8; b: int16; }; } +storage { [0] s: T; [1] n: int256; [2] u: uint256; } +create { n = -56 as int256; u = 3; } +code { + let a = n as int8; + let b = n as int16; + let ua = u as int8; + let ub = u as int16; + ${writes} +}`; + + const result = await executeProgram(source, { + calldata: "", + optimizationLevel: level, + }); + expect(result.callSuccess).toBe(true); + expect(await result.getStorage(0n)).toBe(expected); + }); + } + } + }); + + describe("narrow storage variables", () => { + const source = `name Narrow; +storage { [0] x: int8; [1] y: int16; [2] n: int256; } +create { x = -56 as int8; n = -2 as int256; } +code { let v = n as int16; y = v; }`; + + for (const level of [0, 1, 2, 3] as const) { + it(`should write only their own bytes (level ${level})`, async () => { + const result = await executeProgram(source, { + calldata: "", + optimizationLevel: level, + }); + expect(result.callSuccess).toBe(true); + expect(await result.getStorage(0n)).toBe(0xc8n); + expect(await result.getStorage(1n)).toBe(0xfffen); + }); + } + }); + + describe("signed storage reads", () => { + const neg = (n: bigint) => 2n ** 256n - n; + + const source = `name SignedReads; +define { + struct R { a: int8; b: int16; c: int64; d: int128; }; +} +storage { + [0] s: R; + [1] x: int8; + [2] y: int16; + [3] m: mapping; + [4] t: R; + [5] n: int256; + [10] o0: int256; + [11] o1: int256; + [12] o2: int256; + [13] o3: int256; + [14] o4: int256; + [15] o5: int256; + [16] o6: int256; + [17] o7: int256; + [18] o8: int256; +} +create { + s.a = -56 as int8; + s.b = -2 as int16; + s.c = -3 as int64; + s.d = -4 as int128; + x = -56 as int8; + y = -300 as int16; + m[5] = -7 as int16; + n = -9 as int256; +} +code { + let a = n as int8; + let b = n as int16; + t.a = a; + t.b = b; + o0 = s.a; + o1 = s.b; + o2 = s.c; + o3 = s.d; + o4 = x; + o5 = y; + o6 = m[5]; + o7 = t.a; + o8 = t.b; +}`; + + const expected = [56n, 2n, 3n, 4n, 56n, 300n, 7n, 9n, 9n].map(neg); + + for (const level of [0, 1, 2, 3] as const) { + it(`should sign-extend narrow signed values (level ${level})`, async () => { + const result = await executeProgram(source, { + calldata: "", + optimizationLevel: level, + }); + expect(result.callSuccess).toBe(true); + for (const [i, value] of expected.entries()) { + expect(await result.getStorage(10n + BigInt(i))).toBe(value); + } + }); + } + }); }); diff --git a/packages/bugc/src/evmgen/generation/instructions/cast.ts b/packages/bugc/src/evmgen/generation/instructions/cast.ts index 65a529592d..6f4d035ef7 100644 --- a/packages/bugc/src/evmgen/generation/instructions/cast.ts +++ b/packages/bugc/src/evmgen/generation/instructions/cast.ts @@ -19,18 +19,26 @@ export function generateCast( const debug = inst.operationDebug; const steps = Ir.Utils.castSteps(inst.value.type, inst.targetType); - const cast = steps + return pipe() + .then(loadValue(inst.value, { debug }), { as: "value" }) + .then(generateCastSteps(steps, debug)) + .then(storeValueIfNeeded(inst.dest, { debug })) + .done(); +} + +/** + * Apply cast steps to the value on top of the stack + */ +export function generateCastSteps( + steps: readonly Ir.Utils.CastStep[], + debug: Ir.Instruction.Debug | undefined, +): Transition { + return steps .map((step) => generateStep(step, debug)) .reduce>( (previous, next) => (state) => next(previous(state)), (state) => state, ); - - return pipe() - .then(loadValue(inst.value, { debug }), { as: "value" }) - .then(cast) - .then(storeValueIfNeeded(inst.dest, { debug })) - .done(); } function generateStep( diff --git a/packages/bugc/src/evmgen/generation/instructions/storage.ts b/packages/bugc/src/evmgen/generation/instructions/storage.ts index 11a85b6c39..6132b3c541 100644 --- a/packages/bugc/src/evmgen/generation/instructions/storage.ts +++ b/packages/bugc/src/evmgen/generation/instructions/storage.ts @@ -1,9 +1,10 @@ -import type * as Ir from "#ir"; +import * as Ir from "#ir"; import type { Stack } from "#evm"; import { type Transition, rebrand, pipe, operations } from "#evmgen/operations"; import { loadValue, storeValueIfNeeded } from "../values/index.js"; +import { generateCastSteps } from "./cast.js"; const { SWAP1, @@ -78,6 +79,21 @@ export function generateRead( return pipe().then(PUSHn(0n, { debug }), { as: "value" }).done(); } +/** + * Sign-extend a value read from (transient) storage to a full word, + * when its type is a signed integer narrower than a word. Storage + * keeps such a value in its own bytes only. + */ +function signExtend( + inst: Ir.Instruction.Read, + debug: Ir.Instruction.Debug, +): Transition { + return generateCastSteps( + Ir.Utils.castSteps(inst.type, Ir.Type.Scalar.uint256), + debug, + ); +} + /** * Storage read: SLOAD with optional partial-slot extraction */ @@ -93,6 +109,7 @@ function generateStorageRead( return pipe() .then(loadValue(inst.slot!, { debug }), { as: "key" }) .then(SLOAD({ debug }), { as: "value" }) + .then(signExtend(inst, debug)) .then(storeValueIfNeeded(inst.dest, { debug })) .done(); } @@ -123,6 +140,7 @@ function generateStorageRead( // shiftedValue & mask .then(AND({ debug }), { as: "value" }) + .then(signExtend(inst, debug)) .then(storeValueIfNeeded(inst.dest, { debug })) .done() ); @@ -143,6 +161,7 @@ function generateTransientRead( return pipe() .then(loadValue(inst.slot!, { debug }), { as: "key" }) .then(TLOAD({ debug }), { as: "value" }) + .then(signExtend(inst, debug)) .then(storeValueIfNeeded(inst.dest, { debug })) .done(); } @@ -174,6 +193,7 @@ function generateTransientRead( ) .then(AND({ debug }), { as: "value" }) + .then(signExtend(inst, debug)) .then(storeValueIfNeeded(inst.dest, { debug })) .done() ); @@ -415,8 +435,13 @@ function generateStorageWrite( // current & clearMask .then(AND({ debug }), { as: "clearedCurrent" }) - // Prepare new value at offset - .then(loadValue(inst.value!, { debug }), { as: "value" }) + // Prepare new value at offset, masked to its length so that + // a sign-extended negative value cannot clobber its neighbors + .then(loadValue(inst.value!, { debug }), { as: "b" }) + .then(PUSHn((1n << (BigInt(length) * 8n)) - 1n, { debug }), { + as: "a", + }) + .then(AND({ debug }), { as: "value" }) .then(PUSHn(BigInt(offset) * 8n, { debug }), { as: "shift" }) .then(SHL({ debug }), { as: "shiftedValue" }) diff --git a/packages/bugc/src/irgen/generate/statements/assign.ts b/packages/bugc/src/irgen/generate/statements/assign.ts index d3c2d6fe9d..dd68424b75 100644 --- a/packages/bugc/src/irgen/generate/statements/assign.ts +++ b/packages/bugc/src/irgen/generate/statements/assign.ts @@ -7,7 +7,11 @@ import { buildExpression } from "../expressions/index.js"; import { Process } from "../process.js"; import type { Context } from "../expressions/context.js"; -import { findStorageAccessChain, emitStorageChainStore } from "../storage.js"; +import { + findStorageAccessChain, + emitStorageChainStore, + emitStorageVariableStore, +} from "../storage.js"; /** * Build an assignment statement @@ -112,18 +116,7 @@ function* assignToTarget(node: Ast.Expression, value: Ir.Value): Process { // Check if it's storage const storageSlot = yield* Process.Storage.findSlot(name); if (storageSlot) { - yield* Process.Instructions.emit({ - kind: "write", - location: "storage", - slot: Ir.Value.constant( - BigInt(storageSlot.slot), - Ir.Type.Scalar.uint256, - ), - offset: Ir.Value.constant(0n, Ir.Type.Scalar.uint256), - length: Ir.Value.constant(32n, Ir.Type.Scalar.uint256), // 32 bytes for uint256 - value, - operationDebug: yield* Process.Debug.forAstNode(node), - } as Ir.Instruction.Write); + yield* emitStorageVariableStore(storageSlot, value, node); return; } diff --git a/packages/bugc/src/irgen/generate/storage.ts b/packages/bugc/src/irgen/generate/storage.ts index 1d22695ee7..7d63cc9982 100644 --- a/packages/bugc/src/irgen/generate/storage.ts +++ b/packages/bugc/src/irgen/generate/storage.ts @@ -128,6 +128,29 @@ function getFieldSize(type: Ir.Type): number { return 32; } +/** + * Emit a write of a whole storage variable. A variable narrower than a + * slot writes only its own bytes, at the low-order end of the slot. + */ +export function* emitStorageVariableStore( + slot: { slot: number; declaration: Ast.Declaration.Storage }, + value: Ir.Value, + node: Ast.Node | undefined, +): Process { + const type = yield* Process.Types.nodeType(slot.declaration); + const size = type ? getFieldSize(fromBugType(type)) : 32; + + yield* Process.Instructions.emit({ + kind: "write", + location: "storage", + slot: Ir.Value.constant(BigInt(slot.slot), Ir.Type.Scalar.uint256), + offset: Ir.Value.constant(0n, Ir.Type.Scalar.uint256), + length: Ir.Value.constant(BigInt(size), Ir.Type.Scalar.uint256), + value, + operationDebug: node ? yield* Process.Debug.forAstNode(node) : {}, + } as Ir.Instruction.Write); +} + /** * Emit a storage chain load */ @@ -277,16 +300,7 @@ export function* emitStorageChainStore( ): Process { // Handle direct storage variable assignment (no accesses) if (chain.accesses.length === 0) { - // Direct storage assignment using new unified format - yield* Process.Instructions.emit({ - kind: "write", - location: "storage", - slot: Ir.Value.constant(BigInt(chain.slot.slot), Ir.Type.Scalar.uint256), - offset: Ir.Value.constant(0n, Ir.Type.Scalar.uint256), - length: Ir.Value.constant(32n, Ir.Type.Scalar.uint256), - value, - operationDebug: node ? yield* Process.Debug.forAstNode(node) : {}, - } as Ir.Instruction.Write); + yield* emitStorageVariableStore(chain.slot, value, node); return; } diff --git a/packages/bugc/src/optimizer/steps/read-write-merging.ts b/packages/bugc/src/optimizer/steps/read-write-merging.ts index cb24ba1c7b..140342d1a6 100644 --- a/packages/bugc/src/optimizer/steps/read-write-merging.ts +++ b/packages/bugc/src/optimizer/steps/read-write-merging.ts @@ -239,7 +239,7 @@ export class ReadWriteMergingStep extends BaseOptimizationStep { // Sort by offset writeInfos.sort((a, b) => Number(a.offset - b.offset)); - // Check if writes are adjacent or overlapping (for simple merging) + // Check that the writes tile one range of the slot const canSimpleMerge = this.areWritesAdjacent(writeInfos); if (!canSimpleMerge) { @@ -253,79 +253,52 @@ export class ReadWriteMergingStep extends BaseOptimizationStep { let combinedValue: Ir.Value | null = null; let combinedDebug: Ir.Instruction.Debug = {}; - for (let i = 0; i < writeInfos.length; i++) { - const info = writeInfos[i]; - const shiftBits = info.offset * 8n; + // Emit `dest = left op right` and return dest as a value + const emit = ( + op: "and" | "shl" | "or", + left: Ir.Value, + right: Ir.Value, + ): Ir.Value => { + const dest = `t${tempCounter++}`; + instructions.push({ + kind: "binary", + op, + left, + right, + dest, + operationDebug: combinedDebug, + }); + return { kind: "temp", id: dest, type: Ir.Type.Scalar.uint256 }; + }; + const constant = (value: bigint): Ir.Value => ({ + kind: "const", + value, + type: Ir.Type.Scalar.uint256, + }); + for (const info of writeInfos) { // Track debug contexts as we process writes combinedDebug = Ir.Utils.combineDebugContexts( combinedDebug, info.write.operationDebug, ); - if (shiftBits > 0n) { - // Need to shift the value - const shiftTemp = `t${tempCounter++}`; - instructions.push({ - kind: "binary", - op: "shl", - left: info.write.value, - right: { - kind: "const", - value: shiftBits, - type: Ir.Type.Scalar.uint256, - }, - dest: shiftTemp, - operationDebug: combinedDebug, - }); - - const shiftedValue: Ir.Value = { - kind: "temp", - id: shiftTemp, - type: Ir.Type.Scalar.uint256, - }; - - if (combinedValue === null) { - combinedValue = shiftedValue; - } else { - // OR with previous combined value - const orTemp = `t${tempCounter++}`; - instructions.push({ - kind: "binary", - op: "or", - left: combinedValue, - right: shiftedValue, - dest: orTemp, - operationDebug: combinedDebug, - }); - combinedValue = { - kind: "temp", - id: orTemp, - type: Ir.Type.Scalar.uint256, - }; - } - } else { - // No shift needed - if (combinedValue === null) { - combinedValue = info.write.value; - } else { - // OR with previous combined value - const orTemp = `t${tempCounter++}`; - instructions.push({ - kind: "binary", - op: "or", - left: combinedValue, - right: info.write.value, - dest: orTemp, - operationDebug: combinedDebug, - }); - combinedValue = { - kind: "temp", - id: orTemp, - type: Ir.Type.Scalar.uint256, - }; - } + // Mask the value to its length, so that a sign-extended + // negative value cannot clobber the other parts. Code generation + // masks the merged value to its length, which covers the last part + let part = info.write.value; + if (info !== writeInfos[writeInfos.length - 1]) { + part = emit("and", part, constant((1n << (info.length * 8n)) - 1n)); + } + + // Place the part relative to the merged write's offset + const shift = (info.offset - writeInfos[0].offset) * 8n; + if (shift > 0n) { + part = emit("shl", part, constant(shift)); } + + combinedValue = + combinedValue === null ? part : emit("or", combinedValue, part); } // Calculate the merged write parameters @@ -384,16 +357,17 @@ export class ReadWriteMergingStep extends BaseOptimizationStep { } /** - * Check if writes are adjacent or can be easily combined + * Check that each write starts where the previous one ends */ private areWritesAdjacent(writeInfos: WriteInfo[]): boolean { for (let i = 1; i < writeInfos.length; i++) { const prev = writeInfos[i - 1]; const curr = writeInfos[i]; - // Check if current write starts at or before previous write ends - if (curr.offset > prev.offset + prev.length) { - // Gap between writes - not adjacent + // Each write must start where the previous one ends. A gap would + // clobber the bytes between them, and with an overlap the later + // write must replace the earlier one's bytes, not combine with them + if (curr.offset !== prev.offset + prev.length) { return false; } }