Perf script parity

[ixtli]

check docs for the story

Summary by CodeRabbit

• New Features

  • Added --inline flag to expand callchains into DWARF inline frames for fold and flamegraph commands.
  • Added AArch64 DWARF unwinding support alongside existing x86-64 support.

• Bug Fixes

  • Fixed build-ID parsing to handle variable-length identifiers from perf records.
  • Improved file-identity matching using device major/minor and inode.
  • Fixed symbol offset reporting for kernel and module symbols.

• Performance

  • Optimized mapping insertion and overlap detection.
  • Added memoization for unwinding info lookups.

• Documentation

  • Added oracle testing framework and architecture-specific unwind specifications.

[coderabbitai]

📝 Walkthrough

Walkthrough

This PR implements architecture-aware unwinding with inline frame expansion for pyroclast's perf.data folding. It adds support for aarch64 alongside x86_64, parses perf header metadata for architecture and event names, introduces symbol offset suffixes to match perf-script output, refactors mappings for device-major/minor identity matching, and includes a Docker-based oracle harness for parity validation against perf/inferno.

Changes

Architecture-aware unwinding with inline frame expansion

Layer / File(s)	Summary
Perf header parsing for architecture and event metadata src/perfdata/header.rs, tests/perfdata_header.rs	Parse HEADER_ARCH feature to extract recording-machine architecture (x86_64/aarch64) and HEADER_EVENT_DESC to resolve event names, enabling architecture-aware sample decoding and event name matching in folding.
Architecture abstractions: PerfArch, PerfUserRegs, AArch64 decoding src/perfdata/unwind.rs, tests/perfdata_unwind.rs	Introduce PerfArch enum and PerfUserRegs union type to decode perf's register masks into architecture-specific register layouts; add PerfAarch64Regs with elfutils-compatible frame-pointer-chain unwinding and memoized CFI coverage checks.
Folding engine: arch-aware unwinding and inline frame rendering src/perfdata/fold.rs, tests/perfdata_fold.rs	Refactor fold.rs to use PerfUserRegs in unwinding paths, implement leaf-only optimization with per-(pid,ip) CFI memoization, add inline frame expansion in perf-script and folded output conditionally based on FoldOptions::inline flag.
Symbol resolution with offsets and build-id-aware keying src/symbols.rs, tests/symbols.rs	Add base_offset field to ResolvedSymbolFrames, implement offset-aware kallsyms and addr2line resolution, change SymbolRequest cache key to use build_id as discriminator, add GNU build-id note parsing for live kernel build-id verification.
Mapping overlaps and file identity matching src/perfdata/mappings.rs, tests/perfdata_mappings.rs	Implement O(log n + matches) fast-path incremental insert for non-overlapping mappings, update file-identity matching to use device major/minor + inode (perf-compatible), refactor symbol_source_key to drop file_identity when build_id present to unify deduplication.
Build-id event parsing with size-driven length src/perfdata/build_id.rs, tests/perfdata_build_id.rs	Update parse_build_id_event to derive build-id length from PERF_RECORD_MISC_BUILD_ID_SIZE bit and size-byte location rather than record-type validation, enabling parsing of perf-generated HEADER_BUILD_ID payloads.
CLI wiring and benchmark integration of inline flag src/cli.rs, src/lib.rs, src/benchmarks.rs, src/backends/linux_perf.rs, tests/benchmarks.rs	Add --inline flag to FoldArgs/PerfScriptArgs/FlamegraphArgs, thread inline setting through benchmark streaming comparisons, enable inline by default in flamegraph and linux_perf backends, implement benchmark_fold_options helper.
Test updates: arch-aware unwinding, inline rendering, offset expectations tests/perfdata_fold.rs, tests/run_cli.rs, tests/symbols.rs, tests/perfdata_header.rs, tests/perfdata_mappings.rs, tests/perfdata_unwind.rs	Update existing tests to use PerfUserRegs wrapper, add aarch64 register decoding and unwind tests with synthetic ELF fixtures, adjust perf-script output expectations for trailing spaces and offset suffixes, extend symbolization tests for qualified frame names and build-id cache keys.
Oracle harness: Docker, scripts, workload for perf parity validation scripts/oracle/*, scripts/perf-oracle, scripts/oracle-compare	Implement containerized oracle workflow to record synthetic workload traces and compare pyroclast vs perf/inferno output, including build-id cache seeding, configurable inline modes, and diff-based parity reporting.
Documentation updates and issue closure .ace-aarch64-unwind-spec.md, .ace-research-perf-unwind.md, .ace-review-findings.md, docs/parity-findings.md, .beads/issues.jsonl	Document aarch64 DWARF unwind specification and implementation plan, capture perf/libdwfl control-flow research, enumerate performance/correctness findings from code review, update parity-findings with oracle results and resolved architecture/inline-name gaps, close related tracking issues.
Miscellaneous: dependencies, platform helpers, gitignore Cargo.toml, src/platform.rs, .gitignore, src/cargo_cli.rs	Add signal-hook Unix dependency and remove procfs dependency, refactor linux_thread_ids_from_proc to direct task directory enumeration, improve cargo_cli package discovery with manifest path canonicalization, add .claude/ to gitignore.

🎯 4 (Complex) | ⏱️ ~60 minutes

🐰 Hops through arches x86 and aarch64,
Inline frames expand, with offsets ornate,
Perf oracle watches, comparing with glee,
Symbol stacks nested, in harmony! 🎪

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

⚔️ Resolve merge conflicts

• Resolve merge conflict in branch perf-script-parity

[coderabbitai]

Actionable comments posted: 11

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

src/benchmarks.rs (2)

369-385: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

inline only reaches the Pyroclast side of streaming comparisons.

run_streaming_comparison_with_symbols(..., inline = true) passes the flag into run_pyroclast_stream, but the Inferno producer still serializes perf-script input with benchmark_fold_options(false). That makes inline benchmarks compare different frame models and report false diffs whenever the perf script is generated from perf.data instead of supplied externally.

Suggested fix

 pub fn run_streaming_comparison_with_symbols<R>(
     perf_data: &Path,
     perf_script: Option<&Path>,
     runner: &R,
     symbols: bool,
     inline: bool,
 ) -> Result<StreamingComparisonReport, String>
 where
     R: CommandRunner + Sync,
 {
     thread::scope(|scope| {
         let (pyro_tx, pyro_rx) = sync_channel(64);
         let (inferno_tx, inferno_rx) = sync_channel(64);
         let pyro_thread =
             scope.spawn(move || run_pyroclast_stream(perf_data, runner, symbols, inline, pyro_tx));
         let inferno_thread = scope
-            .spawn(move || run_inferno_stream(perf_data, perf_script, runner, symbols, inferno_tx));
+            .spawn(move || run_inferno_stream(perf_data, perf_script, runner, symbols, inline, inferno_tx));

 fn run_inferno_stream<R>(
     perf_data: &Path,
     perf_script: Option<&Path>,
     runner: &R,
     symbols: bool,
+    inline: bool,
     line_sender: SyncSender<String>,
 ) -> Result<ProducerResult, String>
 where
     R: CommandRunner + Sync,
 {
@@
                     if symbols {
                         let resolver =
                             perf_symbol_resolver_for_current_home(runner, &export_perf_data);
                         write_inferno_perf_script_file_with_symbols(
                             &export_perf_data,
-                            benchmark_fold_options(false),
+                            benchmark_fold_options(inline),
                             &resolver,
                             &mut stdin,
                         )
                     } else {
                         write_inferno_perf_script_file_with_options(
                             &export_perf_data,
-                            benchmark_fold_options(false),
+                            benchmark_fold_options(inline),
                             &mut stdin,
                         )
                     }?;

Also applies to: 753-773

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/benchmarks.rs` around lines 369 - 385, The inline flag is only passed to
run_pyroclast_stream causing Inferno to still serialize perf-script input with
benchmark_fold_options(false); update run_streaming_comparison_with_symbols to
forward the inline boolean into run_inferno_stream (in the scope.spawn call) and
ensure the Inferno producer uses benchmark_fold_options(true) when inline is
true so both producers use the same frame-model; locate the
run_streaming_comparison_with_symbols function and adjust the call to
run_inferno_stream and any benchmark_fold_options usage to accept and respect
the inline parameter (also apply the same change to the other occurrence
mentioned).

---

187-208: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Honor inline in the unsymbolized fold benchmark path.

run_fold_benchmark_with_runner(..., symbols = false, inline = true) still falls back to run_fold_benchmark(input), which hardcodes benchmark_fold_options(false). The new parameter is therefore ignored for the non-symbolized branch, so the reported folded byte/line counts are wrong whenever callers request inline expansion.

Suggested fix

 pub fn run_fold_benchmark_with_runner<R>(
     input: &Path,
     runner: &R,
     symbols: bool,
     inline: bool,
 ) -> Result<FoldBenchmarkReport, String>
 where
     R: CommandRunner,
 {
     if symbols {
         let resolver = perf_symbol_resolver_for_current_home(runner, input);
         run_fold_benchmark_with_writer(input, |writer| {
             write_folded_perfdata_file_with_symbols(
                 input,
                 benchmark_fold_options(inline),
                 &resolver,
                 writer,
             )
         })
     } else {
-        run_fold_benchmark(input)
+        run_fold_benchmark_with_writer(input, |writer| {
+            write_folded_perfdata_file_with_options(
+                input,
+                benchmark_fold_options(inline),
+                writer,
+            )
+        })
     }
 }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/benchmarks.rs` around lines 187 - 208, The non-symbol branch in
run_fold_benchmark_with_runner ignores the inline parameter by calling
run_fold_benchmark(input) which uses a hardcoded benchmark_fold_options(false);
change the else branch to call run_fold_benchmark_with_writer and invoke
write_folded_perfdata_file (or the equivalent non-symbol writer function)
passing benchmark_fold_options(inline) so inline expansion is honored. Update
run_fold_benchmark_with_runner to use run_fold_benchmark_with_writer with a
closure that calls write_folded_perfdata_file(input,
benchmark_fold_options(inline), writer) instead of run_fold_benchmark(input).

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In @.ace-aarch64-unwind-spec.md:
- Around line 37-38: The spec incorrectly documents HEADER_ARCH as feature bit
5; update the documentation to state HEADER_ARCH is feature bit 6 to match the
implementation used by header_arch_from_file and header.rs (leave the perf
string format note intact: u32 len + bytes, see perf util/header.c
write_arch/read). Ensure any mention of "feature bit 5" is replaced with
"feature bit 6" so the doc and the code remain consistent.

In @.ace-research-perf-unwind.md:
- Around line 159-163: The fenced code blocks around the snippet containing
symbol_conf.hide_unresolved, entry->ms.sym, append_inlines and
callchain_cursor_append need blank lines before and after to satisfy MD031; add
an empty line immediately above the opening ``` and immediately below the
closing ``` for the blocks at the cited locations. Also fix the ordered-list
prefix style violations (MD029) in the block covering lines 347–357 by making
the list prefixes consistent (e.g., convert all list markers to numeric "1." or
to the chosen style) so the entire list uses a single, uniform ordered-list
prefix format.

In @.ace-review-findings.md:
- Around line 13-185: The markdown headings in the findings block (e.g., lines
starting with "### PERF-1", "### PERF-2", ..., "### CORR-1", etc.) lack the
required blank line(s) before/after them and trigger MD022; edit
.ace-review-findings.md to ensure each heading has an empty line above and below
(normalize to one blank line surrounding each top-level and subsection heading)
so all "### ..." headings are separated by a blank line from adjacent paragraphs
or other headings and the file passes markdownlint MD022.

In `@docs/parity-findings.md`:
- Line 34: Tighten the inline code span around `__libc_start_main` and the
adjacent `[unknown]` so there are no internal leading/trailing spaces inside the
backticks (remove any extra spaces between the backticks and the text) to
satisfy markdownlint MD038; update the text fragment containing
`__libc_start_main` and `[unknown]` accordingly.

In `@scripts/oracle/record-in-container.sh`:
- Around line 16-17: The find command substitution assigning PERF_BIN can fail
under set -euo pipefail and cause the script to exit before the subsequent guard
runs; change the assignment to make the command safe (e.g., append || true or
wrap the pipeline in a brace with || true) so PERF_BIN="$(find
/usr/lib/linux-tools* -name perf -type f 2>/dev/null | head -n 1 || true)" (or
equivalent) and keep the existing guard [ -n "$PERF_BIN" ] || { echo "no perf
binary found" >&2; exit 1; } to handle the missing binary.

In `@src/cargo_cli.rs`:
- Around line 627-637: When checking whether the package manifest is inside
crate_root, canonicalize the crate_root the same way you canonicalize
manifest_path so symlinked paths compare correctly; compute a
canonical_crate_root from crate_root (e.g.,
crate_root.canonicalize().as_deref().unwrap_or(&crate_root) or equivalent) and
use canonical_crate_root in the starts_with check instead of the raw crate_root
(references: crate_root, package_metadata.manifest_path, manifest_path,
find_crate_root).

In `@src/perfdata/build_id.rs`:
- Around line 145-153: Update the documentation for parse_build_id_event(s) to
reflect that these functions intentionally accept perf-style feature records
with header.type == 0 and do not error on a non-build-id record type; describe
that framing is based on the size field (read via read_u16 at offset 4) and
PERF_RECORD_MISC_BUILD_ID_SIZE is used to obtain build_id_size (offset into the
build_id area), so callers should not expect a type validation error anymore and
should rely on the size-based parsing semantics implemented in
parse_build_id_event and parse_build_id_events.

In `@src/perfdata/fold.rs`:
- Around line 3061-3063: The write call is emitting the raw path into perf
script output, skipping normalization for kernel DSO names; update the places
that write mapped-unknown-symbol frames (the write!(writer,
"{prefix}{address:16x} {label} ({path})") usages around the
mapped-unknown-symbol handling) to call perf_script_dso_name(path) first (as
done earlier with let path = perf_script_dso_name(path)), ensuring every place
that formats "({path})" uses the normalized path returned by
perf_script_dso_name so kernel mappings like `[kernel.kallsyms]_...` are
normalized consistently.

In `@src/perfdata/unwind.rs`:
- Around line 485-508: The match currently swallows architecture/register
mismatches in the unwind loop (the `_ => {}` arm), hiding contract drift between
the PerfUnwinder variant and the PerfUserRegs variant; replace that silent no-op
with an explicit failure signal (e.g., panic!, unreachable!, or a debug_assert!)
that includes the mismatched values so it is diagnosable — update the catch-all
arm in the match over (self, regs) to call something like
unreachable!("arch/register mismatch: unwinder={:?} regs={:?}", self, regs) or
debug_assert!(false, ...) (ensure PerfUserRegs and the unwinder type implement
Debug or format a descriptive message) so mismatches in the code paths (the
branches using unwinder.iter_frames, regs.to_framehop_regs,
push_perf_unwind_address) are surfaced instead of returning empty frames.
- Around line 410-419: The memo in has_unwind_info_for_ip currently inserts
every queried IP into unwind_info_memo which can grow unbounded; change
has_unwind_info_for_ip to bound the cache by checking
unwind_info_memo.borrow().len() (or a tracked count) against a small
MAX_UNWIND_MEMO_ENTRIES constant and, when the limit is reached, either clear
the map (or skip inserting new entries) before inserting the result; keep the
existing lookup logic and still clear the memo when reported_modules changes,
but ensure you reference has_unwind_info_for_ip, unwind_info_memo, and
reported_modules so the cache trimming/reset is applied just prior to the insert
to prevent unbounded growth.

In `@src/platform.rs`:
- Around line 66-74: The current read_dir pipeline for building tids silently
drops errors via filter_map(Result::ok); update it to either log each dropped
error or propagate the first encountered error so callers can distinguish an
empty dir from read failures: modify the iterator that uses read_dir(&task_dir)?
and filter_map(Result::ok) so errors are handled explicitly (e.g., replace
filter_map(Result::ok) with a map that matches Result::Err(e) and either calls
log::warn! with context including task_dir and e before returning None, or
returns Err(e) to propagate using a try/collect pattern), keeping the rest of
the file-name-to-u32 parsing and the tids Vec<u32> collection unchanged.

---

Outside diff comments:
In `@src/benchmarks.rs`:
- Around line 369-385: The inline flag is only passed to run_pyroclast_stream
causing Inferno to still serialize perf-script input with
benchmark_fold_options(false); update run_streaming_comparison_with_symbols to
forward the inline boolean into run_inferno_stream (in the scope.spawn call) and
ensure the Inferno producer uses benchmark_fold_options(true) when inline is
true so both producers use the same frame-model; locate the
run_streaming_comparison_with_symbols function and adjust the call to
run_inferno_stream and any benchmark_fold_options usage to accept and respect
the inline parameter (also apply the same change to the other occurrence
mentioned).
- Around line 187-208: The non-symbol branch in run_fold_benchmark_with_runner
ignores the inline parameter by calling run_fold_benchmark(input) which uses a
hardcoded benchmark_fold_options(false); change the else branch to call
run_fold_benchmark_with_writer and invoke write_folded_perfdata_file (or the
equivalent non-symbol writer function) passing benchmark_fold_options(inline) so
inline expansion is honored. Update run_fold_benchmark_with_runner to use
run_fold_benchmark_with_writer with a closure that calls
write_folded_perfdata_file(input, benchmark_fold_options(inline), writer)
instead of run_fold_benchmark(input).

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 60950ccd-1830-4324-b7b2-6f0eaa7f4a0c

📥 Commits

Reviewing files that changed from the base of the PR and between dee5c3b and b940ddc.

⛔ Files ignored due to path filters (1)

• Cargo.lock is excluded by !**/*.lock

