diff options
Diffstat (limited to 'TODO')
| -rw-r--r-- | TODO | 74 |
1 files changed, 60 insertions, 14 deletions
@@ -72,21 +72,67 @@ done-ish: -## The scenario suite mutates a PERSISTENT database +## Test hygiene (fixed, worth remembering) -tests/test_scenarios.py runs against targets/echo.i64 and every edit it makes is -saved there. A scenario that undefines an instruction leaves that instruction -undefined for every later run — decomp_follow_self started failing "for no -reason" and stayed failing until the .i64 was deleted and re-analysed. +tests/test_scenarios.py used to run against targets/echo.i64 and IDA saved every +edit it made, so each run inherited the previous run's damage. It cost real time +twice: decomp_follow_self "started failing" with no code change (an earlier +scenario had undefined an instruction), and an edit-position check looked flaky +one run in three, which nearly got written up as an async race. -That also poisoned an investigation: an `edit_keeps_view` check looked flaky -(cursor jumping 0x20c6 -> 0x2094 about one run in three) and was almost -certainly the database drifting between runs, not a race. Any conclusion drawn -from repeated runs of a mutating scenario is suspect. +Now: the suite copies the binary into a temp dir and seeds it from a golden +database (<target>.pristine.i64, built once, never written back). Every run +starts from identical bytes and the tracked target is never touched. -Worth fixing properly: either give the suite a scratch copy of the binary per -run, or have mutating scenarios undo themselves. Until then, `rm targets/*.i64` -before trusting a failure that appeared without a code change. +The general rule this came from: a suite whose result depends on its own history +can't be trusted to accuse the code. tests/test_blob_ui.py builds a throwaway +binary; test_project_ui.py stages copies; test_thumb_ui.py deletes the .i64 +before each phase because the T flag and segment bitness are SAVED in it. -Coverage for "an edit must not move the view" lives in tests/test_blob_ui.py, -which builds its own throwaway binary and can mutate freely. +## decomp_map was NOT the problem (corrected) + +I recorded here that decomp_map returned four entries for cat's main and blamed +the tool. It doesn't: called directly it returns 769 lines, 475 with addresses, +for exactly that function. The four-line map belonged to a PLT stub the +decompiler had momentarily switched to, sampled mid-bounce. + +The real fault was the resync decision in _seek_split using _split_range, which +is maintained by a guarded async path and lags. A stale range made every step +look like a function change, so the decompiler thrashed +(main -> stub -> main), each bounce paying a synchronous 769-line map fetch on +the UI thread. Fixed by deciding from the map the trail painting already holds, +which is keyed to what the decompiler currently HAS loaded. + +Still true and worth knowing: the decompiler attributes only about half of a +function's instructions to a line, so the pseudocode cursor moves on those and +waits on the rest. The tempting fallback — nearest mapped address at or before +the pc — is UNSOUND: C lines are not monotonic in address, and it resolved an +instruction early in main to a line near the end of the function. + +FIXED: the split view and the trace path used to keep two parallel maps of the +same thing, fetched separately and keyed differently — the split one on _cur (the +cursor's function), the trace one on the decompiler's loaded function. That is +how they ended up describing different functions. There is now one index, keyed +to what the decompiler HOLDS, and both read it. + +## Stale navigations (fixed for seeks; general case left alone) + +A navigation runs in a worker and its result is applied when it lands. The trace's +OPENING seek goes to t=0, which for a normal binary is _start, and that +navigation is slow — so it used to arrive after later seeks and drag the cursor +back to _start while the trace was elsewhere. It never settled (measured stable +for 3+ seconds), and anything cursor-based done just after a seek then acted on +the wrong address. + +The listing path had no staleness guard at all; the decompiler path got one in +756589a. Fixed by giving the listing completion the same check +(_open_at_if_current) and bumping _nav_seq on each SEEK. + +Deliberately NOT bumped in _goto_ea for every navigation. That is the more +general rule — "the last thing you asked for wins" — and I tried it, but it also +means an ordinary follow can be dropped by whatever navigates next, and a full +suite run turned up a follow_xrefs failure with it in place (the same check has +flaked before, so it is not proof, but the mechanism is real and the evidence I +have is only about seeks). If rapid follows ever show the same drift, the general +bump is the fix — with a test that a follow in flight survives an unrelated +navigation. |