diff options
Diffstat (limited to '')
| -rw-r--r-- | docs/CODEMODE_UPSTREAM.md | 269 | ||||
| -rw-r--r-- | docs/GRAPH_VIEW.md | 106 | ||||
| -rw-r--r-- | docs/NEXUS_UPSTREAM.md | 385 | ||||
| -rw-r--r-- | docs/PAGING_FINDINGS.md | 14 | ||||
| -rw-r--r-- | docs/PROJECTS.md | 10 | ||||
| -rw-r--r-- | docs/SPLIT_VIEW.md | 8 | ||||
| -rw-r--r-- | docs/TEXTUAL_NOTES.md | 9 |
7 files changed, 508 insertions, 293 deletions
diff --git a/docs/CODEMODE_UPSTREAM.md b/docs/CODEMODE_UPSTREAM.md deleted file mode 100644 index f903599..0000000 --- a/docs/CODEMODE_UPSTREAM.md +++ /dev/null @@ -1,269 +0,0 @@ -# Findings from porting a real client to IDA Code Mode - -Notes for the `ida-codemode` maintainers, gathered while porting **ida-tui** (a -Textual TUI frontend for IDA) from a private idalib worker to -`ida_codemode.client.DatabaseHandle`. - -Everything below is measured, not inferred. Where we worked around something, the -workaround is named so you can judge whether the library should make it -unnecessary. - -**Environment:** ida-codemode 0.3.1, IDA 9.4 (idalib), Linux, single managed -worker backend, quiet box. Target for timings: `targets/echo` unless stated. - -**What the client does**, for scale: it renders a continuous disassembly listing, -pseudocode, a CFG graph view and a hex view, paging over the database as the user -scrolls. It is latency-sensitive in a way an agent-driven MCP client is not — a -keypress must repaint. It issues ~1–8 operations per user action. - ---- - -## 1. `timeout_trace` enables line tracing in every frame — 52x on IDA calls - -**Highest-impact item by a wide margin.** - -`runtime.py` wraps every `execute_python` in `sys.settrace(timeout_trace)` to -enforce the deadline. `timeout_trace` ends with `return timeout_trace`, and -returning a trace function from a `'call'` event asks CPython to trace **every -line of that frame**. So every line of every function the snippet touches pays a -Python-level callback, and the specialising interpreter is disabled throughout. - -Measured inside the worker, same process, same database: - -| | traced (stock) | untraced | native idalib | -|---|---|---|---| -| `ida_bytes.get_flags(ea)` | 5.49 µs | 0.106 µs | 0.119 µs | -| our 200-row listing page | 20.2 ms | 2.0 ms | — | - -Untraced matches a plain idalib process, so the trace hook accounts for -essentially all of it. For us this was the single largest cost in the port — -larger than HTTP, serialisation and IDA itself combined. - -Reproduce inside any `execute_python`: - -```python -import sys, time, ida_bytes -def bench(): - t = time.perf_counter() - for _ in range(20000): ida_bytes.get_flags(0x1000) - return (time.perf_counter() - t) / 20000 * 1e6 -traced = bench() -old = sys.gettrace(); sys.settrace(None) -try: untraced = bench() -finally: sys.settrace(old) -result = {"traced_us": traced, "untraced_us": untraced} -``` - -**Suggested fixes, cheapest first** - -1. `return None` from `timeout_trace` instead of itself. You keep `'call'`-event - deadline checks — which is enough to interrupt anything that calls a function - — and drop per-line tracing entirely. -2. On 3.12+, use `sys.monitoring` with only the events you need; it is designed - for exactly this and is far cheaper than `settrace`. -3. Or drop the trace and rely on the `threading.Timer` → - `ida_kernwin.set_cancelled()` path you already have, accepting that a - pure-Python loop with no calls in it cannot be interrupted. - -**Our workaround** (we would rather not ship it): the snippet detaches the trace -and restores it in a `finally`. That gives up deadline enforcement for -pure-Python loops inside our own code; your native cancel timer is unaffected and -still fires. Every client that does real work per call will eventually find this -and do the same, which is an argument for fixing it in the runtime. - ---- - -## 2. `to_jsonable` dominates any large result - -`execute_python` runs `to_jsonable()` over whatever the snippet returns. Our -answers are already JSON-safe and they are big — a 200-row listing page is -roughly 10k small objects. - -| | cost | -|---|---| -| `to_jsonable(page)` | 66.2 ms | -| `json.dumps(page, separators=(",",":"))` — same data | 0.58 ms | -| serialised size | 34.9 KB | - -That is 114x, and it was 72% of the page's total cost before we changed it. - -**Suggested fixes** - -- Fast-path values that are already JSON-safe (a cheap recursive type check that - bails to the original object beats rebuilding it), or -- let a snippet opt out by returning an already-serialised payload — a documented - envelope such as `{"__json__": "<...>"}`, or simply passing `str`/`bytes` - through untouched. - -**Our workaround:** snippets `json.dumps` inside the database process and return -one string, which the client parses. `to_jsonable` then walks a single scalar. -Cost went 66.2 ms → ~0.6 ms. It works, but every client with a large result set -has to discover and re-implement it. - ---- - -## 3. The per-operation floor is `execute_sync`, not HTTP - -Same worker, same connection, 200 iterations: - -| | cost | -|---|---| -| `GET /health` (no `execute_sync`) | **0.165 ms** | -| `execute_python("result = 1")` | **2.025 ms** | - -HTTP framing is ~7% of the floor; marshalling the operation onto IDA's main -thread is the other ~93%. The worker runs IDA's own `kernwin.serve()`, so this is -plausibly IDA's dispatch latency rather than anything you control — but it is -worth **documenting**, because it sets a hard 2 ms per-operation budget that -shapes how a client must be designed. - -It did not hurt us (our call volume is 1–8 per user action; 4 calls to build a -1060-block graph), but a client that makes one call per row or per symbol will be -20–100x slower than an in-process one and the authors will not know why. - -**Suggested fixes:** document the floor; and consider a batch endpoint — accept -`[{op, args}, ...]` and dispatch them within a single `execute_sync` — which -would let chatty clients amortise it without redesigning around it. - ---- - -## 4. Loader switches on an existing database are a FATAL, not an error - -Opening a target that already has an `.i64`, while passing spawn-only options, -kills the worker: - -``` -FATAL ERROR: @0:636[] -Switch '-b400' can be used only when loading a new file -``` - -The client sees only: - -``` -IDAConnectionError: idalib worker launcher <pid> exited with status 1 -``` - -This is easy to hit and hard to diagnose: it is the natural second run of -anything that opens a raw blob (`processor=`/`image_base=`/`file_type=` are -recorded in the database the first run produced). Our test suite hit it as a -crash five minutes into a run. - -**Suggested fixes** - -- In `DatabaseHandle.open()`, when the resolved IDB already exists and - `new_database` is not set, either ignore the spawn-only options or raise a - typed error naming them — before handing them to IDA. -- Propagate the worker's fatal text into the client exception. The message - already exists on the worker's stderr; losing it turns a one-line fix into a - bisect. - -**Our workaround:** the client checks whether the expected IDB exists and drops -`processor`/`image_base`/`file_type` when it does. - ---- - -## 5. Deleting or replacing an IDB under a live lease fails silently - -A suite that did "delete the `.i64`, reopen the same path" (safe when it owned a -private worker) now races the previous worker's lease grace. The reopen produced -a handle that never became usable, with no error — just a database with no -listing, and every wait timing out. - -**Suggested fixes** - -- Detect that the IDB backing a registered instance has been removed or replaced - and fail loudly (the registry already holds `idb_key`). -- Expose a **public** "wait until this database is released" primitive. We needed - one and ended up reaching into `registry.REGISTRY_DIR` and `FileLock` to build - it, which is not an API we should be depending on. -- Document the lease-grace window as part of the lifecycle contract. - ---- - -## 6. No close-without-save, and no rollback - -A managed worker saves when its final lease closes. A GUI handle leaves GUI state -as-is. Neither gives a client a way to say "discard what I did". - -ida-tui had a "discard & quit" that we could not port; it is now "leave as-is & -quit", and we cannot honestly promise the user their edits are not persisted. - -**Suggested fixes:** a close policy on a lease the client created -(`close(save=False)`), or a transaction/rollback API, or a documented -disposable-copy pattern that clients can follow. - ---- - -## 7. No change notification for shared databases - -The lease reports liveness, not mutations. If a GUI user or another Code Mode -client renames or retypes while we are attached, our materialised caches (name -generation, decompilation, listing pages) are silently stale. Our own edits -invalidate correctly; someone else's cannot. - -**Suggested fix — cheap and sufficient:** a monotonic database revision counter, -bumped on any mutating operation and exposed on `/health` (and ideally on the -lease event stream). Clients can then invalidate by comparing one integer. A full -change feed would be better but is much more work; the counter alone would make -shared editing safe for every caching client. - ---- - -## 8. Package exports and API surface stability - -`ida_codemode/__init__.py` exports nothing, so a library consumer must import -from submodules: - -```python -from ida_codemode.client import DatabaseHandle, ClientError, RemoteError, InstanceDisconnectedError -from ida_codemode.registry import REGISTRY_DIR, FileLock, RegistryEntry, canonical_path, idb_key, scan_instances -from ida_codemode.resolver import IdbBusy, expected_idb_path -``` - -Some of those are clearly internals (`FileLock`, `REGISTRY_DIR`) that we only -touch because no public equivalent exists (see §5). - -**Suggested fix:** export `DatabaseHandle` and the public exception types from the -package root, and mark the intended-public registry helpers explicitly. It also -makes "what is API and what is internal" answerable, which right now it is not. - ---- - -## 9. A testing note: `DatabaseHandle.open()`'s 30 keyword-only options - -The port we started from called `open(..., loading_address=...)`. The real -parameter is `image_base`. Every `connect()` would have raised `TypeError` on the -first call, and its contract tests passed anyway, because a hand-written fake -handle accepts `**kwargs`. - -Not a library bug — but with 30 keyword-only options it is a very easy mistake, -and it is invisible to exactly the offline tests people write. - -**Suggested fix:** ship `py.typed` and/or a `Protocol` for the handle, so a fake -can be checked against the real signature and a typo is caught statically. (We -added a test asserting our kwargs are a subset of -`inspect.signature(DatabaseHandle.open).parameters`, which is a poor substitute.) - ---- - -## Priority, from a client author's view - -| # | item | impact | fixable by you? | -|---|---|---|---| -| 1 | `timeout_trace` line tracing | 52x on IDA calls, 10x on real operations | yes, one line | -| 2 | `to_jsonable` on large results | 114x on serialisation | yes | -| 7 | no change/revision counter | correctness for shared editing | yes, cheap | -| 4 | loader switches fatal on reopen | crashes, hard to diagnose | yes | -| 5 | replaced/deleted IDB under lease | silent hang | yes | -| 6 | no close-without-save | a feature we had to drop | design question | -| 8 | package exports | forces internal imports | yes, trivial | -| 3 | 2 ms `execute_sync` floor | shapes client design | document; maybe batch | -| 9 | typed handle for fakes | catches a whole bug class | yes | - -Items 1 and 2 together were the difference between "the port is 35x slower than -the private worker it replaced" and "the port is within 2x, and faster on several -operations". Both are in the runtime, not in client code — which is why they are -worth fixing centrally rather than leaving each client to rediscover. - -Happy to supply the benchmark harness (it is backend-agnostic and runs against -both our old worker and Code Mode), or to test a patch. diff --git a/docs/GRAPH_VIEW.md b/docs/GRAPH_VIEW.md index 29b280e..0b24aa5 100644 --- a/docs/GRAPH_VIEW.md +++ b/docs/GRAPH_VIEW.md @@ -32,6 +32,7 @@ extra work. | `0` | jump to the entry block | | `z` | zoom: full → compact → collapsed | | `m` | show / hide the minimap | +| `e` | layout engine: auto → native → triskel | | `f` | centre on the current block | | `Enter` | follow — stays in the graph when the target is a block of this function | | `x` `n` `y` `;` | xrefs / rename / retype / comment, exactly as in the listing | @@ -53,10 +54,64 @@ Growing a second disassembly renderer for graph mode would have been the real cost. The backend adds exactly one operation, `flowchart(addr)` in -`idatui/codemode_client.py`, which returns block ranges and typed edges — **not** +`idatui/nexus_client.py`, which returns block ranges and typed edges — **not** text. -## Layout (`idatui/graph.py`) +## Two layout engines + +`graph.layout(blocks, sizer, engine=...)` takes `auto` (the default, also +`$IDATUI_GRAPH_ENGINE`), `native` or `triskel`, and `e` cycles them in the view. +`auto` prefers **triskel** where it is installed and the function is at most 180 +blocks, and falls back to **native** otherwise — including if triskel raises, +which is never fatal, and the status line then says why. + +The 180 is an interactivity budget: layout runs on every open and every zoom +keypress, and triskel's cost knees hard just past it (174 blocks: 66 ms; +233 blocks: 489 ms; 329: 555 ms; 424: 1.5 s, against native's 25/72/93/144). + +| | native | triskel | +|---|---|---| +| algorithm | layered Sugiyama, below | SESE decomposition ([paper](https://hal.science/hal-04996939)) | +| ships with | always, pure python | needs `pytriskel` (patched fork, unpublished) | +| shape | wide and short | narrow and tall | +| crossings | more | far fewer | +| 87-block `main` | 15 ms, 1202×444 | 37 ms, 845×789 | +| 424-block `sub_3720` | 145 ms | 1.5 s (so `auto` won't) | + +On the 128-function corpus with realistic box sizes, triskel draws fewer +crossings on 12 functions, the same on 9, more on 3 — and the wins are where it +matters: `sub_5CA0` 41 → 6, `sub_2C90` 32 → 7, `sub_2C00` 12 → 0. It also routes +loop edges around the side of the graph the way IDA does, instead of straight +back up the middle. It is not a clean sweep: on `sub_69C0` (109 blocks) its +narrower canvas packs edges tighter and it ends up with *more* cells shared +between edges than native (1280 vs 935). + +### The triskel path (`idatui/graph_triskel.py`) + +The whole impedance mismatch lives in that one module. Three things keep it +small: triskel's routes are already orthogonal (0 diagonal segments in 2471), its +ports already land spread along the box border, and — because our fork made the +spacing settable — **we hand it cell counts rather than pixels**, so nothing is +ever rounded and two edge lanes can never land on the same row. + +What it does not do is trust the library with degenerate input. Triskel's graph +root is **whichever node was created first**, and every one of its analyses walks +out from there, so anything the root cannot reach is undefined behaviour — it +throws `EMPTY BL` from its SESE bracket lists, or, when the entry block has no +successors at all, segfaults. That is not survivable: a crash in a C extension +takes the TUI with it, with no chance to fall back. So the entry is created +first, orphan blocks are attached to it with **phantom edges** that steer the +layout but are never drawn, and reachability is *asserted in python* before +crossing into C++. + +The rest is handled before the call too: self-loops (drawn as `↺`; they make +triskel throw), and edges routed through a block, which are detoured and +re-verified. Whatever is left over falls back to native rather than reach the +screen wrong — currently 8 layouts in 1200 (`ls`, three zoom levels each), all +of them triskel leaving two boxes a few columns into each other, which in a +terminal means one block's disassembly overwriting another's. + +## Layout (`idatui/graph.py`, the native engine) Pure python: no IDA, no Textual, no I/O, so it is unit-tested offline in milliseconds (`tests/test_graph.py`, which needs no worker). Textbook Sugiyama, @@ -130,9 +185,16 @@ listing. A CFG that size is not a picture anyone can read — IDA's own is a hairball there too (1853 crossings on the worst function in `targets/echo`). This is a feature, not a shortcoming. -Known cosmetic gap: a back edge leaves its tail's *top* border (`┴`) and arrows -up into the head's *bottom* (`▲`). Correct and readable, but IDA runs loop edges -around the side of the graph. +Known cosmetic gap **of the native engine**: a back edge leaves its tail's *top* +border (`┴`) and arrows up into the head's *bottom* (`▲`). Correct and readable, +but IDA runs loop edges around the side of the graph — which is exactly what the +triskel engine does, so `e` is the workaround. + +That difference is why an edge's arrowhead is decided by `Route.flipped` and not +by geometry. The native engine reverses back edges to get a DAG, so its polyline +runs *against* control flow and the arrow belongs at the start; triskel keeps the +real direction. Reading the direction off the drawing would silently reverse +every loop edge on one of the two engines. ## Driving it @@ -151,8 +213,36 @@ drive raw graph action=zoom - `experiments/cfg_dump.py` — freeze real CFGs from a binary to JSON. - `experiments/graph_spike.py` — lay out and render a corpus function to stdout, - or `--stats` the whole corpus. Uses `idatui.graph`, so it exercises the - shipping engine with no worker in the loop. + or `--stats` the whole corpus; `--engine` picks the backend. Uses + `idatui.graph`, so it exercises the shipping engine with no worker in the loop. - `experiments/graph_smoke.py` — end-to-end: tool → domain → layout. - `experiments/graph_shot.py` — render the real view headless at a chosen size - (the pane you are in is usually too narrow to judge it). + (the pane you are in is usually too narrow to judge it); takes an engine as + its fifth argument. + +## Installing the triskel engine + +It is optional; without it everything works and `auto` means `native`. + +It needs `pytriskel`, and specifically a **patched build that is not published +anywhere yet**. Upstream's wheels stop at cp313 with no sdist (so there is +nothing to install on 3.14), and on any version their `get_waypoints()` raises, +which means no edge routes at all. Until that fork is released you will get the +native engine — which is the default, ships with the repo, and is fully +supported. The rest of this section only applies if you already have a patched +build tree. + +**Install it into the interpreter the launcher actually runs**, which is +`$IDATUI_PYTHON` and defaults to `~/ida-venv/bin/python` — *not* the repo's +`.venv`, which is only what the tests use. Getting this wrong is the one way to +see `no pytriskel in ...` in the status bar while `tests/test_graph.py` happily +exercises both engines; the message names the interpreter for that reason. + +```bash +"$IDATUI_PYTHON" -m pip install /path/to/triskel/bindings/python +.venv/bin/python -m pip install /path/to/triskel/bindings/python # for the tests +``` + +Needs cmake, ninja and a C++23 compiler at install time; the wheel is built from +source for whichever interpreter runs pip. `$IDATUI_TRISKEL_PATH` can point at a +build tree instead of installing. diff --git a/docs/NEXUS_UPSTREAM.md b/docs/NEXUS_UPSTREAM.md new file mode 100644 index 0000000..f4dcf91 --- /dev/null +++ b/docs/NEXUS_UPSTREAM.md @@ -0,0 +1,385 @@ +# Findings from porting a real client to IDA Nexus + +Notes for the `ida-nexus` maintainers, gathered while porting **ida-tui** (a +Textual TUI frontend for IDA) from a private idalib worker to +`ida_nexus.DatabaseHandle`. + +Everything below is measured, not inferred. Where we worked around something, the +workaround is named so you can judge whether the library should make it +unnecessary. + +**Environment:** ida-nexus 0.3.1, IDA 9.4 (idalib), Linux, single managed +worker backend, quiet box. Target for timings: `targets/echo` unless stated. + +> **Status against the protocol-6 event-stream development tree, based on 0.6.1 +> (upstream `439289f`) — every item re-checked.** +> +> | item | verdict | +> |---|---| +> | 1 `timeout_trace` line tracing | ✅ **fixed in 0.3.2** — no `settrace` in the runtime at all | +> | 2 `to_jsonable` on large results | ✅ **fixed in 0.3.2** — `dumps_json` C fast path | +> | 3 2 ms `execute_sync` floor | ✅ **fixed in 0.3.2, 7.0x** — 2.055 ms → 0.294 ms | +> | 4 loader switches fatal on reopen | **partial** — normal reopen fixed in 0.5.x; direct `.i64` paths remain [issue #36](https://github.com/HexRaysSA/ida-nexus/issues/36) | +> | 5 IDB replaced under a live lease | **stale** — out-of-band replacement is outside the supported lifecycle, as it is for the IDA GUI | +> | 6 close without save | **fixed in protocol 6** — the final managed-worker lease can choose `shutdown_database(save=False)` | +> | 7 no change notification | ✅ **fixed in protocol 6** — `DatabaseHandle.subscribe_idb_events()` streams revisioned, operation-attributed IDB changes | +> | 8 package exports | ✅ **fixed in 0.5.x** — a real `__all__` on the package root | +> | 9 no `py.typed` / handle Protocol | ✅ **fixed in 0.5.x** — `ida_nexus/py.typed` ships | +> +> **0.5.x restructured the package**, which is why the old "these files are +> byte-identical" re-check recipe no longer works: `client.py` → `handle.py`, +> `registry.py` → `_registry.py` + `instances.py`, `resolver.py` → `_resolver.py`, +> and the loader options moved into a frozen `DatabaseOpenOptions` dataclass. +> Everything private is now underscore-prefixed, so the cheap re-check after an +> upstream pull is simply: does anything we import still appear in +> `ida_nexus.__all__`? +> +> 0.5.3 → 0.6.1 changed **nothing** we depend on: `__init__.py`, `handle.py`, +> `instances.py`, `options.py`, `errors.py` and `models.py` are byte-identical +> between those two releases. 0.6.1 only collapses the six console scripts into a +> single `ida-nexus` command. +> +> Both client-side workarounds re-measured at **0.99x and 0.97x** on 0.3.2 — +> i.e. nothing — and are deleted. Remote code is now ordinary typed Python, +> installed as content-addressed modules by ida-nexus. Harness: +> `experiments/bench_pack_trace.py`. + +**What the client does**, for scale: it renders a continuous disassembly listing, +pseudocode, a CFG graph view and a hex view, paging over the database as the user +scrolls. It is latency-sensitive in a way an agent-driven MCP client is not — a +keypress must repaint. It issues ~1–8 operations per user action. + +--- + +## 1. `timeout_trace` enables line tracing in every frame — 52x on IDA calls + +**Highest-impact item by a wide margin.** — ✅ **FIXED in 0.3.2.** The runtime no +longer installs a trace hook at all; cancellation is a C-level thread interrupt. +Our `sys.settrace(None)` workaround is deleted as of `a5137fe`. + +`runtime.py` wraps every `execute_python` in `sys.settrace(timeout_trace)` to +enforce the deadline. `timeout_trace` ends with `return timeout_trace`, and +returning a trace function from a `'call'` event asks CPython to trace **every +line of that frame**. So every line of every function the snippet touches pays a +Python-level callback, and the specialising interpreter is disabled throughout. + +Measured inside the worker, same process, same database: + +| | traced (stock) | untraced | native idalib | +|---|---|---|---| +| `ida_bytes.get_flags(ea)` | 5.49 µs | 0.106 µs | 0.119 µs | +| our 200-row listing page | 20.2 ms | 2.0 ms | — | + +Untraced matches a plain idalib process, so the trace hook accounts for +essentially all of it. For us this was the single largest cost in the port — +larger than HTTP, serialisation and IDA itself combined. + +Reproduce inside any `execute_python`: + +```python +import sys, time, ida_bytes +def bench(): + t = time.perf_counter() + for _ in range(20000): ida_bytes.get_flags(0x1000) + return (time.perf_counter() - t) / 20000 * 1e6 +traced = bench() +old = sys.gettrace(); sys.settrace(None) +try: untraced = bench() +finally: sys.settrace(old) +result = {"traced_us": traced, "untraced_us": untraced} +``` + +**Suggested fixes, cheapest first** + +1. `return None` from `timeout_trace` instead of itself. You keep `'call'`-event + deadline checks — which is enough to interrupt anything that calls a function + — and drop per-line tracing entirely. +2. On 3.12+, use `sys.monitoring` with only the events you need; it is designed + for exactly this and is far cheaper than `settrace`. +3. Or drop the trace and rely on the `threading.Timer` → + `ida_kernwin.set_cancelled()` path you already have, accepting that a + pure-Python loop with no calls in it cannot be interrupted. + +**Our workaround** (we would rather not ship it): the snippet detaches the trace +and restores it in a `finally`. That gives up deadline enforcement for +pure-Python loops inside our own code; your native cancel timer is unaffected and +still fires. Every client that does real work per call will eventually find this +and do the same, which is an argument for fixing it in the runtime. + +--- + +## 2. `to_jsonable` dominates any large result + +**FIXED in 0.3.2**, via the first suggested fix below: +`serialization.dumps_json` calls `json.dumps(value, default=to_jsonable)`, so a +JSON-safe result never enters the Python walker. Our packing workaround measured +0.97x and has been deleted. + +`execute_python` runs `to_jsonable()` over whatever the snippet returns. Our +answers are already JSON-safe and they are big — a 200-row listing page is +roughly 10k small objects. + +| | cost | +|---|---| +| `to_jsonable(page)` | 66.2 ms | +| `json.dumps(page, separators=(",",":"))` — same data | 0.58 ms | +| serialised size | 34.9 KB | + +That is 114x, and it was 72% of the page's total cost before we changed it. + +**Suggested fixes** + +- Fast-path values that are already JSON-safe (a cheap recursive type check that + bails to the original object beats rebuilding it), or +- let a snippet opt out by returning an already-serialised payload — a documented + envelope such as `{"__json__": "<...>"}`, or simply passing `str`/`bytes` + through untouched. + +**Retired workaround:** snippets used to `json.dumps` inside the database process +and return one string, which the client parsed. The typed remote API now owns +strict argument/result encoding, and ida-tui contains no generated script +strings or packing envelope. + +--- + +## 3. The per-operation floor is `execute_sync`, not HTTP + +✅ **FIXED in 0.3.2 — 7.0x.** Re-measured as a same-box A/B by checking the +installed editable checkout back to `4195f21` and forward again, 200 iterations +each, `targets/echo`: + +| | 0.3.1 | 0.3.2 | | +|---|---|---|---| +| `GET /health` | 0.497 ms | 0.318 ms | 1.6x | +| `execute_python("result = 1")` | **2.055 ms** | **0.294 ms** | **7.0x** | + +The 0.3.1 column reproduces the original 2.025 ms measurement below almost +exactly, which is what makes the 0.3.2 column believable. `execute_python` now +costs about the same as a bare HTTP GET, so the `execute_sync` marshalling that +was ~93% of the floor is essentially gone. The design advice below — "a client +that makes one call per row will be 20–100x slower than an in-process one" — is +correspondingly much weaker now. + +Original 0.3.1 measurement, same worker, same connection, 200 iterations: + +| | cost | +|---|---| +| `GET /health` (no `execute_sync`) | **0.165 ms** | +| `execute_python("result = 1")` | **2.025 ms** | + +HTTP framing is ~7% of the floor; marshalling the operation onto IDA's main +thread is the other ~93%. The worker runs IDA's own `kernwin.serve()`, so this is +plausibly IDA's dispatch latency rather than anything you control — but it is +worth **documenting**, because it sets a hard 2 ms per-operation budget that +shapes how a client must be designed. + +It did not hurt us (our call volume is 1–8 per user action; 4 calls to build a +1060-block graph), but a client that makes one call per row or per symbol will be +20–100x slower than an in-process one and the authors will not know why. + +**Suggested fixes:** document the floor; and consider a batch endpoint — accept +`[{op, args}, ...]` and dispatch them within a single `execute_sync` — which +would let chatty clients amortise it without redesigning around it. + +--- + +## 4. Loader switches on an existing database are a FATAL, not an error — one edge remains + +Opening a target that already has an `.i64`, while passing spawn-only options, +kills the worker: + +``` +FATAL ERROR: @0:636[] +Switch '-b400' can be used only when loading a new file +``` + +The client sees only: + +``` +IDAConnectionError: idalib worker launcher <pid> exited with status 1 +``` + +This is easy to hit and hard to diagnose: it is the natural second run of +anything that opens a raw blob (`processor=`/`image_base=`/`file_type=` are +recorded in the database the first run produced). Our test suite hit it as a +crash five minutes into a run. + +**Suggested fixes** + +- In `DatabaseHandle.open()`, when the resolved IDB already exists and + `new_database` is not set, either ignore the spawn-only options or raise a + typed error naming them — before handing them to IDA. +- Propagate the worker's fatal text into the client exception. The message + already exists on the worker's stderr; losing it turns a one-line fix into a + bisect. + +**Our workaround:** the client checked whether the expected IDB exists and +dropped `processor`/`image_base`/`file_type` when it did. + +**FIXED in 0.5.x**, with exactly this fix, in `_resolver._build_worker_command`: + +```python +if input_path == expected_idb and input_path != source: + # Loader/import switches are baked into an existing IDB... + options = WorkerLaunchOptions() +``` + +Our workaround is therefore deleted. **One narrow case remains**: the strip needs +`input_path != source`, so passing an `.i64` path *directly* together with load +options (`ida-tui foo.i64 --processor arm`) still forwards the switches and still +fatals. Our old guard keyed on "the target IDB exists" and so covered it. It is a +nonsense invocation and no ida-tui code path generates it — the project layer +always passes `output_database`, and `_needs_load_options` bails when an `.i64` +exists — but the library boundary should still reject or normalize it rather +than launch a known-fatal IDA command. Tracked upstream as +[issue #36](https://github.com/HexRaysSA/ida-nexus/issues/36). + +--- + +## 5. Deleting or replacing an IDB under a live lease — STALE + +The original suite deleted an `.i64` while a private worker still had it open, +then immediately reopened the same path. That ownership model no longer applies: +IDA Nexus databases are shared resources, and the IDA GUI itself does not survive +out-of-band replacement of its open database. Detecting arbitrary filesystem +replacement is therefore not part of the supported lifecycle. + +The actionable lifecycle gaps that originally forced private-registry access are +fixed. `find_database_owner()` and `wait_database_released()` are public exports; +`DatabaseHandle.close(wait_for_database=True)` can wait for a final managed close; +a draining owner remains registered until the IDB is actually closed; and +`new_database=True` refuses to replace a live owner. + +ida-tui now uses the public owner/release API while recreating a database and no +longer reaches into registry locks. Owner loss is attach-only: ida-tui will +rediscover a replacement GUI or worker, but will never turn a +user-closing-the-GUI action into an implicit headless reopen. There is no +remaining upstream request in this section. + +--- + +## 6. Close without save — FIXED in protocol 6 + +`DatabaseHandle.shutdown_database(save=False)` can discard a managed idalib +worker when the requesting handle is its only active lease and no other operation +is running. The server rejects GUI databases and shared workers. + +The coherent ownership model is the **final lease**, not necessarily the lease +that spawned the worker. Releasing a non-final lease makes no whole-database save +decision; responsibility transfers to the leases that remain. The final client +can save or discard the shared session. A client that needs its work to survive +regardless of that later decision must call `save_database()` before releasing +its lease. + +This does not claim to provide per-client rollback. Discard applies to all +changes since the last database save, and attempting it while another lease is +active is correctly rejected. That is the same ref-counted lifetime model used +by other shared resources and requires no separate starter capability. + +The upstream gap is therefore closed. ida-tui now routes its discard action +through `shutdown_database(save=False)`: a final managed-worker lease discards, +while GUI-backed and still-shared sessions transfer finalization to their owner +or remaining leases. + +--- + +## 7. No change notification for shared databases — FIXED in protocol 6 + +`DatabaseHandle.subscribe_idb_events()` now returns a closeable iterator over +structured IDB changes. Each event carries a monotonic revision plus +`operation_id`/`operation_label` attribution and an opaque `origin_id`. +`DatabaseHandle.owns_event()` compares that origin with the handle's lease, so a +caching client does not need to generate, retain, or race operation IDs itself. + +ida-tui keeps one subscription for its active database, asks the handle to drop +its own events, and batches peer events behind a 200 ms quiet period. One batch +invalidates the function, listing, decompiler, graph, strings, linkage, segment +and byte caches, then reloads the visible view in place. Closing or switching +databases closes the subscription, so the blocking event reader does not leak. + +--- + +## 8. Package exports and API surface stability — FIXED in 0.5.x + +`ida_nexus/__init__.py` used to export nothing, so a library consumer had to +import from submodules, including things that were clearly internals (`FileLock`, +`REGISTRY_DIR`, `canonical_path`, `idb_key`, `scan_instances`) that we only +touched because no public equivalent existed. + +**Suggested fix was:** export `DatabaseHandle` and the public exception types from +the package root, and mark the intended-public registry helpers explicitly. + +**That is what 0.5.x did.** Everything we need is now on the package root, and +the internals moved behind an underscore: + +```python +from ida_nexus import DatabaseHandle, DatabaseOpenOptions, DatabaseInstance +from ida_nexus import RemoteError, DatabaseBusyError, DatabaseDisconnectedError +from ida_nexus import discover_databases, find_database_owner, wait_database_released +``` + +The two lock-poking helpers we had reimplemented client-side +(`_wait_for_entry_release`) are now `wait_database_released()`, and our +registry-scanning ownership check is now `find_database_owner()`. Both are +deleted from our tree. Note `find_database_owner()` *raises* +`AmbiguousDatabaseError` where our scan silently took the first match — a +behaviour improvement, but callers need a handler. + +--- + +## 9. A testing note: `DatabaseHandle.open()`'s 30 keyword-only options — FIXED in 0.5.x + +The port we started from called `open(..., loading_address=...)`. The real +parameter is `image_base`. Every `connect()` would have raised `TypeError` on the +first call, and its contract tests passed anyway, because a hand-written fake +handle accepts `**kwargs`. + +Not a library bug — but with 30 keyword-only options it is a very easy mistake, +and it is invisible to exactly the offline tests people write. + +**Suggested fix:** ship `py.typed` and/or a `Protocol` for the handle, so a fake +can be checked against the real signature and a typo is caught statically. (We +added a test asserting our kwargs are a subset of +`inspect.signature(DatabaseHandle.open).parameters`, which is a poor substitute.) + +**0.5.x ships `ida_nexus/py.typed`**, and the 30 keyword-only options became a +frozen `DatabaseOpenOptions` dataclass — which is strictly better, because an +invented option name is now a `TypeError` at construction rather than something a +`**kwargs` fake swallows. Our subset test survives in two halves +(`_open_kwargs_are_real` for `open()`, `_option_fields_are_real` for the +dataclass fields), because the offline contract suite must keep running with no +`ida_nexus` installed at all and therefore still fakes both. + +--- + +## Priority, from a client author's view + +| # | item | impact | fixable by you? | +|---|---|---|---| +| ~~1~~ | ~~`timeout_trace` line tracing~~ | ~~52x on IDA calls~~ | ✅ fixed in 0.3.2 | +| ~~2~~ | ~~`to_jsonable` on large results~~ | ~~114x on serialisation~~ | ✅ fixed in 0.3.2 | +| ~~3~~ | ~~2 ms `execute_sync` floor~~ | ~~shapes client design~~ | ✅ fixed in 0.3.2, 7.0x | +| ~~7~~ | ~~no change/revision counter~~ | ~~correctness for shared editing~~ | ✅ fixed in protocol 6 | +| 4 | direct `.i64` forwards loader-only options | fatal worker startup | [issue #36](https://github.com/HexRaysSA/ida-nexus/issues/36) | +| ~~5~~ | ~~replaced/deleted IDB under lease~~ | ~~out-of-contract filesystem mutation~~ | **stale** | +| ~~6~~ | ~~no close without save~~ | ~~could not discard a managed session~~ | **fixed in protocol 6: final lease decides** | +| ~~8~~ | ~~package exports~~ | ~~forces internal imports~~ | ✅ fixed in 0.5.x | +| ~~9~~ | ~~typed handle for fakes~~ | ~~catches a whole bug class~~ | ✅ fixed in 0.5.x (`py.typed` + options dataclass) | + +Items 1 and 2 together were the difference between "the port is 35x slower than +the private worker it replaced" and "the port is within 2x, and faster on several +operations". Both are in the runtime, not in client code — which is why they are +worth fixing centrally rather than leaving each client to rediscover. + +**Both landed in 0.3.2**, along with item 3 — all three performance items are now +fixed upstream, and both client-side workarounds could be measured at parity and +retired. That is the outcome this document was written for. + +**What is left is entirely non-performance.** Items 6 through 9 are fixed, and +item 5 is stale because out-of-band replacement is not a supported lifecycle for +either IDA Nexus or the IDA GUI. One narrow piece remains: **4**, normalize or +reject loader-only options when the source is itself an existing `.i64` +([issue #36](https://github.com/HexRaysSA/ida-nexus/issues/36)). + +Happy to supply the benchmark harness (it is backend-agnostic and runs against +both our old worker and IDA Nexus), or to test a patch. diff --git a/docs/PAGING_FINDINGS.md b/docs/PAGING_FINDINGS.md index bd6c38f..f343973 100644 --- a/docs/PAGING_FINDINGS.md +++ b/docs/PAGING_FINDINGS.md @@ -3,9 +3,9 @@ Measured against a real target: `libcrypto.so.3` (5.7 MB, **10,092 functions**, biggest function **52,120 instructions**). These constraints drive the domain / paging layer. The measurements below came from the former ida-pro-mcp tool -backend. The Code Mode port preserves the adapter response shapes and conservative +backend. The IDA Nexus port preserves the adapter response shapes and conservative page sizes, but executes enumeration through ida-domain; old server caps and RTT -numbers are historical rather than Code Mode constraints. +numbers are historical rather than IDA Nexus constraints. ## Response shape (list_* / *_query tools) @@ -93,17 +93,17 @@ disasm totals are **top-level** fields, not under `asm`: (correct). The pseudocode view must handle "decompilation failed" gracefully — fall back to the disassembly view or show an error panel. -Code Mode returns the complete execution result directly; ida-tui no longer +IDA Nexus returns the complete execution result directly; ida-tui no longer needs MCP structured-content/download-URL recovery for large pseudocode bodies. -## Code Mode lifecycle +## IDA Nexus lifecycle -`CodeModeClient` owns an authenticated SSE lease on a registered database: +`NexusClient` owns an authenticated SSE lease on a registered database: * A matching GUI is preferred and remains open when the TUI exits. -* Otherwise Code Mode reuses or starts a shared managed idalib worker. +* Otherwise IDA Nexus reuses or starts a shared managed idalib worker. * Releasing one lease never terminates another client's session. A managed - worker saves and exits after its final lease under Code Mode's grace policy. + worker saves and exits after its final lease under IDA Nexus's grace policy. * Lease loss surfaces as `IDAConnectionError`; reconnect performs discovery again and may bind a newly-created instance. It does not silently swap the handle underneath an operation. diff --git a/docs/PROJECTS.md b/docs/PROJECTS.md index 0efee31..52dbf83 100644 --- a/docs/PROJECTS.md +++ b/docs/PROJECTS.md @@ -7,7 +7,7 @@ search across all of them, and (later) follow calls from one into another. ## The constraint that shapes everything -IDA still exposes one active database per GUI/idalib process. Code Mode makes +IDA still exposes one active database per GUI/idalib process. IDA Nexus makes those instances discoverable and shareable: each project entry retains one `DatabaseHandle` lease, which may target a registered GUI or a managed idalib worker. N resident project databases can therefore mean up to N processes, but @@ -32,13 +32,13 @@ crypto library. Two capabilities that feel like one, but aren't: -1. **Switching** to a binary needs a *live Code Mode lease*. +1. **Switching** to a binary needs a *live IDA Nexus lease*. 2. **Searching across** binaries does *not* — if a per-binary index (functions, strings, imports/exports) is cached on disk. That split is the unlock: project-wide search stays instant across every binary, including ones never opened this session, and only *jumping* to a hit costs a -Code Mode attach/open. +IDA Nexus attach/open. ## Layout @@ -85,11 +85,11 @@ basename and must be unique (it names the staged file). ## Runtime -- **`DatabasePool`** — one `CodeModeClient` lease per resident binary, attached +- **`DatabasePool`** — one `NexusClient` lease per resident binary, attached lazily on first switch and LRU-released when the advisory memory budget is exceeded. Eviction explicitly saves managed IDBs but never implicitly saves a GUI. Closing a lease never kills a GUI or another client's managed worker; - Code Mode owns final worker shutdown. + IDA Nexus owns final worker shutdown. - **`BinaryState`** — per binary: `client, program, nav, cur, func_index, pref/active/split, filter`. Switching snapshots the current state and restores the target's. `_after_reconnect` provides the client/program swap seam. diff --git a/docs/SPLIT_VIEW.md b/docs/SPLIT_VIEW.md index c421656..6e9b938 100644 --- a/docs/SPLIT_VIEW.md +++ b/docs/SPLIT_VIEW.md @@ -32,7 +32,7 @@ known technique: The old ida-pro-mcp backend derived the per-line marker via `cfunc.get_line_item(line, col=0, …).get_ea()`. To get the **full set**, sweep every column of the line (`get_line_item(line, x, …).get_ea()` for `x` in -`0..len`) and collect distinct non-`BADADDR` EAs. The Code Mode adapter's +`0..len`) and collect distinct non-`BADADDR` EAs. The IDA Nexus adapter's `decomp_map(ea)` operation returns `[{line, primary_ea, eas:[…]}, …]`; invert for `ea → line`. @@ -78,14 +78,14 @@ decomp→listing uses `ListingModel.ensure_ea`. Tab re-links from the new driver Still single-ea per line (one instruction highlighted); the region comes in phase 3. -**Phase 3 — rich highlight. DONE.** The Code Mode `decomp_map` operation -(`idatui/codemode_client.py`) sweeps `cfunc.get_line_item` across every column of +**Phase 3 — rich highlight. DONE.** The IDA Nexus `decomp_map` operation +(`idatui/nexus_client.py`) sweeps `cfunc.get_line_item` across every column of each pseudocode line and collects the EAs from each item's `dstr()` (`'EA: desc'` — the same source as the `/*ea*/` marker, so it aligns). `Program.decomp_map(ea)` returns the per-line ea lists (cached by name-gen); the app loads it async into `_split_eamap` / `_split_ea2line` and `_sync_split` bands the **whole** instruction region of a C line (and uses the exact ea→line inverse for the reverse). Falls -back to the single marker until the map lands. Verified on a real Code Mode database +back to the single marker until the map lands. Verified on a real IDA Nexus database (alignment + multi-instruction region band). **Phase 4 — polish. DONE.** diff --git a/docs/TEXTUAL_NOTES.md b/docs/TEXTUAL_NOTES.md index 10df669..aba84ae 100644 --- a/docs/TEXTUAL_NOTES.md +++ b/docs/TEXTUAL_NOTES.md @@ -53,6 +53,15 @@ Hard-won Textual behaviour and the patterns this app relies on. Pairs with `build_byte_to_codepoint_dict`, so character offsets smear on non-ASCII), and a `TextAreaTheme` that sets `base_style` overrides the widget's CSS colours — ours sets only `syntax_styles` so the editor keeps the app's background. +- **Fixed-size splash art disappears instead of shrinking.** The kitty image is + scaled by the terminal into whatever cell box you place it in (`c=`/`r=`), so + sizing it to the artwork's natural height and then asking "is there room?" is + all-or-nothing — a 31-row zellij pane was ONE row short of the 41 the splash + wanted, and the logo silently vanished. Size the art to the room instead + (`logo_cells(max_rows)`), and keep the chrome constant honest: + `LOGO_CHROME_ROWS = 10` is border 2 + padding 2 + the art's margin 1 + title 1 + + note 1+1 + help 1+1, which the old `rows + 9` under-counted by one, so at + exactly the threshold the help line was clipped off the bottom. - **Centre modals with a rule, not a list.** `ModalScreen { align: center middle; }` matches subclasses, so every dialog inherits it and the next one is centred for free. Naming the screens instead (`SymbolPalette, StringsPalette, …`) is |