📒 Files selected for processing (34)

• .ace-aarch64-unwind-spec.md
• .ace-research-perf-unwind.md
• .ace-review-findings.md
• .beads/issues.jsonl
• .gitignore
• Cargo.toml
• docs/parity-findings.md
• scripts/oracle-compare
• scripts/oracle/Dockerfile
• scripts/oracle/compare-in-container.sh
• scripts/oracle/record-in-container.sh
• scripts/oracle/run-in-container.sh
• scripts/oracle/workload.rs
• scripts/perf-oracle
• src/backends/linux_perf.rs
• src/benchmarks.rs
• src/cargo_cli.rs
• src/cli.rs
• src/lib.rs
• src/perfdata/build_id.rs
• src/perfdata/fold.rs
• src/perfdata/header.rs
• src/perfdata/mappings.rs
• src/perfdata/unwind.rs
• src/platform.rs
• src/symbols.rs
• tests/benchmarks.rs
• tests/perfdata_build_id.rs
• tests/perfdata_fold.rs
• tests/perfdata_header.rs
• tests/perfdata_mappings.rs
• tests/perfdata_unwind.rs
• tests/run_cli.rs
• tests/symbols.rs

💤 Files with no reviewable changes (1)

• Cargo.toml

👮 Files not reviewed due to content moderation or server errors (8)

