fix: inc_abs_global never wires target-side relocation evidence - #149
Merged
Conversation
render_target_coff_for_candidate() can reconstruct the objdiff target
side with a matching symbol relocation for absolute-address references
(absolute_address_relocations(), already used by
bink_buffer_set_direct_draw_forwarder), but inc_abs_global() set
evidence={"absoluteAddress": ...} instead of the
absoluteAddressRelocations shape the consumer actually reads. The
target side was always rendered as a raw byte blob with the address
baked in literally, so any correct candidate referencing the global
through a compiler-visible symbol relocation could never byte-match --
confirmed against the real MSVC8/wine toolchain on FUN_004a23b0 in
swkotor-parity-inv, where an instruction-for-instruction identical
inline-asm rewrite still reported DIFF_ARG_MISMATCH. With the fix,
the same candidate reaches objdiff differences: 0.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Ros3797gzvmswnJudQ1Znk
4 tasks
Pre-existing on master; blocks CI's ruff lint step. Unrelated to the relocation-evidence fix itself; fixing since it's a one-line, zero-risk removal blocking merge readiness.
Two pre-existing CI failures on master, discovered while verifying the
swkotor.exe autonomous recovery loop end-to-end:
- tests/test_rewrite_queue.py's two concurrency tests create
multiprocessing.Process with the default start method, which is "spawn"
on macOS (vs "fork" on Linux). Spawn re-imports the target function in a
fresh interpreter rather than reusing the parent's loaded image -- this
fails on macOS CI with ModuleNotFoundError ("tests" package not reliably
importable by the fresh interpreter) and, for a locally-nested function,
AttributeError (can't pickle a <locals> function at all). Reproduced
locally on Linux via an explicit spawn context to confirm the exact
failure class, then fixed by forcing multiprocessing.get_context("fork")
explicitly (available on both Linux and macOS, the only two CI
platforms) and moving the previously-nested _writer to module level as
defense in depth.
- source_parity_synthesize.py: removed an unused `.state.now` import
(ruff F401), unrelated pre-existing dead import blocking the lint step.
601 unit tests pass; ruff clean.
Co-authored-by: Copilot <th3w1zard1@users.noreply.github.com>
bodencrouch
pushed a commit
that referenced
this pull request
Jul 30, 2026
bodencrouch
added a commit
that referenced
this pull request
Jul 30, 2026
…t generalize) (#157) * fix: inc_abs_global never wires target-side relocation evidence render_target_coff_for_candidate() can reconstruct the objdiff target side with a matching symbol relocation for absolute-address references (absolute_address_relocations(), already used by bink_buffer_set_direct_draw_forwarder), but inc_abs_global() set evidence={"absoluteAddress": ...} instead of the absoluteAddressRelocations shape the consumer actually reads. The target side was always rendered as a raw byte blob with the address baked in literally, so any correct candidate referencing the global through a compiler-visible symbol relocation could never byte-match -- confirmed against the real MSVC8/wine toolchain on FUN_004a23b0 in swkotor-parity-inv, where an instruction-for-instruction identical inline-asm rewrite still reported DIFF_ARG_MISMATCH. With the fix, the same candidate reaches objdiff differences: 0. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ros3797gzvmswnJudQ1Znk * feat(source-parity): add shared single-address relocation-evidence helper U1 of docs/plans/2026-07-30-001-fix-generalize-relocation-evidence-plan.md * feat(source-parity): wire relocation evidence into 10 single-address rules float_multiply_global, import_call_self_stdcall, global_setter_u32_stdcall, call_indirect_zero, virtual_call_eq_global, import_call_return_self, global_indexed_store_cdecl, import_call_arg_return_one_stdcall8, global_virtual_call_stack_arg, global_field_eq_one_bool now populate absoluteAddressRelocations, closing the same target-side rendering gap inc_abs_global had (PR #149). U2 of docs/plans/2026-07-30-001-fix-generalize-relocation-evidence-plan.md * Revert "feat(source-parity): wire relocation evidence into 10 single-address rules" This reverts commit f802d73. * docs(source-parity): document why the multi-address rules need no relocation fix Real-toolchain A/B testing (real MSVC8/wine compile + real objdiff, not just unit-level assertions) overturned this plan's premise for both rep_stos_global_clear and global_and_global_bool: - rep_stos_global_clear's only candidate embeds every address as literal bytes via inline-asm _emit, not a pointer dereference -- it already reaches differences: 0 with no change. - global_and_global_bool dereferences addresses via literal C pointer casts, which MSVC compiles as bare immediates (no relocation) -- adding target-side absoluteAddressRelocations against a literal-cast candidate was A/B tested and found to be strictly worse (ARGUMENT_MISMATCH 6 vs 2 on the best profile), not better. Reverted. The corrected understanding: relocation evidence only helps when the candidate's own compiled object references the address through a compiler-emitted relocation (a named extern symbol), which none of these mechanical rule generators produce -- they all synthesize literal pointer casts. This changes the scope of U2/U4 in docs/plans/2026-07-30-001-fix-generalize-relocation-evidence-plan.md. U3 of docs/plans/2026-07-30-001-fix-generalize-relocation-evidence-plan.md * test(source-parity): guard against pairing relocation evidence with literal casts U4 of docs/plans/2026-07-30-001-fix-generalize-relocation-evidence-plan.md, with corrected scope: guards the real anti-pattern found during U2/U3 (absoluteAddressRelocations paired with a literal pointer cast, which real-toolchain testing showed makes objdiff matching worse, not better) rather than the plan's original premise (missing evidence), which real verification overturned. * docs(plan): record corrected findings from real-toolchain verification U2 and U3's original premise (generalize PR #149's relocation-evidence fix to 12 more mechanical rule generators) was overturned by real MSVC8/wine + objdiff A/B testing during implementation: every one of those rules synthesizes a literal pointer cast, which MSVC compiles as a bare immediate with no relocation, so the target-side reconstruction this evidence enables has nothing matching to mirror on the candidate side. Confirmed to make matching measurably worse for the tested representatives, not better. The plan is updated in place to record what was actually found and shipped: a reusable helper (U1), a regression test guarding the real anti-pattern found (U4), and the real generalization opportunity this surfaced (relocation-evidence inheritance for subagent-rewrite candidates) deferred as follow-up. * fix(review): warn against misuse, broaden anti-pattern regex, add positive-path test Code review (ce-code-review) on PR #151 found and validated three issues: - single_absolute_address_relocation()'s docstring didn't warn against the exact misuse this PR's own investigation found and reverted (pairing the helper with a literal-cast candidate). Added an explicit negative-case warning. - LITERAL_CAST_RE only matched the *(TYPE *)0x{addr} idiom, missing two other literal-cast forms already present in the file (indexed-store and assign-then-deref) -- confirmed latent (neither currently pairs with absoluteAddressRelocations, so not a live miss today) but a real robustness gap. Broadened the regex to cover all three forms; reverified zero false positives against bink_buffer_set_direct_draw_forwarder. - The regression test's "allowed pattern" branch (a rule generator that legitimately uses absoluteAddressRelocations with a named symbol) had no dedicated positive-path test -- only incidental pass-through of the whole-file scan. Added a test that directly targets bink_buffer_set_direct_draw_forwarder as the positive-path exemplar. 693 tests pass (up from 692). * fix(review): tighten positive-path test to exact relocation content Round-2 code review found test_legitimate_named_symbol_usage_is_not_flagged asserted only len(candidates) >= 1 and len(relocations) >= 1 -- loose enough that a regression dropping 5 of the real 6 relocation entries, or corrupting their offsets/symbols, would still pass. Validated directly: the real call returns 2 candidates with 6 relocation entries in the first. Tightened to exact equality on both counts and full relocation content. 693 tests pass. * fix(review): parametrize anti-pattern detection test over all 3 broadened cast forms Round-3 code review found round 1's LITERAL_CAST_RE broadening (to catch indexed-store and assign-then-deref literal-cast idioms, not just the original dereferenced-cast form) had no test proving it actually catches those two forms -- only the original narrow synthetic fixture was tested. Validated directly: narrowing the regex back to its pre-round-1 form left every existing test passing. Parametrized test_check_actually_detects_the_anti_pattern over all three idioms. Confirmed the fix has teeth: temporarily narrowing the regex makes exactly the two new parametrized cases fail (indexed-store, assign-then-deref) while the original case still passes. 695 tests pass. * fix: remove unused .state.now import (pre-existing ruff F401) Pre-existing on master before PR #149/#151 -- blocks CI lint step (ruff check --no-fix) on both PR branches. Unrelated to the relocation- evidence work; fixing since it's a one-line, zero-risk removal blocking merge readiness. 601 unit tests pass. --------- Co-authored-by: Copilot <th3w1zard1@users.noreply.github.com> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
render_target_coff_for_candidate()can reconstruct the objdiff target object with a matching symbol relocation for absolute-address globals (absolute_address_relocations()), already used bybink_buffer_set_direct_draw_forwarder.inc_abs_global()never populated theabsoluteAddressRelocationsevidence shape the consumer reads — it setevidence={"absoluteAddress": ...}instead, an unrelated key. The target side was always rendered as a raw byte blob with the address baked in literally.FUN_004a23b0inswkotor-parity-inv: an instruction-for-instruction identical inline-asm rewrite (inc dword ptr [DAT_00830540]) still reportedDIFF_ARG_MISMATCH.inc_abs_global()now computes the DIR32 relocation offset (2) and a_DAT_<addr>-style symbol, matching the codebase's existing global-naming convention. With the fix, the same candidate reachesobjdiff differences: 0(verified against the real toolchain, not just unit tests).Test plan
tests/test_inc_abs_global_relocation_evidence.py) confirminc_abs_global()populatesabsoluteAddressRelocationsand thatrender_target_coff_for_candidate()emits a.longrelocation instead of raw bytestest_e2e_cancelled_profile.pyconfirmed present on master before this change too — Ghidra-environment flakes, not regressions)FUN_004a23b0:differences: 0(wasmismatchedbefore this fix)🤖 Generated with Claude Code
https://claude.ai/code/session_01Ros3797gzvmswnJudQ1Znk