fix: packaged-source stdcall symbols silently fell back to cdecl naming - #155
Merged
Merged
Conversation
added 4 commits
July 30, 2026 07:23
Found while manually promoting a real swkotor.exe near-miss (sub_6c60, a 16-byte-stack stdcall stub returning a constant) through the actual production pipeline: the compiled candidate was byte-identical to the target (mov eax, 0x1; ret 0x10, 8 bytes both sides) but objdiff still reported a mismatch, because the target-side synthetic symbol was named plain `_sub_6c60` while the real compiled object's MSVC-decorated stdcall symbol was `_sub_6c60@16` -- a symbol-name mismatch masking an otherwise- exact code match. Root cause: packaged_stack_bytes() only derives the stack-byte count from external row metadata (automaticGenerator.stackBytes, a trailing `@N` in row["name"], or a trailing `_N` in c_name) -- none of which apply to a plain decompiler-named function like `sub_6c60` with no such annotation anywhere. infer_packaged_symbol() correctly detects `stdcall` from the source text itself, but falls through to cdecl naming whenever packaged_stack_bytes() returns None, silently producing the wrong symbol for real stdcall functions. Fix: when no external metadata carries the stack-byte count, packaged_stack_bytes() now counts it directly from the function's own parsed parameter list (4 bytes per param, 8 for undefined8/double/long long/__int64), rather than giving up. Verified end-to-end against the real MSVC8/wine toolchain and the real vacuum runner CLI: sub_6c60 now promotes to verified/ with a genuine objdiff differences: 0 receipt -- the first real accept for this swkotor.exe work dir. 601 unit tests pass; ruff clean.
…tack bytes The `_(\d+)$` fallback in packaged_stack_bytes() matched against c_name to support deliberately-named helpers (e.g. "helper_16" meaning 16 stack bytes), but it also matched Ghidra's own auto-generated `sub_<hex>` names whenever the hex address happened to contain only decimal digits (no a-f), silently misreading the function's own address as a stack-byte count (e.g. sub_11240 -> 11240) instead of falling through to the correct source-derived parameter count. This pre-empted the real decoration for a large fraction of swkotor.exe's packaged-source stdcall candidates. Verified end-to-end against the real MSVC8/wine toolchain: sub_11240 now promotes to verified/ with objdiff differences:0, on top of the sub_6c60 accept from the prior stdcall-decoration fix.
A packaged-source candidate calling another sub_XXXX/FUN_XXXX function had no prototype for it in scope, so the implicit C declaration always defaulted to cdecl regardless of the callee's real calling convention. When the real callee is stdcall (cleans its own stack), the caller wrongly emitted a spurious `add esp, N` after the call -- a genuine instruction-stream mismatch, not just a naming issue. infer_callee_prototype()/infer_callee_prototypes() reuse the callee's own packaged-source candidate.c (a sibling directory under the same source-generation root, always derivable from the candidate's own row) and run it back through the existing infer_packaged_callconv()/ packaged_stack_bytes() inference to emit a correctly-decorated extern prototype before the caller's body. Verified end-to-end: sub_88c0 (calls sub_7830) now compiles without the spurious add-esp cleanup once sub_7830's own packaged source is corrected to declare __stdcall. Full byte-for-byte match for this family is still blocked by a separate, deeper gap -- packaged-source tasks don't carry call-site relocation evidence (callSymbol/absoluteAddressRelocations) the way other rule generators already do, so the target-side synthetic COFF can't symbolically link the call and instead embeds the target's raw relative displacement -- but this fix eliminates the actual instruction mismatch a compile-only fix can address, and is independently verified via regression tests.
Code review (ce-code-review, 7 personas) on the callee calling-convention inference surfaced two real correctness bugs and a test-infra gap, all fixed: - infer_callee_prototype() hardcoded `void` as the emitted prototype's return type. Any caller consuming the callee's return value (the common Ghidra pattern `iVar1 = calleeName(...)`) would now fail to compile against the wrong void-returning prototype -- a regression introduced by the callee-convention fix itself. Now infers the real return type from the callee's own source, falling back to `int` (not `void`) when unparseable, since an implicit pre-C99 declaration already defaults to int and a caller that ignores the return value still compiles fine against it. - infer_callee_prototype() derived param count purely from stack_bytes // 4, assuming every param is 4 bytes. A callee with an 8-byte param (undefined8/double/long long/__int64) would emit a wrong-arity prototype (arity mismatch against the real call site). Now bails (returns None) when the callee's own parameter list contains an 8-byte type, per the review's cross-reviewer-corroborated finding (correctness, maintainability, adversarial all independently flagged this). - Both new regression test files were missing `pytestmark = pytest.mark.unit` (every sibling test file for this module sets it), so `uv run pytest -m unit` silently skipped all 15 of this session's new regression tests. Independently confirmed via direct collection before applying. Also records which callee prototypes were actually inferred into GeneratedCandidate.evidence (`calleePrototypesInferred`), per the agent-native reviewer's finding that this decision was previously invisible in generation.json, making future mismatch debugging harder. 616 unit tests pass (up from 601 -- the marker fix restored the missing 15).
4 tasks
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
Found while manually promoting a real swkotor.exe near-miss through the actual production pipeline (
agentdecompile-vacuum-runner).sub_6c60(a 16-byte-stack stdcall stub returning a constant) compiled to machine code byte-identical to the target (mov eax, 0x1; ret 0x10, 8 bytes both sides), but objdiff still reported a mismatch — the target-side synthetic symbol was named plain_sub_6c60while the real compiled object's MSVC-decorated stdcall symbol was_sub_6c60@16. A symbol-name mismatch masking an otherwise byte-exact code match.Root cause:
packaged_stack_bytes()only derives the stack-byte count from external row metadata (automaticGenerator.stackBytes, a trailing@Ninrow["name"], or a trailing_Ninc_name) — none of which apply to a plain decompiler-named function likesub_6c60.infer_packaged_symbol()correctly detectsstdcallfrom the source text itself but silently falls through to cdecl naming wheneverpackaged_stack_bytes()returnsNone.Fix: when no external metadata carries the stack-byte count, count it directly from the function's own parsed parameter list (4 bytes/param, 8 for
undefined8/double/long long/__int64).Real-world impact: this is a systemic bug affecting any packaged-source
__stdcallfunction with parameters where the row metadata doesn't already carrystackBytes— likely blocking many otherwise-correct candidates across the whole proof-target queue from ever matching, not just this one function.Test plan
ruffcleanagentdecompile-vacuum-runnerCLI:sub_6c60now promotes toverified/with a genuinedifferences: 0objdiff receipt — a real, byte-accurate recovered function forswkotor.exe🤖 Generated with Claude Code
https://claude.ai/code/session_01Ros3797gzvmswnJudQ1Znk