perf(relinker): replace O(n²) zero-padding scan in StrictReachability

[silentone12725]

What

addGap and buildEdges each called isZeroPadding(address, end) at every instruction boundary. isZeroPadding scans from the current address to the region end, so the total work for a region with N instructions followed by a trailing-zero suffix of length Z was O(N×Z) — quadratic when Z is large.

Replace with trailingZeroStart, which scans backwards from the region end once to find the start of the trailing-zero suffix. The decode loop then uses that position as its stopping point, removing the per-instruction zero check entirely. The decode call itself continues to receive the original region end as the byte limit so that multi-byte instructions whose body contains zero bytes are decoded correctly.

buildEdges previously reset lastFlow to Sequential when the walk met zero padding, which gave a region ending in a jump or return followed by padding its fall-through edge. The reset is kept: it applies when the walk stops before region.End, not when the last instruction merely ends in zero bytes. fallthroughAfterZeroPadding in StrictNidFilterTests.cpp covers it and fails without the reset.

This PR no longer carries the ElfReader.cpp change; that stays in #1521.

Tested

• Linux: strict_nid_filter_tests passed, including the new fallthroughAfterZeroPadding case, which fails when the reset is removed.

• Conventions check: passed (python3 tools/check_conventions.py --base origin/main)

• Timing (AnalyzeStrictReachability, one run of zero bytes followed by two nops, g++ -O2, best of 3; first reported by @Dextroid17, re-measured here):

  zero bytes	main	this PR
  25 000	45.6 ms	1.0 ms
  50 000	182.8 ms	2.0 ms
  100 000	718.1 ms	4.2 ms
  200 000	2898.7 ms	8.8 ms

  main quadruples with every doubling, this PR doubles. Results are identical at every size.

• Differential fuzz: 30 000 random inputs (jumps, returns, import thunks, UD2, zero runs of 1-40 bytes, optional trailing padding, optional function regions) give the same result or the same exception on main and this PR. The same fuzz flags 23 inputs when the lastFlow reset is removed and 9 405 with the earlier effectiveEnd != region.End condition.