• src/perfdata/header.rs
• tests/perfdata_header.rs
• src/perfdata/mappings.rs
• src/symbols.rs
• scripts/oracle-compare
• scripts/oracle/Dockerfile
• scripts/oracle/compare-in-container.sh
• scripts/oracle/workload.rs

[coderabbitai]

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Correct the HEADER_ARCH feature-bit value in the spec.

Line 37 says HEADER_ARCH is feature bit 5, but the implementation uses bit 6; this mismatch will mislead future maintenance/debugging.

📝 Suggested doc fix

-`header.rs`). Add `header_arch_from_file` reading HEADER_ARCH (feature bit 5;
+`header.rs`). Add `header_arch_from_file` reading HEADER_ARCH (feature bit 6;

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	`header.rs`). Add `header_arch_from_file` reading HEADER_ARCH (feature bit 5;
	perf string format: u32 len + bytes, see perf util/header.c write_arch/read).
	`header.rs`). Add `header_arch_from_file` reading HEADER_ARCH (feature bit 6;
	perf string format: u32 len + bytes, see perf util/header.c write_arch/read).

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @.ace-aarch64-unwind-spec.md around lines 37 - 38, The spec incorrectly
documents HEADER_ARCH as feature bit 5; update the documentation to state
HEADER_ARCH is feature bit 6 to match the implementation used by
header_arch_from_file and header.rs (leave the perf string format note intact:
u32 len + bytes, see perf util/header.c write_arch/read). Ensure any mention of
"feature bit 5" is replaced with "feature bit 6" so the doc and the code remain
consistent.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Resolve markdownlint violations in this doc block.

