From 087952414efc9185f83c358c015e7709444f6155 Mon Sep 17 00:00:00 2001 From: Copilot Date: Thu, 30 Jul 2026 07:23:19 -0500 Subject: [PATCH 1/4] fix: packaged-source stdcall symbols silently fell back to cdecl naming 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. --- .../source_parity_synthesize.py | 33 +++++++- ...test_packaged_stdcall_symbol_decoration.py | 76 +++++++++++++++++++ 2 files changed, 106 insertions(+), 3 deletions(-) create mode 100644 tests/test_packaged_stdcall_symbol_decoration.py diff --git a/src/agentdecompile_recovery/source_parity_synthesize.py b/src/agentdecompile_recovery/source_parity_synthesize.py index 482ce25e..77f8c30a 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 @@ -19195,7 +19194,7 @@ def infer_packaged_symbol(row: dict[str, Any], source: str, c_name: str, suffix: return match.group(1) return c_name callconv = infer_packaged_callconv(source, suffix) - stack_bytes = packaged_stack_bytes(row, c_name) + stack_bytes = packaged_stack_bytes(row, c_name, source=source) if callconv == "stdcall" and stack_bytes is not None: return f"_{c_name}@{stack_bytes}" if callconv == "fastcall" and stack_bytes is not None: @@ -19203,7 +19202,26 @@ def infer_packaged_symbol(row: dict[str, Any], source: str, c_name: str, suffix: return cdecl_symbol(c_name) -def packaged_stack_bytes(row: dict[str, Any], c_name: str) -> int | None: +# Pseudo-types (and their real-type equivalents) that occupy 8 bytes on the +# stack rather than the 4-byte default -- matched against a parameter's type +# tokens (everything before the parameter name). +_EIGHT_BYTE_PARAM_TYPE_RE = re.compile(r"\b(undefined8|double|long\s+long|__int64)\b") + + +def _count_stack_bytes_from_parameter_list(params_text: str) -> int: + params_text = params_text.strip() + if not params_text or params_text == "void": + return 0 + total = 0 + for param in params_text.split(","): + param = param.strip() + if not param: + continue + total += 8 if _EIGHT_BYTE_PARAM_TYPE_RE.search(param) else 4 + return total + + +def packaged_stack_bytes(row: dict[str, Any], c_name: str, *, source: str | None = None) -> int | None: generator = row.get("automaticGenerator") if isinstance(row.get("automaticGenerator"), dict) else {} stack_bytes = optional_int(generator.get("stackBytes")) if stack_bytes is not None: @@ -19215,6 +19233,15 @@ def packaged_stack_bytes(row: dict[str, Any], c_name: str) -> int | None: match = re.search(r"_(\d+)$", c_name) if match: return int(match.group(1)) + if source: + # No external metadata carries the stack-byte count (common for a + # plain decompiler-named function like sub_1234 with no @N suffix + # anywhere) -- count it directly from the parsed function's own + # parameter list rather than silently falling back to cdecl naming + # for what may genuinely be a stdcall/fastcall function. + sig_match = re.search(rf"\b{re.escape(c_name)}\s*\(([^)]*)\)", source) + if sig_match: + return _count_stack_bytes_from_parameter_list(sig_match.group(1)) return None diff --git a/tests/test_packaged_stdcall_symbol_decoration.py b/tests/test_packaged_stdcall_symbol_decoration.py new file mode 100644 index 00000000..58efd32a --- /dev/null +++ b/tests/test_packaged_stdcall_symbol_decoration.py @@ -0,0 +1,76 @@ +"""Regression test: packaged-source candidates for __stdcall functions must +compute the real `@N` stack-byte-decorated symbol name, not silently fall +back to plain cdecl naming. + +Found while manually promoting a real swkotor.exe near-miss (sub_6c60) to +verified/: the candidate compiled to byte-identical machine code as the +target (`mov eax, 0x1; ret 0x10`, both 8 bytes) but objdiff still reported +`match_percent: 0.0` because the target-side synthetic symbol was named +`_sub_6c60` (infer_packaged_symbol's cdecl fallback) while the real compiled +object's stdcall-decorated symbol was `_sub_6c60@16` -- a symbol-name +mismatch masking an otherwise-exact code match. Root cause: +packaged_stack_bytes() only derives stack bytes 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` -- so it returns None and infer_packaged_symbol() +falls through to cdecl naming even though it already detected `stdcall` from +the source text itself. +""" + +from __future__ import annotations + +from agentdecompile_recovery.source_parity_synthesize import ( + infer_packaged_symbol, + packaged_stack_bytes, +) + + +def test_packaged_stack_bytes_counts_stdcall_params_from_source_when_no_metadata() -> None: + row: dict = {"name": "sub_6c60"} + source = ( + "undefined4 __stdcall sub_6c60(undefined4 param_1, undefined4 param_2, " + "undefined4 param_3, undefined4 param_4)\n\n{\n return 1;\n}\n" + ) + + assert packaged_stack_bytes(row, "sub_6c60", source=source) == 16 + + +def test_packaged_stack_bytes_zero_for_void_params() -> None: + # Name deliberately avoids an all-decimal hex suffix (e.g. "sub_1000") + # -- that collides with a separate, pre-existing heuristic + # (packaged_stack_bytes' `_(\d+)$` match against c_name) that + # misinterprets an address-derived name as a stack-byte annotation. + # Not this fix's concern; see the module docstring for the narrower + # bug this test suite targets. + row: dict = {"name": "sub_1a00"} + source = "undefined4 __stdcall sub_1a00(void)\n\n{\n return 1;\n}\n" + + assert packaged_stack_bytes(row, "sub_1a00", source=source) == 0 + + +def test_packaged_stack_bytes_prefers_explicit_metadata_over_source_parse() -> None: + row = {"name": "sub_2000", "automaticGenerator": {"stackBytes": 8}} + source = "undefined4 __stdcall sub_2000(undefined4 a, undefined4 b, undefined4 c)\n\n{\n return 1;\n}\n" + + # Explicit metadata (8) wins over the source's 3-param count (12). + assert packaged_stack_bytes(row, "sub_2000", source=source) == 8 + + +def test_infer_packaged_symbol_produces_correct_stdcall_decoration_from_source_alone() -> None: + row: dict = {"name": "sub_6c60"} + c_name = "sub_6c60" + source = ( + "undefined4 __stdcall sub_6c60(undefined4 param_1, undefined4 param_2, " + "undefined4 param_3, undefined4 param_4)\n\n{\n return 1;\n}\n" + ) + + assert infer_packaged_symbol(row, source, c_name, ".c") == "_sub_6c60@16" + + +def test_infer_packaged_symbol_still_falls_back_to_cdecl_when_params_unparseable() -> None: + row: dict = {"name": "sub_9999"} + c_name = "sub_9999" + # No matching function definition in the source at all -- can't count params. + source = "// no function body here\n" + + assert infer_packaged_symbol(row, source, c_name, ".c") == "_sub_9999" From 3766c9dee068fa18963f09c7da281f6c179f6bad Mon Sep 17 00:00:00 2001 From: Copilot Date: Thu, 30 Jul 2026 07:52:09 -0500 Subject: [PATCH 2/4] fix: packaged_stack_bytes misreads all-decimal sub_ address as stack 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_` 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. --- .../source_parity_synthesize.py | 16 +++++-- ...test_packaged_stdcall_symbol_decoration.py | 42 +++++++++++++++---- 2 files changed, 48 insertions(+), 10 deletions(-) diff --git a/src/agentdecompile_recovery/source_parity_synthesize.py b/src/agentdecompile_recovery/source_parity_synthesize.py index 77f8c30a..3161d879 100755 --- a/src/agentdecompile_recovery/source_parity_synthesize.py +++ b/src/agentdecompile_recovery/source_parity_synthesize.py @@ -19221,6 +19221,9 @@ def _count_stack_bytes_from_parameter_list(params_text: str) -> int: return total +_AUTO_DECOMPILER_NAME_RE = re.compile(r"^(?:sub|FUN)_[0-9a-fA-F]+$") + + def packaged_stack_bytes(row: dict[str, Any], c_name: str, *, source: str | None = None) -> int | None: generator = row.get("automaticGenerator") if isinstance(row.get("automaticGenerator"), dict) else {} stack_bytes = optional_int(generator.get("stackBytes")) @@ -19230,9 +19233,16 @@ def packaged_stack_bytes(row: dict[str, Any], c_name: str, *, source: str | None match = re.search(r"@(\d+)$", name) if match: return int(match.group(1)) - match = re.search(r"_(\d+)$", c_name) - if match: - return int(match.group(1)) + # Skip the "_" suffix heuristic for Ghidra's default auto-named + # functions (sub_/FUN_) -- when the address happens to be + # composed entirely of decimal digits (no a-f), this would misread the + # function's own address as a stack-byte annotation (e.g. sub_11240 -> + # wrongly inferred as 11240 stack bytes). The suffix is only meaningful + # for deliberately-named helpers (e.g. "helper_16" meaning 16 bytes). + if not _AUTO_DECOMPILER_NAME_RE.match(c_name): + match = re.search(r"_(\d+)$", c_name) + if match: + return int(match.group(1)) if source: # No external metadata carries the stack-byte count (common for a # plain decompiler-named function like sub_1234 with no @N suffix diff --git a/tests/test_packaged_stdcall_symbol_decoration.py b/tests/test_packaged_stdcall_symbol_decoration.py index 58efd32a..3b453224 100644 --- a/tests/test_packaged_stdcall_symbol_decoration.py +++ b/tests/test_packaged_stdcall_symbol_decoration.py @@ -1,4 +1,4 @@ -"""Regression test: packaged-source candidates for __stdcall functions must +r"""Regression test: packaged-source candidates for __stdcall functions must compute the real `@N` stack-byte-decorated symbol name, not silently fall back to plain cdecl naming. @@ -15,6 +15,15 @@ function like `sub_6c60` -- so it returns None and infer_packaged_symbol() falls through to cdecl naming even though it already detected `stdcall` from the source text itself. + +A follow-up sweep of cached objdiff verify results (see session notes) found +a second, larger-impact instance of the same class: the `_(\d+)$` fallback +against c_name matches Ghidra's own auto-generated `sub_`/`FUN_` +names whenever the hex address happens to contain only decimal digits (no +a-f), misreading the function's own address as a bogus stack-byte count +(e.g. `sub_11240` -> 11240). This silently pre-empted the correct +source-derived parameter count for roughly a third of the swkotor.exe +near-miss stdcall population found in the same sweep. """ from __future__ import annotations @@ -36,18 +45,37 @@ def test_packaged_stack_bytes_counts_stdcall_params_from_source_when_no_metadata def test_packaged_stack_bytes_zero_for_void_params() -> None: - # Name deliberately avoids an all-decimal hex suffix (e.g. "sub_1000") - # -- that collides with a separate, pre-existing heuristic - # (packaged_stack_bytes' `_(\d+)$` match against c_name) that - # misinterprets an address-derived name as a stack-byte annotation. - # Not this fix's concern; see the module docstring for the narrower - # bug this test suite targets. row: dict = {"name": "sub_1a00"} source = "undefined4 __stdcall sub_1a00(void)\n\n{\n return 1;\n}\n" assert packaged_stack_bytes(row, "sub_1a00", source=source) == 0 +def test_packaged_stack_bytes_does_not_misread_all_decimal_address_as_suffix() -> None: + # sub_11240's own address (0x11240) happens to be all decimal digits, so + # the c_name "sub_11240" superficially looks like it ends in a "_" + # stack-byte annotation. Before this fix, packaged_stack_bytes() matched + # that address digit run via `_(\d+)$` and returned 11240 (nonsense) for + # ANY sub_XXXX name whose hex address contains no a-f digit, completely + # bypassing the real 1-param count from source (4 bytes) and silently + # producing a target-scale symbol suffix that could never match a real + # compiled candidate. This affected roughly a third of the swkotor.exe + # near-miss stdcall population discovered in this session's cache sweep. + row: dict = {"name": "sub_11240"} + source = "undefined4 __stdcall sub_11240(undefined4 *param_1)\n\n{\n return 1;\n}\n" + + assert packaged_stack_bytes(row, "sub_11240", source=source) == 4 + + +def test_packaged_stack_bytes_still_honors_deliberate_helper_suffix() -> None: + # A manually-authored helper name like "helper_16" (not Ghidra's default + # sub_/FUN_ auto-naming) legitimately uses a trailing "_N" to mean N + # stack bytes -- the auto-decompiler-name guard must not suppress this. + row: dict = {"name": "helper_16"} + + assert packaged_stack_bytes(row, "helper_16") == 16 + + def test_packaged_stack_bytes_prefers_explicit_metadata_over_source_parse() -> None: row = {"name": "sub_2000", "automaticGenerator": {"stackBytes": 8}} source = "undefined4 __stdcall sub_2000(undefined4 a, undefined4 b, undefined4 c)\n\n{\n return 1;\n}\n" From 616a928bf91fd88911151e11f7ca43a5abe5eefc Mon Sep 17 00:00:00 2001 From: Copilot Date: Thu, 30 Jul 2026 08:06:38 -0500 Subject: [PATCH 3/4] feat: infer callee calling convention for packaged-source candidates 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. --- .../source_parity_synthesize.py | 48 ++++++++- ...test_packaged_callee_calling_convention.py | 98 +++++++++++++++++++ 2 files changed, 145 insertions(+), 1 deletion(-) create mode 100644 tests/test_packaged_callee_calling_convention.py diff --git a/src/agentdecompile_recovery/source_parity_synthesize.py b/src/agentdecompile_recovery/source_parity_synthesize.py index 3161d879..e40ed8ec 100755 --- a/src/agentdecompile_recovery/source_parity_synthesize.py +++ b/src/agentdecompile_recovery/source_parity_synthesize.py @@ -19118,6 +19118,48 @@ def generate(row: dict[str, Any], max_variants: int) -> list[GeneratedCandidate] return candidates +_CALLEE_CALL_RE = re.compile(r"\b(sub_[0-9a-fA-F]+|FUN_[0-9a-fA-F]+)\s*\(") + + +def infer_callee_prototype(callee_name: str, source_generation_root: Path) -> str | None: + """Infer a calling-convention-correct extern prototype for a callee by + reusing its own packaged-source candidate (a sibling directory under the + same source-generation root) and running it through the same + infer_packaged_callconv/packaged_stack_bytes inference already used for + the caller itself. + + Without this, a call to another sub_XXXX/FUN_XXXX function compiles with + an implicit (cdecl) declaration regardless of the callee's real calling + convention, so a stdcall/fastcall callee causes the caller to emit a + spurious `add esp, N` stack-cleanup instruction that the real compiled + binary never has (the real callee already cleaned the stack itself). + """ + matches = sorted(source_generation_root.glob(f"{callee_name}_*/candidate.c")) + if not matches: + return None + callee_source = matches[0].read_text(encoding="utf-8", errors="replace") + callconv = infer_packaged_callconv(callee_source, ".c") + if callconv not in {"stdcall", "fastcall"}: + return None + stack_bytes = packaged_stack_bytes({"name": callee_name}, callee_name, source=callee_source) + if stack_bytes is None or stack_bytes % 4 != 0: + return None + param_count = stack_bytes // 4 + params = ", ".join(["unsigned int"] * param_count) if param_count else "void" + keyword = "__stdcall" if callconv == "stdcall" else "__fastcall" + return f"extern void {keyword} {callee_name}({params});" + + +def infer_callee_prototypes(source: str, self_name: str, source_generation_root: Path) -> str: + names = sorted({name for name in _CALLEE_CALL_RE.findall(source) if name != self_name}) + prototypes = [] + for name in names: + prototype = infer_callee_prototype(name, source_generation_root) + if prototype: + prototypes.append(prototype) + return "\n".join(prototypes) + + def packaged_source_candidate(row: dict[str, Any]) -> GeneratedCandidate | None: if not row.get("sourceTask"): return None @@ -19133,7 +19175,11 @@ def packaged_source_candidate(row: dict[str, Any]) -> GeneratedCandidate | None: # Packaged .c sources are raw decompiler output (e.g. Ghidra's undefined4/code/byte # pseudo-types) with no typedefs of their own; without the shim MSVC/clang fail to # even parse the file, so nothing downstream ever reaches objdiff. - compile_source = build_shim(source) + "\n\n" + source if suffix == ".c" else source + if suffix == ".c": + callee_prototypes = infer_callee_prototypes(source, c_name, source_path.parent.parent) + compile_source = build_shim(source) + (f"\n{callee_prototypes}\n" if callee_prototypes else "") + "\n" + source + else: + compile_source = source return GeneratedCandidate( rule=str(automatic_generator.get("rule") or "packaged-source"), variant="packaged-source", diff --git a/tests/test_packaged_callee_calling_convention.py b/tests/test_packaged_callee_calling_convention.py new file mode 100644 index 00000000..66d86298 --- /dev/null +++ b/tests/test_packaged_callee_calling_convention.py @@ -0,0 +1,98 @@ +"""Regression test: packaged-source candidates that call other sub_XXXX/ +FUN_XXXX functions must declare the callee's real calling convention, +not silently let it default to cdecl. + +Found during the swkotor.exe cache sweep: sub_88c0's real target calls +sub_7830 (2 args) with no stack cleanup after the call (`call sub_7830 / ret +0x4`), because sub_7830 is itself __stdcall and cleans its own 8 bytes. The +packaged-source candidate for sub_88c0 has no prototype for sub_7830 in +scope, so the implicit C declaration defaults to cdecl and the caller wrongly +emits `add esp, 0x8` after the call -- a real instruction-stream mismatch, +not just a symbol-naming issue. + +infer_callee_prototype()/infer_callee_prototypes() fix this by reusing the +callee's own packaged-source candidate.c (a sibling directory under the same +source-generation root) and running it back through the same +infer_packaged_callconv()/packaged_stack_bytes() inference already used for +the caller itself, to emit a correctly-decorated extern prototype before the +caller's body. +""" + +from __future__ import annotations + +from pathlib import Path + +from agentdecompile_recovery.source_parity_synthesize import ( + infer_callee_prototype, + infer_callee_prototypes, +) + + +def _write_callee_candidate(root: Path, name: str, entry: str, source: str) -> None: + directory = root / f"{name}_{entry}" + directory.mkdir(parents=True, exist_ok=True) + (directory / "candidate.c").write_text(source, encoding="utf-8") + + +def test_infer_callee_prototype_declares_stdcall_from_sibling_candidate(tmp_path: Path) -> None: + _write_callee_candidate( + tmp_path, + "sub_7830", + "407830", + "undefined4 __stdcall sub_7830(undefined4 param_1,int param_2)\n\n{\n return 1;\n}\n", + ) + + prototype = infer_callee_prototype("sub_7830", tmp_path) + + assert prototype == "extern void __stdcall sub_7830(unsigned int, unsigned int);" + + +def test_infer_callee_prototype_returns_none_for_cdecl_callee(tmp_path: Path) -> None: + _write_callee_candidate( + tmp_path, + "sub_1e5670", + "41e5670", + "void sub_1e5670(int param_1)\n\n{\n return;\n}\n", + ) + + assert infer_callee_prototype("sub_1e5670", tmp_path) is None + + +def test_infer_callee_prototype_returns_none_when_sibling_missing(tmp_path: Path) -> None: + assert infer_callee_prototype("sub_deadbeef", tmp_path) is None + + +def test_infer_callee_prototypes_skips_self_recursive_calls(tmp_path: Path) -> None: + _write_callee_candidate( + tmp_path, + "sub_1060", + "401060", + "undefined4 __stdcall sub_1060(undefined4 param_1)\n\n{\n return sub_1060(param_1);\n}\n", + ) + source = "undefined4 __stdcall sub_1060(undefined4 param_1)\n\n{\n return sub_1060(param_1);\n}\n" + + assert infer_callee_prototypes(source, "sub_1060", tmp_path) == "" + + +def test_infer_callee_prototypes_declares_multiple_callees(tmp_path: Path) -> None: + _write_callee_candidate( + tmp_path, + "sub_7830", + "407830", + "undefined4 __stdcall sub_7830(undefined4 param_1,int param_2)\n\n{\n return 1;\n}\n", + ) + _write_callee_candidate( + tmp_path, + "sub_7fe0", + "407fe0", + "undefined4 __stdcall sub_7fe0(undefined4 param_1,int param_2)\n\n{\n return 1;\n}\n", + ) + source = ( + "void __stdcall sub_88e0(undefined4 param_1)\n\n" + "{\n sub_7830(param_1, 2);\n sub_7fe0(param_1, 2);\n}\n" + ) + + prototypes = infer_callee_prototypes(source, "sub_88e0", tmp_path) + + assert "extern void __stdcall sub_7830(unsigned int, unsigned int);" in prototypes + assert "extern void __stdcall sub_7fe0(unsigned int, unsigned int);" in prototypes From cbf796bda687e84fc592d088f73275899395599e Mon Sep 17 00:00:00 2001 From: Copilot Date: Thu, 30 Jul 2026 08:22:12 -0500 Subject: [PATCH 4/4] fix(review): correct return type, guard 8-byte params, add unit markers 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). --- .../source_parity_synthesize.py | 30 ++++++++++- ...test_packaged_callee_calling_convention.py | 51 +++++++++++++++++-- ...test_packaged_stdcall_symbol_decoration.py | 4 ++ 3 files changed, 81 insertions(+), 4 deletions(-) diff --git a/src/agentdecompile_recovery/source_parity_synthesize.py b/src/agentdecompile_recovery/source_parity_synthesize.py index e40ed8ec..e43dbda8 100755 --- a/src/agentdecompile_recovery/source_parity_synthesize.py +++ b/src/agentdecompile_recovery/source_parity_synthesize.py @@ -19121,6 +19121,24 @@ def generate(row: dict[str, Any], max_variants: int) -> list[GeneratedCandidate] _CALLEE_CALL_RE = re.compile(r"\b(sub_[0-9a-fA-F]+|FUN_[0-9a-fA-F]+)\s*\(") +def _infer_callee_return_type(callee_source: str, callee_name: str) -> str: + # Falls back to "int" (not "void") when unparseable: an implicit + # pre-C99 declaration defaults to int, and a caller that ignores the + # return value still compiles fine against an "int" prototype -- unlike + # "void", which is a hard compile error for any caller that consumes it + # (e.g. the common Ghidra pattern `iVar1 = calleeName(...)`). + match = re.search(rf"(^|\n)([A-Za-z_][A-Za-z0-9_ \*]*?)\s+{re.escape(callee_name)}\s*\(", callee_source) + if not match: + return "int" + # The captured group spans everything before the name, which includes + # any calling-convention keyword (e.g. "undefined4 __stdcall") since the + # regex must expand past it to reach the required whitespace+name + # boundary -- strip those keywords back out, since the prototype already + # adds the calling-convention keyword explicitly. + rettype = re.sub(r"\b(__stdcall|__fastcall|__cdecl)\b", "", match.group(2)).strip() + return rettype or "int" + + def infer_callee_prototype(callee_name: str, source_generation_root: Path) -> str | None: """Infer a calling-convention-correct extern prototype for a callee by reusing its own packaged-source candidate (a sibling directory under the @@ -19144,10 +19162,18 @@ def infer_callee_prototype(callee_name: str, source_generation_root: Path) -> st stack_bytes = packaged_stack_bytes({"name": callee_name}, callee_name, source=callee_source) if stack_bytes is None or stack_bytes % 4 != 0: return None + sig_match = re.search(rf"\b{re.escape(callee_name)}\s*\(([^)]*)\)", callee_source) + if sig_match and _EIGHT_BYTE_PARAM_TYPE_RE.search(sig_match.group(1)): + # packaged_stack_bytes only tracks total byte count, not individual + # param widths -- an 8-byte param would make stack_bytes // 4 emit + # the wrong number of (all 4-byte) params, an arity mismatch against + # the real call site. Bail rather than emit a wrong-arity prototype. + return None param_count = stack_bytes // 4 params = ", ".join(["unsigned int"] * param_count) if param_count else "void" keyword = "__stdcall" if callconv == "stdcall" else "__fastcall" - return f"extern void {keyword} {callee_name}({params});" + return_type = _infer_callee_return_type(callee_source, callee_name) + return f"extern {return_type} {keyword} {callee_name}({params});" def infer_callee_prototypes(source: str, self_name: str, source_generation_root: Path) -> str: @@ -19175,6 +19201,7 @@ def packaged_source_candidate(row: dict[str, Any]) -> GeneratedCandidate | None: # Packaged .c sources are raw decompiler output (e.g. Ghidra's undefined4/code/byte # pseudo-types) with no typedefs of their own; without the shim MSVC/clang fail to # even parse the file, so nothing downstream ever reaches objdiff. + callee_prototypes = "" if suffix == ".c": callee_prototypes = infer_callee_prototypes(source, c_name, source_path.parent.parent) compile_source = build_shim(source) + (f"\n{callee_prototypes}\n" if callee_prototypes else "") + "\n" + source @@ -19196,6 +19223,7 @@ def packaged_source_candidate(row: dict[str, Any]) -> GeneratedCandidate | None: "packagedSource": str(source_path), "packagedSourceSha256": hashlib.sha256(source.encode("utf-8")).hexdigest(), "sourceOrigin": row.get("sourceOrigin"), + "calleePrototypesInferred": callee_prototypes.splitlines(), }, source_suffix=suffix, semantic_source=row.get("semanticSource") is not False, diff --git a/tests/test_packaged_callee_calling_convention.py b/tests/test_packaged_callee_calling_convention.py index 66d86298..81beae50 100644 --- a/tests/test_packaged_callee_calling_convention.py +++ b/tests/test_packaged_callee_calling_convention.py @@ -16,17 +16,32 @@ infer_packaged_callconv()/packaged_stack_bytes() inference already used for the caller itself, to emit a correctly-decorated extern prototype before the caller's body. + +Two follow-up fixes from code review are also covered here: +- The emitted prototype's return type is inferred from the callee's own + source (falling back to "int", never "void") -- a hardcoded "void" broke + compilation for any caller that consumes the callee's return value (the + common Ghidra pattern `iVar1 = calleeName(...)`). +- A callee whose parameter list contains an 8-byte type (undefined8/double/ + long long/__int64) is skipped (returns None) rather than emitting a + wrong-arity prototype: packaged_stack_bytes only tracks total byte count, + and stack_bytes // 4 assumes every param is 4 bytes. """ from __future__ import annotations from pathlib import Path +import pytest + from agentdecompile_recovery.source_parity_synthesize import ( + _infer_callee_return_type, infer_callee_prototype, infer_callee_prototypes, ) +pytestmark = pytest.mark.unit + def _write_callee_candidate(root: Path, name: str, entry: str, source: str) -> None: directory = root / f"{name}_{entry}" @@ -44,7 +59,7 @@ def test_infer_callee_prototype_declares_stdcall_from_sibling_candidate(tmp_path prototype = infer_callee_prototype("sub_7830", tmp_path) - assert prototype == "extern void __stdcall sub_7830(unsigned int, unsigned int);" + assert prototype == "extern undefined4 __stdcall sub_7830(unsigned int, unsigned int);" def test_infer_callee_prototype_returns_none_for_cdecl_callee(tmp_path: Path) -> None: @@ -94,5 +109,35 @@ def test_infer_callee_prototypes_declares_multiple_callees(tmp_path: Path) -> No prototypes = infer_callee_prototypes(source, "sub_88e0", tmp_path) - assert "extern void __stdcall sub_7830(unsigned int, unsigned int);" in prototypes - assert "extern void __stdcall sub_7fe0(unsigned int, unsigned int);" in prototypes + assert "extern undefined4 __stdcall sub_7830(unsigned int, unsigned int);" in prototypes + assert "extern undefined4 __stdcall sub_7fe0(unsigned int, unsigned int);" in prototypes + + +def test_infer_callee_prototype_infers_non_default_return_type(tmp_path: Path) -> None: + _write_callee_candidate( + tmp_path, + "sub_9000", + "409000", + "uint __stdcall sub_9000(undefined4 param_1)\n\n{\n return 1;\n}\n", + ) + + assert infer_callee_prototype("sub_9000", tmp_path) == "extern uint __stdcall sub_9000(unsigned int);" + + +def test_infer_callee_return_type_falls_back_to_int_when_unparseable() -> None: + assert _infer_callee_return_type("// no function signature here\n", "sub_9010") == "int" + + +def test_infer_callee_prototype_skips_callee_with_eight_byte_param(tmp_path: Path) -> None: + # sub_9020's real stack cleanup would be 12 bytes (one 4-byte + one + # 8-byte param), but stack_bytes // 4 has no way to know that one slot + # is 8 bytes -- it would emit 3 plain `unsigned int` params, an arity + # mismatch against the real 2-argument call site. Bail instead. + _write_callee_candidate( + tmp_path, + "sub_9020", + "409020", + "undefined4 __stdcall sub_9020(undefined4 param_1,double param_2)\n\n{\n return 1;\n}\n", + ) + + assert infer_callee_prototype("sub_9020", tmp_path) is None diff --git a/tests/test_packaged_stdcall_symbol_decoration.py b/tests/test_packaged_stdcall_symbol_decoration.py index 3b453224..b53bbfce 100644 --- a/tests/test_packaged_stdcall_symbol_decoration.py +++ b/tests/test_packaged_stdcall_symbol_decoration.py @@ -28,11 +28,15 @@ from __future__ import annotations +import pytest + from agentdecompile_recovery.source_parity_synthesize import ( infer_packaged_symbol, packaged_stack_bytes, ) +pytestmark = pytest.mark.unit + def test_packaged_stack_bytes_counts_stdcall_params_from_source_when_no_metadata() -> None: row: dict = {"name": "sub_6c60"}