diff --git a/docs/brainstorms/2026-07-30-relocation-evidence-generalization-requirements.md b/docs/brainstorms/2026-07-30-relocation-evidence-generalization-requirements.md new file mode 100644 index 00000000..d13b2a14 --- /dev/null +++ b/docs/brainstorms/2026-07-30-relocation-evidence-generalization-requirements.md @@ -0,0 +1,40 @@ +--- +date: 2026-07-30 +topic: relocation-evidence-generalization +--- + +# Generalize target-side relocation evidence across mechanical rule generators + +## Summary + +Generalize the relocation-evidence fix shipped in PR #149 (`inc_abs_global`) to every other mechanical rule generator in `src/agentdecompile_recovery/source_parity_synthesize.py` with the same gap, via a shared helper plus a regression test that prevents the pattern from recurring in future rules. + +## Problem Frame + +`render_target_coff_for_candidate()` can reconstruct the objdiff target object with a matching symbol relocation for absolute-address (`DIR32`) references, via `absolute_address_relocations()`, which reads `candidate.evidence["absoluteAddressRelocations"]`. Without it, the target side falls back to a raw byte blob with the address baked in literally, and any candidate that correctly references the same global through a compiler-visible symbol can never byte-match — confirmed empirically (PR #149) against the real MSVC8/wine toolchain and real objdiff. + +`inc_abs_global` had this gap and is now fixed. A scan for the same shape (a rule generator embedding a raw absolute address in generated C source without populating `absoluteAddressRelocations`) found roughly twenty more rule functions with the identical pattern. Some of these rules validate a single fixed-offset address the same way `inc_abs_global` did (e.g. `float_multiply_global`); others may reference multiple addresses or non-global values the initial heuristic scan can't distinguish without closer reading per function. Left unfixed, any function in a real recovery run whose target-matching rule falls into this class faces the same unclosable ceiling `inc_abs_global` did — a candidate can be semantically and structurally perfect and still never reach `objdiff differences: 0`. + +## Requirements + +- R1. Every mechanical rule generator that embeds an absolute global address in its generated C source populates `absoluteAddressRelocations` in its `GeneratedCandidate.evidence`, following the same `{offset, type: "IMAGE_REL_I386_DIR32", symbol, decodedAddress}` shape `absolute_address_relocations()` already reads. +- R2. A shared helper covers the common single-address case so each affected rule adopts the fix in one line rather than a hand-rolled dict, mirroring the naming convention already established (`_DAT_`-style symbols). +- R3. Rules with more than one absolute-address reference (following the `bink_buffer_set_direct_draw_forwarder` shape) are verified individually rather than assumed to fit the single-address helper. +- R4. A regression test scans rule generators for the anti-pattern (a literal hex address baked into generated C source with no matching relocation evidence) so a future new rule cannot reintroduce this gap silently. +- R5. Each fixed rule is verified against the real MSVC8/wine toolchain and real objdiff for at least one representative byte pattern, not just a unit-level assertion on the evidence shape (the same standard PR #149 held itself to). + +## Scope Boundaries + +- Does not change the objdiff verification harness itself (`run_objdiff`, `parse_objdiff_report`) — PR #149 established that harness is correct once given matching relocation evidence. +- Does not attempt to fix the separate, already-documented "remaining 64" candidate-generation-quality backlog item (Ghidra-synthesized type names, embedded `int3` artifacts) — that is a distinct class of bug. +- Does not run a full proof-campaign or vacuum loop against `swkotor.exe` to chase new verified accepts — that is a separate, later effort this fix unblocks but does not itself perform. + +## Dependencies / Assumptions + +- Assumes the exact set of affected rule generators is confirmed by reading each flagged function during planning/implementation, not taken as a fixed count from the initial heuristic scan. +- Assumes the real MSVC8/wine toolchain used to verify PR #149 (`VC_ROOT=/run/media/brunner56/MyBook/Toolchains/msvc8.0-main`, `WINEPREFIX=target/wine-smoke-prefix`) remains available for per-rule verification. + +## Outstanding Questions + +**Deferred to Planning** +- Whether the regression test (R4) should be a static scan over rule-generator source text, or a data-driven check that calls every rule generator with a representative byte pattern and inspects its returned `evidence` — the right mechanism depends on how uniformly testable the ~20 rules turn out to be once read individually. diff --git a/docs/plans/2026-07-30-001-fix-generalize-relocation-evidence-plan.md b/docs/plans/2026-07-30-001-fix-generalize-relocation-evidence-plan.md new file mode 100644 index 00000000..6810fd22 --- /dev/null +++ b/docs/plans/2026-07-30-001-fix-generalize-relocation-evidence-plan.md @@ -0,0 +1,136 @@ +--- +title: "fix: Generalize target-side relocation evidence across mechanical rule generators" +date: 2026-07-30 +origin: docs/brainstorms/2026-07-30-relocation-evidence-generalization-requirements.md +--- + +# fix: Generalize target-side relocation evidence across mechanical rule generators + +## Summary + +Investigated extending the `absoluteAddressRelocations` fix shipped in PR #149 (`inc_abs_global`) to twelve other mechanical rule generators in `src/agentdecompile_recovery/source_parity_synthesize.py`. Real-toolchain A/B testing overturned the premise: none of the twelve need the fix, and adding it made two confirmed cases measurably worse. Shipped a shared helper (for any future rule that legitimately needs it), reverted the incorrect wiring, and added a regression test guarding the real anti-pattern this investigation found. + +## Problem Frame + +`render_target_coff_for_candidate()` reconstructs the objdiff target object with a matching symbol relocation for absolute-address (`DIR32`) references, via `absolute_address_relocations()`, which reads `candidate.evidence["absoluteAddressRelocations"]`. This only helps when the candidate's own compiled object *also* references the address through a compiler-emitted relocation — a named `extern` symbol, as in a packaged-source candidate's `DAT_00830540 = DAT_00830540 + 1;`, or a subagent-produced inline-asm rewrite referencing a named symbol. + +`inc_abs_global` was fixed in PR #149 on that premise. A full read of every other rule generator embedding a raw absolute address found twelve more candidates for the same fix: ten single-address, two multi-address. Real-toolchain A/B testing (real MSVC8/wine compile + real objdiff, not just unit-level shape assertions) overturned that premise for all twelve: every one of them synthesizes a raw C **literal pointer cast** (`*(unsigned int *)0x...`), not a named-symbol reference — and MSVC compiles a literal cast as a bare immediate with no relocation on the candidate side at all. Reconstructing a *symbolic* target against a candidate that stays *literal* does not help; it was confirmed to introduce spurious `ARGUMENT_MISMATCH` entries that don't exist without the evidence (`global_and_global_bool`: 2 → 6 on the best profile; one of the ten single-address rules showed the same pattern). A/B testing `inc_abs_global`'s own already-shipped fix against its own literal-cast candidate found it neutral (identical mismatch counts with and without the evidence) — its value comes entirely from making the evidence *available* for a later, differently-constructed candidate (e.g. a subagent rewrite) to match against, not from helping its own candidate. + +## Requirements + +- R1. (Superseded — see Key Technical Decisions.) Originally: each of the ten single-address rule generators populates `absoluteAddressRelocations`. Real-toolchain testing found none of the ten need it; none were changed. +- R2. A shared helper (`single_absolute_address_relocation`) produces the `absoluteAddressRelocations` list for the single-address case, available for any rule that genuinely needs it (a candidate referencing the address through a named symbol, not a literal cast), using the same `_DAT_`-style symbol naming `inc_abs_global` established. +- R3. (Superseded — see Key Technical Decisions.) Originally: `rep_stos_global_clear` and `global_and_global_bool` populate `absoluteAddressRelocations`. Real-toolchain testing found neither needs it: `rep_stos_global_clear` already reaches `differences: 0` via literal `_emit` bytes; `global_and_global_bool`'s literal-cast candidate was measurably worse with the evidence added. Neither was changed. +- R4. A regression test guards the *actual* anti-pattern this investigation found: a rule generator populating `absoluteAddressRelocations` while its own generated source only references that address via a literal pointer cast (confirmed to make matching worse, not better). +- R5. (Superseded.) Originally called for real-toolchain verification of at least three fixed rules. Real-toolchain verification did run — on `float_multiply_global`, `global_and_global_bool`, and `inc_abs_global` itself — but as A/B comparisons proving the fix should *not* be applied, not as confirmation of a successful fix. + +## Key Technical Decisions + +- **The premise was wrong; real-toolchain A/B testing found this during implementation, not planning.** `absoluteAddressRelocations` only helps when the candidate's own compiled object contains a matching relocation. Every one of the twelve rule generators in scope synthesizes a literal pointer cast, which MSVC compiles as a bare immediate — no relocation exists on the candidate side for the evidence to usefully mirror. Confirmed by A/B testing three representative cases against the real MSVC8/wine toolchain and real objdiff: + - `float_multiply_global`: unfixed best-profile histogram `{ARGUMENT_MISMATCH: 2, INSERTION: 14}`; with the evidence added, `{ARGUMENT_MISMATCH: 4, INSERTION: 14}` — worse. + - `global_and_global_bool`: unfixed `{ARGUMENT_MISMATCH: 2, INSERTION: 4}`; with the evidence added, `{ARGUMENT_MISMATCH: 6, INSERTION: 4}` — worse. + - `inc_abs_global` (PR #149's own shipped fix): identical histograms with and without the evidence on its own candidate — neutral. Its value is that the evidence field is now *available* on that `GeneratedCandidate` for a differently-constructed candidate for the same target (e.g. a subagent rewrite referencing a named `DAT_` symbol) to carry matching evidence of its own — demonstrated live in this session by combining a subagent's inline-asm rewrite (`inc dword ptr [DAT_00830540]`) with hand-attached relocation evidence, reaching `differences: 0`. + - `rep_stos_global_clear`'s only returned candidate embeds every address as literal `_emit` bytes, not a pointer dereference at all — a different reason the evidence doesn't apply, verified to already reach `differences: 0` with no change. +- **U2's ten-rule wiring and U3's `global_and_global_bool` wiring were implemented, real-toolchain-tested, found to regress matching, and reverted** (see git history on this branch: commits wiring the fix, followed by a revert commit and a documentation commit explaining why). The shared helper (U1) was kept — it's correct and reusable for any future rule that legitimately references an address through a named symbol. +- **No changes to the objdiff verification harness itself.** `run_objdiff` and `render_target_coff_for_candidate` are correct as-is; this investigation was about which candidates should populate evidence for them, not about the harness. + +## Scope Boundaries + +- Does not touch `run_objdiff`, `parse_objdiff_report`, or `render_target_coff_for_candidate` — confirmed correct. +- Does not attempt the separate, already-documented "remaining 64" candidate-generation-quality backlog item (Ghidra-synthesized type names, embedded `int3` artifacts) — a distinct class of bug. +- Does not run a proof-campaign or vacuum loop against `swkotor.exe` to chase new verified accepts. + +### Deferred to Follow-Up Work + +- **The real generalization opportunity this investigation surfaced**: relocation evidence should be inherited or computed automatically wherever a subagent-rewrite or packaged-source candidate is constructed for a near-miss target that references an absolute address — not attached per mechanical rule generator. This session's subagent-rewrite proof required hand-attaching the evidence to the rewrite candidate; whether `pending_rewrite_variant()` (or wherever mechanism-3 candidates are actually constructed in the production path) does this automatically was not verified and is out of scope here. +- Any additional rule generators beyond the twelve investigated here that may embed absolute addresses in less obvious forms — the regression test (U4) is a static-source-text scan and may not catch every future variant. + +## Implementation Units + +### U1. Shared single-address relocation-evidence helper + +**Goal:** Add a helper that produces the `absoluteAddressRelocations` list for the common single-address case, matching the shape `inc_abs_global` already emits. + +**Requirements:** R2 + +**Dependencies:** None + +**Files:** +- Modify: `src/agentdecompile_recovery/source_parity_synthesize.py` (add helper near `inc_abs_global`, e.g. adjacent to line 573) +- Test: `tests/test_relocation_evidence_helper.py` + +**Approach:** A small function taking `(offset: int, addr: int)` (or `(offset, addr, symbol_prefix)` if a rule needs a non-`DAT_` symbol convention) and returning the single-entry `absoluteAddressRelocations` list, mirroring the literal `inc_abs_global` currently constructs inline. `inc_abs_global` itself is not required to switch to the helper (it already works and is out of scope for behavior change), but new callers use it. + +**Patterns to follow:** `inc_abs_global` (`src/agentdecompile_recovery/source_parity_synthesize.py:573`) for the exact shape and the `_DAT_` symbol convention. + +**Test scenarios:** +- Happy path: helper called with a representative offset/address returns a one-entry list with `type: "IMAGE_REL_I386_DIR32"`, correct `offset`, `symbol` formatted as `_DAT_`, and `decodedAddress` formatted as `0x`. +- Edge case: address value with leading zero bytes (e.g. `0x00830540`) formats consistently with the existing `inc_abs_global` convention. + +**Verification:** Unit tests pass; output shape matches what `absolute_address_relocations()` (`:21725`) already reads without modification. + +--- + +### U2. Investigate the ten single-address rules (superseded — reverted) + +**Goal:** Originally: wire the U1 helper into each of the ten confirmed single-address rule generators. **Actual outcome:** implemented, real-toolchain-verified, found to regress matching for the tested representative (`float_multiply_global`), and reverted for all ten. + +**Requirements:** R1 (superseded) + +**Dependencies:** U1 + +**Files:** +- No net change to `src/agentdecompile_recovery/source_parity_synthesize.py` (wired, then reverted via `git revert`) +- `tests/test_rule_generator_relocation_evidence.py` was added, then removed by the revert + +**Finding:** All ten rules synthesize a raw literal pointer cast (`*(type *)0x{addr:08x}`), which MSVC compiles as a bare immediate with no relocation. Adding target-side `absoluteAddressRelocations` against a literal candidate does not help. A/B tested for `float_multiply_global` against the real MSVC8/wine toolchain: unfixed best-profile histogram `{ARGUMENT_MISMATCH: 2, INSERTION: 14}` vs. fixed `{ARGUMENT_MISMATCH: 4, INSERTION: 14}` — worse, not better. + +**Verification:** `git log` on this branch shows the wiring commit followed by a revert commit; the ten rules are unchanged from their pre-plan state. + +--- + +### U3. Investigate the two multi-address rules (superseded — one reverted, one never applicable) + +**Goal:** Originally: wire per-relocation evidence into `rep_stos_global_clear` and `global_and_global_bool`. **Actual outcome:** neither needed the fix. + +**Requirements:** R3 (superseded) + +**Dependencies:** None + +**Files:** +- No net change to `src/agentdecompile_recovery/source_parity_synthesize.py` +- `tests/test_multi_address_rule_relocation_evidence.py` documents the finding for both rules + +**Finding:** +- `rep_stos_global_clear`'s only returned candidate embeds every address as literal bytes via inline-asm `_emit` directives, not a pointer dereference — verified against the real toolchain to already reach `differences: 0` with no change. +- `global_and_global_bool` dereferences two addresses via literal casts, same as U2's rules. A/B tested: unfixed `{ARGUMENT_MISMATCH: 2, INSERTION: 4}` vs. fixed `{ARGUMENT_MISMATCH: 6, INSERTION: 4}` — worse. Wired, then reverted before commit. + +**Verification:** `tests/test_multi_address_rule_relocation_evidence.py` asserts the current (correct, unchanged) shape for `global_and_global_bool` — no `absoluteAddressRelocations` in its evidence. + +--- + +### U4. Regression test guarding the real anti-pattern (scope corrected) + +**Goal:** Originally: guard against a rule generator embedding an address with no relocation evidence. **Corrected goal** (per the U2/U3 findings): guard against a rule generator pairing `absoluteAddressRelocations` with a literal pointer cast — the actual anti-pattern this investigation found and fixed. + +**Requirements:** R4 + +**Dependencies:** U2, U3 (their findings are what U4 guards) + +**Files:** +- Test: `tests/test_no_unrelocated_absolute_addresses.py` + +**Approach:** An AST scan over `source_parity_synthesize.py` for rule-generator functions (matching the `(row, c_name, data)` parameter signature) whose body both sets `absoluteAddressRelocations` in evidence and contains a literal-cast pattern (`*(type *)0x{addr}`-shaped) referencing an address. `inc_abs_global` is exempted — its own literal-cast candidate is neutral (not harmful) per direct A/B testing, and the evidence field serves a different, later-constructed candidate rather than its own. + +**Patterns to follow:** None directly — a new kind of test for this codebase. + +**Test scenarios:** +- Happy path: the scan against the current, corrected state of `source_parity_synthesize.py` finds zero violations. +- Failure-path (regression guard): a synthetic in-test function shaped exactly like the mistake this test exists to prevent (literal cast + relocation evidence) is detected by the scan. + +**Verification:** Both tests pass; the synthetic-violation test confirms the check has teeth, not that it passes vacuously. + +## Dependencies / Assumptions + +- Assumes the real MSVC8/wine toolchain used to verify PR #149 (`VC_ROOT=/run/media/brunner56/MyBook/Toolchains/msvc8.0-main`, `WINEPREFIX=target/wine-smoke-prefix`) remains available for the R5 sample verification. +- Assumes no other rule generators beyond these twelve reference absolute addresses in a form the manual read missed (see Deferred to Follow-Up Work). diff --git a/src/agentdecompile_recovery/source_parity_synthesize.py b/src/agentdecompile_recovery/source_parity_synthesize.py index ab4541a4..960ff607 100755 --- a/src/agentdecompile_recovery/source_parity_synthesize.py +++ b/src/agentdecompile_recovery/source_parity_synthesize.py @@ -24,7 +24,6 @@ from typing import Any, Iterable from .package_verify import build_shim, compile_with_msvc -from .state import now ROOT = Path.cwd() DEFAULT_VC_ROOT: Path | None = None @@ -629,6 +628,36 @@ def inc_abs_global(row: dict[str, Any], c_name: str, data: bytes) -> list[Genera ] +def single_absolute_address_relocation(offset: int, addr: int) -> list[dict[str, Any]]: + """Return the absoluteAddressRelocations shape for one absolute-address reference. + + Matches the target-side reconstruction absolute_address_relocations() reads + (render_target_coff_for_candidate) -- without this, the synthetic target + object renders the address as a raw byte blob instead of a symbol + relocation, and a candidate referencing the same global through a + compiler-visible symbol can never byte-match it. + + Do NOT call this for a candidate whose generated source only references + the address via a literal pointer cast (e.g. `*(unsigned int *)0x...`) -- + MSVC compiles a literal cast as a bare immediate with no relocation, so + there is nothing on the candidate side for this evidence to mirror. + Confirmed via real MSVC8/wine + objdiff A/B testing to make matching + measurably worse for literal-cast candidates (see + docs/plans/2026-07-30-001-fix-generalize-relocation-evidence-plan.md). + Only use this when the generated source references the address through a + named extern symbol, as bink_buffer_set_direct_draw_forwarder does. + """ + + return [ + { + "offset": offset, + "type": "IMAGE_REL_I386_DIR32", + "symbol": f"_DAT_{addr:08x}", + "decodedAddress": f"0x{addr:08x}", + } + ] + + def inc_field_return_stack4(row: dict[str, Any], c_name: str, data: bytes) -> list[GeneratedCandidate]: if len(data) != 10 or data[0] != 0x8B or data[1] != 0x41 or data[3] != 0x40 or data[4] != 0x89 or data[5] != 0x41: return [] diff --git a/tests/test_multi_address_rule_relocation_evidence.py b/tests/test_multi_address_rule_relocation_evidence.py new file mode 100644 index 00000000..d2baa5b2 --- /dev/null +++ b/tests/test_multi_address_rule_relocation_evidence.py @@ -0,0 +1,63 @@ +"""U3 of docs/plans/2026-07-30-001-fix-generalize-relocation-evidence-plan.md: +findings on the two multi-address rule generators the plan targeted. + +Real-toolchain verification (real MSVC8/wine compile + real objdiff, not just +unit-level evidence-shape assertions) overturned this plan's premise for both +rules -- see the plan's amended Key Technical Decisions for the full writeup. +Neither rule was changed; this file documents why, so the finding isn't lost +or rediscovered as an open gap. + +**The corrected understanding:** `render_target_coff_for_candidate()`'s +relocation reconstruction only helps when the candidate's OWN compiled object +also references the address through a compiler-emitted relocation (e.g. a +named `extern` symbol, as in a packaged-source candidate's +`DAT_00830540 = DAT_00830540 + 1;`, or a subagent's inline-asm rewrite +referencing a named symbol). Every rule generator in this codebase that +synthesizes a raw C literal pointer cast (`*(unsigned int *)0x...`) compiles +that address as a bare immediate with NO relocation on the candidate side -- +MSVC does not route a literal address cast through an extern symbol. Adding +`absoluteAddressRelocations` to reconstruct a *symbolic* target side against a +candidate that stays *literal* does not help; it was empirically confirmed to +introduce spurious `ARGUMENT_MISMATCH` entries that don't exist without the +evidence (verified for `global_and_global_bool` and, separately, for one of +the ten single-address rules this plan also reverted -- see git history on +this branch). + +- `global_and_global_bool` dereferences two absolute addresses via literal + casts. A/B tested against the real toolchain: WITHOUT relocation evidence, + the best profile shows `ARGUMENT_MISMATCH: 2`; WITH the evidence this plan + originally added, the same profile shows `ARGUMENT_MISMATCH: 6` -- strictly + worse. The evidence was reverted; no fix was needed or applied. +- `rep_stos_global_clear` was a **false positive** from a different angle: + its only returned candidate embeds every address as literal bytes via + inline-asm `_emit` directives, not any pointer dereference at all. Verified + directly against the real toolchain: it already reaches `differences: 0` + with no evidence change. +""" + +from __future__ import annotations + +from agentdecompile_recovery.source_parity_synthesize import global_and_global_bool + + +def test_global_and_global_bool_uses_literal_address_casts_not_relocations() -> None: + """Documents the current (correct, unchanged) shape: no + absoluteAddressRelocations, because the candidate source uses literal + pointer casts that a real MSVC compile bakes as bare immediates, not a + relocation an objdiff target reconstruction could usefully mirror. + """ + + left = 0x00830560 + right = 0x00830564 + data = ( + b"\xa1" + + left.to_bytes(4, "little") + + b"\x8b\x0d" + + right.to_bytes(4, "little") + + b"\x23\xc8\x3b\xc8\x0f\x94\xc0\xc3" + ) + candidates = global_and_global_bool({}, "FUN_test", data) + assert len(candidates) == 1 + assert "absoluteAddressRelocations" not in candidates[0].evidence + assert f"0x{left:08x}" in candidates[0].source + assert f"0x{right:08x}" in candidates[0].source diff --git a/tests/test_no_unrelocated_absolute_addresses.py b/tests/test_no_unrelocated_absolute_addresses.py new file mode 100644 index 00000000..d365316a --- /dev/null +++ b/tests/test_no_unrelocated_absolute_addresses.py @@ -0,0 +1,160 @@ +"""U4 of docs/plans/2026-07-30-001-fix-generalize-relocation-evidence-plan.md. + +**Scope correction (documented in the plan's amended Key Technical +Decisions):** the plan originally set out to guard against rule generators +that embed a raw absolute address in generated C source *without* populating +`absoluteAddressRelocations`. Real-toolchain investigation (real MSVC8/wine +compile + real objdiff) overturned that premise: `absoluteAddressRelocations` +only helps when the candidate's own compiled object references the address +through a compiler-emitted relocation (a named `extern` symbol) -- not a raw +literal pointer cast (`*(unsigned int *)0x...`), which MSVC compiles as a bare +immediate with no relocation. Adding the evidence to a literal-cast candidate +was A/B tested and found to make matching *worse* (spurious +`ARGUMENT_MISMATCH` entries), not better -- confirmed for +`global_and_global_bool` and for one of the ten single-address rules this +plan's U2 originally (and incorrectly) wired up, then reverted. + +This test guards the corrected, real anti-pattern instead: a rule generator +must never populate `absoluteAddressRelocations` for an address its own +generated source only references via a literal pointer cast. `inc_abs_global` +is exempt from this check -- its own literal-cast candidates are unaffected +either way (real-toolchain A/B testing showed identical results with and +without the evidence), and the evidence field exists there specifically so a +*different*, later-constructed candidate for the same target (e.g. a +subagent rewrite referencing a named `DAT_` symbol, per +docs/solutions/architecture-patterns/rewrite-queue-subagent-fulfillment.md) +can carry matching relocation evidence of its own. Extending that +inheritance to fire automatically for every packaged-source/rewrite +candidate is a distinct, deferred piece of work -- see the plan's Scope +Boundaries. +""" + +from __future__ import annotations + +import ast +import re +from pathlib import Path + +import pytest + +from agentdecompile_recovery.source_parity_synthesize import ( + BINK_BUFFER_SET_DIRECT_DRAW_FORWARDER, + bink_buffer_set_direct_draw_forwarder, +) + +SOURCE_PATH = Path(__file__).resolve().parent.parent / "src" / "agentdecompile_recovery" / "source_parity_synthesize.py" + +# Rules exempt from this check: their own literal-cast candidate is +# unaffected by the evidence (verified real-toolchain neutral, not harmful), +# and the evidence exists to serve a different, later-constructed candidate. +EXEMPT_RULE_FUNCTIONS = {"inc_abs_global"} + +LITERAL_CAST_RE = re.compile(r"\([^)]*\*\)\s*0x\{[a-zA-Z_]+") + + +def _iter_rule_generator_functions(tree: ast.Module) -> list[ast.FunctionDef]: + functions = [] + for node in ast.walk(tree): + if not isinstance(node, ast.FunctionDef): + continue + args = [a.arg for a in node.args.args] + if args[:3] == ["row", "c_name", "data"]: + functions.append(node) + return functions + + +def _function_source(source_lines: list[str], node: ast.FunctionDef) -> str: + return "\n".join(source_lines[node.lineno - 1 : node.end_lineno]) + + +def test_no_rule_generator_pairs_relocation_evidence_with_a_literal_cast() -> None: + text = SOURCE_PATH.read_text(encoding="utf-8") + tree = ast.parse(text) + source_lines = text.splitlines() + + violations = [] + for node in _iter_rule_generator_functions(tree): + if node.name in EXEMPT_RULE_FUNCTIONS: + continue + body = _function_source(source_lines, node) + if "absoluteAddressRelocations" not in body: + continue + if LITERAL_CAST_RE.search(body): + violations.append(node.name) + + assert violations == [], ( + f"Rule generator(s) {violations} populate absoluteAddressRelocations " + "while their generated source only references the address via a " + "literal pointer cast -- real-toolchain testing showed this makes " + "objdiff matching worse, not better (see this test's module " + "docstring). Either remove the evidence, or change the generated " + "source to reference the address through a named extern symbol." + ) + + +def test_legitimate_named_symbol_usage_is_not_flagged() -> None: + """Positive-path case: bink_buffer_set_direct_draw_forwarder is the one + pre-existing rule that genuinely needs absoluteAddressRelocations -- it + references its addresses through named extern symbols, not a literal + cast. Deliberately targets this function (rather than relying on the + whole-file scan's incidental pass) so the check's "allowed pattern" + branch has its own coverage, not just the negative case. + """ + + candidates = bink_buffer_set_direct_draw_forwarder({}, "FUN_test", BINK_BUFFER_SET_DIRECT_DRAW_FORWARDER) + assert len(candidates) == 2 + relocations = candidates[0].evidence.get("absoluteAddressRelocations") + assert relocations == [ + {"offset": 0x14, "type": "IMAGE_REL_I386_DIR32", "symbol": "_recovery_global_30068c6c", "decodedAddress": "0x30068c6c"}, + {"offset": 0x19, "type": "IMAGE_REL_I386_DIR32", "symbol": "_recovery_global_30068c70", "decodedAddress": "0x30068c70"}, + {"offset": 0x1F, "type": "IMAGE_REL_I386_DIR32", "symbol": "_recovery_global_30068c68", "decodedAddress": "0x30068c68"}, + {"offset": 0x36, "type": "IMAGE_REL_I386_DIR32", "symbol": "_recovery_global_30068c6c", "decodedAddress": "0x30068c6c"}, + {"offset": 0x3C, "type": "IMAGE_REL_I386_DIR32", "symbol": "_recovery_global_30068c70", "decodedAddress": "0x30068c70"}, + {"offset": 0x42, "type": "IMAGE_REL_I386_DIR32", "symbol": "_recovery_global_30068c68", "decodedAddress": "0x30068c68"}, + ] + + text = SOURCE_PATH.read_text(encoding="utf-8") + tree = ast.parse(text) + source_lines = text.splitlines() + functions = {node.name: node for node in _iter_rule_generator_functions(tree)} + body = _function_source(source_lines, functions["bink_buffer_set_direct_draw_forwarder"]) + assert "absoluteAddressRelocations" in body + assert LITERAL_CAST_RE.search(body) is None + + +_ANTI_PATTERN_SOURCE_LINES = { + "dereferenced-cast": ' source = f"*(unsigned int *)0x{addr:08x} = 1;"\n', + "indexed-store": ' source = f"((unsigned int *)0x{addr:08x})[index] = value;"\n', + "assign-then-deref": ( + ' source = f"unsigned int *slot = (unsigned int *)0x{addr:08x}; *slot = 1;"\n' + ), +} + + +@pytest.mark.parametrize("source_line", _ANTI_PATTERN_SOURCE_LINES.values(), ids=_ANTI_PATTERN_SOURCE_LINES.keys()) +def test_check_actually_detects_the_anti_pattern(source_line: str) -> None: + """Regression guard on the check itself: prove it fires on a synthetic + function shaped exactly like the mistake this test exists to prevent -- + covering all three literal-cast idioms LITERAL_CAST_RE was broadened to + catch (dereferenced cast, indexed-store, assign-then-deref), not just the + first one. Without this, a future narrowing of the regex back to only the + first idiom would pass every test in this file silently. + """ + + synthetic_source = ( + "def fake_rule(row: dict[str, Any], c_name: str, data: bytes) -> list[GeneratedCandidate]:\n" + " addr = u32(data[2:6])\n" + + source_line + + " return [GeneratedCandidate(\n" + ' rule="fake", variant="fake", c_name=c_name, symbol=c_name,\n' + " source=source, callconv=\"cdecl\", return_type=\"void\",\n" + ' evidence={"absoluteAddressRelocations": [{"offset": 2}]},\n' + " )]\n" + ) + tree = ast.parse(synthetic_source) + source_lines = synthetic_source.splitlines() + functions = _iter_rule_generator_functions(tree) + assert len(functions) == 1 + body = _function_source(source_lines, functions[0]) + assert "absoluteAddressRelocations" in body + assert LITERAL_CAST_RE.search(body) is not None diff --git a/tests/test_relocation_evidence_helper.py b/tests/test_relocation_evidence_helper.py new file mode 100644 index 00000000..8ec738e8 --- /dev/null +++ b/tests/test_relocation_evidence_helper.py @@ -0,0 +1,32 @@ +"""Tests for the shared single-address relocation-evidence helper. + +See src/agentdecompile_recovery/source_parity_synthesize.py's +single_absolute_address_relocation() and the inc_abs_global fix it +generalizes (PR #149) -- both exist to give render_target_coff_for_candidate() +a symbol relocation to reconstruct on the target side, instead of a raw byte +blob that a correctly-compiled candidate can never byte-match. +""" + +from __future__ import annotations + +from agentdecompile_recovery.source_parity_synthesize import single_absolute_address_relocation + + +def test_single_absolute_address_relocation_shape() -> None: + relocations = single_absolute_address_relocation(offset=6, addr=0x00830540) + + assert relocations == [ + { + "offset": 6, + "type": "IMAGE_REL_I386_DIR32", + "symbol": "_DAT_00830540", + "decodedAddress": "0x00830540", + } + ] + + +def test_single_absolute_address_relocation_leading_zero_address() -> None: + relocations = single_absolute_address_relocation(offset=0, addr=0x00001000) + + assert relocations[0]["symbol"] == "_DAT_00001000" + assert relocations[0]["decodedAddress"] == "0x00001000"