Line 159/Line 163 trip MD031 (fenced blocks need surrounding blank lines), and Line 347/Line 357 trip MD029 (ordered-list prefix style). These are quick fixes and remove lint noise.

Also applies to: 347-357

🧰 Tools

🪛 markdownlint-cli2 (0.22.1)

[warning] 159-159: Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)

---

[warning] 163-163: Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @.ace-research-perf-unwind.md around lines 159 - 163, The fenced code blocks
around the snippet containing symbol_conf.hide_unresolved, entry->ms.sym,
append_inlines and callchain_cursor_append need blank lines before and after to
satisfy MD031; add an empty line immediately above the opening ``` and
immediately below the closing ``` for the blocks at the cited locations. Also
fix the ordered-list prefix style violations (MD029) in the block covering lines
347–357 by making the list prefixes consistent (e.g., convert all list markers
to numeric "1." or to the chosen style) so the entire list uses a single,
uniform ordered-list prefix format.

Source: Linters/SAST tools

[coderabbitai]

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Normalize heading spacing to satisfy MD022 across sections.

Multiple headings in this range are missing required surrounding blank lines per markdownlint (MD022). Please normalize heading spacing throughout this block to keep docs lint clean.

🧰 Tools

🪛 LanguageTool

[grammar] ~21-~21: Ensure spelling is correct
Context: ...fold.rs:1418/1440/1450 for every MMAP/MMAP2/MMAP2-build-id record. - What's wrong: `remove_overlapping_mapp...

