Repository navigation
feat(tools): symbolize the Windows samplers' stacks - #9572
Conversation
|
Other open pull requests touch the same code. Merging one will break the other, or both implement the same thing. If this one builds on them, list them in Depends on.
Some comparisons could not be completed because a pull request conflicts with the base branch. Review these shared files manually before concluding that there is no overlap.
|
MarcZX10
left a comment
There was a problem hiding this comment.
I found three correctness gaps in the advertised sampler inputs:
-
PeModule.name()checks that an RVA is in some code section, then bisects one global symbol-address list. If an RVA is in a later code section before that section's first symbol, the last symbol from an earlier section is returned and the frame is mislabeled. The description says addresses before the first symbol stay unsymbolized. Keep symbol lookup bounded to the containing code section (and add a two-code-section test with an address before the second section's first symbol). -
Address-file rows contain both self and inclusive counts (
module offset self inclusive), butload()stores onlyparts[2], andsummarize()emits no inclusive table for address input. This drops data fromAPS5_SAMPLE_THREADSand the address-file form of the other samplers despite the tool description promising hottest-by-inclusive output. Preserve and subtract both counters, then test that inclusive totals are reported correctly. -
The
APS5_SAMPLE_THREADSoutput has no# module name size timestamprecords (the currentProcessSampler::write()emits only rounds, thread headings and address rows). Thus this input bypasses the PE size/timestamp guard and can silently symbolize addresses against a different build. Add the module identity metadata to that sampler output, or make the address-input path explicitly refuse/mark unverified symbolization when it cannot validate the binaries; cover the mismatch case.
The hosted conventions, eligibility, intake, labels, relinker matrix and sanitizer checks pass. The broader hosted Linux/Windows jobs are skipped. The author reports seven Python cases passing on Windows and a real capture, but the cases shown do not cover the multiple-code-section lookup or the missing metadata/inclusive-count paths above.
|
Thanks, @MarcZX10. All three are addressed in 0e039b9, on top of 0bf64f3.
Checks:
Not done: a new capture with the sampler change, which needs a game run. Until then, the new |
…nts and refuse unrecorded modules Review of boykopovar#9572 found three gaps: - PeModule.name() checked that an RVA was in some code section, then bisected one list of every symbol, so an address in a later code section before that section's first symbol took the last symbol of an earlier section. Each code section now keeps its own sorted list. - Address files keep both counters ('module offset self inclusive'): the inclusive count is kept and subtracted by --since, and the summary lists functions by inclusive samples as well, summed over each function's addresses (an upper bound when a function is on one stack more than once). The total comes from the '# N samples' line or from the 'thread <id> samples <n>' headings of APS5_SAMPLE_THREADS, which the loader previously misread as a stack line; --thread keeps one thread. - The address writers of both samplers wrote no '# module' records, so those files skipped the size and timestamp check. Both now write them (through the helper the collapsed-stack writer uses), and the tool leaves a module unsymbolized when the samples have no record for it, unless --unverified is given. Five new cases cover a second code section, inclusive totals, --since on an address file, thread headings with --thread, and the missing record with and without --unverified. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Re-reviewed head 0e039b91bb459299dec2b446592d5e6ad2cd28df. The three correctness issues from my earlier review are addressed: lookup is bounded to the containing code section, address input now retains and reports inclusive counts (including --since subtraction), and both address samplers write module size/timestamp records while unknown modules remain unsymbolized unless --unverified is explicit. I found no remaining defect in those paths. I ran the PR's exact tools/tests/test_symbolize_samples.py against its matching tools/symbolize_samples.py: all 11 tests pass. git diff --check and the current-main tools/check_conventions.py pass. Hosted eligibility, conventions, labels, report, and all four relinker jobs pass; Linux/Windows product build jobs are skipped by repository policy. The Windows capture and Windows Python 3.15 results in the description remain author-reported, not runs I performed. Please refresh this tooling PR from current main before asking for final review: check_main_sync.py reports 2 commits ahead / 107 behind (merge base 6ef5a1c4c), though it currently merges cleanly. The overlap workflow's partial comparison identifies the shared docs/dev/BUILD.md edit with merged #8087; that is not an open-PR ordering conflict, and I manually confirmed the PR's edits document this tool. The local overlap helper could not finish because the configured upstream remote points directly at the repository, while this script expects an upstream/main branch ref; its fetch failed, so I am not treating that as a clean automated scan. I have not found a code-level blocker after the fixes; this request is for current-main freshness only. I withdraw the queue-based hold; it is fine to keep submitting PRs and they will be handled in due course.
I’ve noticed that holding this PR because of #5239 isn’t necessary. It’s fine for PRs to keep coming in; they’ll be handled in due course. Please disregard my earlier request to hold or defer it.
|
Thanks @MarcZX10, for re-checking the three fixes and running the tests. I'm converting this to a draft under the #5239 hold, since no title run depends on it. Before marking it ready again I'll rebase it onto current |
0e039b9 to
b902ffc
Compare
…nts and refuse unrecorded modules Review of boykopovar#9572 found three gaps: - PeModule.name() checked that an RVA was in some code section, then bisected one list of every symbol, so an address in a later code section before that section's first symbol took the last symbol of an earlier section. Each code section now keeps its own sorted list. - Address files keep both counters ('module offset self inclusive'): the inclusive count is kept and subtracted by --since, and the summary lists functions by inclusive samples as well, summed over each function's addresses (an upper bound when a function is on one stack more than once). The total comes from the '# N samples' line or from the 'thread <id> samples <n>' headings of APS5_SAMPLE_THREADS, which the loader previously misread as a stack line; --thread keeps one thread. - The address writers of both samplers wrote no '# module' records, so those files skipped the size and timestamp check. Both now write them (through the helper the collapsed-stack writer uses), and the tool leaves a module unsymbolized when the samples have no record for it, unless --unverified is given. Five new cases cover a second code section, inclusive totals, --since on an address file, thread headings with --thread, and the missing record with and without --unverified. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Rebased onto |
There was a problem hiding this comment.
Re-reviewed exact head b902ffc467c3de65689e70e552825073265c6084. The earlier code findings are resolved: lookups stay within the containing code section, address input preserves both counters through --since, and the samplers record module identity before the tool resolves frames. I found no new implementation defect in those paths.
Checks I ran on this exact head: - tools/tests/test_symbolize_samples.py: 11/11 passed. - tools/check_conventions.py --base upstream/main: passed. - tools/check_test_registration.py --scripts: 172 registered scripts. - git diff --check: passed; merge-tree with current main is clean. - x86_64-w64-mingw32-g++ -std=c++20 -Wall -Wextra -Werror -fsyntax-only -Icore/libs core/libs/prx/libSceAgcDriver/Execution/src/WorkerSampler.cpp: passed. This is a compile check, not a Windows runtime/capture.
Two items remain before final review. First, this branch is based on 8b3a6ba88d8fc74e768f1a7e8e9e36fb69842768, 41 commits behind current upstream/main 329efe1bfdaff116c57cdf62718026ee4f5521ae; please update and rerun the focused checks. Second, the new C++ sampler output (# module name size timestamp) has not yet been exercised through a fresh capture. The synthetic PE tests validate parsing, but do not verify that the sampler writes records matching the binaries actually sampled. Please take one Windows capture on the updated head, run it through symbolize_samples.py with those exact binaries, and confirm matching modules symbolize while mismatched/unrecorded modules remain unsymbolized.
I withdraw my earlier intake objection and the queue-based draft rationale. I am not asking that the PR stay draft because of queue size. Hosted relinker/matrix checks pass; hosted product Linux/Windows jobs are skipped. The reported Windows test/build results and previous Spirit of the Island capture remain author-reported; I did not run a Windows runtime capture here.
I’ve noticed that holding this PR because of #5239 isn’t necessary. It’s fine for PRs to keep coming in; they’ll be handled in due course. Please disregard my earlier request to hold or defer it.
…nts and refuse unrecorded modules Review of boykopovar#9572 found three gaps: - PeModule.name() checked that an RVA was in some code section, then bisected one list of every symbol, so an address in a later code section before that section's first symbol took the last symbol of an earlier section. Each code section now keeps its own sorted list. - Address files keep both counters ('module offset self inclusive'): the inclusive count is kept and subtracted by --since, and the summary lists functions by inclusive samples as well, summed over each function's addresses (an upper bound when a function is on one stack more than once). The total comes from the '# N samples' line or from the 'thread <id> samples <n>' headings of APS5_SAMPLE_THREADS, which the loader previously misread as a stack line; --thread keeps one thread. - The address writers of both samplers wrote no '# module' records, so those files skipped the size and timestamp check. Both now write them (through the helper the collapsed-stack writer uses), and the tool leaves a module unsymbolized when the samples have no record for it, unless --unverified is given. Five new cases cover a second code section, inclusive totals, --since on an address file, thread headings with --thread, and the missing record with and without --unverified. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
b902ffc to
0ec1451
Compare
|
@MarcZX10 thanks. Rebased onto The fresh capture on this head needs a native title run. On this machine those go through the shared FIFO launch queue, which is currently busy with other runs. Once a slot comes through, I'll capture on this exact head, symbolize it with the exact binaries that were sampled, and post the matched/unmatched module counts here. The PR stays a draft until then. |
|
I’ve noticed that asking you to hold this PR was unnecessary. It’s fine to keep submitting PRs; maintainers will handle them in due course. |
|
@MarcZX10 here is the fresh Windows capture you asked for, run through Capture.
Matching binaries ( System DLLs outside Mismatched binary. The same samples with
Every module in this capture has a record, so it doesn't exercise the unrecorded case. That path stays covered by the unit tests. Marking ready for review. |
MarcZX10
left a comment
There was a problem hiding this comment.
Thanks for adding the fresh Windows capture on head 0ec1451. It now demonstrates matching module metadata symbolizing against the runtime binaries and a same-size/different-timestamp module remaining unsymbolized; that resolves the capture gap from my prior review. I refreshed checks locally: conventions, test registration (171 scripts), and diff whitespace pass. One current-main item remains: check_main_sync.py --base upstream/main --head <head> reports this branch 2 commits ahead / 527 behind, with a clean merge. The earlier reviewed fixes appear preserved, but this branch is no longer based on current main as required by the PR checklist. Please merge current main and rerun the focused Python suite and conventions on that resulting head; I can then review the combined diff for regressions. No queue-size objection remains, and I am not asking that this PR be held or closed because of intake volume.
The Windows samplers (APS5_SAMPLE_WORKER, APS5_SAMPLE_COMMITTER and
APS5_SAMPLE_THREADS) write every frame as module+0xRVA, and BUILD.md left
naming them to the reader ("for example with nm on the .prx"). Doing that
by hand means finding each module's image base, choosing between COFF
symbols and the export table, and keeping the files cumulative totals in
mind.
tools/symbolize_samples.py reads a collapsed-stack file or the address
file, opens each module it finds in --libs as a PE32+ image, and names
every frame module!function:
- From the COFF symbol table that the MinGW-built .prx and .dll keep:
external and static symbols in code sections, at the section's RVA
plus the symbol value. Addresses outside a code section, or before the
first symbol, stay module+0xRVA.
- From the export table, prefixed with "~", for a module without
symbols, such as the runtime DLLs.
- A module whose SizeOfImage or TimeDateStamp differs from the sampler's
"# module" line is left unsymbolized, with a warning: its addresses
would name the wrong functions.
--since subtracts an earlier copy of the same file, which keeps only the
samples taken between the two copies (the files hold totals since
start-up). --out writes the named stacks for speedscope or flamegraph.pl.
--demangle uses c++filt when it is on PATH. It needs no binutils
otherwise.
test_symbolize_samples (registered with CTest) builds small PE32+ images
with COFF symbols and with an export table, and covers short, string-table
and static names, the ignored section and data symbols, addresses outside
code, the export fallback, the size and timestamp check, --since with
--out, the address-file format and rejected lines.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…nts and refuse unrecorded modules Review of boykopovar#9572 found three gaps: - PeModule.name() checked that an RVA was in some code section, then bisected one list of every symbol, so an address in a later code section before that section's first symbol took the last symbol of an earlier section. Each code section now keeps its own sorted list. - Address files keep both counters ('module offset self inclusive'): the inclusive count is kept and subtracted by --since, and the summary lists functions by inclusive samples as well, summed over each function's addresses (an upper bound when a function is on one stack more than once). The total comes from the '# N samples' line or from the 'thread <id> samples <n>' headings of APS5_SAMPLE_THREADS, which the loader previously misread as a stack line; --thread keeps one thread. - The address writers of both samplers wrote no '# module' records, so those files skipped the size and timestamp check. Both now write them (through the helper the collapsed-stack writer uses), and the tool leaves a module unsymbolized when the samples have no record for it, unless --unverified is given. Five new cases cover a second code section, inclusive totals, --since on an address file, thread headings with --thread, and the missing record with and without --unverified. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Rebased onto current
Python (CPython 3.15.1, Windows 11):
The native capture from my earlier comment ( |
0ec1451 to
cdc21a9
Compare
Dismissing a stale review: the requested changes were addressed after it (new head and/or updated description with the asked-for runs) and the merge queue blocks on it. Please request changes again if anything is still open.
THEMANnt
left a comment
There was a problem hiding this comment.
Reviewed head cdc21a9897e7a61c7b19298c0d8460f3fa89c4a1. No actionable findings in this focused Windows sampler-symbolization review.
The helper maps COFF code symbols and export fallbacks within their code sections, retains unresolved frames, and checks recorded module size/timestamp before trusting a binary. Both address and collapsed-stack inputs support window subtraction; thread-address files can be filtered by thread. The native change shares the existing module-record writer and adds those identity records to address/process output as well as collapsed stacks.
Validation: Windows/Python 3.11.9/MinGW GCC 15.2.0: all 11 symbolizer tests pass without skips. The exact WorkerSampler.cpp translation unit passes syntax checking. Compiled and executed the exact writeModules method against a real loaded executable, verified its size/timestamp against the Python PE parser, and symbolized seven samples of a known function from the executable's actual COFF table. Conventions, whitespace and script registration pass. Baseline has no symbolizer module, so its grafted-test import failure is not a sensitivity demonstration. Hosted relinker/ASan and policy checks pass; product jobs are skipped.
Scope: Synthetic sampler formats, a native metadata/real-PE round trip and full-unit syntax. No live suspended-thread sampling session, full driver rebuild or game execution.
|
Checked the section-bounded symbol lookup, both counters through |
What
The Windows samplers (
APS5_SAMPLE_WORKER,APS5_SAMPLE_COMMITTER,APS5_SAMPLE_THREADS) write every frame asmodule+0xRVA, anddocs/dev/BUILD.mdleft naming them to the reader ("for example withnmon the.prx"). Doing that by hand takes three steps:tools/symbolize_samples.pyreads a collapsed-stack file or the address file. It opens each module it finds in--libsas a PE32+ image and names every framemodule!function:.prxand.dllkeep: external and static symbols in code sections, at the section's RVA plus the symbol value. An address outside a code section, or before the first symbol, staysmodule+0xRVA.~prefix to mark the less exact name.SizeOfImageorTimeDateStampdiffers from the sampler's# moduleline, its frames are left unsymbolized and a warning is printed, because its addresses would name the wrong functions. That is easy to get wrong when main and a candidate runtime are swapped between runs.Options:
--since <earlier copy>subtracts an earlier copy of the same file, keeping only the samples taken between the two copies. The sampler rewrites cumulative totals every 20 s, so two copies taken during a run give a steady-state window that leaves out loading.--outwrites the named stacks for speedscope.app orflamegraph.pl.--demangleusesc++filtwhen it is onPATH.Nothing else is needed: no binutils, and the PE parsing is in Python.
docs/dev/BUILD.mdpoints at the tool where it described offline symbolization.Tested
tools/tests/test_symbolize_samples.py, registered with CTest assymbolize_samples, builds small PE32+ images, one with COFF symbols and one with an export table. At headcdc21a9897(onmain0db47b263d) its 11 cases pass in 3 of 3 runs on Windows 11 with CPython 3.15.1. They cover:--sincewith--outon stack files, and--sinceon both counters of an address file;tools/tests/test_*.pyscripts thatCMakeLists.txtregisters as CTest entries pass when run directly at that head.APS5_SAMPLE_WORKERfile pair from Spirit of the Island's title screen (copies at 62 s and 142 s, symbolized against the libraries that ran) takes 7.9 s. Excerpt:System DLLs outside
--libs(ntdll.dll,KERNEL32.DLL) staymodule+0xRVA. Passing--libs C:\Windows\System32names them from their export tables.tools/check_conventions.pypasses.tools/env_reference.py --checkreportsAPS5_NO_WGIundocumented, which comes fromlibSceVideoOuton currentmainand is not touched here.Checklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file.mdfiles or images added to the repository (attach them to this PR)🤖 Generated with Claude Code