Skip to content

fix(aarch64): stop recording a branch target as an inbound call reference - #307

Merged
danielplohmann merged 3 commits into
danielplohmann:masterfrom
r0ny123:upstream-aarch64-tailcall-seed-evidence
Sep 7, 2026
Merged

fix(aarch64): stop recording a branch target as an inbound call reference#307
danielplohmann merged 3 commits into
danielplohmann:masterfrom
r0ny123:upstream-aarch64-tailcall-seed-evidence

Conversation

@r0ny123

@r0ny123 r0ny123 commented Aug 30, 2026

Copy link
Copy Markdown
Contributor

Candidate discovery infers an alignment that function starts obey on a given image and refuses a candidate below it. Two things are exempt: an exception record, and an inbound call reference. Both are the image's own evidence that the address is an entry — the comment on that exemption in common/FunctionCandidateManager.py says exactly that.

The AArch64 backend seeds a candidate at the target of a backward branch or of a short no-frame stub, and the AArch64 candidate manager recorded the branching instruction as an inbound call reference for it.

A branch is not a call. So the seed manufactured the very evidence the alignment floor trusts, and mid-function addresses went through the exemption. That is worse than a stray false positive: on a routine only the gap scan recovers, the smuggled interior address is analysed in the first pass, takes the bytes, and the real start is never reported at all.

The shared implementation in common/ records no reference. This was an AArch64-only override.

Fix

Book the candidate; do not record the reference.

-        d.fc_manager.addTailcallCandidate(target, reference_source=i_address)
+        d.fc_manager.addTailcallCandidate(target)

The address is still discovered, still queued, still analysed — it just competes on what it actually has (its alignment, its prologue if it has one) instead of on a call that does not exist. With the parameter's only caller gone, the override drops it and the dead score_changed bookkeeping with it; what remains over the base class is the queueing the base leaves to its caller.

Measured

Three corpora, exact-address match, macro means. Ground truth is a compiler's, a linker's, or the Go runtime's — never another disassembler's.

corpus n PPV TPR TP FP
Go (pclntab) 47 before 94.293 99.367 165,627 10,514
after 94.369 99.367 165,627 10,354
AArch64 ELF (gcc cross, symbol tables) 72 before 80.554 95.964 54,979 7,968
after 80.566 95.966 54,982 7,953
ARM64 Mach-O (LC_FUNCTION_STARTS) 11 before 94.220 96.711 2,532 235
after 94.220 96.714 2,532 235

175 false positives removed, 3 functions recovered, no corpus loses recall.

Two details worth having rather than hiding:

  • On the AArch64 ELF set the per-cell picture is not uniformly better — 22 of 72 cells move, 33 false positives go and 18 appear. A candidate analysed later leaves different bytes for the gap scan. No cell loses a true positive.
  • On Mach-O the totals are flat because one binary gains a function (LockBit_3e4bbd21756a) and one loses one (RustyPages_e98756472404, at 0x10000548c). That is the single recall cost anywhere in the three corpora, and it is offset inside the same corpus.

What the removed addresses actually are

On netjson_darwin-arm64_default, 42 addresses stop being functions. All 42 are false positives, and all 42 sit strictly inside a real function — interior words, which is what the alignment floor exists to refuse and what the manufactured reference was letting through.

Two alternatives, both measured and both rejected

Gate the whole seed behind RESOLVE_TAILCALLS, as the sibling bl fall-through site does: removes 339 false positives but costs 26 true positives, on RustyPages_e98756472404 (−17), osx.frostyferret_ef27a525ec0b (−4) and osx.poseidonstealer_a494eb0be9d1 (−5). A recall drop is the reject criterion.

Exempt is_tailcall from the alignment floor, so the removed reference is not what keeps a genuine target eligible:

Go PPV Go FP Mach-O TP
baseline 94.293 10,514 2,532
this branch 94.369 10,354 2,532
this branch + the exemption 94.293 10,514 2,532

