mirror of
https://github.com/odin-lang/Odin.git
synced 2026-09-02 02:03:35 +00:00
x86: a baked enc_hint must name a form of its own mnemonic
mnemonic_builders.odin bakes `global_index + 1` into Instruction.enc_hint so the encoder can skip the O(forms) match scan. The index is GLOBAL, so any edit to the encoding table that inserts or removes a form shifts every index after it and leaves the builders naming another instruction's encoding -- and nothing noticed: the encoder took the form, emitted its bytes and returned success. It has happened twice.6e17e7a2dleft 2130 of 3671 builders wrong;36af73834regenerated both halves and cleared it;baae2636b(adding `in`/`out`) re-broke 37;9ae9a9bf9shifted an early mnemonic and broke 3393 of 3802 -- including CALL, whose r/m64 builder then encoded 0F 8A (JPE) instead of FF /2, turning every indirect call into a conditional jump. The symptom was a segfault in a JIT'd program, arbitrarily far from the cause. Regenerated: 3820 of 3820 builders now correct. Three guards so it cannot return silently: - the encoder checks, under ODIN_DEBUG, that the hinted form lies inside its own mnemonic's ENCODE_RUNS entry. Release keeps the byte-for-byte fast path; this is a regeneration-time mistake and only has to be caught once by anyone running tests. - the generator stamps BUILDER_TABLE_FINGERPRINT, an FNV-1a hash of every ENCODE_RUNS (start, count), into the file it emits. - run_builder_generation_test recomputes it from the loaded tables and fails with the command to regenerate. Known-failing at this commit: four SAL/SHL cases, a separate defect in the alias table, fixed in the commit that follows. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Riok9vMpkLmo78wsVKJHhz
This commit is contained in:
@@ -159,6 +159,27 @@ encode :: proc(
|
||||
if mode == ._64 && inst.enc_hint != ENC_HINT_NONE && int(inst.enc_hint) <= len(ENCODE_FORMS) {
|
||||
form_index = int(inst.enc_hint) - 1
|
||||
matched_enc = &ENCODE_FORMS[form_index]
|
||||
/* THE HINT MUST NAME A FORM OF THIS MNEMONIC. The bounds check above is not enough: the
|
||||
index is GLOBAL and baked into `mnemonic_builders.odin` at generation time, so any
|
||||
later edit to the encoding table that inserts or removes a form shifts every index
|
||||
after it and leaves the builders naming someone else's instruction. Nothing here
|
||||
noticed — the encoder took the form, emitted its bytes, and returned success.
|
||||
|
||||
It has happened twice. `6e17e7a2d` left 2130 of 3671 builders pointing at the wrong
|
||||
form; `36af73834` regenerated both halves and cleared it; `baae2636b` (adding `in`
|
||||
and `out`) re-broke 37; `9ae9a9bf9` shifted an early mnemonic and broke 3393 of 3802,
|
||||
including CALL — whose r/m64 builder then encoded `0F 8A` (JPE) instead of `FF /2`,
|
||||
turning every indirect call into a conditional jump. The symptom was a segfault in a
|
||||
JIT'd program, arbitrarily far from the cause.
|
||||
|
||||
Debug-only, so the release fast path is byte-for-byte what it was: this is a
|
||||
REGENERATION-TIME mistake, and it only has to be caught once by anyone running tests. */
|
||||
when ODIN_DEBUG {
|
||||
run := ENCODE_RUNS[inst.mnemonic]
|
||||
if form_index < int(run.start) || form_index >= int(run.start) + int(run.count) {
|
||||
panic("rexcode/x86: a baked enc_hint names a form outside its own mnemonic's run — mnemonic_builders.odin is stale relative to tables/x86.encode_*.bin. Regenerate it: odin run core/rexcode/isa/x86/tools/gen_mnemonic_builders.odin -file")
|
||||
}
|
||||
}
|
||||
} else {
|
||||
// Resolve the form on the matcher path (memoizing cache + scan) in a
|
||||
// separate, non-inlined proc so this hot loop stays lean for the hint
|
||||
|
||||
File diff suppressed because it is too large
Load Diff
@@ -3204,7 +3204,44 @@ tb_check :: proc(name: string, typed, generic: x86.Instruction) {
|
||||
}
|
||||
}
|
||||
|
||||
/* THE BUILDERS AND THE TABLES MUST BE THE SAME GENERATION.
|
||||
|
||||
`run_typed_builder_tests` below compares a HAND-LISTED sample of typed builders against the matcher
|
||||
path, which is the right shape of check and covers about forty of 3820 builders. CALL r/m64 is not
|
||||
among them — so when `9ae9a9bf9` shifted the encode table without regenerating the builders, its
|
||||
baked hint 495 came to name JPE's `0F 8A` and this suite stayed green while every indirect call in
|
||||
every JIT compiled to a conditional jump.
|
||||
|
||||
This closes it for ALL of them at once. A baked hint is valid exactly when it lands inside its own
|
||||
mnemonic's run, so the whole invariant is a function of ENCODE_RUNS; the generator records a
|
||||
fingerprint of the runs it generated against, and this compares it to the live table. Regenerating
|
||||
the builders is what makes them agree again — one command, printed here so nobody has to find it. */
|
||||
run_builder_generation_test :: proc() {
|
||||
fingerprint := u64(0xcbf2_9ce4_8422_2325)
|
||||
for m in x86.Mnemonic {
|
||||
r := x86.ENCODE_RUNS[m]
|
||||
for part in ([2]u64{u64(r.start), u64(r.count)}) {
|
||||
for shift: uint = 0; shift < 64; shift += 8 {
|
||||
fingerprint = (fingerprint ~ ((part >> shift) & 0xff)) * 0x0000_0100_0000_01b3
|
||||
}
|
||||
}
|
||||
}
|
||||
if fingerprint == x86.BUILDER_TABLE_FINGERPRINT {
|
||||
g_stats.passed += 1
|
||||
g_stats.cases_validated += 1
|
||||
return
|
||||
}
|
||||
g_stats.failed += 1
|
||||
fmt.printf(" %sFAIL%s mnemonic_builders.odin is STALE: it bakes form indices for an ENCODE_RUNS\n", RED, RESET)
|
||||
fmt.printf(" layout of %016x, but the loaded tables are %016x. Every builder past the\n",
|
||||
x86.BUILDER_TABLE_FINGERPRINT, fingerprint)
|
||||
fmt.printf(" first inserted form now names another mnemonic's encoding. Regenerate:\n")
|
||||
fmt.printf(" odin run core/rexcode/isa/x86/tools/gen_mnemonic_builders.odin -file\n")
|
||||
}
|
||||
|
||||
run_typed_builder_tests :: proc() {
|
||||
run_builder_generation_test()
|
||||
|
||||
md8 := x86.mem_base_disp(x86.RBP, -16)
|
||||
md32 := x86.mem_base_disp(x86.RCX, 100000)
|
||||
mbi := x86.mem_base_index_disp(x86.R8, x86.RDX, 4, 32)
|
||||
|
||||
@@ -214,6 +214,31 @@ main :: proc() {
|
||||
|
||||
}
|
||||
|
||||
/* THE TABLE THIS FILE WAS GENERATED AGAINST, as a fingerprint over ENCODE_RUNS.
|
||||
|
||||
Every builder below bakes an ABSOLUTE index into ENCODE_FORMS. Whether that index is still
|
||||
right depends on exactly one thing — the (start, count) run of its mnemonic — so hashing the
|
||||
runs captures the whole invariant: if this fingerprint still matches the live table, every
|
||||
baked hint is still inside its own mnemonic's forms; if it does not, this file is stale and
|
||||
every builder after the first inserted form names someone else's instruction.
|
||||
|
||||
It has gone stale twice, silently, because nothing compared the two halves: `6e17e7a2d` left
|
||||
2130 of 3671 builders pointing at the wrong form, and after `36af73834` regenerated both,
|
||||
`baae2636b` and `9ae9a9bf9` re-broke 37 and then 3393 of 3802 — the latter including CALL,
|
||||
whose r/m64 builder encoded `0F 8A` (JPE) instead of `FF /2`, turning every indirect call
|
||||
into a conditional jump. `tests/test.odin` checks this constant. */
|
||||
fingerprint := u64(0xcbf2_9ce4_8422_2325)
|
||||
for m in x86.Mnemonic {
|
||||
r := x86.ENCODE_RUNS[m]
|
||||
for part in ([2]u64{u64(r.start), u64(r.count)}) {
|
||||
for shift: uint = 0; shift < 64; shift += 8 {
|
||||
fingerprint = (fingerprint ~ ((part >> shift) & 0xff)) * 0x0000_0100_0000_01b3
|
||||
}
|
||||
}
|
||||
}
|
||||
strings.write_string(&sb, "\n// The ENCODE_RUNS layout these hints were generated against — see the note in the generator.\n")
|
||||
strings.write_string(&sb, fmt.tprintf("BUILDER_TABLE_FINGERPRINT :: u64(0x%016x)\n", fingerprint))
|
||||
|
||||
output := strings.to_string(sb)
|
||||
|
||||
err := os.write_entire_file(#directory + "/../mnemonic_builders.odin", transmute([]u8)strings.concatenate({GEN_ATTRIB, output}))
|
||||
|
||||
Reference in New Issue
Block a user