(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)

---

[uncategorized] ~34-~34: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...* high. ### PERF-4 (HIGH) — Per-sample full stack unwind is re-run up to 9× via the modul...

(EN_COMPOUND_ADJECTIVE_INTERNAL)

---

[style] ~41-~41: It might be better to use ‘time’ with the time-relative pronoun ‘when’. (Alternatively, use ‘in/on which’.)
Context: ... is O(n²) over FDE ranges at module-add time - Where: src/perfdata/unwind.rs:476-487 — `...

(WHEN_WHERE)

🪛 markdownlint-cli2 (0.22.1)

[warning] 13-13: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

---

[warning] 20-20: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

---

[warning] 27-27: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

---

[warning] 34-34: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

---

[warning] 41-41: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

---

[warning] 48-48: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

---

[warning] 55-55: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

---

[warning] 62-62: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

---

[warning] 69-69: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

---

[warning] 76-76: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

---

[warning] 82-82: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

---

[warning] 89-89: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

---

[warning] 96-96: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

---

[warning] 107-107: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

---

[warning] 114-114: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

---

[warning] 121-121: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

---

[warning] 128-128: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

---

[warning] 135-135: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

---

[warning] 142-142: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

---

[warning] 149-149: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

---

[warning] 156-156: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

---

[warning] 166-166: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

---

[warning] 171-171: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

---

[warning] 178-178: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

---

[warning] 185-185: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @.ace-review-findings.md around lines 13 - 185, The markdown headings in the
findings block (e.g., lines starting with "### PERF-1", "### PERF-2", ..., "###
CORR-1", etc.) lack the required blank line(s) before/after them and trigger
MD022; edit .ace-review-findings.md to ensure each heading has an empty line
above and below (normalize to one blank line surrounding each top-level and
subsection heading) so all "### ..." headings are separated by a blank line from
adjacent paragraphs or other headings and the file passes markdownlint MD022.

Source: Linters/SAST tools

[coderabbitai]

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Fix MD038 code-span spacing at this line.

Line 34 includes a code span that markdownlint flags for internal spacing (MD038). Tightening that span formatting will clear the warning.

🧰 Tools

🪛 markdownlint-cli2 (0.22.1)

[warning] 34-34: Spaces inside code span elements

(MD038, no-space-in-code)

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/parity-findings.md` at line 34, Tighten the inline code span around
`__libc_start_main` and the adjacent `[unknown]` so there are no internal
leading/trailing spaces inside the backticks (remove any extra spaces between
the backticks and the text) to satisfy markdownlint MD038; update the text
fragment containing `__libc_start_main` and `[unknown]` accordingly.