It gives back all 160 Go false positives, per cell exactly, because every candidate this site seeds is a tailcall candidate — so exempting the flag re-admits the whole population. And it does not save the one function it was proposed to save: RustyPages is 501 either way. On that image identified_alignment is 4 and 0x10000548c is 4-aligned, so the floor never refused it; that loss is analysis order, not eligibility, and a remedy aimed at the floor cannot reach it.

Tests

tests/testAArch64TailcallSeedEvidence.py, new:

  • End-to-end. An image with 22 call-referenced 16-aligned leaves, so the floor really does infer 16; one 16-aligned routine only the gap scan can find; and a routine ending in a backward branch into that routine's body at a merely 4-aligned word. Asserts the interior word is not a function and that the routine it sits inside is recovered. Before the fix both fail: the interior word is a function and the real start is missing.
  • Unit. A booked tailcall carries no inbound call reference and the floor then refuses it — with the control that an address something really calls is still exempt at the same alignment, so the exemption is not removed, only stopped from being manufactured.
  • Control. The branching routine and the driver are recovered either way.

tests/testLateCandidateProduction.py stops passing a reference source. Those tests are about late candidates reaching the queue rather than about scoring, and their addresses are 16-aligned so they score above zero without one. The one scenario whose subject really was the call reference is rewritten to assert what now carries it — alignment alone.

Validation

ruff check / ruff format --check pass, full suite green on this branch rebased onto current master (1820 passed, 1 skipped, 2584 subtests), diff coverage 100% on the changed lines. No frozen fixture baseline moves and the fixture benchmark gate is green.

Relationship to #299 and #300

This change is also sitting inside #299 and #300. Both of those were opened from branches that had my fork's master merged into them repeatedly, so their diffs are much wider than their titles suggest and they overlap each other as well.

This PR is the same change on its own, rebased onto current master, so it can be reviewed and measured for what it actually is. If you would rather take it through one of those, close this one; if you take this one, the matching hunks fall out of theirs on rebase.

The ty commit at the tip

The last commit on this branch is #302's fix, cherry-picked unchanged.

Master fails Code Quality on its own right now: 572acd7 bumped ty to 0.0.74, and its new unsound-return-statement / unsound-assignment rules become errors under [tool.ty.rules] all = "error". That is 41 errors across 15 files, none of which this PR touches — so every PR opened against master inherits a red check that says nothing about its own diff.

Carrying #302's commit here means this PR's CI reflects only this PR's work. It is the same commit, unmodified, so the moment #302 lands this one is empty and falls out on rebase — there is nothing to untangle later. If you would rather look at this branch without it, merge #302 first and I will drop it.

Checked against ty 0.0.74 itself rather than whatever an older local pin resolves to: on master it exits 1 with 41 errors, on this branch it exits 0 with none.

…ence

Candidate discovery infers an alignment that function starts obey and
refuses a candidate below it, with two exemptions: an exception record,
and a call reference. Both are the image's own evidence that an address is
an entry. The comment on that exemption says so.

The AArch64 backend seeds a candidate at the target of a backward branch
or a short no-frame stub, and its candidate manager recorded the branching
instruction as an inbound *call* reference for it. A branch is not a call.
That put mid-function addresses through the exemption, where they outrank
the routine they sit inside -- and on a routine the gap scan is what
recovers, the interior address was analysed first and took the bytes.

Booking the candidate is kept. Only the manufactured evidence goes, so the
address is still discovered and now competes on what it actually has.

Measured against ground truth on three corpora, macro means, all cells:

  Go, 47 cells, pclntab truth
    PPV 94.293 -> 94.369 at TPR 99.367 -> 99.367, 160 fewer false
    positives, no true positive lost
  AArch64 ELF, 72 cells, compiler symbol tables
    PPV 80.554 -> 80.566 at TPR 95.964 -> 95.966, 15 fewer false
    positives, 3 more functions recovered, no cell loses one
  ARM64 Mach-O, 11 cells, LC_FUNCTION_STARTS
    PPV 94.220 -> 94.220 at TPR 96.711 -> 96.714, false positives
    unchanged, one binary gains a function and one loses one

