From 3e3eec090df51b2df72bc35a6b1c878d1cc9999d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fl=C4=81vius?= Date: Sun, 16 Aug 2026 12:05:42 -0400 Subject: [PATCH] x86: +r decoding was legacy-only, so BSWAP could not be read back MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `bswap` is `0F C8+rd` — the register rides in the opcode's low three bits, like `push`/`pop`/`xchg`/`mov`, but behind an escape byte. The decoder's retry at the +r base opcode was gated on `esc == .NONE`, so only the register-0 forms (`0F C8` = bswap eax, `48 0F C8` = bswap rax) landed on a table entry directly; the other seven registers came back INVALID_OPCODE. Emission was always correct — every `bswap` this compiler has ever produced runs — but a disassembly containing one stopped dead, which is how it surfaced: three functions in sigil's JIT corpus disassembled to a header with no instructions under it. The table was never the problem. BSWAP is in `tablegen/encoding_table.odin` and in the generated decode table, exactly where it should be. Fixing the gate meant looking at why the retry existed twice. There is a correct +r handler further down, carrying comments that describe two bugs already found and fixed in it — "scan ALL entries, not just the first", "scan for the sized register rather than assuming op0" — and the copy inside the retry had received neither, plus a third of its own. So the retry now re-runs only the LOOKUP and falls through to the single handler, which fixes two more instruction families that were equally undecodable: - `xchg rAX, r` (90+rd): 0x90's run sorts NOP ahead of XCHG, and the copy tested only the first entry for a +r form, so every `xchg rAX, r` was rejected. - `push bx` / `pop bx` / `mov cx, imm` (any +r under 0x66): the copy passed `prefix` where the legacy row wants 0, since for legacy opcodes 0x66 is operand size rather than part of the opcode's identity. Two supporting changes. The shared handler's default answer is now the first +r entry rather than `idx.start`, so a fall-through at 0x90 cannot answer NOP for an `xchg`. And a base-opcode retry that does not land on a +r form is now an error rather than a fall-through: decoding 0x0E as the 0x08 entry (OR) would be a wrong instruction reported confidently, which is worse than an unknown byte. Verified three ways. Every opcode in every escape map was decoded before and after: 154 combinations changed, every one of them INVALID -> valid, and nothing that already decoded decodes differently. All 24 BSWAP encodings and all 96 legacy +r encodings were diffed against llvm-mc's disassembly and agree operand-for-operand. Six new decode-only cases pin the three families; five of them fail without this change. (Two remaining differences from llvm-mc are deliberate and documented here: implicit accumulators are left unmaterialized so a short form re-encodes to the short form, and `48 90` reads as `xchg rax` rather than `nop`.) Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Riok9vMpkLmo78wsVKJHhz --- core/rexcode/isa/x86/decoder.odin | 82 ++++++++++++++-------------- core/rexcode/isa/x86/tests/test.odin | 27 +++++++++ 2 files changed, 68 insertions(+), 41 deletions(-) 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) + } }