Source: Linters/SAST tools

[coderabbitai]

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail
# Verifies current shell behavior: command substitution exits early under pipefail.
if bash -lc 'set -euo pipefail; PERF_BIN="$(find /definitely-missing/linux-tools* -name perf -type f 2>/dev/null | head -n 1)"; echo reached'; then
  echo "unexpected: did not fail"
else
  echo "expected: assignment failed before fallback guard"
fi

Repository: mjc/pyroclast

Length of output: 224

---

Prevent premature exit during perf binary discovery under set -euo pipefail.

With set -euo pipefail, the PERF_BIN="$(find ... | head -n 1)" command substitution can terminate the script when find fails (e.g., /usr/lib/linux-tools* missing), so the intended Line 17 guard ([ -n "$PERF_BIN" ] ...) may never run.

💡 Suggested fix

 if ! perf version >/dev/null 2>&1; then
-    PERF_BIN="$(find /usr/lib/linux-tools* -name perf -type f 2>/dev/null | head -n 1)"
+    PERF_BIN="$(find /usr/lib/linux-tools* -name perf -type f 2>/dev/null | head -n 1 || true)"
     [ -n "$PERF_BIN" ] || { echo "no perf binary found" >&2; exit 1; }
     perf() { "$PERF_BIN" "$@"; }
 fi

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@scripts/oracle/record-in-container.sh` around lines 16 - 17, The find command
substitution assigning PERF_BIN can fail under set -euo pipefail and cause the
script to exit before the subsequent guard runs; change the assignment to make
the command safe (e.g., append || true or wrap the pipeline in a brace with ||
true) so PERF_BIN="$(find /usr/lib/linux-tools* -name perf -type f 2>/dev/null |
head -n 1 || true)" (or equivalent) and keep the existing guard [ -n "$PERF_BIN"
] || { echo "no perf binary found" >&2; exit 1; } to handle the missing binary.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Update the documented error contract after removing type validation.

parse_build_id_event now intentionally accepts perf-style feature records with header.type == 0, but the parse_build_id_events docs still state it errors on wrong record type. Please align docs with current behavior.

Suggested diff

-/// Returns an error when a record is truncated, has the wrong type, or carries
-/// non-UTF-8 filename data.
+/// Returns an error when a record is truncated or carries non-UTF-8 filename
+/// data.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/perfdata/build_id.rs` around lines 145 - 153, Update the documentation
for parse_build_id_event(s) to reflect that these functions intentionally accept
perf-style feature records with header.type == 0 and do not error on a
non-build-id record type; describe that framing is based on the size field (read
via read_u16 at offset 4) and PERF_RECORD_MISC_BUILD_ID_SIZE is used to obtain
build_id_size (offset into the build_id area), so callers should not expect a
type validation error anymore and should rely on the size-based parsing
semantics implemented in parse_build_id_event and parse_build_id_events.

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Normalize kernel DSO names in mapped-unknown-symbol frames too.