No corpus loses recall. 175 false positives go and 3 functions come back.
Per cell it is not uniformly better -- on the AArch64 ELF set 33 false
positives go and 18 appear, because a candidate analysed later leaves
different bytes for the gap scan -- but no cell trades a true positive for
it.

Gating the whole seed on RESOLVE_TAILCALLS was measured as the alternative
and rejected: 339 fewer false positives but 26 true positives lost, on
three Mach-O binaries that nothing else reaches.

The intel backend is untouched. It uses the shared base, which records no
reference and is unchanged.
…cstring

The two comments describing what each part of the image is for say why the
fixture is shaped that way, which is one fact and belongs in one place. The
two that stay decode a magic constant and record how many entries the
alignment inference needs, neither of which the code can say.
Keep the 0.0.74 bump and satisfy the new unsound-return/assignment/yield
rules with annotations on the public DTO and helper surfaces. Ignore ty
in Dependabot so the next patch release cannot redden master unattended.
@danielplohmann
danielplohmann merged commit 53e72c3 into danielplohmann:master Sep 7, 2026
26 checks passed
danielplohmann added a commit that referenced this pull request Sep 7, 2026
Five small corrections noticed while reviewing #302 through #311, each one
something a merged PR left slightly short of right and none worth a PR of
its own. Collected in #317 as they were found.

`README`: the address-space section told a consumer the backend is named
"in two places, report.architecture and metadata.language". The second is
wrong in the direction the section exists to prevent. metadata.language is
a source-language score map -- a single decisive entry on the managed
backends ({'.net': 1.0}, {'dalvik': 1.0}) but a distribution on the native
ones that carries a .net score -- so branching on .net appearing in it can
read a native report as managed and then treat virtual addresses as file
offsets. The README already stated the score-map contract 130 lines below,
so it contradicted itself. report.architecture is authoritative and is now
named as the field to branch on.

`smda_instruction_matches_capstone` gets its `-> bool` back. #302 dropped
the annotation to satisfy ty 0.0.74's unsound-return-statement, but the
cause is that capstone_instruction is untyped, so `.size` is Unknown and
the comparison infers Unknown. bool() around the return makes the declared
type true instead of removing the declaration.

`.pre-commit-config.yaml` moves to `id: ruff-check`. At ruff-pre-commit
v0.16.x the hook reports itself as "ruff (legacy alias)"; astral renamed it,
and the alias will presumably go in a future major.

`GAP_SEQUENCES` gains a note that it is read in seven places outside the
alignment cut, so an encoding added there changes all padding-aware
discovery rather than one caller -- which is what makes a measurement over
a change to that table hard to attribute.

`test_late_tailcall_does_not_rebuild_queue` says what it is for now that
#307 removed the scoring path it guarded, so a future reader does not have
to work out whether a trivially-true assertion is load-bearing.

Two items from #317 are deliberately not here, because both need work
upstream of the annotation rather than the annotation itself, and #317 now
records what each one runs into:

  - typing SmdaFunction.blocks as Dict[int, List[SmdaInstruction]] is
    accurate and does restore getInstructionsForBlock's return type, but it
    surfaces two no-matching-overload errors where getOpcHashSequence and
    its sibling join instruction.bytes, which is Optional[str]. In practice
    a block instruction always carries bytes, so this is latent rather than
    live -- but it is a real soundness gap and wants its own change.

  - extract_strings' Tuple[str, Any, Any, str] cannot be narrowed while
    read_string, read_go_string and derefs are untyped: every element of
    the yielded tuple infers Unknown, so even `str` in the first position
    is unprovable. Typing those three comes first.

