diff --git a/core/rexcode/isa/x86/decoder.odin b/core/rexcode/isa/x86/decoder.odin index 3ed24dc02..e883944ab 100644 --- a/core/rexcode/isa/x86/decoder.odin +++ b/core/rexcode/isa/x86/decoder.odin @@ -437,49 +437,37 @@ decode_opcode :: proc(state: ^Decoder_State) -> (entry: ^Decode_Entry, vex_entry idx, mand_66 = resolve_66(DECODE_INDEX_ESC_0F3A, opcode, prefix, state) } - // If not found, try +r encoding (opcode with register in low 3 bits) - if idx.count == 0 && esc == .NONE { - base_opcode := opcode & 0xF8 // Mask off low 3 bits - idx = didx(DECODE_INDEX_LEGACY, prefix, base_opcode) + /* NOTHING AT THIS OPCODE -- RETRY AT THE +r BASE, where the register rides in the opcode's low + three bits (`50+rd`, `90+rd`, `B8+rd`, `0F C8+rd`, ...). The table stores ONE entry for the + base and the shared +r handler below reads the register out of `opcode & 7`. - // Check if this is actually an Op_R encoding - if idx.count == 0 { - return nil, nil, .INVALID_OPCODE + THIS IS NOT LEGACY-ONLY. Gating the retry on `esc == .NONE` is what made BSWAP undecodable: + `0F C8+rd` is the one +r instruction that lives behind an escape byte, so every `bswap` except + the one whose register happens to be 0 (`bswap rax` = `0F C8`, which lands on the entry + exactly) came back INVALID_OPCODE. Emission was always correct -- only reading it back failed. + + The retry deliberately does NOT re-implement the +r decode; it only re-runs the LOOKUP and + falls through to the one handler. There used to be a second copy of that handler here, and it + had drifted from the original in three ways, each its own silent wrong answer: it tested only + the FIRST entry for OP_R (so every `xchg rAX, r` was rejected, 0x90's run having NOP sorted + ahead of XCHG), it passed `prefix` where the legacy row wants 0 (so `66 53`, `push bx`, was + rejected), and it read the operand size off `ops[0]` (which for XCHG is the implicit + accumulator, not the sized operand). All three had already been found and fixed once, in the + handler below -- none of the fixes reached the copy, because nothing made them one thing. */ + retried_plus_r := false + if idx.count == 0 { + base_opcode := opcode & 0xF8 // mask off the register bits + switch esc { + case .NONE: + idx = didx(DECODE_INDEX_LEGACY, 0, base_opcode) // prefix 0: see the primary lookup above + case ._0F: + idx, mand_66 = resolve_66(DECODE_INDEX_ESC_0F, base_opcode, prefix, state) + case ._0F38: + idx, mand_66 = resolve_66(DECODE_INDEX_ESC_0F38, base_opcode, prefix, state) + case ._0F3A: + idx, mand_66 = resolve_66(DECODE_INDEX_ESC_0F3A, base_opcode, prefix, state) } - if first := &LEGACY_DECODE_ENTRIES[idx.start]; first.enc[0] == .OP_R { - // Store the register number for later operand decoding - state.opcode_reg = opcode & 0x07 - - // For Op_R with multiple entries (e.g., PUSH/POP with R64 and R16), - // select based on prefix_66 and default_64 flag - if idx.count > 1 { - for i in 0.. (entry: ^Decode_Entry, vex_entry for i in 0.. (entry: ^Decode_Entry, vex_entry return first_entry, nil, .NONE } + /* The base-opcode retry above is ONLY meaningful for a +r form, and we now know this is not one. + Falling through with a masked opcode would decode some unrelated neighbour — 0x0E would come + back as the 0x08 entry (OR) — which is a wrong instruction reported confidently, strictly worse + than saying the byte is not one we know. */ + if retried_plus_r { + return nil, nil, .INVALID_OPCODE + } + // Multi-entry opcode: disambiguate by the ModR/M byte (fixed byte / ST(i) // range / /digit) and operand-size state. if idx.count > 1 { diff --git a/core/rexcode/isa/x86/tests/test.odin b/core/rexcode/isa/x86/tests/test.odin index 6131459a0..7a2a702ac 100644 --- a/core/rexcode/isa/x86/tests/test.odin +++ b/core/rexcode/isa/x86/tests/test.odin @@ -2943,6 +2943,24 @@ run_decode_only_tests :: proc() { {name = "decode: sse", test_type = .Decode_Only, input_code = {0x0F, 0x57, 0xC0, 0x0F, 0x28, 0xC1, 0x0F, 0x58, 0xC2}}, {name = "decode: vex", test_type = .Decode_Only, input_code = {0xC5, 0xF8, 0x57, 0xC0, 0xC5, 0xF8, 0x28, 0xC1}}, {name = "decode: call/jmp", test_type = .Decode_Only, input_code = {0xE8, 0x00, 0x00, 0x00, 0x00, 0xEB, 0xF9}}, + + /* +r FORMS, where the register rides in the opcode's low three bits. The decoder finds these + by retrying the lookup at `opcode & 0xF8`, and that retry had three holes, each of which + made a perfectly ordinary instruction undecodable while emission stayed correct. + + BSWAP is `0F C8+rd` -- the ONE +r instruction behind an escape byte. The retry used to be + gated on there being no escape byte, so only `bswap eax`/`bswap rax` (register 0, landing on + the table entry exactly) decoded and the other seven registers were INVALID_OPCODE. */ + {name = "decode: bswap +r (0F C8+rd)", test_type = .Decode_Only, input_code = {0x0F, 0xC8, 0x0F, 0xC9, 0x0F, 0xCC, 0x0F, 0xCF}}, + {name = "decode: bswap +r rex.w", test_type = .Decode_Only, input_code = {0x48, 0x0F, 0xC8, 0x48, 0x0F, 0xCF}}, + {name = "decode: bswap +r rex.b", test_type = .Decode_Only, input_code = {0x41, 0x0F, 0xC8, 0x49, 0x0F, 0xCF}}, + // XCHG rAX,r is `90+rd`, and 0x90's run has NOP sorted ahead of it -- the retry used to test + // only the FIRST entry for a +r form, so every `xchg rAX, r` was rejected. + {name = "decode: xchg rAX,r (90+rd)", test_type = .Decode_Only, input_code = {0x90, 0x91, 0x97, 0x48, 0x91, 0x66, 0x91}}, + // The legacy +r families under an operand-size prefix: the retry passed the 0x66 row, where + // the legacy table wants row 0 (for legacy opcodes 0x66 is operand size, not part of identity). + {name = "decode: +r with 66 prefix", test_type = .Decode_Only, input_code = {0x66, 0x53, 0x66, 0x5B, 0x66, 0xB9, 0x00, 0x00}}, + {name = "decode: push/pop/mov +r", test_type = .Decode_Only, input_code = {0x53, 0x5B, 0x41, 0x54, 0x41, 0x5C, 0xB9, 0x00, 0x00, 0x00, 0x00}}, } for t in tests { run_test(t) } } @@ -3582,4 +3600,13 @@ main :: proc() { run_benchmarks() print_summary() + + /* FAIL THE PROCESS WHEN TESTS FAILED. Without this the binary exits 0 no matter what, and an exit + code is the only thing a runner can rely on -- `build.lua` looked for the words "N failed" in the + output instead, which the summary here prints as "N FAILED", so it never matched. Two independent + holes, both open, meant the x86 suite could fail every case it has and the build still reported + PASS. Found when five deliberately-broken decode cases came back green. */ + if g_stats.failed > 0 { + os.exit(1) + } }