mirrors/Odin
1
0
Fork 0
mirror of https://github.com/odin-lang/Odin.git synced 2026-08-27 15:31:33 +00:00
Code Issues Packages Projects Releases Wiki Activity

x86: +r decoding was legacy-only, so BSWAP could not be read back

Browse Source 
`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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Riok9vMpkLmo78wsVKJHhz
This commit is contained in:
Flāvius
2026-08-16 12:05:42 -04:00
parent 2e8d3b9e10
commit 3e3eec090d
2 changed files with 68 additions and 41 deletions
Download Patch File Download Diff File Expand all files Collapse all files

82
core/rexcode/isa/x86/decoder.odin
View File

@@ -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..<int(idx.count) {
e := &LEGACY_DECODE_ENTRIES[int(idx.start) + i]
op0 := e.ops[0]
if state.prefix_66 {
if op0 == .R16 {
return e, nil, .NONE
}
} else {
is_64 := state.mode == ._64 && (e.flags.default_64 || (state.rex & 0x08 != 0))
if is_64 && op0 == .R64 {
return e, nil, .NONE
}
if !is_64 && op0 == .R32 {
return e, nil, .NONE
}
// i386: default_64 entries are the "default operand size" form,
// which is 32-bit; bytes encode the same as long-mode R64+default_64.
if state.mode == ._32 && op0 == .R64 && e.flags.default_64 {
return e, nil, .NONE
}
}
}
}
return first, nil, .NONE
}
return nil, nil, .INVALID_OPCODE
retried_plus_r = true
}
if idx.count == 0 {
@@ -497,6 +485,10 @@ decode_opcode :: proc(state: ^Decoder_State) -> (entry: ^Decode_Entry, vex_entry
for i in 0..<int(idx.count) {
if entry_has_opr(&LEGACY_DECODE_ENTRIES[int(idx.start) + i]) {
uses_op_r = true
// The DEFAULT answer for a +r opcode is the first +r entry, not the first entry
// outright: 0x90 sorts NOP ahead of XCHG, so should the size selection below ever
// fall through for an `xchg rAX, r`, `idx.start` would answer NOP.
first_entry = &LEGACY_DECODE_ENTRIES[int(idx.start) + i]
break
}
}
@@ -548,6 +540,14 @@ decode_opcode :: proc(state: ^Decoder_State) -> (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 {

27
core/rexcode/isa/x86/tests/test.odin
View File

@@ -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)
}
}