Validation: ty check exit 0 with the error count unchanged at zero and the
warning count unchanged, ruff check and ruff format --check clean, both
pre-commit ruff hooks pass under the new id, full suite 1882 passed, 2
skipped, 2591 subtests.

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
@r0ny123
r0ny123 deleted the upstream-aarch64-tailcall-seed-evidence branch September 7, 2026 17:38
danielplohmann added a commit that referenced this pull request Sep 8, 2026
Finishes the draft now that #309, #311 and #312 have landed. Adds their
sections, bumps `VERSION` and `__version__` to 4.5.1, and moves the date to the
release day.

The three new entries: #309's exception-directory refusal of interior gap
candidates on x64, with the chained-versus-primary distinction and the
seeding-is-not-suppression principle that keeps carved records out of it; #311's
four candidate-quality defects, including the corrected entry at `0x40df30` on
`aarch64_static_xored` and the metadata-coverage gate; and #312's ARM64
counterpart, which reconstructs the extent an ARM64 `RUNTIME_FUNCTION` does not
carry and hoists the shared lookup into `common/`.

`USE_MACHO_ADDRESS_REF_CANDIDATES` gets its own **Defaults changed** section. It
is the only default whose value moves in this release, it changes recovery
output on Mach-O input with no action by the caller, and a default going from
off to on inside a follow-ups PR is exactly the kind of thing that gets lost.

The closing movement section is rewritten to describe v4.5.0 -> v4.5.1 rather
than any single PR, and both figures in it were measured on this tree against
independent truth rather than quoted:

  rust_pe_gnu_xored, vs its 2,186 retained COFF .text function symbols
    2,355 -> 2,201 starts, +26 real / -180 false
    recall    92.452% -> 93.641%
    precision 85.817% -> 93.003%

  11-sample ARM64 Mach-O corpus, vs LC_FUNCTION_STARTS (2,056 truth)
    TP 1,787 -> 1,818, FP 1,113 -> 1,107
    recall 86.916% -> 88.424%, no sample losing recall

Two headline figures are marked as the contributor's own because they cannot be
reproduced here at all: the `bti j` 803/0 split, since no bundled fixture holds
a single `bti j` word, and #312's ARM64 PE result, since a scan of all 104
fixture files finds 8 i386 PEs, 4 AMD64, one ReadyToRun image and no `0xAA64`.
Saying which figures are ours and which are theirs is more useful than a uniform
tone of confidence.

`Turtle_5f9cd91d8d1d`'s 12 dropped starts are attributed rather than left as an
unexplained delta: they bisect to #307's inbound-call fix and fall inside the
175 false positives that change already measured, and the sample carries no
LC_FUNCTION_STARTS to score them against individually.

Closes #319

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MDCLUmqGzE5fwJqUWsSStr
danielplohmann added a commit that referenced this pull request Sep 8, 2026
* chore: draft the v4.5.1 release notes

The changelog entry for what has landed since v4.5.0 (6cc5aca): #302, #303,
#304, #305, #306, #307, #308, #310 and #316, plus the Dependabot ty bump
572acd7 that #302 had to answer for.

Held as a draft rather than a release. The version is deliberately NOT
bumped here -- #309, #311 and #312 are reviewed and wanted but waiting on a
rebase, and splitting one coherent accuracy pass across two releases makes
both entries weaker. What this commit does is stop the writeup living in a
scratchpad while that happens.

To finish it: add sections for #309, #311 and #312, bump VERSION in
src/smda/SmdaConfig.py and __version__ in src/smda/__init__.py, and move the
date to the actual release day.

No escaper output changed anywhere in this set -- the only edit to
intel/definitions.py adds two GAP_SEQUENCES entries, which feed padding
detection and not escaping, and testEscaperFingerprint passes unchanged --
so ESCAPER_DOWNWARD_COMPATIBILITY stays at 4.4.5 and
INTEL_PIC_HASH_ESCAPE_VERSION at 4.3.5, and no report needs reprocessing.

Figures are the contributors' own except where this session reproduced them,
and the two that were reproduced are stated as measured here: the padding
cut's analysis-time cost (+3.4% claimed, +3.84% measured) and the fixture
movement on rust_pe_gnu_xored (+26 real starts against +8 false, scored
against that fixture's own COFF symbol table rather than the corpus macro
mean).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* chore: fold #318 into the v4.5.1 release notes

