fix(cpu): PPCBUG-700 VMX128 register accessors match canary bitfield layout

Independent review of P3 batch 2 (52ece4b) found that all three VMX128
register accessors disagreed with canary's FormatVX128/VX128_R bitfield
struct (`xenia-canary/src/xenia/cpu/ppc/ppc_decode_data.h:484-663`). The
audit at line 2958 had marked these "confirmed-clean" but had miscounted
LSB-first bitfield offsets.

Canary's actual layout (LSB-first, GCC/Clang/MSVC on x86):
  VA128 = VA128l(5) | VA128h(1)<<5 | VA128H(1)<<6
        = PPC[11:15] | PPC[26]<<5 | PPC[21]<<6  (7-bit selector, 3 fields)
  VB128 = VB128l(5) | VB128h(2)<<5
        = PPC[16:20] | PPC[30:31]<<5            (7-bit selector, 2 fields)
  VD128 = VD128l(5) | VD128h(2)<<5
        = PPC[6:10]  | PPC[28:29]<<5            (7-bit selector, 2 fields)
  VX128_R Rc = PPC[25]  (host bit 6)             not PPC[27] as prior fix had

The buggy convention was internally consistent with hand-crafted test
fixtures (which set bits 29/21/22 to encode the high registers, matching
the buggy accessor). Real Xbox 360 game code follows canary's convention,
so any production VMX128 instruction with VR >= 32 was silently mis-decoded
— but no unit test exercised that path until the va128 fix in 52ece4b
exposed the inconsistency.

Changes:
- decoder.rs: rewrite va128/vb128/vd128/vx128r_rc_bit to canary positions.
  Drop the speculative `key4_dt` dot-form dispatch in decode_op6 — canary
  has no separate dot-form opcodes for VX128_R compute ops; Rc is a
  runtime modifier read by the interpreter via vx128r_rc_bit().
- decoder.rs tests: rewrite vmx128_test_word helper for canary layout;
  rename/re-encode vmx128_vd128_*, vmx128_va128_*, vmx128_vb128_* tests.
- interpreter.rs: update encode_vpkd3d128 test helper to encode VD via
  canary's VD128h field; tests now pass vd=96 explicitly.
- tests/disasm_goldens.rs: replace the vrlimi128/vsrw128/vpermwi128/
  vperm128 hand-encoded raws with canary-compliant encodings; introduce
  a shared `encode_vx128` helper.
- tests/golden/vmx128_registers.json: re-encode 9 entries (vperm128,
  vsrw128 ×2, vpermwi128, vrlimi128 ×2, vmaddfp128, vmaddcfp128,
  vnmsubfp128) to canary-compliant raws preserving the same expected
  operand strings.
- audit-findings.md: new PPCBUG-700 entry documenting the discovery and
  invalidating the audit's "confirmed-clean" assessment.

Affects all VMX128 binary ops (vaddfp128, vsubfp128, vmulfp128, vand128,
vor128, vxor128, vnor128, vandc128, vsel128, vslo128, vsro128, vperm128,
vsrw128, vmaddfp128, vmaddcfp128, vnmsubfp128, vpkd3d128, vpkshss128,
vpkshus128, vpkswss128, vpkswus128, vpkuhum128, vpkuhus128, vpkuwum128,
vpkuwus128, vmsum3fp128, vmsum4fp128, vrlimi128, vpermwi128 — 30+
opcodes), plus VX128_R compare dot-forms.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
MechaCat02
2026-05-02 11:22:20 +02:00
parent 2be25bdd41
commit 7609dcd406
5 changed files with 207 additions and 157 deletions

View File

@@ -3414,3 +3414,38 @@ has the wrong extraction. The disassembler was written independently and got the
degenerate cases.
IDs PPCBUG-655 through PPCBUG-679 are unallocated — no further bugs found in Phase C3.
---
## Phase C4 — Post-merge audit corrections (2026-05-02)
### PPCBUG-700 — VMX128 register accessors disagreed with canary's bitfield layout (HIGH)
- **Severity**: HIGH (silent mis-decoding of any VMX128 instruction with a register >= 32)
- **Status**: applied
- **Locations**: `decoder.rs:138-160` (`va128`/`vb128`/`vd128`), `decoder.rs:80` (`vx128r_rc_bit`)
- **Discovery**: independent reviewer of the P3 phase merge, comparing our rust accessors
against canary's `FormatVX128`/`VX128_2`/`VX128_4`/`VX128_5`/`VX128_R` bitfield struct
in `xenia-canary/src/xenia/cpu/ppc/ppc_decode_data.h:484-663`.
- **Symptom**: this entry contradicts the audit's own line 2958 ("confirmed-clean")
assessment. The previous audit miscounted bit-field offsets — under x86_64 LSB-first
C++ bitfield packing, the canary fields land at:
- `VA128 = VA128l(5) | VA128h(1)<<5 | VA128H(1)<<6` = PPC[11-15] | PPC[26]<<5 | PPC[21]<<6 (3 fields, 7 bits)
- `VB128 = VB128l(5) | VB128h(2)<<5` = PPC[16-20] | PPC[30-31]<<5 (2 fields, 7 bits)
- `VD128 = VD128l(5) | VD128h(2)<<5` = PPC[6-10] | PPC[28-29]<<5 (2 fields, 7 bits)
- `Rc` (VX128_R only) = PPC[25] (host bit 6) — not PPC[27] as PPCBUG-422/562 prescribed.
Rust code instead used va128: PPC[11-15] | PPC[29]<<5 (one bit, wrong position); vb128:
PPC[16-20] | PPC[28]<<5 | PPC[30]<<6 (wrong positions); vd128: PPC[6-10] | PPC[21]<<5 |
PPC[22]<<6 (wrong positions); vx128r_rc_bit at PPC[27].
- **Why it lurked**: the buggy convention was internally consistent with hand-crafted
test fixtures (which set bit 29 / 21 / 22 to encode "high" registers, matching the
buggy accessor). Real Xbox 360 game code follows canary's convention, so any production
encoding with VR >= 32 was silently mis-decoded — but no unit test exercised that path.
- **Fix**: rewrite the four accessors to canary's bit positions; rewrite the
`vmx128_test_word` helper and unit tests; re-encode the goldens for vmaddfp128/
vmaddcfp128/vnmsubfp128/vperm128/vsrw128/vpermwi128/vrlimi128. Drop the speculative
`key4_dt` dot-form dispatch in `decode_op6` (canary has no separate dot-form opcodes
for VX128_R compute ops; Rc is a runtime modifier). Update `encode_vpkd3d128` test
helper for canary's VD128h placement.
- **Cross-reference**: invalidates the audit's confirmed-clean note at line 2958.
Subsumes the partial fix-shape proposed in PPCBUG-422 (Rc-bit position).