state: recongize simple branch for finding closure layout - #29
Conversation
MeteorsLiu
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
MeteorsLiu
changed the title
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: closure conditional-select & store register tracking
This PR expands armPreservesRegisters / x86PreservesRegisters to allow conditional selects (CSEL/CMOVcc family) and capture stores (STR/STP, MOV-to-memory) on the fallthrough path between a newobject allocation and the closure PC store.
Assessment: The register-preservation logic is correct and appropriately conservative, and the test coverage is thorough.
- Stores correctly skip the
Args[0]source-register check (store = true), since a store reads its source rather than writing it, while still enforcing base-register preservation for pre/post-index writeback modes (verified byarm64-store-pre/post-index-objectandarm64-pair-store-*cases). - x86 MOV-to-memory is safely restricted to
MOVonly — no auto-writeback addressing exists on x86, so no register is clobbered; RMW forms of ADD/SUB/etc. remain rejected. - Conditional selects flow through the destination-register preservation check on both ISAs, with partial-width writes normalized to their 64-bit register.
TestClosureConditionalRegistersexhaustively exercises every opcode/condition across widths and destinations. - No bounds/decoding-safety concerns:
arm64asm.Inst.Argsis a fixed[4]Arg, and all operand accesses use safe type assertions. No performance regression (the x86boundarycursor keeps decode work linear).
One optional documentation nit noted inline; no blocking issues.
Note: go test ./internal/state/ could not run in this environment due to a pre-existing toolchain linkage error (invalid reference to reflect.ptrMap under go1.26.6), unrelated to this diff.
| // Pre/post-indexed loads also write their base register. | ||
| // Pre/post-indexed loads and stores also write their base register. | ||
| return mem.Mode == arm64asm.AddrOffset || armPreservesRegister(mem.Base, pcReg) | ||
| case arm64asm.MemExtend: |
There was a problem hiding this comment.
[P3] Clarify why MemExtend stores/loads return true unconditionally
The MemExtend branch returns true without checking the base/index against pcReg, unlike the MemImmediate branch just above which validates mem.Base. This is correct — ARM64 register-offset addressing has no writeback form, so base/index registers are only read, never written — but the asymmetry is not self-evident. A one-line comment (mirroring the pre/post-index comment above) noting that register-offset loads/stores never write their address registers would help future readers.
1 participant
No description provided.