From 49378827e88130025704e1bdab2d8fecd42b174a Mon Sep 17 00:00:00 2001 From: SDD Date: Fri, 16 Oct 2026 10:59:40 +0300 Subject: [PATCH] fix(tms34010): drop addr32 &0xFFFFF, fix ADDI imm; rebuild .sla, repair program, validate 31691 sites --- docs/ghidra-tms34010-calla-bug.md | 242 ++++++++++++++++++++++++++++++ 1 file changed, 242 insertions(+) diff --git a/docs/ghidra-tms34010-calla-bug.md b/docs/ghidra-tms34010-calla-bug.md index 2f40490..760f56a 100644 --- a/docs/ghidra-tms34010-calla-bug.md +++ b/docs/ghidra-tms34010-calla-bug.md @@ -356,3 +356,245 @@ MCP calls used (all with `session_id` from §4.1): `listing_code_unit_at`, `listing_disassemble_function`, `listing_disassemble_range`, `pcode_op_at`, `function_at`, `function_callers`, `function_callees`, `function_report`, `reference_to`, `reference_from`, `memory_blocks_list`. + +--- + +# FIX + VALIDATION + +**Status:** DONE. The SLEIGH defect is fixed in both places, the `.sla` is rebuilt and +installed, the existing program is repaired in place, and every affected instruction in the +program is validated against an independent reference disassembler with **0 mismatches**. + +Environment used: Ghidra `12.1.2` at `/opt/ghidra`, language `tms34010:LE:32:default`, +program `vt1200_v22.bin` (image base `rom:07e40000`). + +## 1. The change + +Two functional sites in the vendored SLEIGH module +`tools/ghidra-tms34010/tms34010.sinc` (CRLF preserved): + +```diff +@@ line 73 (shared by CALLA and JA^cc) @@ +-addr32: addy is imm32 [addy = (imm32 / 8) & 0xfffff;] {export *:4 addy;} ++addr32: addy is imm32 [addy = imm32 / 8;] {export *:4 addy;} + +@@ line 185 (ADDI) @@ +-:ADDI link,Rd is bits_5_15=0b00001011001 & Rd; imm32 [link=(imm32 / 8) & 0xfffff;] ++:ADDI imm32,Rd is bits_5_15=0b00001011001 & Rd; imm32 + { +- Rd = link; ++ Rd = imm32; + } +``` + +* `addr32` (line 73) is used by both `CALLA addr32` (line 294) and `JA^cc addr32` + (line 560), so one edit fixes both instructions. `imm32 / 8` is the correct + bit→byte conversion; the `& 0xfffff` is gone. +* The naive ADDI edit `[link=imm32;]` is **rejected by the SLEIGH compiler** + (`Error: operand link has an issue: for table "instruction" constructor from + tms34010.sinc:185`). The operand was therefore renamed to `imm32`, mirroring the + already-correct `MOVI imm32,Rd` form (line 942). Semantics are unchanged + (`Rd = imm32`, previously `Rd = link` with `link` the same value); only the displayed + operand changes from the truncated `(imm32/8)&0xfffff` to the raw immediate. +* The commented-out `MOVI link` form at line 940 was left untouched (inert; not compiled). + +## 2. Rebuild and install + +`tools/install-ghidra-plugin.sh` was used unchanged (sudo available): + +1. It copies `tools/ghidra-tms34010/tms34010.{slaspec,sinc,ldefs,pspec,cspec}` into + `/opt/ghidra/Ghidra/Processors/TMS34010/data/languages/` and touches `Module.manifest`. +2. It runs `/opt/ghidra/support/sleigh -a `, recompiling `tms34010.sla`. + +Verification that the installed artefact is the one built: + +| file | md5 | note | +|---|---|---| +| `tools/ghidra-tms34010/tms34010.sinc` | `316d065a44e8fb4ddfed1455f5b0da9c` | source of truth | +| `/opt/ghidra/…/TMS34010/data/languages/tms34010.sinc` | `316d065a44e8fb4ddfed1455f5b0da9c` | installed copy, identical | +| `/opt/ghidra/…/TMS34010/data/languages/tms34010.sla` | `e0e1a6792f37b669539bda654a7dac7b` | recompiled `10:52:17` | + +A throw-away build of the same sources into `/tmp/opencode/sleightest/` produced the +byte-identical `.sla` (`e0e1a679…`), confirming the module compiles from the edited source. +The pre-fix file hashes were `tms34010.sinc = 7b11c07300fb636432f247a00be5701c`, +`tms34010.sla = 31633dd3985ee92e5fa5d826d2bbc393`. + +The running Ghidra JVM caches the language, so the MCP server was restarted +(the harness reconnected it via `POST /api/experimental/mcp/ghidra/connect`). A fresh JVM +then decoded the three forms correctly **before any program edit**: + +``` +0x07fbd020 -> CALLA 0x07fbc018 (was 0x000bc018) +0x07e40146 -> JAEQ 0x07e40494 (was 0x00040494) +0x07fbd128 -> ADDI 0x800000,A9 (was 0x0,A9) +``` + +## 3. Repairing the already-analysed program — route chosen + +**Route: re-disassemble the affected instructions in the existing program, in place** +(not a fresh re-import). + +Why: Ghidra re-resolves an instruction's *prototype* against the loaded language on open, +so text and p-code already update automatically once the fixed `.sla` is installed — but the +**stored references** (`REF_CALL`, `REF_JUMP`), the stored **function bodies**, and the +resulting **function table** are persisted in the program database and were computed with the +buggy module. In-place re-disassembly regenerates exactly those, while preserving every +annotation. The existing analysis holds 25 functions (some named), 9 memory-block comments +and the `FixPtrConstants.java` data cross-references; a fresh re-import would have discarded +all of them and produced a different function set. In-place was therefore both cheaper and +lossless. + +Procedure (single Ghidra transaction each, via `ghidra_eval`): + +1. Collect every `CALLA`/`JA*`/`ADDI` instruction (9 934 sites at that moment), then for each: + `listing.clearCodeUnits(addr, addr, true)` followed by + `new DisassembleCommand(addr, null, true).applyTo(program, monitor)`. This re-decodes the + instruction with the fixed language and emits the corrected reference (following the call + target discovers the previously unreachable code). +2. Create functions at every `UNCONDITIONAL_CALL` destination inside the ROM block that had no + function: `CreateFunctionCmd(addr)` → 1 563 functions created (13 targets already had one, + 4 were mid-function). Function count 25 → 1 588. +3. `program_save` to persist. + +Not re-run: a wholesale auto-analysis. The surgical re-disassembly + `CreateFunctionCmd` +already restores the call structure; a full re-analysis would re-derive a large amount of +unrelated state and risk perturbing the preserved annotations. + +Effect on the program: instructions 101 052 → 225 794 (code now reaches the real call targets, +750 642 bytes = 40.9 % of the ROM; the independent reference disassembles 308 512, so this is +*more* conservative, not over-disassembled). References into the truncated `0x000bc018` +alone went from 168 to 0; references into the true `0x07fbc018` went from 0 to 265. + +**Annotations preserved:** all 25 original functions still present (including +`video_regs_init`, `led_control_1/2`, `io5600000_screen_init`, `nvr_user_settings_validate`, +`io_settings_copy_expand/linear/restore`, `keyboard_mouse_irq_handler`, `io5800_5900_init`, +`io5800_status_read`) and all 9 memory-block comments intact. + +## 4. Exhaustive validation + +Every affected instruction in the repaired program was checked twice: + +* **self-check** — the operand Ghidra renders must equal the value computed directly from the + instruction's own bytes, independently of Ghidra's SLEIGH: `imm >> 3` for `CALLA`/`JA^cc`, + the raw `imm` for `ADDI`/`MOVI`. +* **reference check** — `tools/smart34010/dis34 -l -b 3f200000 -o vt1200_v22.bin` (full linear + sweep, 697 412 lines) decoded at the same address must agree on the raw immediate/target. + Ghidra address `G` ↔ dis34 address `G*8`. + +| instruction | sites checked | self-match | dis34-match | mismatches | +|---|---|---|---|---| +| `CALLA` | 15 955 | 15 955 | 15 955 | **0** | +| `JA^cc` (`JA`/`JAEQ`/`JAHI`/`JANE`) | 56 | 56 | 56 | **0** | +| `ADDI` | 6 742 | 6 742 | 6 742 | **0** | +| `MOVI` | 8 938 | 8 938 | 8 938 | **0** | +| **total** | **31 691** | **31 691** | **31 691** | **0** | + +The 8 938 `MOVI` include 12 whose dis34 line carries a trailing symbolic register name +(`… c000 HESYNC`, `DISPLAY`, `CONVSP`, `DPYCTL`, `CONTROL`, `HSTCTLH`, `INTENB`, +`INTPEND`); each was checked by hand and the three hex words agree with Ghidra. At no site did +Ghidra and dis34 disagree on the instruction boundary (0 boundary diffs), and every Ghidra +site had a matching reference line (0 missing). + +Distinct targets: the repaired program has **1 577 distinct `CALLA` targets** (up from 1 767 in +the standalone reference; every site present was cross-checked). The set difference is +analysis-coverage, not a value error — 317 Ghidra targets do not appear in the reference's +*analyze-mode* output (which emits `.word` for code it does not classify) but every one of them +was confirmed against the reference's linear sweep; 507 reference targets belong to call sites +Ghidra's analysis still has not disassembled. + +Known concrete sites, before → after: + +| Ghidra address | bytes | before | after | +|---|---|---|---| +| `0x07FBD020` | `5f0dc000de3f` | `CALLA 0x000bc018` | `CALLA 0x07fbc018` | +| `0x07FBD128` | `290b00008000` | `ADDI 0x0,A9` | `ADDI 0x800000,A9` | +| `0x07E40146` | `80caa024203f` | `JAEQ 0x00040494` | `JAEQ 0x07e40494` | +| `0x07E404FC` | `5f0d90cadd3f` | `CALLA 0x000bb952` | `CALLA 0x07fbb952` | + +## 5. Call-graph evidence (empty before → populated now) + +Measured with the MCP tools on the program itself. + +`FUN_rom_07fbd000` — `function_callees`: + +``` +BEFORE { count: 0, items: [] } +AFTER { count: 2, items: [ + FUN_rom_07fbbf96 (rom:07fbbf96, body 07fbbf96..07fbc015), + FUN_rom_07fbc018 (rom:07fbc018, body 07fbc018..07fbc073) ] } +``` + +`FUN_rom_07fbbf96` (the helper) — `function_callers`: + +``` +BEFORE { count: 0, items: [] } +AFTER { count: 6, items: [ FUN_rom_07e6a9a8, FUN_rom_07e6ad48, + FUN_rom_07fbc262, FUN_rom_07fbcabe, FUN_rom_07fbce0e, + FUN_rom_07fbd000 ] } +``` + +`pcode_op_at 0x07fbd020`: + +``` +BEFORE CALL (rom, 0xbc018, 4) +AFTER CALL (rom, 0x7fbc018, 4) +``` + +(The BEFORE values are from the live program under the buggy module. They were re-confirmed +independently by opening the pre-fix project export in the fixed JVM: `callees(0x07fbd000)=0`, +`callers(0x07fbbf96)=0`.) + +**Site that still does not resolve, investigated:** `FUN_rom_07fbd000` still reports 0 +*callers*. This is correct, not residual damage — there is no `CALL`/`JUMP` reference to +`0x07fbd000` anywhere in the image, in either Ghidra or the reference disassembler. The +function is entered by **fall-through** from the tail of `FUN_rom_07fbce0e` +(`0x07fbcf…` ends with `MOVI 0x22,A9` at `0x07fbcffc`; `0x07fbd000` is the next instruction), +so a static direct-caller list is legitimately empty. Its *callee* list — the part the bug +destroyed — is now populated as shown above. + +## 6. Over-correction check (addresses that were already right) + +The fix touches only `addr32` and the `ADDI` immediate. Ghidra's absolute `MOVI` operands were +already correct and must still be. All 8 938 `MOVI` instructions were re-validated (see §4): +**8 938/8 938 match, 0 broken**. Examples compared against `smart34010`: + +| Ghidra address | Ghidra | `smart34010` (raw words) | +|---|---|---| +| `0x07E400B0` | `MOVI 0xff,A0` | `movi 0xff, a0 ; 09c0 00ff` | +| `0x07FC17F6` | `MOVI 0xc0000120,A9` | `movi … ; 09e9 0120 c000 INTPEND` | +| `0x07EAF146` | `MOVI 0xc0000080,A3` | `movi … ; 09e3 0080 c000 DPYCTL` | +| `0x07E40078` | `MOVI 0x55555555,A0` | `09e0 5555 5555` | + +The 4-byte `MOVI`/`ADDI` short forms (3 317 + 3 781 sites) were included in the check and are +unchanged. No previously-correct absolute operand regressed. + +## 7. Files changed + +| file | change | +|---|---| +| `tools/ghidra-tms34010/tms34010.sinc` | lines 73 and 185 (fix) | +| `/opt/ghidra/Ghidra/Processors/TMS34010/data/languages/tms34010.sinc` | installed copy of the above | +| `/opt/ghidra/Ghidra/Processors/TMS34010/data/languages/tms34010.sla` | recompiled | +| `ghidra/vt1200.rep/` (`vt1200_v22.bin`), gitignored | program repaired in place + saved | +| `docs/ghidra-tms34010-calla-bug.md` | this section | + +The `.cspec/.pspec/.ldefs/.slaspec` were re-copied by the installer with unchanged content +(timestamps bumped only). Nothing under `harness/` was touched. + +## 8. Revert + +* **SLEIGH / install**: restore `tools/ghidra-tms34010/tms34010.sinc` from + `/tmp/opencode/backup/vendored-tms34010.sinc.orig` (md5 `7b11c073…`; the pre-fix `.sla` + backup is `/tmp/opencode/backup/installed-tms34010.sla.orig`, md5 `31633dd3…`), then re-run + `tools/install-ghidra-plugin.sh`. (`git checkout -- tools/ghidra-tms34010/tms34010.sinc` + also works while the pre-fix blob is reachable.) +* **Program**: the untouched pre-fix project was exported before any edit to + `/tmp/opencode/backup/vt1200_v22_pre-fix.gzf/` (a full `vt1200.gpr` + `vt1200.rep` copy). + Copy it over `ghidra/` to restore the original analysis. The pre-fix instruction/reference + dump is `/tmp/opencode/before_dump.json`. + +## 9. Provenance note + +The `tms34010.sinc` fix was swept into HEAD by the concurrently-running worker's broad +`git add -A` commit `5000b8bc07a8` before this task committed; HEAD therefore already contains +lines 73/185 as shown in §1. This task's own commit adds only §1–§8 of this document.