diff options
| -rw-r--r-- | .auto/check_rename.py | 120 | ||||
| -rwxr-xr-x | .auto/checks.sh | 14 | ||||
| -rw-r--r-- | .auto/log.jsonl | 2 | ||||
| -rw-r--r-- | .auto/wip-chunk.patch | 53 | ||||
| -rw-r--r-- | idatui/domain.py | 35 |
5 files changed, 212 insertions, 12 deletions
diff --git a/.auto/check_rename.py b/.auto/check_rename.py new file mode 100644 index 0000000..c617aa6 --- /dev/null +++ b/.auto/check_rename.py @@ -0,0 +1,120 @@ +#!/usr/bin/env python3 +"""Correctness gate for the listing's rename handling (run by .auto/checks.sh). + +A rename does not move any listing row, so ``Program.bump_names`` keeps the +segment's walk and only marks the rendered text stale; ``ListingModel`` re-renders +a block at a time as rows are read. That is worth 500x on a big binary (the +alternative re-walks the whole segment to find a row the cursor was already on), +and it is exactly the kind of optimisation that fails *quietly*: the pane keeps +showing the old name and nothing errors. + +Two things are checked, because they exercise different paths and only the first +was ever caught by accident: + +* a NARROW read (``get`` per row, what painting does), and +* a WIDE read (``window`` over thousands of rows, what search's body build does) + +both have to come back with the new name -- and the whole model has to match one +built from scratch, row for row. + + ~/ida-venv/bin/python .auto/check_rename.py [targets/echo] +""" +from __future__ import annotations + +import os +import shutil +import sys + +HERE = os.path.dirname(os.path.abspath(__file__)) +ROOT = os.path.dirname(HERE) +sys.path.insert(0, ROOT) +sys.path.insert(0, HERE) + +from bench import stage # noqa: E402 +from idatui.domain import Program # noqa: E402 +from idatui.worker_client import WorkerClient # noqa: E402 + + +def rename(client, ea: int, name: str) -> None: + client.call("rename", batch={"func": [{"addr": hex(ea), "name": name}]}) + + +def main() -> int: + target = sys.argv[1] if len(sys.argv) > 1 else "targets/echo" + d, path = stage(os.path.join(ROOT, target)) + fails: list[str] = [] + client = WorkerClient(path) + try: + prog = Program(client) + idx = prog.functions() + idx.load_all() + funcs = idx.all_loaded() + if len(funcs) < 4: + print(f"{target}: too few functions to check") + return 1 + seg = sorted(funcs, key=lambda f: f.addr)[len(funcs) // 2].addr + model = prog.listing(seg) + model.load_all() + total = len(model) + + for k, victim in enumerate(sorted(funcs, key=lambda f: -f.size)[:2]): + new = f"_check_rename_{os.getpid()}_{k}" + rename(client, victim.addr, new) + prog.bump_names() + + # WIDE read -- what building the search body does. This is the one + # that used to come back with the old names: an oversized refetch + # overflowed the heads tool's row cap, failed its sequence check and + # left the block untouched. + wide = [] + for base in range(0, total, 4096): + wide.extend(model.window(base, min(4096, total - base))) + # "Is the old name gone" is not a sound test -- `main` is a token of + # `__libc_start_main` and can legitimately appear in a comment or a + # string. That the NEW name arrived proves the refresh ran; that the + # model matches a rebuild, below, proves it ran correctly. + shown = sum(1 for h in wide if h is not None + and (new in (h.text or "") or new == (h.name or ""))) + if not shown: + fails.append(f"wide read after renaming {victim.name} -> {new}: " + f"no row shows the new name") + + # NARROW read -- what painting does -- and the whole model against a + # rebuild, row for row. + kept = [(h.ea, h.kind, h.text, h.name) if h else None + for h in (model.get(i) for i in range(total))] + prog._listings.clear() + fresh_model = prog.listing(seg) + fresh_model.load_all() + fresh = [(h.ea, h.kind, h.text, h.name) if h else None + for h in (fresh_model.get(i) for i in range(len(fresh_model)))] + if kept != fresh: + bad = next((i for i, (a, b) in enumerate(zip(kept, fresh)) + if a != b), None) + fails.append(f"kept model != rebuilt model after renaming " + f"{victim.name}: first difference at row {bad}: " + f"{kept[bad] if bad is not None else None} vs " + f"{fresh[bad] if bad is not None else None}") + + rename(client, victim.addr, victim.name) + prog.bump_names() + prog._listings.clear() + model = prog.listing(seg) + model.load_all() + total = len(model) + + print(f"rename handling: {total} rows checked wide and narrow, " + f"{len(fails)} problems") + for f in fails: + print(" FAIL", f) + return 1 if fails else 0 + finally: + try: + client.close() + except Exception: # noqa: BLE001 + pass + shutil.rmtree(d, ignore_errors=True) + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/.auto/checks.sh b/.auto/checks.sh index 5323fce..d4120b0 100755 --- a/.auto/checks.sh +++ b/.auto/checks.sh @@ -4,7 +4,12 @@ # 1. .auto/check_search.py -- the search fast paths against the plain loop. # Caches that go stale still return AN answer, so no scenario test can see # them; this compares fast and slow directly, for every typed prefix. -# 2. tests/run.py -- the project's own front door: every suite, pure and IDA, +# 2. .auto/check_rename.py -- a rename keeps the listing's walk and only +# re-renders its text. When that goes wrong the pane simply keeps showing +# the old name, which nothing else notices. Checks a NARROW read (what +# painting does) and a WIDE one (what building the search body does), and +# the whole model against a rebuild. +# 3. tests/run.py -- the project's own front door: every suite, pure and IDA, # ~185s. It used to be only the scenario suite here, and that gap cost a # real regression: a faster worker connect left the loading overlay up a # moment longer relative to the index finishing, and project mode's first @@ -28,6 +33,13 @@ search=$("$PY" .auto/check_search.py targets/echo 2>&1) || { } echo "$search" | tail -1 +rn=$("$PY" .auto/check_rename.py targets/echo 2>&1) || { + echo "--- the listing is wrong after a rename ---" + echo "$rn" | tail -20 + exit 1 +} +echo "$rn" | tail -1 + out=$(python3 tests/run.py 2>&1) || true echo "$out" | tail -2 diff --git a/.auto/log.jsonl b/.auto/log.jsonl index 951b62f..5feb84e 100644 --- a/.auto/log.jsonl +++ b/.auto/log.jsonl @@ -36,3 +36,5 @@ {"run":30,"commit":"da1dfe8","metric":19834.8,"metrics":{"lg_boot_ms":676.9,"lg_decomp_ms":2698.8,"lg_graph_ms":892.1,"lg_hex_ms":440.4,"lg_index_ms":93.6,"lg_listing_cold_ms":438.5,"lg_listing_warm_ms":420.8,"lg_nav_ms":6646.7,"lg_palette_ms":4.7,"lg_render_ms":215.1,"lg_search_ms":767,"lg_split_ms":1327.6,"pure_graph_ms":213.6,"sm_boot_ms":468,"sm_decomp_ms":1317.1,"sm_graph_ms":730.7,"sm_hex_ms":483.2,"sm_index_ms":2.6,"sm_listing_cold_ms":266.1,"sm_listing_warm_ms":268.1,"sm_nav_ms":288.6,"sm_palette_ms":0.3,"sm_render_ms":253,"sm_search_ms":47.1,"sm_split_ms":874.2,"fails":0},"status":"keep","description":"decomp_map: stop sweeping every column three times over. It allocated three ctree_item_t SWIG objects PER COLUMN, swept the tagged line length (124 columns for a 23-column line), and called dstr() — which formats a whole 'EA: description' string — for every column even though consecutive columns report the same ctree item. Now: one item, no head/tail, visible columns only, and dstr() only when the item's obj_id changes.","timestamp":1786070905591,"segment":5,"confidence":null,"asi":{"gains":"total 33502 -> 19835 (-40.8%); lg_split 10189 -> 1328 (-87%); sm_split 6034 -> 874 (-86%)","work_unchanged_proof":"split_mapped_lines is 985 (echo) and 2069 (bash) BEFORE and AFTER -- the same per-line instruction sets are produced, from the same number of lines","equivalence":"/tmp/dmapeq2.py calls the shipped tool and a copy of the pre-change implementation for the same functions and compares the whole payload: 0 real mismatches over 165 functions on echo/ls_ttl/bash. (Two apparent ones were my reference's placeholder error string for functions Hex-Rays refuses.) With Hex-Rays warm for both sides the sweep is 8.9-9.8x faster; bash's 25 largest went 67.4s -> 6.8s.","the_three_wastes":["three ctree_item_t SWIG allocations per COLUMN -- one per call is enough, and head/tail are filled but never read, so pass None","range(len(sl.line)) is the TAGGED length: sl.line still carries IDA's colour tags, so a 23-column line was swept 124 times. tag_remove's length is the real bound.","item.dstr() formats a description string for every column; consecutive columns are nearly always the same ctree item. Comparing item.it.obj_id first collapses that to one format per item -- and the result is deduped by `seen` anyway, so skipping a repeat cannot change it."],"lesson_repeated":"this is the third time a cost was invisible because the benchmark did not exercise the feature (graph opens in v2, the non-scenario suites in v5, split view here). Coverage of the FEATURE matters more than precision on the ones already covered.","next_action_hint":"budget now: lg_nav 6647 (34%), decomp 4016, split 2202, graph 1623, listing 1394, boot 1145, hex 924, search 814, render 468. Split is still 2.2s for 24 functions -- what is left there is ida_hexrays.decompile inside decomp_map, which duplicates the decompile the view already did."}} {"type":"config","name":"ida-tui performance (v7 bench: rename covered too)","metricName":"total_ms","metricUnit":"ms","bestDirection":"lower"} {"run":31,"commit":"7b8c37a","metric":33242.6,"metrics":{"lg_boot_ms":710.8,"lg_decomp_ms":2401.2,"lg_graph_ms":898.1,"lg_hex_ms":448.7,"lg_index_ms":72.1,"lg_listing_cold_ms":439.3,"lg_listing_warm_ms":426.8,"lg_nav_ms":7060.2,"lg_palette_ms":4.8,"lg_rename_ms":10055,"lg_render_ms":220.2,"lg_search_ms":774.6,"lg_split_ms":3539.8,"pure_graph_ms":215.1,"sm_boot_ms":429.1,"sm_decomp_ms":1244.1,"sm_graph_ms":744.3,"sm_hex_ms":444.2,"sm_index_ms":2.5,"sm_listing_cold_ms":269.8,"sm_listing_warm_ms":263.3,"sm_nav_ms":282.9,"sm_palette_ms":0.3,"sm_rename_ms":638.3,"sm_render_ms":255.6,"sm_search_ms":44.2,"sm_split_ms":1357.4,"fails":0},"status":"keep","description":"RE-BASELINE (v7 bench). Rename — the commonest operation in reverse engineering — was not covered, and it costs 10.1s for SIX renames on bash (1.7s each) because bump_names discards the segment's ListingModel and the reload re-walks it from the start. lg_split also rose to 3540ms: the split phase now runs after renames have thrown the listing away.","timestamp":1786073435987,"segment":6,"confidence":null,"asi":{"hypothesis":"keep probing unbenched features -- the last two probes each found a 10x","finding":"rename costs 1.7s per rename on bash (10.1s for six) and 106ms on echo. Program.bump_names() clears _listings, so the reload builds an empty ListingModel and ensure_ea walks the segment from its start to find the row the cursor was already on.","the_irony":"idatui/edit_ctl.py already documents that a rename cannot move a row: 'a rename or comment doesn't change how many rows anything takes' -- it restores the cursor by INDEX afterwards. It just throws away the walk that gives the index meaning.","fix_ready_and_verified":".auto/wip-renamekeep.patch. ListingModel.invalidate_text() keeps the walk and marks the rendered text stale; _ensure_text re-renders a 500-head block at a time, snapped out to whole ADDRESS groups (a function start emits three banner rows at the same ea, so an unsnapped block boundary refetches the group and never lines up). If a refetch does come back with a different head sequence it sets stale_structure and Program.listing() rebuilds, so a mis-routed structural edit degrades to today's behaviour instead of showing stale names.","measured":"bash, cursor at row 220036 of 228659: viewport back in 10.3ms instead of 6320ms (593x). A FULL re-read of every row is break-even with a rebuild (6.8s vs 6.9s), which is the right shape -- refreshing N heads costs what loading N heads costs.","equivalence":"/tmp/renameeq.py renames a function, snapshots every row (ea, kind, text, name) from the kept model, then rebuilds from scratch and compares: 0 mismatches over 5952 rows (echo), 28807 (ls_ttl) and 228659 (bash), three renames each.","one_open_question":"one full-suite run showed a follow_xrefs failure with the patch in; it passed 10/10 in isolation afterwards and 6/7 full runs green vs 6/6 without. Watch it.","next_action_hint":"apply the patch"}} +{"run":32,"commit":"44c311a","metric":25563.7,"metrics":{"lg_boot_ms":732.6,"lg_decomp_ms":2714,"lg_graph_ms":919.3,"lg_hex_ms":566.8,"lg_index_ms":71.4,"lg_listing_cold_ms":441.4,"lg_listing_warm_ms":411.7,"lg_nav_ms":6708,"lg_palette_ms":4.8,"lg_rename_ms":741.5,"lg_render_ms":227.7,"lg_search_ms":3314.1,"lg_split_ms":2596.9,"pure_graph_ms":213.6,"sm_boot_ms":422.9,"sm_decomp_ms":1284,"sm_graph_ms":789.5,"sm_hex_ms":468.1,"sm_index_ms":2.6,"sm_listing_cold_ms":258.7,"sm_listing_warm_ms":257.3,"sm_nav_ms":306.5,"sm_palette_ms":0.3,"sm_rename_ms":388.2,"sm_render_ms":256,"sm_search_ms":105.1,"sm_split_ms":1360.5,"fails":0},"status":"keep","description":"A rename keeps the listing's walk instead of throwing it away. bump_names now marks the rendered text stale (invalidate_text) and ListingModel re-renders a 500-head block at a time on demand, snapped out to whole address groups; a refetch that comes back with a different head sequence sets stale_structure so Program.listing() rebuilds. lg_rename 10055 -> 742.","timestamp":1786073731826,"segment":6,"confidence":null,"asi":{"gains":"total 33243 -> 25564 (-23.1%); lg_rename 10055 -> 742 (-93%); sm_rename 638 -> 388; lg_split 3540 -> 2597 (the split phase runs after the renames and no longer inherits a discarded listing)","honest_tradeoff":"lg_search 775 -> 3314. The cost did not vanish, it moved: the six renames used to pay for a full rebuild each (10s), and search then found a warm model. Now renames are ~120ms and the first WHOLE-SEGMENT search afterwards pays to re-render the blocks it reads. Net -7.7s, and the cost only lands if you search the entire segment right after renaming. A full re-read is break-even with a rebuild by design -- re-rendering N heads costs what loading N heads costs.","work_unchanged":"rename_ok 6/6 both targets, search_hits 91783, split_mapped_lines 2070, graph_blocks 1000, decomp_ok 12 -- all identical to the baseline","design_notes":["blocks are snapped out to whole ADDRESS groups: a function start emits three banner rows at the same ea, so an unsnapped boundary refetches the group, never lines up, and would leave stale names forever","on a sequence mismatch the model sets stale_structure and Program.listing() rebuilds -- degrading to the old behaviour rather than showing an old name","_renamed is a boolean gate so that before the first rename every read takes exactly the path it always did, with no extra lock round trips"],"equivalence":"/tmp/renameeq.py: rename, snapshot every row (ea, kind, text, name) from the kept model, rebuild from scratch, compare. 0 mismatches over 5952 rows (echo), 28807 (ls_ttl), 228659 (bash), three renames each. Full 830-check gate green.","viewport_case":"bash with the cursor at row 220036: the listing is back in 10.3ms instead of 6320ms","next_action_hint":"lg_nav 6708 and lg_search 3314 are the top two. Search after a rename could refresh in PAGE-sized chunks driven from load_all rather than block-by-block from window()."}} +{"run":33,"commit":"44c311a","metric":27571.1,"metrics":{"lg_boot_ms":714.2,"lg_decomp_ms":2426.5,"lg_graph_ms":914.1,"lg_hex_ms":436.7,"lg_index_ms":109.8,"lg_listing_cold_ms":772.1,"lg_listing_warm_ms":437.2,"lg_nav_ms":6876.9,"lg_palette_ms":4.7,"lg_rename_ms":732.9,"lg_render_ms":224.7,"lg_search_ms":1209,"lg_split_ms":6623.7,"pure_graph_ms":214.1,"sm_boot_ms":463.8,"sm_decomp_ms":1282.5,"sm_graph_ms":755,"sm_hex_ms":444.3,"sm_index_ms":2.3,"sm_listing_cold_ms":268.7,"sm_listing_warm_ms":269.4,"sm_nav_ms":307.9,"sm_palette_ms":0.3,"sm_rename_ms":384.3,"sm_render_ms":260.2,"sm_search_ms":67.7,"sm_split_ms":1368.2,"fails":0},"status":"discard","description":"Chunk the post-rename text refresh into TEXT_BLOCK pieces instead of one call for the whole requested range (a search window asks for thousands of rows and the heads tool caps a response at 2000, so the oversized call came back short, failed the sequence check and condemned the model to a rebuild). Fixes lg_search 3314 -> 1209, but lg_split 2597 -> 6624 and total 25564 -> 27571: doing it properly is SLOWER here than the accidental rebuild was.","timestamp":1786074100440,"segment":6,"confidence":3.8252964033077643,"asi":{"hypothesis":"one heads call for a whole search-sized window overflows the tool's 2000-row cap, so the refresh always failed its sequence check and forced a rebuild","hypothesis_confirmed":"yes -- chunking removed the spurious rebuild and lg_search fell 3314 -> 1209","but":"lg_split rose 2597 -> 6624. The accidental rebuild was CHEAPER overall than refreshing block by block, because this bench reads most of the segment after renaming and 450 block calls cost more than one linear rebuild.","UNEXPLAINED_AND_MUST_BE_RESOLVED":"work counters moved between the two runs: lg_decomp_lines 3436 -> 3233, lg_split_mapped_lines 2070 -> 1990, lg_search_hits 91783 -> 92733. Same database (targets/bash.i64 mtime unchanged, staged fresh per run), same fixed function set. Something about which path runs is changing what the app SEES. That has to be understood before any version of this is kept -- a perf change must not alter observable work.","leads":["phase_rename renames six functions and undoes them; a function whose original name was auto-generated (sub_X) comes back as a USER name sub_X. Check whether that changes the listing (a label row, or is_auto_name affecting annotate).","the search terms are 'mov' and 'call'; the temporary name _bench_<pid>_<k> contains a 'c', so leftover names would inflate a 'c'-prefixed search -- but the terms are full words, so check for residue directly.","lg_listing_rows is a streamed-progress reading, not a work counter -- ignore that one."],"work_preserved":".auto/wip-chunk.patch","next_action_hint":"resolve the counter drift first. Then, if the chunking is kept, try TEXT_BLOCK = 2000 (the tool's cap) so a wholesale refresh costs about what a rebuild costs while a viewport still needs one call."}} diff --git a/.auto/wip-chunk.patch b/.auto/wip-chunk.patch new file mode 100644 index 0000000..4e03aa7 --- /dev/null +++ b/.auto/wip-chunk.patch @@ -0,0 +1,53 @@ +diff --git a/idatui/domain.py b/idatui/domain.py +index 386b3e8..1f8263d 100644 +--- a/idatui/domain.py ++++ b/idatui/domain.py +@@ -919,24 +919,37 @@ class ListingModel: + def _ensure_text(self, j0: int, j1: int) -> None: + """Re-render physical heads [j0, j1) if a rename staled them. + +- The block is snapped out to whole ADDRESS groups. A function start emits +- three banner rows and its code row at the same ea, and a labelled +- instruction emits two -- so a block boundary that fell inside one of +- those groups would refetch the whole group and never line up again. ++ Done a block at a time. One call for the whole range would be simpler but ++ the ``heads`` tool caps a response at 2000 rows, so a wide request (the ++ search body asks for thousands at once) would come back short, fail the ++ sequence check, and condemn the model to a rebuild it did not need. ++ """ ++ blk = self.TEXT_BLOCK ++ with self._lock: ++ n = len(self._heads) ++ start = (max(j0, 0) // blk) * blk ++ while start < min(j1, n): ++ self._ensure_text_block(start, min(start + blk, n)) ++ start += blk ++ ++ def _ensure_text_block(self, j0: int, j1: int) -> None: ++ """Re-render one block, snapped out to whole ADDRESS groups. ++ ++ A function start emits three banner rows and its code row at the same ea, ++ and a labelled instruction emits two -- so a boundary falling inside one ++ of those groups would refetch the whole group, never line up, and leave ++ the old names on screen for good. + """ + with self._lock: + gen = self._text_gen + n = len(self._heads) +- j0 = max(j0, 0) +- j1 = min(j1, n) +- if j1 <= j0: ++ a = max(j0, 0) ++ b = min(j1, n) ++ if b <= a: + return + head_gen = self._head_gen +- if all(head_gen[j] == gen for j in range(j0, j1)): ++ if all(head_gen[j] == gen for j in range(a, b)): + return +- blk = self.TEXT_BLOCK +- a = (j0 // blk) * blk +- b = min(((j1 - 1) // blk + 1) * blk, n) + eas = self._head_eas + while a > 0 and eas[a - 1] == eas[a]: + a -= 1 diff --git a/idatui/domain.py b/idatui/domain.py index 386b3e8..1f8263d 100644 --- a/idatui/domain.py +++ b/idatui/domain.py @@ -919,24 +919,37 @@ class ListingModel: def _ensure_text(self, j0: int, j1: int) -> None: """Re-render physical heads [j0, j1) if a rename staled them. - The block is snapped out to whole ADDRESS groups. A function start emits - three banner rows and its code row at the same ea, and a labelled - instruction emits two -- so a block boundary that fell inside one of - those groups would refetch the whole group and never line up again. + Done a block at a time. One call for the whole range would be simpler but + the ``heads`` tool caps a response at 2000 rows, so a wide request (the + search body asks for thousands at once) would come back short, fail the + sequence check, and condemn the model to a rebuild it did not need. + """ + blk = self.TEXT_BLOCK + with self._lock: + n = len(self._heads) + start = (max(j0, 0) // blk) * blk + while start < min(j1, n): + self._ensure_text_block(start, min(start + blk, n)) + start += blk + + def _ensure_text_block(self, j0: int, j1: int) -> None: + """Re-render one block, snapped out to whole ADDRESS groups. + + A function start emits three banner rows and its code row at the same ea, + and a labelled instruction emits two -- so a boundary falling inside one + of those groups would refetch the whole group, never line up, and leave + the old names on screen for good. """ with self._lock: gen = self._text_gen n = len(self._heads) - j0 = max(j0, 0) - j1 = min(j1, n) - if j1 <= j0: + a = max(j0, 0) + b = min(j1, n) + if b <= a: return head_gen = self._head_gen - if all(head_gen[j] == gen for j in range(j0, j1)): + if all(head_gen[j] == gen for j in range(a, b)): return - blk = self.TEXT_BLOCK - a = (j0 // blk) * blk - b = min(((j1 - 1) // blk + 1) * blk, n) eas = self._head_eas while a > 0 and eas[a - 1] == eas[a]: a -= 1 |