docs: investigate generalizing relocation-evidence fix (found: doesn't generalize) - #151
Closed
bodencrouch wants to merge 10 commits into
Closed
Conversation
added 10 commits
July 30, 2026 03:49
…lper U1 of docs/plans/2026-07-30-001-fix-generalize-relocation-evidence-plan.md
…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
…address rules" This reverts commit f802d73.
…ocation 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
…iteral 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.
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.
…itive-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).
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.
…ened 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.
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
1 participant
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
Depends on #149 (targets that branch, not master, until it merges).
Investigated generalizing PR #149's
absoluteAddressRelocationsfix frominc_abs_globalto 12 other mechanical rule generators insource_parity_synthesize.pywith the same superficial shape (raw absolute address embedded in generated C source, no relocation evidence).Real-toolchain A/B testing overturned the premise. All 12 rules synthesize a literal pointer cast (
*(type *)0x{addr:08x}), which MSVC compiles as a bare immediate with no relocation on the candidate side — there's nothing for the target-side relocation reconstruction to usefully mirror. Confirmed against the real MSVC8/wine toolchain + real objdiff:float_multiply_global: unfixed{ARGUMENT_MISMATCH: 2, INSERTION: 14}→ fixed{ARGUMENT_MISMATCH: 4, INSERTION: 14}— worseglobal_and_global_bool: unfixed{ARGUMENT_MISMATCH: 2, INSERTION: 4}→ fixed{ARGUMENT_MISMATCH: 6, INSERTION: 4}— worseinc_abs_global's own already-shipped fix: identical histograms with/without the evidence on its own candidate — neutral. Its value is that the evidence field is available for a differently-constructed later candidate (e.g. a subagent rewrite referencing a namedDAT_<addr>symbol) to match against — demonstrated live by combining a subagent rewrite with hand-attached evidence to reachdifferences: 0.rep_stos_global_clear: different reason, same conclusion — its only candidate embeds addresses as literal_emitbytes, not a pointer dereference. Already reachesdifferences: 0.What shipped
single_absolute_address_relocation) for any future rule that genuinely references an address through a named symbol (not a literal cast) — the 12 investigated here don't qualify.global_and_global_boolwiring was implemented, tested, found to regress matching, and reverted (see commit history: wire → revert → document).absoluteAddressRelocationswith a literal-cast candidate.Test plan
tests/test_relocation_evidence_helper.py— the U1 helpertests/test_multi_address_rule_relocation_evidence.py— documents whyglobal_and_global_boolneeds no fixtests/test_no_unrelocated_absolute_addresses.py— guards the real anti-pattern, verified to actually fire on a synthetic violation🤖 Generated with Claude Code
https://claude.ai/code/session_01Ros3797gzvmswnJudQ1Znk