#318 landed as 395c88d after this entry was written. Its five corrections go
under Housekeeping: the README's metadata.language claim, the restored
`-> bool`, the ruff-check hook rename, the GAP_SEQUENCES reach note, and the
docstring on the queue-rebuild test.

Keeping this current as things merge is the point of the branch existing, so
it does not go stale between now and the release.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* chore: cut v4.5.1

Finishes the draft now that #309, #311 and #312 have landed. Adds their
sections, bumps `VERSION` and `__version__` to 4.5.1, and moves the date to the
release day.

The three new entries: #309's exception-directory refusal of interior gap
candidates on x64, with the chained-versus-primary distinction and the
seeding-is-not-suppression principle that keeps carved records out of it; #311's
four candidate-quality defects, including the corrected entry at `0x40df30` on
`aarch64_static_xored` and the metadata-coverage gate; and #312's ARM64
counterpart, which reconstructs the extent an ARM64 `RUNTIME_FUNCTION` does not
carry and hoists the shared lookup into `common/`.

`USE_MACHO_ADDRESS_REF_CANDIDATES` gets its own **Defaults changed** section. It
is the only default whose value moves in this release, it changes recovery
output on Mach-O input with no action by the caller, and a default going from
off to on inside a follow-ups PR is exactly the kind of thing that gets lost.

The closing movement section is rewritten to describe v4.5.0 -> v4.5.1 rather
than any single PR, and both figures in it were measured on this tree against
independent truth rather than quoted:

  rust_pe_gnu_xored, vs its 2,186 retained COFF .text function symbols
    2,355 -> 2,201 starts, +26 real / -180 false
    recall    92.452% -> 93.641%
    precision 85.817% -> 93.003%

  11-sample ARM64 Mach-O corpus, vs LC_FUNCTION_STARTS (2,056 truth)
    TP 1,787 -> 1,818, FP 1,113 -> 1,107
    recall 86.916% -> 88.424%, no sample losing recall

Two headline figures are marked as the contributor's own because they cannot be
reproduced here at all: the `bti j` 803/0 split, since no bundled fixture holds
a single `bti j` word, and #312's ARM64 PE result, since a scan of all 104
fixture files finds 8 i386 PEs, 4 AMD64, one ReadyToRun image and no `0xAA64`.
Saying which figures are ours and which are theirs is more useful than a uniform
tone of confidence.

`Turtle_5f9cd91d8d1d`'s 12 dropped starts are attributed rather than left as an
unexplained delta: they bisect to #307's inbound-call fix and fall inside the
175 false positives that change already measured, and the sample carries no
LC_FUNCTION_STARTS to score them against individually.

Closes #319

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MDCLUmqGzE5fwJqUWsSStr

* chore: record binweight's serialized type change in the v4.5.1 entry

Left open as a question in yesterday's review and resolved as "yes, it belongs
in the entry". #302 moved `SmdaFunction.binweight`'s class default from int `0`
to float `0.0` while clearing ty 0.0.74's diagnostics, and `binweight` is
serialized through `toDict()`.

The blast radius is small but real: the per-block accumulation already adds
`float(...)`, so every function with at least one block was a float before this
release too. Only a function with no blocks -- a zero-function or error report
-- keeps the class default, and that value now writes as `0.0` where it wrote
`0`. Verified on both trees rather than reasoned about: `SmdaFunction.binweight`
is `0` at 6cc5aca and `0.0` at d111548.

MCRIT stores these reports, so a consumer diffing them byte-for-byte will see
it even though nothing reads the field as an integer. That is exactly the kind
of change that costs someone an afternoon if it is not written down.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MDCLUmqGzE5fwJqUWsSStr

* chore: fold the two remaining #317 items into the v4.5.1 entry

#320 and #321 finish the review pass that #318 started, so the release entry
should carry all seven of its corrections rather than five.