Line 3089 writes ({path}) directly, so kernel mappings such as [kernel.kallsyms]_... bypass perf_script_dso_name and can diverge from perf-script output on unsymbolized paths.

🐛 Proposed fix

 fn write_perf_script_mapped_unknown_symbol_frame_fragment<W>(
     writer: &mut W,
     prefix: &str,
     address: u64,
     path: &str,
 ) -> Result<(), String>
 where
     W: IoWrite + ?Sized,
 {
-    write!(writer, "{prefix}{address:16x} {UNKNOWN_FRAME} ({path})")
+    let path = perf_script_dso_name(path);
+    write!(writer, "{prefix}{address:16x} {UNKNOWN_FRAME} ({path})")
         .map_err(|error| format!("failed to write perf script output: {error}"))
 }

Also applies to: 3080-3090

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/perfdata/fold.rs` around lines 3061 - 3063, The write call is emitting
the raw path into perf script output, skipping normalization for kernel DSO
names; update the places that write mapped-unknown-symbol frames (the
write!(writer, "{prefix}{address:16x} {label} ({path})") usages around the
mapped-unknown-symbol handling) to call perf_script_dso_name(path) first (as
done earlier with let path = perf_script_dso_name(path)), ensuring every place
that formats "({path})" uses the normalized path returned by
perf_script_dso_name so kernel mappings like `[kernel.kallsyms]_...` are
normalized consistently.

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Bound the exact-IP unwind memo to prevent unbounded growth.

Line 418 inserts every queried IP into unwind_info_memo, and the map is only cleared when a new module is added (Lines 383-387). In steady-state sampling with high IP cardinality, this can grow indefinitely and drive avoidable memory pressure.

💡 Suggested guard (bounded cache with simple reset)

+const UNWIND_INFO_MEMO_MAX_ENTRIES: usize = 200_000;
+
 pub fn has_unwind_info_for_ip(&self, ip: u64) -> bool {
     if let Some(&cached) = self.unwind_info_memo.borrow().get(&ip) {
         return cached;
     }
     let covered = self
         .reported_modules
         .iter()
         .any(|module| module.unwind_ranges.iter().any(|range| range.contains(&ip)));
-    self.unwind_info_memo.borrow_mut().insert(ip, covered);
+    let mut memo = self.unwind_info_memo.borrow_mut();
+    if memo.len() >= UNWIND_INFO_MEMO_MAX_ENTRIES {
+        memo.clear();
+    }
+    memo.insert(ip, covered);
     covered
 }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/perfdata/unwind.rs` around lines 410 - 419, The memo in