• Build workflow run on the fork, dispatched on head cfb2198c: passed for Linux, Windows, and the relinker tests on Ubuntu (g++), macOS (clang++) and Windows (g++) (run 37833572111). It is not in the PR checks because the upstream run for this head is waiting for maintainer approval (CI: 38 open PR heads are queued at action_required, so their PRs show no checks #1631).

Changes since the first revision

• Review by @Dextroid17 found that dropping the padding break also dropped the lastFlow = Sequential reset, so a region ending in a jump or return followed by zero padding lost its fall-through edge. Fixed by resetting lastFlow when the walk stops before region.End.
• The first suggested condition (effectiveEnd != region.End) broke the existing ud1DeadTail case, because zero bytes inside the last instruction's own operand are not padding. The condition used here is address < region.End.
• Added fallthroughAfterZeroPadding to StrictNidFilterTests.cpp; it fails without the reset.
• The ElfReader.cpp hunk is removed from this PR; it belongs to fix(relinker): reject zero section header entry size in ElfReader #1521.
• Added measured timings and a differential fuzz against main (see Tested).
• Rebased onto current main.

Checklist

• Based on current main; no other open PR implements the same functions
• One topic per PR; follow-ups go in a new PR
• No comments in code except technical debt
• Unimplemented paths throw; silent stubs are listed in TechnicalDebt
• No notes, investigation .md files or images added to the repository
• New third-party code is a submodule built from source
• Generic behaviour, not specific to one title
• Depends on: none
• AI-assisted: yes

[klosixgaming-arch]

Good performance optimization that eliminates the O(n²) zero-padding scan in StrictReachability. The approach of finding the trailing-zero start once and using it as a stopping point is elegant and efficient.

Strengths:

• Clear explanation of the problem (quadratic complexity with large trailing-zero suffixes)
• Simple, focused change that doesn't alter behavior
• Proper testing with existing test suite

Minor notes:

1. The PR is marked as draft with "needs rebase" label - appropriate given main has moved forward
2. Consider whether this should be combined with #1523 which also touches StrictReachability.cpp, to avoid merge conflicts

This looks ready to merge once rebased and CI passes.

[klosixgaming-arch]

Good performance optimization and correctness fix. This PR addresses two important issues in the relinker.

Strengths:

• StrictReachability: Smart optimization replacing O(n²) zero-padding scan with a single backward scan to find the trailing zero start position - this will significantly improve analysis time for large code regions with padding
• ElfReader: Important correctness fix adding validation for zero entry sizes in program and section headers, preventing memory exhaustion from crafted ELF files

Technical notes:

• The approach of computing effectiveEnd once per region and using it as the decode loop's stopping point is clean and efficient
• Proper handling of edge cases (empty regions, all-zero padding)
• Good documentation of both issues in the PR description

These changes improve both performance and robustness without changing behavior. Ready to merge.

[silentone12725]

Hi @klosixgaming-arch, the branch has been rebased onto main and conventions checks pass. Would you be able to merge this when you get a chance? Thanks!

[Dextroid17]

Verified this with a standalone harness against main's analyser and yours, on two shapes: the one your title is about, and one where the change is not equivalent. Both numbers are reproducible from the commands at the end.

1. The perf claim holds — and here is the timing your description does not have. Shape: a 99 998-byte run of zero bytes (49 999 add [rax],al instructions) with two nops at the far end, so the tail is never all-zero and isZeroPadding is called at every instruction boundary, each call scanning to the next non-zero byte:

main   zeros=100000 (99998 zero + 2 nop)   378.825 ms    instructions: 50001   regions 1/1
#1522  same input                            3.419 ms    instructions: 50001   regions 1/1

111× faster, identical results. Worth putting in the description — a perf(...) PR currently has no number in it, and the maintainer's review of the sibling PRs asked for exactly that.

2. But it is not a pure refactor: one edge is lost. buildEdges reads lastFlow after the loop:

if (!endsFlow(lastFlow) && isCode(region.End)) region.Edges.insert(region.End);

and the loop you replaced set lastFlow = Codegen::ControlFlowKind::Sequential before breaking on zero padding. That reset is what made the region fall through to region.End. Without it, lastFlow is the flow kind of the last decoded instruction, so a region whose code ends with an unconditional jump or a return followed by zero padding no longer gets that edge.

Harness case: A = [0x1000,0x1010) is one jmp rel32 then zero padding; B = [0x1010,0x1018) is an FF 25 import thunk for slot 0x2000:

main    live imports: 1  (0x2000)   regions live/total: 3/3   instructions: 2
#1522   live imports: 0             regions live/total: 1/3   instructions: 1

The strict filter can therefore drop an import that main keeps — the kind of difference that shows up as a title failing to resolve a NID after conversion, not as a test failure, which is why the suite passes either way.

3. The fix is one line, and I checked that it restores it. After the loop:

if (effectiveEnd != region.End) lastFlow = Codegen::ControlFlowKind::Sequential;

fixed   live imports: 1  (0x2000)   regions live/total: 3/3   instructions: 2

(effectiveEnd != region.End is exactly "the walk met the padding", which is when the old loop reset lastFlow.)

4. This PR also carries #1521's change. core/relinker/relinker/src/parsing/ElfReader.cpp +6/-0 is the same two zero-entry-size guards as #1521 — a second topic in a perf PR. Beyond the one-topic rule, both guards are unreachable on main: ReadProgramHeaders already throws when ProgramHeaderCount != 0 && ProgramHeaderEntrySize != 56 (a superset of == 0, and a run reports that message), and ReadSectionHeaders() has no caller in the tree. So merging this would land dead code as a side effect of a perf fix. I would drop that hunk here and let #1521 stand on its own.

Reproducing any of this:

# the analyser, from the two revisions, with a ~40-line harness (no full build needed)
g++ -std=gnu++20 -O2 -I core/relinker/domain/include -I core/relinker/elfpatcher/include \
    -I core/relinker/codegen/include -I core/relinker/relinker/include harness.cpp \
    <main-or-pr-StrictReachability.cpp> \
    core/relinker/codegen/src/x86/X64InstructionDecoder.cpp \
    core/relinker/relinker/src/analysis/UnusedNidFilter.cpp \
    core/relinker/relinker/src/analysis/UnusedNidFilter/ControlFlowGraph.cpp \
    core/relinker/relinker/src/analysis/UnusedNidFilter/EntryPointCollector.cpp \
    core/relinker/relinker/src/analysis/UnusedNidFilter/RelativeRelocationIndex.cpp \
    core/relinker/relinker/src/analysis/UnusedNidFilter/GotAccessIndex.cpp -o harness

Happy to paste the harness inline if that is easier than re-deriving it — it prints the live imports, the live/total region counts and the timing above.

Written with AI assistance.

[adriadam10]

Checked the new trailingZeroStart loop against the old per-instruction check: same stop point, and lastFlow is reset to Sequential exactly when the walk ends before region.End. Relinker ctest (42 tests, incl. strict_nid_filter) green locally; removing the address < region.End reset makes fallthroughAfterZeroPadding fail. CI green on the head. Merging.