Both are behaviour-neutral typing work, but the Housekeeping text says what each
one actually decided rather than listing them as annotations: `blocks` being
typed forces the PIC/OPC hash path to refuse an instruction with no bytes
instead of blanking it, and `extract_strings` could not be narrowed until three
helpers and three `SmdaReport` attributes were declared first.

Behaviour-neutrality is stated with the evidence rather than as a claim -- the
corpus benchmark ran on #320 and reports 0 of 155 files differing across
175,946 functions, which is a stronger statement than the bundled fixtures can
make.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MDCLUmqGzE5fwJqUWsSStr

* chore: format the v4.5.1 entry as a nested list

@r0ny123 asked on #319 whether the changelog could be formatted as a list to
make it easier to read. At ~2,700 words on a single line, this entry was the one
that provoked the question, so it gets the treatment now rather than waiting for
the wider keep-a-changelog adoption tracked in #323.

Deliberately narrow: only the v4.5.1 entry changes shape. The other 192 entries
keep the one-line format, because converting them is a large, low-value rewrite
that loses nuance in translation, and #323 proposes preserving them verbatim
under an `Older releases` heading instead.

Structure is the release summary as the top-level bullet, one sub-bullet per
section, and a further level inside the three sections that cover more than one
change -- Recovery (intel) splits into the switch-table fix, the alignment cut
and the exception-directory refusal; Recovery (AArch64) into the inbound-call
fix, the BTI work and the candidate-quality follow-ups; Housekeeping into the
`ty` bump, the `ruff` pin, the five #318 corrections, the `binweight`
serialization note and the two #317 typing items.

The text is unchanged. Verified mechanically rather than by eye: stripping list
markers and collapsing whitespace gives a string identical to the original entry.
The two exceptions are disclosed rather than silent -- the first sub-bullet of
Recovery (intel) and of Recovery (AArch64) had their opening letter capitalised,
because splitting the section header onto its own line left them starting a
bullet mid-sentence. Housekeeping's opens on a code span and needed nothing.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MDCLUmqGzE5fwJqUWsSStr

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
danielplohmann added a commit that referenced this pull request Sep 10, 2026
Three review findings on #300, all in text or in one expression, fixed here rather
than sent back for another round.

The config comments for USE_LSDA_LANDING_PADS and USE_ELF_FDE_INTERIOR_GAPS carried
the per-rule figures measured before #304/#307/#309/#310/#311 landed -- the same
baseline the PR description retracted, because master's own PPV on that corpus had
moved from 76.676 to 91.497 while the branch waited. RESOLVE_TAILCALLS likewise still
described the AArch64 bl fall-through gate as it measured before #307 changed what
addTailcallCandidate does, and omitted the AArch64 ELF corpus entirely, which is the
one row where the gate is a trade. All three now carry the re-measured attribution
against the tree #300 landed on, and the tailcall block says what the 53 lost true
positives are and how USE_ELF_EH_FRAME_CANDIDATES repays them. A description records
a conversation; a config comment is what the next reader has.

getExceptionDirectory matched the data directory with "EXCEPTION" not in
str(directory.type), a substring of the enum's repr that would also claim any future
type whose name contains it. The typed lookup is already the idiom in two other
places, one of them the AArch64 walk over the same directory.

The aarch64_static baseline moved by two starts and the test comment explained one
of them. 0x40DF34 is refused, but it was not recovered before either, so the pair the
fixture actually lost is 0x400350 and 0x40DF30 -- both mid-function instructions
inside a declared FDE, which is a plainer reading than the Binary Ninja disagreement
the comment led with. 0x400350 is now asserted alongside the other two.

Also removes a comment left dangling in intel/FunctionCandidateManager.__init__ by
#312, which hoisted the attribute it documented into common/ where the same text
already sits.

Suite 2060 passed, 1 skipped, 2593 subtests; ruff clean; ty exit 0 with 0
error-level diagnostics.

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants