feat(recomp): discover packed jal-only entries via cross-unit manifests and thread-entry scan - #150
Conversation
f96656a to
1b4571f
Compare
…ts and thread-entry scan A callee reached only by a jal/j immediate from a caller outside the byte range grouped into one function unit is disassembled as unreachable tail code inside whichever unit contains it, but gets no callable entry of its own. At runtime lookupFunction() reports it not-found and the recovery path silently substitutes broken behavior instead of failing loudly. The existing post-pass discoverAdditionalEntryPoints() already handles same-invocation cross-function targets via resume mapping; two forms survived. Part A - cross-compilation-unit / overlay callers: each invocation now emits external_call_targets.txt (jal/j targets in an executable section but outside its own recompiled ranges) into the output directory, and a new [general] external_call_target_manifests config array ingests manifests produced by other invocations. Ingested targets that land inside this invocation's function ranges are registered through the existing resume-entry mapping (m_resumeEntryTargetsByOwner), so lookupFunction(target) dispatches into the owner unit at the right pc. The emit-selection logic is a public static (CollectExternalCallTargets) for unit testing. Part B - data-embedded thread entries: a new analysis pass locates CreateThread (syscall 0x20) call sites (direct syscall or jal to a libkernel wrapper), recovers the static ThreadParam pointer in $a0 via a bounded lui/addiu/ori constant walk (including the jal delay slot), reads the entry function pointer from ELF data at param+4, and registers entries that fall inside recompiled function ranges via the same resume mapping. Scope is CreateThread-with-immediate-ThreadParam only: StartThread (0x22), runtime-computed param pointers, and handler-registration variants are not covered. Both discovery paths feed the existing registration machinery; no runtime or emitter behavior changes. Includes unit tests for manifest parsing, the external-target selection helper, and the thread-entry discovery helper, plus end-to-end regression tests that drive the real recompile pipeline and assert registration through the emitted function table.
1b4571f to
a5e4a6a
Compare
| // Collects jal/j targets that fall in executable sections but outside every | ||
| // recompiled local function range - candidate cross-unit call targets to | ||
| // publish in the external call-target manifest. Sorted and de-duplicated. | ||
| static std::vector<uint32_t> CollectExternalCallTargets( |
There was a problem hiding this comment.
This method currently requires the target to be inside an executable section of the caller's ELF. This appears to exclude the main use case described by the PR: a direct jal from a main ELF to a separately compiled overlay whose target range is not represented by any executable section in the main ELF.
The current unit test explicitly expects targets outside the current ELF's executable sections to be discarded, while the end-to-end test starts from a manually pre-created manifest and therefore never exercises cross-ELF manifest emission.
Could we add a real two-ELF regression test where ELF A contains a jal to code that exists only in ELF B, then verify that A emits the target and B ingests it? Unless both ELFs happen to have overlapping executable section ranges, the current filter seems to drop exactly the cross-unit target this feature is intended to preserve.
| } | ||
|
|
||
| loadExternalCallTargetManifests(); | ||
| discoverAdditionalEntryPoints(); |
There was a problem hiding this comment.
There seems to be a build-order problem here. Each invocation loads sibling manifests before discoverAdditionalEntryPoints(), but only emits its own manifest at the end of that pass.
For mutually calling units, such as a main ELF and an overlay that call into each other, a clean build creates a dependency cycle: each output requires the other unit's manifest before that manifest has been produced. The first invocation only logs a warning and continues, leaving its generated registration table incomplete until it is run again.
The end-to-end test pre-creates the manifest, so it does not exercise this clean-build scenario. Could manifest generation be split into a separate analysis phase, or could the project provide an explicit deterministic two-pass orchestration? At minimum, silently continuing when a configured manifest is missing seems unsafe because it produces valid-looking but incomplete output.
| // rs/rt/rd from the raw instruction bits (see R5900Decoder::decodeInstruction), | ||
| // but for a 26-bit jump target those bit positions are part of the target, not a | ||
| // register field, so treating them as a register write would be spurious. | ||
| uint32_t writtenRegisterOrZero(const Instruction &inst) |
There was a problem hiding this comment.
this method treats rt as the destination of every non-SPECIAL, non-J-type instruction. That is not valid for stores, branches, and several coprocessor instructions. For example, sw $a0, ... reads $a0 but would currently invalidate the resolved $a0, causing a false negative.
There is also a false-positive direction: previous jal instructions are treated as writing no tracked register, so a resolved $a0 can survive across an arbitrary function call even though $a0 is caller-saved and may have been clobbered by the callee. The linear propagation walk also crosses control-flow instructions without checking whether the materialization dominates the CreateThread call.
Could this use an opcode-aware register-def helper and stop or invalidate state at earlier calls and control-flow boundaries? Restricting the analysis to the call site's basic block plus the delay slot would be more conservative.
Please add regression tests for at least:
lui $a0, ...
sw $a0, 0($sp)
jal CreateThread
addiu $a0, $a0, ...
and for an unrelated jal between the $a0 materialization and the CreateThread call.
| for (const auto &[functionStart, instructions] : decodedFunctions) | ||
| { | ||
| const size_t window = std::min(instructions.size(), kThreadWrapperScanWindow); | ||
| bool sawAddiuV1Syscall = false; |
There was a problem hiding this comment.
sawAddiuV1Syscall becomes true, the wrapper scan accepts any later syscall in the scan window, even if $v1 was overwritten in between.
For example, this is currently classified as a CreateThread wrapper even though the executed syscall number is 0x21:
addiu $v1, $zero, 0x20
addiu $v1, $zero, 0x21
syscall
That can cause ordinary calls to the function to be interpreted as CreateThread calls and arbitrary memory to be read as a ThreadParam.
Could the wrapper scan apply the same $v1 clobber-aware logic used by the inline-syscall scan and include a negative regression test for this sequence?
…e collector CollectExternalCallTargets gated inclusion on the target landing inside one of the caller's own executable sections. That drops every genuine cross-unit/overlay target by construction, since a callee living in a separately recompiled unit is never inside the caller's own sections - exactly the case this manifest mechanism exists to support. The caller cannot know a callee unit's section layout, so emission must not filter on the caller's own code sections. Replace the inclusion gate with a narrow exclusion: drop only targets landing inside the caller's own data/bss (the one real garbage source - a mis-decoded jal into non-code bytes), and otherwise emit every jal/j target outside every local recompiled function. The ingesting unit's findContainingFunction stays the authoritative filter over these candidates.
… hard-fail on a missing configured manifest Manifest emission depends only on this unit's own decoded functions and sections, never on any ingested sibling manifest, so a unit's emitted manifest is a fixpoint after one emission pass regardless of build order. Only ingestion depends on siblings having already run. A single build invocation could previously race a multi-unit build: unit A's generate pass would silently drop unit B's targets if B's manifest hadn't been emitted yet, and a genuinely missing manifest only produced a warning. Split recompile() into an emit-manifest-only analysis phase and the existing generate phase (recompile(bool emitManifestOnly = false)), wired to a new --emit-manifest-only CLI flag. A multi-unit build now runs every unit's analysis phase first (any order), then every unit's generate phase, by which point every configured sibling manifest is guaranteed to exist. Emission moves out of discoverAdditionalEntryPoints and into recompile() itself, running unconditionally in both phases. Since the two-phase flow removes any legitimate reason for a configured manifest to still be missing by generate time, loadExternalCallTargetManifests now hard-fails (propagating through recompile() to a non-zero process exit) instead of warning and continuing on an unreadable manifest path.
…ction for the thread-entry scan The $a0 constant-propagation walk tracked "who writes this register" via a single writtenRegisterOrZero helper (I-type -> rt, SPECIAL -> rd) with no concept of what an instruction actually reads vs. writes, and no bound on how far it could walk relative to control flow. That let a store of $a0 (which reads it) spuriously erase a resolved value, and let the walk trust a value across an unrelated intervening call or branch that does not dominate the CreateThread site - a walk with no block/dominance check can easily accept a stale register value as if it were still live. Replace it with mayWriteGprs: a small conservative superset of the GPRs an instruction may write, keyed off opcode (+ SPECIAL function) so unit tests built by hand behave identically to real decoded ELF instructions. Stores and coprocessor stores read their operand and are excluded; COP0/COP1/ COP2/MMI are treated conservatively as writing both rt and rd, since an MFC*-style GPR write can't be distinguished from an MTC*-style GPR read without sub-decoding fmt - always the safe direction (a missed entry, never a false survival). Bound the constant-propagation walk (and the Step-2 $v1 back-scan) to the call site's basic block: scan backward for the nearest preceding control-transfer instruction and start the walk after its delay slot. A basic block has no interior control transfer, so a call can never sit mid-block - a resolved $a0 can no longer survive across an unrelated call, and the walk never trusts a value across a branch/jump it doesn't dominate. Also close the one remaining gap this leaves: a forward jal/j elsewhere in the function landing inside the walked window is a join point the backward scan can't see, so shrink the walk to start at that join too.
…er clobber Step 1's wrapper-detection scan set sawAddiuV1Syscall on the first addiu $v1,$zero,0x20 seen and never cleared it, so a candidate wrapper whose $v1 was reassigned again before the syscall (e.g. addiu $v1,$zero,0x20; addiu $v1,$zero,0x21; syscall) was still misclassified as a CreateThread wrapper using the earlier, no-longer-live 0x20. Mirror the inline scan's own clobber handling: reset the flag on any later write to $v1 that is not itself the 0x20 materialization, via the same mayClobber predicate Step 2 already uses.
…-entry walk The basic-block backward-scan restriction already answers the dominance concern the maintainer raised. The extra forward jal/j join scan was incomplete (it can only see J/JAL joins; branch targets are PC-relative and never expressible as an absolute jump target), unpinned by any test, and contained an out-of-bounds instruction access when the call site was the last decoded instruction and its predecessor was a control transfer (walkStart == instructions.size()). Remove it; walkStart's only remaining consumer is the bounded resolution loop.
|
All four points were correct, thanks especially for catching that the cross-ELF emit path was never actually exercised. Reworked (follow-up commits; body updated):
|
2 participants
Discover packed jal-only entries via cross-unit manifests and thread-entry scan
This revision responds to all four review comments with the design each comment
converged on.
1. Emission is a permissive candidate collector; ingestion is authoritative
CollectExternalCallTargetsused to gate a candidate on landing inside one of thecaller's own executable sections. That excludes every genuine cross-unit/overlay
target by construction: a callee living in a separately recompiled unit is never
inside the caller's own sections, which is exactly the case this manifest mechanism
exists to support.
The caller cannot know a callee unit's section layout, so emission no longer filters
on the caller's own code sections. It now emits every jal/j target that lands outside
every local recompiled function, excluding only targets landing inside the caller's
own data/bss (the one real garbage source — a mis-decoded jal into non-code bytes).
Cross-ELF/overlay targets entirely outside every one of the caller's own sections are
now included. The ingesting unit's
discoverAdditionalEntryPoints→findContainingFunctionis unchanged and stays the authoritative filter: it accepts acandidate only when it lands on a real decoded instruction boundary inside a
recompiled, non-stub, non-skipped, non-entry function of that unit, so a garbage
candidate from another unit simply matches nothing and is dropped there.
2. Two-phase build: analysis phase, then generate phase; missing manifest hard-fails
Manifest emission depends only on a unit's own decoded functions and sections, never
on any ingested manifest — every unit's emitted manifest is a fixpoint after one
emission pass, independent of build order. Only ingestion depends on siblings having
already run.
recompile()gains anemitManifestOnlyparameter (recompile(bool emitManifestOnly = false), default preserves every existing call site). Emission nowruns unconditionally at the top of
recompile(), in both modes. In analysis-onlymode,
recompile()stops right after emitting, before ingesting any manifest ordiscovering entry points. A new
--emit-manifest-onlyCLI flag drives this fromps2recomp. A multi-unit build runs every unit's analysis phase first (any order,since it never depends on siblings), then every unit's normal generate phase, by
which point every configured sibling manifest is guaranteed to exist.
Since the two-phase flow removes any legitimate reason for a configured manifest to
still be missing by generate time,
loadExternalCallTargetManifestsnow returnsbooland hard-fails (reported and propagated throughrecompile()to a non-zeroprocess exit) when a configured manifest path cannot be opened, instead of warning
and silently continuing. No escape hatch — the two-phase flow is the supported way to
avoid tripping this.
3. Conservative def model + basic-block restriction for the thread-entry scan
The
$a0constant-propagation walk previously modeled "who writes this register" asa single
writtenRegisterOrZerohelper (I-type → rt,OPCODE_SPECIAL→ rd) with noconcept of what an instruction actually reads vs. writes, and with no bound
relative to control flow. That let a store of
$a0— which reads it, not writes it —spuriously erase a resolved value, and let the walk trust a value across an unrelated
intervening call or branch that does not dominate the
CreateThreadcall site.Two independent mechanisms fix this, and both are required:
mayWriteGprs(inst): a conservative superset of the GPRs an instruction may write,keyed off opcode (+
SPECIALfunction) rather than the decoder-derivedmodifiesGPR/isStore/… booleans, so hand-built unit-test instructions and realdecoded ELF instructions behave identically. Stores and coprocessor stores read
their operand and are excluded.
COP0/COP1/COP2/MMIare treatedconservatively as writing both
rtandrd, since anMFC*-style GPR write can'tbe distinguished from an
MTC*-style GPR read without sub-decodingfmt— this isthe accepted, safe-direction tradeoff: it can only cost a missed entry (e.g. an
mtc1 $a0that only reads$a0), never let a stale value survive.$v1back-scan in Step 2) is now bounded tothe call site's basic block: scan backward for the nearest preceding
control-transfer instruction and start the walk right after its delay slot. A basic
block has no interior control transfer, so a call can never sit mid-block — a
resolved
$a0can no longer survive across an unrelated intervening call, and thewalk never trusts a value across a branch/jump it doesn't dominate.
writtenRegisterOrZero/writesTrackedRegisterare retired; both call sites now gothrough
mayWriteGprs/mayClobber.4. Wrapper scan resets on a later
$v1clobberStep 1's
CreateThread-wrapper detection set a flag on the firstaddiu $v1,$zero,0x20seen in a function's first few instructions and never clearedit, so a candidate wrapper whose
$v1was reassigned again before the syscall (e.g.addiu $v1,$zero,0x20; addiu $v1,$zero,0x21; syscall) was still misclassified as aCreateThreadwrapper using the earlier, no-longer-live0x20. The scan now resetson any later write to
$v1that is not itself the0x20materialization, mirroringthe inline scan's own clobber handling and reusing the same
mayClobberpredicate.Testing
Build and run the full suite:
Read the
Failed: 0line at the end rather than relying on a pasted count — it goesstale the moment a test is added on either side of a rebase.
Highlights, each independently runnable by name against the
ps2xTesttarget:two-ELF: A emits T, B ingests T):drives two real, independent
PS2Recompilerinstances over two separate ELFfixtures. Unit A's
jaltargets an addressTthat exists only inside unit B andlies entirely outside every section of A. A's analysis phase emits
T(order-independent of B's own analysis phase — checked byte-for-byte); B then ingests A's
manifest and registers
Tinto its own owning function; a negative arm confirmsTis not registered when B has no manifest configured.
missing configured manifest hard-fails at generate time/existing (even empty) configured manifest does not hard-fail: the hard-fail half-guard pair.analysis phase never hard-fails on a missing sibling manifestandtwo-phase clean build of mutually-calling units: the two-phase orchestration guarantees, the latterdriving two ELFs that call into each other's mid-body targets through a full
analysis-then-generate cycle for both.
store between materialization and call does not clobber $a0,strong load-clobber pin, andunrelated jal between materialization and CreateThread is not resolved:the store/load def-model pins and the basic-block-restriction pin (the maintainer's
exact negative sequence).
wrapper scan resets on a later $v1 clobber/wrapper scan reset is conditional on a $v1 write (positive half-guard): the wrapper-scan reset pair (the maintainer'sexact negative sequence, plus its half-guard).
CollectExternalCallTargetsselection test that codifiedthe bug is split into two: a foreign/overlay target outside every section of the
unit is now collected, and a target landing inside the caller's own data section is
still excluded. The other four selection tests in that group are unchanged.
Every pinned property here has a source mutation that isolates it — see the evidence
doc alongside this PR for the full mutation-to-test table, including the couple of
mutations whose blast radius is broader than a single test (documented there with
why, rather than narrowed to fit a false single-test story).