Conversation
A review flagged the unguarded `target < b.end` back-edge check as a weakening of the x64 assertion. It is not: `target < b.end` is the correct, architecture-neutral predicate. collect_basic_blocks ends a block at its terminating branch, omits call targets from `branches`, and stores a conditional's fallthrough as a target equal to b.end -- so `< b.end` excludes fallthrough and forward edges and matches exactly the backward (loop) targets, including a target that lands inside a block after MSVC alignment padding. The stricter `target <= b.start` is the wrong one: it misses the interior landing. Verified empirically -- with `<= b.start` the x86 fixture FAILS (found_back_edge == false) while x64 passes, confirming x86 exercises the interior-target case and the neutral predicate is required, not a weakening. Predicate unchanged; only the comment is expanded to explain this and prevent re-flagging. ctest x86 24/24, x64 24/24; stress loop_correctness passes on both.
exhaustive_decode kept a thread-local open-addressing "seen" set sized up front
from max_size (table_capacity = max(65536, max_size*64), rounded to a power of
two). That reserved the worst case on the FIRST call regardless of the function
actually decoded: for max_size=100000 the table is ~96 MiB per worker on x86
(~192 MiB on x64). On 32-bit this exhausted the address space under
parallel_for, which is why populate_function_buckets_heuristic capped max_size
at 8192 on x86, trading away large-function coverage.
Replace the fixed table with detail::SeenSet, a grow-on-demand open-addressing
set that starts at 4096 slots and doubles + rehashes (via its dirty list) only
when a function truly needs it. It stays thread-local and grow-only, so a
thread's footprint tracks the largest function it actually decodes.
- Work ceiling preserved exactly: the old max_seen derived from the thread's
grow-only table high-water, so a thread that once ran a large max_size kept
that ceiling for later calls. Reproduced with a thread-local high-water
(g_seen_ceiling), independent of the now-small table, so the decoded-address
bound is unchanged for every representable input. Budget power-of-two math is
saturating (a huge/malformed max_size can't wrap or spin the shift -- the old
code left that as UB).
- No separate branch ceiling needed: every enqueue is preceded by decoding a
branch instruction (a seen insert), so branches.size() <= 1 + seen.count and
the outer loop stops at seen.count == seen_budget.
- Allocation failure is latched: a failed grow() sets an OOM flag so it is never
retried, and the outer branch loop stops -- one graceful abort under memory
pressure instead of re-attempting a large calloc per queued branch.
- Hot path is now one merged check-and-insert probe (was two: contains then
insert).
- With the table no longer tied to max_size, the x86 8192 cap is removed:
populate_function_buckets_heuristic uses 100000 on both arches again, so x86
regains full large-function coverage with no memory blowup.
Tests (TestScanCoverage.cpp), deterministic and layout-independent:
- SeenSet: grow-at-50%, entries survive rehash, cleanup clears membership but
keeps capacity, budget returns Full, duplicate returns Present without
consuming budget/dirty (checked before the budget), null handling, max_cap
ceiling, and the OOM latch (no-retry contract + begin() clears it). Red-
verified: breaking the rehash makes the survive-rehash test fail.
- exhaustive_decode with SIZE_MAX max_size returns cleanly (saturating budget).
- Large generated corpora force many real growths/rehashes on the decode path
(the other tests all fit the initial table) and reuse the table across
functions: a 40k-NOP sled (assert exact decode count, identical on reuse) and
a 20k "JNZ +0" sled whose fixture shape is asserted (conditional branch that
resolves to the next instruction) and whose enqueue path is proven exercised
via ctx.branch_start advancing on every instruction (would be 1, not P+1, if
the work-list silently stopped following JNZ).
Measured (x86 scan-bounds heuristic workload):
- Seen-table memory: 39 x 6 MiB (~234 MiB aggregate, all fixed) -> 35 x 24 KiB
(~840 KiB), zero growth events -- the module's functions all fit the initial
table. ~280x reduction; work ceiling unchanged.
- End-to-end (stress x30, fresh processes): x86 416 -> 306 ms/run (~26%), x64
296 -> 290 ms/run (neutral). Process-lifetime runs (each pays the per-worker
allocation); steady-state within a long-lived process reuses the table and is
analytically neutral-to-better (one probe vs two).
Profiling note: no PMU/uarch profile was collected. VTune's event-based sampling
requires an Intel PMU and cannot sample this AMD 7950X3D ("cannot recognize the
processor"); only its software-sampling hotspots run. AMD uProf (the appropriate
AMD profiler) collects via time-based software sampling, but its event-based/PMU
collection reported "Core PMC counters are not available" in the current
(non-elevated) profiling session -- possibly an elevation/driver/VBS
restriction, not run down further. The perf A/B above is therefore wall-clock
process runs against a stashed baseline, plus direct allocation instrumentation
for the memory figures.
Verified: ctest x86 24/24, x64 24/24; determinism stress (scan-bounds/resolve/
path/coverage) clean on both arches. x64 decode behavior unchanged (same
ceiling, same results; only the table's allocation strategy differs).
Track target architecture and stored image bases for mapped PE images, thread analysis context through decode and pointer-width-sensitive scans, and make RTTI vtable discovery target-aware. Add deterministic PE32-on-x64 coverage while preserving host-default behavior.
Replace the AnalysisContext translation layer with a single optional TargetArch parameter threaded (host-default) through the decode, scan, and RTTI paths, so a 64-bit process can analyze a mapped 32-bit PE with the minimum change over the original. On Windows a SEC_IMAGE-mapped image is relocated by the loader, so its stored pointers are already real host addresses -- no address translation is needed. That lets the whole stored/preferred image-base machinery, the dual TargetArch/AnalysisContext overloads, the hand-rolled MSVC demangler, and the wholesale RTTI/PE-reader rewrites be dropped. Net footprint over the pre-feature baseline is ~+280/-136 across 5 files (was ~+1160/-484). x64 code paths are unchanged (arch defaults to host); mapped modules use width-aware decode/scan and match RTTI by decorated name. All 25 tests pass on x64 and x86; the cross-arch test proves PE32 mapping, decode, displacement resolution, pointer/function-bounds scanning, and RTTI from x64.
The kananlib-cross-arch-test target used Windows-only PE constants and relies on SEC_IMAGE loader relocation, so it cannot build or run on Linux -- mark it windows-only (matching the other PE-mapping tests) so CI's Linux job skips it. Also read the complete-object-locator at the target pointer width in find_vtables' sort comparator; the host-width read would mis-read a foreign 32-bit module's locator slot. Exercised by a second same-named vtable in the fixture.
There was a problem hiding this comment.
Pull request overview
This PR adds cross-architecture PE analysis so a 64-bit host can map and analyze a 32-bit (PE32) image correctly, and replaces the fixed-size exhaustive_decode seen-table with a grow-on-demand structure to remove the prior x86 max_size cap.
Changes:
- Introduces
TargetArch+ runtime decode/pointer-width selection across decode/scan primitives. - Reworks
exhaustive_decode’s seen tracking into a growable open-addressingdetail::SeenSetwith OOM latching and adds coverage/stress tests. - Adds a Windows-only cross-arch integration test that builds and maps a synthetic relocatable PE32 and validates decode/scan/RTTI behavior.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| test/TestScanCoverage.cpp | Adds deterministic SeenSet unit tests and decode-growth stress coverage; extends RWX test helper to allow custom sizes. |
| test/TestCrossArch.cpp | New Windows-only synthetic PE32 cross-arch integration test for mapping, decode, scan, function bounds, and RTTI. |
| test/StressTest.cpp | Clarifies back-edge detection rationale in CFG loop-correctness stress test. |
| test/CMakeLists.txt | Adds the new test target and tweaks FetchContent declarations. |
| test/cmake.toml | Registers the new kananlib-cross-arch-test target and updates sample-dir definitions. |
| src/Scan.cpp | Plumbs TargetArch through scan/decode APIs and adjusts pointer-width-sensitive logic for x86 targets. |
| src/RTTI.cpp | Adds mapped-module RTTI walking at target pointer width and updates vtable sorting to read locator pointers at target width. |
| src/Module.cpp | Adds PE optional-header-magic-based get_module_arch() and mapped-module detection; fixes PE32 ImageBase reads on x64. |
| include/utility/Scan.hpp | Adds runtime decode_mode/decode_data, TargetArch parameters with defaults, and new growable detail::SeenSet. |
| include/utility/Module.hpp | Introduces TargetArch, host_arch(), pointer_width(), and module-arch/mapped-module APIs. |
Comments suppressed due to low confidence (2)
test/TestCrossArch.cpp:144
IMAGE_BASE_RELOCATION::SizeOfBlockshould be DWORD-aligned.22(8-byte header + 7*2-byte entries) is not; add anIMAGE_REL_BASED_ABSOLUTEpadding entry and setSizeOfBlockto 24 to keep the reloc data spec-compliant.
auto* pointer_reloc = reinterpret_cast<IMAGE_BASE_RELOCATION*>(bytes.data() + 0x60C);
pointer_reloc->VirtualAddress = 0x2000;
pointer_reloc->SizeOfBlock = 22;
auto* pointer_entries = reinterpret_cast<uint16_t*>(pointer_reloc + 1);
pointer_entries[0] = IMAGE_REL_BASED_HIGHLOW << 12; // ptr @ 0x2000
test/CMakeLists.txt:72
- Same issue as above for spdlog:
FetchContent_Declare(spdlog SYSTEM ...)is incompatible with the currentcmake_minimum_required(VERSION 3.15)and can break configuration.
message(STATUS "Fetching spdlog (v1.12.0)...")
FetchContent_Declare(spdlog SYSTEM
GIT_REPOSITORY
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| it = std::find(it, end, value); | ||
| if (it != end) { | ||
| return (uintptr_t)it; | ||
| } | ||
| ++it; |
| } | ||
| return utility::scan_data(start, length, (uint8_t*)&ptr, sizeof(uintptr_t)); | ||
| } |
| for (auto addr = region_start; addr + sizeof(uintptr_t) < region_end; addr += sizeof(uintptr_t)) { | ||
| const auto potential_fn_ptr = *(uintptr_t*)addr; | ||
| const auto ptr_width = pointer_width(arch); | ||
| for (auto addr = region_start; addr + ptr_width < region_end; addr += ptr_width) { |
| optional.SizeOfHeapReserve = 0x100000; | ||
| optional.SizeOfHeapCommit = 0x1000; | ||
| optional.NumberOfRvaAndSizes = IMAGE_NUMBEROF_DIRECTORY_ENTRIES; | ||
| optional.DataDirectory[IMAGE_DIRECTORY_ENTRY_BASERELOC] = {0x3000, 34}; |
| message(STATUS "Fetching bddisasm (v1.37.0)...") | ||
| FetchContent_Declare(bddisasm | ||
| FetchContent_Declare(bddisasm SYSTEM | ||
| GIT_REPOSITORY |
| return ntHeaders->OptionalHeader.Magic == IMAGE_NT_OPTIONAL_HDR32_MAGIC | ||
| ? TargetArch::X86 | ||
| : TargetArch::X64; |
…make drift get_module_arch classified any optional-header magic that was not PE32 as X64, so a ROM/malformed header flipped an x86 host to 64-bit decode and 8-byte pointers. Only the two defined magics now select an arch; anything else degrades to host_arch(). scan_ptr_noalign searched sizeof(uintptr_t) bytes for an X64 target -- the host's width, so a 32-bit host scanned 4 bytes instead of 8 (the aligned scan_ptr already used an explicit uint64_t). Both now match the target width. scan_value_aligned incremented the iterator after std::find returned end, forming a pointer past one-past-the-end (UB, carried over from the original scan_ptr); it now breaks instead -- behaviorally identical, the loop exited immediately either way. Revert unintended cmkr regeneration drift: FetchContent_Declare(... SYSTEM ...) requires CMake 3.25 but the project declares 3.15, and the KANANLIB_SAMPLE_DIR wiring for the cross-arch target is dead since the test builds its PE32 in-memory. Fixture: pad the second base-relocation block to a DWORD-aligned SizeOfBlock (24) per the PE spec. Both regressions are x86-host-only and are now covered by tests that fail on the x86 build before this change and pass after; x64 was green throughout. ctest 25/25 on x64 and x86.
|
Thanks — I verified each point against the code and its history rather than taking them at face value. 5 of 6 were real and are fixed; 1 I'm declining here with reasoning. Both suppressed low-confidence comments were also valid and are fixed. How the two behavioral claims were provenBoth are x86-host-only, which is why the existing suite never caught them. I added two discriminating tests and ran them on both builds before changing any production code:
Pre-fix x86 failures: Fixed
Declining, with reasoning
for (auto addr = region_start; addr + sizeof(uintptr_t) < region_end; addr += sizeof(uintptr_t)) {The only change here was substituting the width constant ( Verification
|
| // ImageBase lives at a different offset/width in PE32 vs PE32+, so read | ||
| // it by the optional header magic rather than the host's compile-time | ||
| // IMAGE_NT_HEADERS layout (which would mis-read a 32-bit image on x64). | ||
| const auto magic = ntHeaders->OptionalHeader.Magic; | ||
| if (magic == IMAGE_NT_OPTIONAL_HDR32_MAGIC) { | ||
| return ((PIMAGE_NT_HEADERS32)ntHeaders)->OptionalHeader.ImageBase; | ||
| } | ||
| return ((PIMAGE_NT_HEADERS64)ntHeaders)->OptionalHeader.ImageBase; |
get_dll_imagebase read the PE32+ ImageBase (offset 24, 8 bytes) for any magic that was not PE32, so a ROM or malformed optional header was mis-read. On an x86 host that also regressed against the original, which read the host layout (offset 28, 4 bytes). Only the two defined magics now select a layout; anything else returns nullopt, matching the DOS/NT signature checks the function already performs. Covered by test_dll_imagebase_reads_by_magic, which pins both real layouts and the rejection, and fails on x64 and x86 before this change.
|
Second review: 1 of 2 valid and fixed. (The Fixed —
|
| host | original | this PR (before fix) |
|---|---|---|
| x64 | offset 24, 8 bytes | offset 24, 8 bytes — unchanged |
| x86 | offset 28, 4 bytes | offset 24, 8 bytes — regressed |
Only the two defined magics now select a layout; anything else returns nullopt, which matches the DOS-signature and NT-signature checks the function already performs (there is no sensible default image base to guess, unlike get_module_arch, where falling back to the host arch is meaningful).
Proof — test_dll_imagebase_reads_by_magic pins all three cases: PE32 reads the 4-byte field at optional-header offset 28, PE32+ reads the 8-byte field at offset 24 (a wrong-layout read lands on the zeroed upper half, so this discriminates), and ROM (0x107) / garbage (0xDEAD) magics are rejected. Unlike last round's two regressions, this one is not host-specific, so it fails on both builds before the change:
pre-fix x64: 7 passed, 1 failed x86: 7 passed, 1 failed
FAIL: !get_dll_imagebase(...).has_value() TestCrossArch.cpp:393
post-fix x64: 8 passed, 0 failed x86: 8 passed, 0 failed
Note the PE32 and PE32+ assertions passed before the fix too — the layout selection was already correct; only the unknown-magic path was wrong.
On the "potentially fault when the optional header isn't large enough" part: reading Magic itself, and the 32-byte span either real layout requires, is the same exposure origin/main had, so I have not added SizeOfOptionalHeader validation here — that would be a broader hardening change with its own risk of rejecting real images, and it belongs in its own commit.
Out of scope — src/Scan.cpp:315, scan_value_aligned type punning
The observation is legitimate C++: casting arbitrary memory to T* and running std::find over it is not strictly conforming, and an unaligned start would be a real problem on an alignment-strict target.
I'm deliberately not changing it in this PR. This is a scan library whose entire premise is reinterpreting foreign process/image memory as typed values — the same pattern is pervasive in Scan.cpp and RTTI.cpp and predates this PR. scan_value_aligned is a faithful extraction of origin/main's scan_ptr loop, which did exactly this with uintptr_t*; the only thing this PR changed is which width is used. Converting it to memcpy-based loads would be a codebase-wide change to a hot scanning path, evaluated on its own merits, not folded into a cross-architecture feature. Both supported targets are x86/x64, where unaligned loads are well-defined at the hardware level.
Verification
ctest 25/25 on x64 and 25/25 on x86 (Win32), cross-arch suite 8/8 on both.
Summary
Adds cross-architecture PE analysis — a 64-bit process can now map and correctly analyze a 32-bit (PE32) image — plus a growable
exhaustive_decodeseen-table that removes the x86max_sizecap.Target architecture becomes a runtime property of the analyzed module rather than a compile-time property of the host build.
Changes
Growable
exhaustive_decodeseen table (Scan.hpp)max_sizecap could be dropped.Cross-architecture PE analysis (
Module.*,Scan.*,RTTI.cpp)TargetArchenum +host_arch()/pointer_width()helpers.get_module_arch()detects a module's architecture from its PE optional-header magic;is_mapped_module()distinguishes images we mapped ourselves from loader-loaded ones.get_dll_imagebase()(ImageBase differs in offset/width between PE32 and PE32+).get_module_size/get_module_sections/map_view_of_pealready worked for PE32 (SizeOfImageandIMAGE_FIRST_SECTIONare magic-independent).TargetArch arch = host_arch():decode_one,get_insn_size,exhaustive_decode,linear_decode,collect_basic_blocks[_into],get_highest_contiguous_block,resolve_displacement,scan_opcode/scan_disasm/scan_mnemonic,scan_ptr/scan_ptr_noalign.#if KANANLIB_ARCH_X86_32branches inresolve_displacement(absolute[disp32]andimm32references) are now runtimearch == X86checks, so an x64 host can resolve x86 references.TypeDescriptordirectly and matching on the decorated name (the host CRT undecorator cannot safely run on a foreign or mappedtype_info).find_vtables' sort reads the locator at the target width.Design note
On Windows a
SEC_IMAGE-mapped image is relocated by the loader, so its stored pointers are already real host addresses. No address translation layer is needed — only the target's instruction mode and pointer width. That keeps the change small: the cross-arch footprint is roughly +280 / −136 across the five library files.Compatibility
x64 behavior is unchanged: every new parameter defaults to
host_arch(), and loaded modules keep the original RTTI path. No call sites required updating.Tests
New
kananlib-cross-arch-test(Windows-only, like the other PE-mapping tests) builds a synthetic relocatable PE32 in-memory and asserts, from both host architectures:mov eax,[disp32]is 5 bytes; differs from a host-mode decode on x64)[disp32]displacement resolution to the mapped addressctest: 25/25 on x64 and 25/25 on x86 (Win32).