has_unwind_info_for_ip currently inserts every queried IP into unwind_info_memo
which can grow unbounded; change has_unwind_info_for_ip to bound the cache by
checking unwind_info_memo.borrow().len() (or a tracked count) against a small
MAX_UNWIND_MEMO_ENTRIES constant and, when the limit is reached, either clear
the map (or skip inserting new entries) before inserting the result; keep the
existing lookup logic and still clear the memo when reported_modules changes,
but ensure you reference has_unwind_info_for_ip, unwind_info_memo, and
reported_modules so the cache trimming/reset is applied just prior to the insert
to prevent unbounded growth.

[coderabbitai]

🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win

Make arch/register mismatches explicit instead of silently returning empty frames.

Line 507 currently swallows mismatches (_ => {}), which can look like a legitimate empty unwind and hide upstream state/contract drift. Prefer an explicit assertion/fallback signal so this failure mode is diagnosable.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/perfdata/unwind.rs` around lines 485 - 508, The match currently swallows
architecture/register mismatches in the unwind loop (the `_ => {}` arm), hiding
contract drift between the PerfUnwinder variant and the PerfUserRegs variant;
replace that silent no-op with an explicit failure signal (e.g., panic!,
unreachable!, or a debug_assert!) that includes the mismatched values so it is
diagnosable — update the catch-all arm in the match over (self, regs) to call
something like unreachable!("arch/register mismatch: unwinder={:?} regs={:?}",
self, regs) or debug_assert!(false, ...) (ensure PerfUserRegs and the unwinder
type implement Debug or format a descriptive message) so mismatches in the code
paths (the branches using unwinder.iter_frames, regs.to_framehop_regs,
push_perf_unwind_address) are surfaced instead of returning empty frames.

[coderabbitai]

🧹 Nitpick | 🔵 Trivial | ⚡ Quick win

Consider logging or propagating read_dir iterator errors.

Line 67's filter_map(Result::ok) silently drops any errors that occur while reading individual directory entries (e.g., permission denied on a specific task entry, I/O errors). While this pattern is common for robust directory traversal, it provides no visibility into failures.

Consider logging dropped errors or at least documenting this behavior, especially since the function later returns a NotFound error when no numeric thread IDs are found—making it unclear whether the directory was truly empty or whether all entries failed to read.

📝 Possible approaches

Option 1: Log dropped errors

     let mut tids = std::fs::read_dir(&task_dir)?
-        .filter_map(Result::ok)
+        .filter_map(|entry| {
+            entry.or_else(|err| {
+                log::debug!("Skipping task entry: {}", err);
+                Err(err)
+            }).ok()
+        })
         .filter_map(|entry| {

Option 2: Document the silent-skip behavior

 /// Reads Linux thread IDs from a procfs root, usually `/proc`.
 ///
+/// Directory entries that cannot be read are silently skipped for robustness.
+///
 /// # Errors

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	let mut tids = std::fs::read_dir(&task_dir)?
	.filter_map(Result::ok)
	.filter_map(|entry| {
	entry
	.file_name()
	.to_str()
	.and_then(|name| name.parse().ok())
	})
	.collect::<Vec<u32>>();
	let mut tids = std::fs::read_dir(&task_dir)?
	.filter_map(|entry| {
	entry.or_else(|err| {
	log::debug!("Skipping task entry: {}", err);
	Err(err)
	}).ok()
	})
	.filter_map(|entry| {
	entry
	.file_name()
	.to_str()
	.and_then(|name| name.parse().ok())
	})
	.collect::<Vec<u32>>();

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/platform.rs` around lines 66 - 74, The current read_dir pipeline for
building tids silently drops errors via filter_map(Result::ok); update it to
either log each dropped error or propagate the first encountered error so
callers can distinguish an empty dir from read failures: modify the iterator
that uses read_dir(&task_dir)? and filter_map(Result::ok) so errors are handled
explicitly (e.g., replace filter_map(Result::ok) with a map that matches
Result::Err(e) and either calls log::warn! with context including task_dir and e
before returning None, or returns Err(e) to propagate using a try/collect
pattern), keeping the rest of the file-name-to-u32 parsing and the tids Vec<u32>
collection unchanged.