docs: investigate generalizing relocation-evidence fix (found: doesn't generalize) - #157
Merged
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
…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.
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.
Supersedes #151, which GitHub auto-closed when its base branch (fix/inc-abs-global-relocation-evidence, from #149) was deleted after #149's squash-merge. Same content, retargeted to master.
Summary
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).Finding: it doesn't generalize cleanly -- see the plan doc and commit history for the A/B revert of the two generators that regressed.
CI: unit tests + headless mode manually dispatched and green on this exact commit; full local suite (709 passed) also green.