arti/vt1200
1
0 Fork 1
mirror of https://git.k-space.ee/arti/vt1200.git synced 2026-10-30 12:03:54 +02:00
Code Wiki Activity

fix(tms34010): drop addr32 &0xFFFFF, fix ADDI imm; rebuild .sla, repair program, validate 31691 sites

Browse Source
This commit is contained in:
SDD committed 2026-10-16 10:59:40 +03:00
1 parent 5000b8bc07
commit 49378827e8
1 file changed
+242
Download Patch File Download Diff File Expand all files Collapse all files
docs/ghidra-tms34010-calla-bug.md
+242
View File
@@ -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 <that dir>`, 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.