Conversation
📝 WalkthroughWalkthroughThe PR adds JITDump parsing and per-PID symbol resolution, injects Bun JITDump enablement into spawned processes, adds Bun-specific runtime warnings, and extends unwind handling for Zig binaries. It also adds Bun end-to-end coverage. ChangesRuntime JITDump integration
Sequence Diagram(s)sequenceDiagram
participant Spawn
participant EventLoop
participant Jitdump
participant TraceHandler
participant Symbolizer
Spawn->>Spawn: detect Bun and inject BUN_JSC_useJITDump=1
EventLoop->>Jitdump: find_jitdump_for_pid(tgid)
EventLoop->>Jitdump: load_from_file(path)
EventLoop->>TraceHandler: register_jit_table(tgid, table)
Symbolizer->>Jitdump: find_jitdump_for_pid(pid)
Symbolizer->>Jitdump: load_from_file(path)
Symbolizer->>Jitdump: resolve(addr)
TraceHandler->>Jitdump: format_jit_symbol(sym)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~90+ minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
profile-bee/src/trace_handler.rs (1)
367-373:⚠️ Potential issue | 🟡 MinorCaching concern: JITDump overrides won't reflect later reloads.
get_exp_stacked_framescaches the symbolized result by(tgid, ktrace_id, utrace_id)for reuse. In streaming/TUI mode,event_loop::reload_jitdump_tablespicks up new symbols between windows, but any stack that hit the cache before those symbols were loaded will keep serving[unknown]from the cached vector — JITDump-based overrides only apply on the first symbolization for a given stack ID. For long-lived processes that JIT functions progressively, this can leave stale[unknown]frames in the flamegraph despite the reload.Consider invalidating (or at least refreshing the JIT-override-dependent entries of) the cache when
reload_jitdump_tablesadds new symbols for a PID, or performing the JIT override on the cached result at lookup time rather than at insertion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@profile-bee/src/trace_handler.rs` around lines 367 - 373, The cached symbolized stacks returned by get_exp_stacked_frames are becoming stale when reload_jitdump_tables adds new JIT symbols; update the cache logic so JIT-dump overrides are not permanently baked into cached entries—either invalidate or version cache entries when reload_jitdump_tables updates symbols for a PID or apply the JIT override at lookup time. Concretely: add a JIT-generation/version or last-jit-update marker per tgid that reload_jitdump_tables increments, include that marker in the cache key or store it with entries and check it in get_exp_stacked_frames before returning self.cache.get(tgid, ktrace_id, utrace_id), and if out-of-date re-run the JIT-override step (or refresh/invalidate the entry) so newly loaded JIT symbols are applied. Ensure references to self.cache.get, get_exp_stacked_frames and reload_jitdump_tables are the touch points for this change.
🧹 Nitpick comments (6)
profile-bee/src/event_loop.rs (2)
544-550:reload_jitdump_tables()runs on everydrain_eventscall, including thecollect_unsymbolizedpath.In raw-capture mode (
collect_unsymbolized), symbolization is deliberately skipped for speed, but this unconditional tail call still does filesystem I/O and parsing per PID per window. Consider skipping the reload whensymbolize == false, since the raw output path never reads the JIT tables anyway.♻️ Proposed refactor
- // Reload JITDump symbol tables at the end of the collection window. - // JIT runtimes (Bun, Java, etc.) write symbols incrementally as code - // is compiled — the initial load on first PID sight may have found an - // empty or nonexistent file. This final pass picks up all symbols - // written during the profiling window. - self.reload_jitdump_tables(); + // Reload JITDump symbol tables at the end of the collection window, + // but only when we intend to symbolize — raw-capture mode doesn't + // consult these tables. + if symbolize { + self.reload_jitdump_tables(); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@profile-bee/src/event_loop.rs` around lines 544 - 550, The call to reload_jitdump_tables() in drain_events causes unnecessary I/O during the collect_unsymbolized/raw-capture path; update drain_events to skip calling reload_jitdump_tables() when symbolization is disabled (check the symbolization flag used elsewhere, e.g. symbolize or the collect_unsymbolized code path) so that reload_jitdump_tables() only runs when symbolization is enabled; locate the drain_events implementation and add a conditional guard around the reload_jitdump_tables() tail call (or early-return for the raw path) so JIT parsing is avoided when symbolize == false or when collect_unsymbolized is active.
342-368:self.known_tgids.clone()per drain cycle is unnecessary.
reload_jitdump_tablesis called at the tail of everydrain_events(so once per collection window / TUI refresh). Cloning the wholeHashSet<u32>each time is avoidable — collect into a smallVec<u32>once via.iter().copied().collect()if you need to release the borrow onself.known_tgids, or restructure to avoid the borrow entirely.Also, on every tick this function does a
stat/openper known PID (viafind_jitdump_for_pid, which doesread_dir("/tmp")for the JSC pattern path). For a system-wide profile with many PIDs this is O(N × dir_size) syscalls per window — probably fine in practice, but worth noting if profiling dense workloads.♻️ Proposed refactor
- pub fn reload_jitdump_tables(&mut self) { - for &tgid in &self.known_tgids.clone() { + pub fn reload_jitdump_tables(&mut self) { + let tgids: Vec<u32> = self.known_tgids.iter().copied().collect(); + for tgid in tgids {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@profile-bee/src/event_loop.rs` around lines 342 - 368, The current reload_jitdump_tables clones self.known_tgids every call which is unnecessary and expensive; change the iteration to first collect the PIDs into a small Vec<u32> (e.g. self.known_tgids.iter().copied().collect()) so you release the borrow on self before calling jitter lookups, then iterate that Vec and call jitdump::find_jitdump_for_pid, trace_handler.jit_table_mut(tgid) and try_load_jitdump_for_pid(tgid) as before; this removes the per-cycle HashSet clone while preserving behavior and prevents holding a borrow across calls that access the filesystem.profile-bee/src/jitdump.rs (2)
108-117:reload_from_filedoesn't re-validate on file rotation.If a JIT runtime rotates or truncates
/tmp/jit-<pid>.dumpbetween reads (e.g., process exited and a reused PID wrote a fresh file), seeking to the oldlast_read_offsetwill land mid-record of the new file and produce garbage before tripping the truncation fallback. Not a bug today given the eBPF lifecycle hooks invalidate caches on exec/exit, but worth guarding — e.g., checkingfile.metadata().len() < self.last_read_offsetand resetting, or comparing the header magic bytes at offset 0 before seeking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@profile-bee/src/jitdump.rs` around lines 108 - 117, In reload_from_file, guard against rotated/truncated files by validating the file before seeking: open the file, check file.metadata().len() and if it's less than self.last_read_offset set self.last_read_offset = 0 (or otherwise reset state), and/or read and verify the header magic bytes at offset 0 match the expected JIT dump signature before doing reader.seek(SeekFrom::Start(self.last_read_offset)); then proceed to call self.read_records(&mut reader) and update symbols as before; make sure to reference and update the fields last_read_offset and symbols and use the existing read_records method when implementing the fallback reset.
342-365: Minor:format!("jit-")is unnecessary and PID substring match can over-accept.Two small points on
find_jitdump_for_pid:
- Line 351:
let prefix = format!("jit-");allocates viaformat!with no interpolation —"jit-"can be used directly (clippyuseless_format).- The JSC pattern match
name.contains(&format!("-{}-", pid_str))is a substring test on the whole filename. For small PIDs this can incidentally match the<tid>or<random>segment rather than the intended<pid>segment (e.g.pid=5matchingjit-5-<otherpid>-abcorjit-123-456-5abcvariants). Splitting on-and checking the PID is at position 2 would be more precise.♻️ Proposed refactor
- // JSC/Bun convention: /tmp/jit-<tid>-<pid>-<random> - // Scan /tmp/ for files matching this pattern. - let prefix = format!("jit-"); - let pid_str = pid.to_string(); - if let Ok(entries) = std::fs::read_dir("/tmp") { - for entry in entries.flatten() { - let name = entry.file_name(); - let name = name.to_string_lossy(); - // Match pattern: jit-<digits>-<pid>-<alphanum> - if name.starts_with(&prefix) && name.contains(&format!("-{}-", pid_str)) { - return Some(entry.path()); - } - } - } + // JSC/Bun convention: /tmp/jit-<tid>-<pid>-<random> + let pid_str = pid.to_string(); + if let Ok(entries) = std::fs::read_dir("/tmp") { + for entry in entries.flatten() { + let name = entry.file_name(); + let name = name.to_string_lossy(); + // Match pattern: jit-<tid>-<pid>-<random> + let parts: Vec<&str> = name.split('-').collect(); + if parts.len() >= 4 && parts[0] == "jit" && parts[2] == pid_str { + return Some(entry.path()); + } + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@profile-bee/src/jitdump.rs` around lines 342 - 365, In find_jitdump_for_pid, avoid the useless allocation from format!("jit-") by using the literal "jit-" and make the JSC/Bun filename match precise by splitting the filename on '-' and verifying the PID is in the expected segment: check that the split parts length is >= 3 and parts[2] == pid_str (instead of name.contains(&format!("-{}-", pid_str))). Keep the existing starts_with("jit-") check (use the literal) and return entry.path() when the split PID matches.profile-bee/bin/profile-bee.rs (1)
612-617: Three near-duplicate warning blocks — consider extracting.The
spawn.is_none() { if let Some(target_pid) = pid { warn_nodejs_without_perf_map(target_pid); warn_bun_without_jitdump(target_pid); } }pattern is now repeated verbatim across the batch path,run_combined_mode, andrun_tui_mode. A tiny helper (e.g.warn_unsupported_jit_runtimes(target_pid)wrapping both calls) would make future additions (Deno, Java, …) a one-liner.Also applies to: 1664-1670, 1773-1779
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@profile-bee/bin/profile-bee.rs` around lines 612 - 617, Extract the duplicated block that checks spawn.is_none() and, when pid is Some, calls warn_nodejs_without_perf_map and warn_bun_without_jitdump into a small helper function (e.g. warn_unsupported_jit_runtimes(target_pid: i32)) and replace the three verbatim sites (the batch path, run_combined_mode, and run_tui_mode) with a single call to that helper; specifically, create the helper that accepts the target_pid and invokes warn_nodejs_without_perf_map(target_pid) and warn_bun_without_jitdump(target_pid), then in each location replace the if spawn.is_none() { if let Some(target_pid) = pid { ... } } block with the same pattern that calls the new helper to keep the spawn/pid check but centralize the warnings.tests/run_e2e.sh (1)
611-664: Optional:test_bun_samples_collectedlargely duplicates the sample check insidetest_bun_jitdump_callstack.Both tests gate on
bun/fixture presence, runrun_bun_profiler, and assertcount_samples > 0. The only extra behavior intest_bun_jitdump_callstackis theassert_stack_containscheck. This mirrors the Node.js split (test_nodejs_samples_collected+test_nodejs_callstack), so it's consistent — but be aware each test spins up a fresh profile run (~1s + overhead), effectively doubling Bun test runtime for a check already covered by the callstack test. Consider collapsing into a single test if CI time becomes a concern.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/run_e2e.sh` around lines 611 - 664, The two tests test_bun_samples_collected and test_bun_jitdump_callstack duplicate the same setup and sample-count check; to reduce redundant runs collapse them by removing test_bun_samples_collected and moving its basic sample existence check into test_bun_jitdump_callstack before the assert_stack_contains call (use the existing run_bun_profiler and count_samples logic), or alternatively have test_bun_jitdump_callstack call test_bun_samples_collected to reuse its check; update references to run_bun_profiler, count_samples, and assert_stack_contains accordingly so only one profiler run is performed per Bun fixture.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@profile-bee/src/jitdump.rs`:
- Around line 689-692: test_find_jitdump_nonexistent currently scans the real
/tmp and can false-fail if a stale jit file exists; update the test to use an
isolated temporary directory instead of the global /tmp by either (a) adding a
helper that searches a provided directory (e.g.,
find_jitdump_for_pid_in_dir(pid: u32, dir: &Path) or make find_jitdump_for_pid
accept a Path argument) and calling that helper from the test with a tempdir
created via tempfile::tempdir(), or (b) change the test to use a randomized high
PID that is extremely unlikely to exist; prefer option (a) so change the
test_find_jitdump_nonexistent to create a tempfile::TempDir, call the
new/modified search function (referencing find_jitdump_for_pid or new
find_jitdump_for_pid_in_dir) against that temp dir, and assert is_none().
- Around line 199-247: The bug is a key mismatch between parse_debug_info and
parse_code_load: change both sides to key pending_debug by code_addr (u64)
instead of using code_index; specifically, in parse_code_load replace the lookup
self.pending_debug.remove(&code_index) with
self.pending_debug.remove(&code_addr) and ensure parse_debug_info inserts into
pending_debug using the debug record's code_addr as the map key; also update the
parse_debug_info doc comment to state that code_addr is the correlation key (not
a "proxy for code_index") so future readers aren't confused.
In `@tests/run_e2e.sh`:
- Around line 661-663: The current regex passed to assert_stack_contains
("hot\|processData\|handleRequest\|serverLoop") is too loose and can match
unrelated native symbols; tighten it by anchoring to exact JS function names
from bun_callstack.js (use word-boundaries like
\bprocessData\b,\bhandleRequest\b,\bserverLoop\b) or replace the check with an
assertion that matches JITDump's collapse-line format (e.g. the resolved
collapse line pattern produced by JITDump for bun_callstack.js) so the test only
passes when symbolization actually resolved JS function names; update the call
to assert_stack_contains to use the new anchored regex or format-specific
pattern.
---
Outside diff comments:
In `@profile-bee/src/trace_handler.rs`:
- Around line 367-373: The cached symbolized stacks returned by
get_exp_stacked_frames are becoming stale when reload_jitdump_tables adds new
JIT symbols; update the cache logic so JIT-dump overrides are not permanently
baked into cached entries—either invalidate or version cache entries when
reload_jitdump_tables updates symbols for a PID or apply the JIT override at
lookup time. Concretely: add a JIT-generation/version or last-jit-update marker
per tgid that reload_jitdump_tables increments, include that marker in the cache
key or store it with entries and check it in get_exp_stacked_frames before
returning self.cache.get(tgid, ktrace_id, utrace_id), and if out-of-date re-run
the JIT-override step (or refresh/invalidate the entry) so newly loaded JIT
symbols are applied. Ensure references to self.cache.get, get_exp_stacked_frames
and reload_jitdump_tables are the touch points for this change.
---
Nitpick comments:
In `@profile-bee/bin/profile-bee.rs`:
- Around line 612-617: Extract the duplicated block that checks spawn.is_none()
and, when pid is Some, calls warn_nodejs_without_perf_map and
warn_bun_without_jitdump into a small helper function (e.g.
warn_unsupported_jit_runtimes(target_pid: i32)) and replace the three verbatim
sites (the batch path, run_combined_mode, and run_tui_mode) with a single call
to that helper; specifically, create the helper that accepts the target_pid and
invokes warn_nodejs_without_perf_map(target_pid) and
warn_bun_without_jitdump(target_pid), then in each location replace the if
spawn.is_none() { if let Some(target_pid) = pid { ... } } block with the same
pattern that calls the new helper to keep the spawn/pid check but centralize the
warnings.
In `@profile-bee/src/event_loop.rs`:
- Around line 544-550: The call to reload_jitdump_tables() in drain_events
causes unnecessary I/O during the collect_unsymbolized/raw-capture path; update
drain_events to skip calling reload_jitdump_tables() when symbolization is
disabled (check the symbolization flag used elsewhere, e.g. symbolize or the
collect_unsymbolized code path) so that reload_jitdump_tables() only runs when
symbolization is enabled; locate the drain_events implementation and add a
conditional guard around the reload_jitdump_tables() tail call (or early-return
for the raw path) so JIT parsing is avoided when symbolize == false or when
collect_unsymbolized is active.
- Around line 342-368: The current reload_jitdump_tables clones self.known_tgids
every call which is unnecessary and expensive; change the iteration to first
collect the PIDs into a small Vec<u32> (e.g.
self.known_tgids.iter().copied().collect()) so you release the borrow on self
before calling jitter lookups, then iterate that Vec and call
jitdump::find_jitdump_for_pid, trace_handler.jit_table_mut(tgid) and
try_load_jitdump_for_pid(tgid) as before; this removes the per-cycle HashSet
clone while preserving behavior and prevents holding a borrow across calls that
access the filesystem.
In `@profile-bee/src/jitdump.rs`:
- Around line 108-117: In reload_from_file, guard against rotated/truncated
files by validating the file before seeking: open the file, check
file.metadata().len() and if it's less than self.last_read_offset set
self.last_read_offset = 0 (or otherwise reset state), and/or read and verify the
header magic bytes at offset 0 match the expected JIT dump signature before
doing reader.seek(SeekFrom::Start(self.last_read_offset)); then proceed to call
self.read_records(&mut reader) and update symbols as before; make sure to
reference and update the fields last_read_offset and symbols and use the
existing read_records method when implementing the fallback reset.
- Around line 342-365: In find_jitdump_for_pid, avoid the useless allocation
from format!("jit-") by using the literal "jit-" and make the JSC/Bun filename
match precise by splitting the filename on '-' and verifying the PID is in the
expected segment: check that the split parts length is >= 3 and parts[2] ==
pid_str (instead of name.contains(&format!("-{}-", pid_str))). Keep the existing
starts_with("jit-") check (use the literal) and return entry.path() when the
split PID matches.
In `@tests/run_e2e.sh`:
- Around line 611-664: The two tests test_bun_samples_collected and
test_bun_jitdump_callstack duplicate the same setup and sample-count check; to
reduce redundant runs collapse them by removing test_bun_samples_collected and
moving its basic sample existence check into test_bun_jitdump_callstack before
the assert_stack_contains call (use the existing run_bun_profiler and
count_samples logic), or alternatively have test_bun_jitdump_callstack call
test_bun_samples_collected to reuse its check; update references to
run_bun_profiler, count_samples, and assert_stack_contains accordingly so only
one profiler run is performed per Bun fixture.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6edf4292-b853-4eae-9fdb-3dc02aead58e
📒 Files selected for processing (9)
profile-bee/bin/profile-bee.rsprofile-bee/src/event_loop.rsprofile-bee/src/jitdump.rsprofile-bee/src/lib.rsprofile-bee/src/spawn.rsprofile-bee/src/symbolize.rsprofile-bee/src/trace_handler.rstests/fixtures/src/bun_callstack.jstests/run_e2e.sh
Add a JITDump binary format parser that resolves JIT-compiled JavaScript function names from runtimes like Bun (JavaScriptCore), Java HotSpot, and LuaJIT. Without this, JIT code in anonymous mmap'd pages shows as [unknown]. Implementation: - New jitdump.rs: zero-dependency parser with BTreeMap for O(log n) address range lookups, incremental reload for streaming modes, tolerates truncated files. Handles JIT_CODE_LOAD, JIT_CODE_MOVE, JIT_CODE_DEBUG_INFO, and JIT_CODE_CLOSE records. Discovers both standard (jit-<pid>.dump) and JSC (jit-<tid>-<pid>-<random>) file naming conventions. - trace_handler.rs: JIT tables per PID, override [unknown] symbols after V8 SFI resolution (V8 takes priority for Node.js, JITDump for non-V8). - event_loop.rs: auto-load JITDump on new PID detection, reload at end of each collection window to catch symbols written during profiling. - spawn.rs: auto-detect bun/bunx and inject BUN_JSC_useJITDump=1. - profile-bee.rs: warn when --pid targets Bun without JITDump file. - symbolize.rs: JITDump override in offline re-symbolization path. - E2E tests: bun_callstack.js fixture + 2 tests verifying sample collection and JS function name resolution via JITDump.
Bug fixes:
- Fix key mismatch between parse_debug_info and parse_code_load: both
now correctly correlate via code_addr (was using code_index in lookup
which would never match the code_addr key from debug_info).
- Fix cache staleness: reload_jitdump_tables now invalidates the symbol
cache (but not the JIT table itself) when new symbols are loaded, so
previously-cached [unknown] frames get re-resolved. Added targeted
invalidate_symbol_cache_for_pid() that doesn't remove V8/JIT readers.
- Fix find_jitdump_for_pid JSC filename parsing: use precise splitn('-')
and check parts[2] == pid_str instead of loose contains() match.
- Guard reload_from_file against truncated/rotated files: if file size <
last_read_offset, reset and re-parse from scratch.
Test improvements:
- Refactor find_jitdump_for_pid into find_jitdump_for_pid_in_dir(pid, dir)
for testability; test uses isolated temp dirs instead of scanning /tmp.
- Added tests for both standard and JSC file naming conventions.
- Tighten E2E regex to require JSC JIT tier prefix (JSC-FTL/JSC-DFG)
before function names, preventing false matches on native symbols.
- Collapse duplicate test_bun_samples_collected into test_bun_jitdump_callstack.
Code quality:
- Extract duplicated warn block into warn_unsupported_jit_runtimes() helper.
- Skip reload_jitdump_tables() when symbolize=false (raw-capture path).
- Replace known_tgids.clone() with .iter().copied().collect::<Vec<_>>().
- Detect Zig binaries and apply stop-unwind override at _start to fix bogus DWARF frames from older Zig compilers - Add Zig symbol demangling (strip __anon_, __struct_, __hex suffixes) - Strip GCC/LLVM clone suffixes (.cold, .constprop, .isra, .llvm.*) - Strip JSC JIT code version hash from JITDump symbols (#DdCypB) - Warn when Bun process is detected without JITDump file during system-wide profiling
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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 `@profile-bee/src/dwarf_unwind.rs`:
- Around line 1038-1065: The Bun test in test_detect_zig_start_address_bun
currently only verifies that detect_zig_start_address does not crash because it
allows None to pass; either change the assertion to require a successful Zig
start address detection for Bun (and validate the returned value), or rename the
test to explicitly describe that it is only a smoke test for a real Zig binary.
- Around line 81-82: The `_start` handling in dwarf_unwind.rs currently drops
the symbol size and only applies the override when `entry.pc == _start`, so
update the `_start` capture path to preserve both address and size and treat the
symbol as a range instead of a single PC. In the logic around `start_addr` and
the unwind-row override, use the recorded `_start` span to override any PCs that
fall within the whole `_start` range, and also allow the override to trigger for
stripped binaries that only provide an address with size 0.
In `@profile-bee/src/jitdump.rs`:
- Around line 249-251: The JITDump record parsing in the name/filename handling
path is allocating a buffer directly from the untrusted payload length, which
can trigger huge allocations before validation. Update the parsing logic in the
relevant reader flow around the record-name handling (including the equivalent
code path at the later matching site) to read the NUL-terminated string into a
capped, fixed-size buffer and then stream-skip any remaining bytes instead of
sizing a Vec from payload_size. Keep the fix localized to the record parsing
functions that currently use remaining and name_and_code so the allocation is no
longer driven by attacker-controlled sizes.
- Around line 214-217: The unknown-record branch in jitdump parsing should
handle truncation the same way as the known-record paths do. Update the record
processing logic around the unknown-type fallback in jitdump.rs so that when
skip_bytes(reader, payload_size) returns UnexpectedEof, you preserve
last_read_offset = record_start and break out of the loop instead of treating it
as a hard error; keep the behavior for other I/O errors unchanged. Locate the
fix in the record-dispatch code that handles unknown record types and the same
truncation handling used by the existing known-record parsing branches.
- Line 69: The JIT symbol range check in the `jitdump.rs` lookup logic can
overflow when computing the end of a symbol range with `code_addr + code_size`.
Update the filter in the JIT symbol matching path to avoid addition overflow by
comparing against a subtraction-based bound instead, using the existing
`sym.code_addr`, `sym.code_size`, and `addr` values in the same lookup function.
- Around line 281-293: The JIT_CODE_MOVE parser in parse_code_move is reading
the record with the wrong fixed layout, so it treats the leading pid/tid/vma
fields as old_code_addr and mis-parses real move records. Update parse_code_move
to consume the full perf jitdump JIT_CODE_MOVE header in the correct order
before reading old_code_addr, new_code_addr, new_code_size, and code_index, and
adjust the FIXED_SIZE check accordingly. Also update the code_move_record test
helper to emit the same full 48-byte payload so the parser and tests match the
jitdump format.
In `@profile-bee/src/trace_handler.rs`:
- Around line 388-394: The register_jit_table() path in trace_handler.rs should
also invalidate any cached stack entries for the same PID, since a fresh JIT
table can arrive after “[unknown]” stacks have already been cached. Update
register_jit_table() to clear the symbol cache for the given tgid before or when
inserting into self.jit_tables, so subsequent lookups can resolve with the new
JIT symbols. Use the existing cache and JitSymbolTable-related symbols in
TraceHandler to keep the fix localized.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2ab38fcb-1b30-4c62-809b-62917cf8c206
📒 Files selected for processing (10)
profile-bee/bin/profile-bee.rsprofile-bee/src/dwarf_unwind.rsprofile-bee/src/event_loop.rsprofile-bee/src/jitdump.rsprofile-bee/src/lib.rsprofile-bee/src/spawn.rsprofile-bee/src/symbolize.rsprofile-bee/src/trace_handler.rstests/fixtures/src/bun_callstack.jstests/run_e2e.sh
✅ Files skipped from review due to trivial changes (1)
- profile-bee/src/lib.rs
🚧 Files skipped from review as they are similar to previous changes (5)
- tests/fixtures/src/bun_callstack.js
- profile-bee/src/spawn.rs
- profile-bee/src/symbolize.rs
- profile-bee/src/event_loop.rs
- profile-bee/bin/profile-bee.rs
Comment on lines
+81
to
+82
| if name == "_start" && symbol.size() > 0 { | ||
| start_addr = Some(symbol.address()); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Carry _start size through and override the whole range.
The helper rejects zero-sized _start symbols and then discards non-zero sizes, while the override only handles entry.pc == _start. PCs inside _start can still use the bogus CFI rows, and stripped binaries with an address but size 0 get no override at all.
Suggested direction
-fn detect_zig_start_address(obj: &object::File) -> Option<u64> {
+fn detect_zig_start_address(obj: &object::File) -> Option<(u64, u64)> {
@@
- if name == "_start" && symbol.size() > 0 {
- start_addr = Some(symbol.address());
+ if name == "_start" {
+ start_addr = Some((symbol.address(), symbol.size()));
@@
- if start_addr.is_none() && name == "_start" && symbol.size() > 0 {
- start_addr = Some(symbol.address());
+ if start_addr.is_none() && name == "_start" {
+ start_addr = Some((symbol.address(), symbol.size()));
@@
- if let Some(start_pc) = zig_start_override {
- let start_pc_relative = (start_pc - base_vaddr) as u32;
+ if let Some((start_pc, start_size)) = zig_start_override {
+ let Some(start_pc_relative) = start_pc
+ .checked_sub(base_vaddr)
+ .and_then(|pc| u32::try_from(pc).ok())
+ else {
+ return Ok((entries, build_id));
+ };
+ let end_pc_relative = start_pc
+ .checked_add(start_size)
+ .and_then(|end| end.checked_sub(base_vaddr))
+ .and_then(|end| u32::try_from(end).ok());
for entry in entries.iter_mut() {
- if entry.pc == start_pc_relative {
+ let in_start = if start_size == 0 {
+ entry.pc == start_pc_relative
+ } else {
+ end_pc_relative
+ .map(|end| entry.pc >= start_pc_relative && entry.pc < end)
+ .unwrap_or(false)
+ };
+ if in_start {
entry.cfa_type = CFA_REG_EXPRESSION;
@@
- break;
}
}
}Also applies to: 97-98, 535-549
🤖 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 `@profile-bee/src/dwarf_unwind.rs` around lines 81 - 82, The `_start` handling
in dwarf_unwind.rs currently drops the symbol size and only applies the override
when `entry.pc == _start`, so update the `_start` capture path to preserve both
address and size and treat the symbol as a range instead of a single PC. In the
logic around `start_addr` and the unwind-row override, use the recorded `_start`
span to override any PCs that fall within the whole `_start` range, and also
allow the override to trigger for stripped binaries that only provide an address
with size 0.
Comment on lines
+1038
to
+1065
| #[test] | ||
| fn test_detect_zig_start_address_bun() { | ||
| // Bun is a Zig-compiled binary — if installed, verify detection | ||
| let candidates = [ | ||
| std::path::PathBuf::from("/usr/local/bin/bun"), | ||
| std::env::var("HOME") | ||
| .map(|h| std::path::PathBuf::from(h).join(".bun/bin/bun")) | ||
| .unwrap_or_default(), | ||
| ]; | ||
| let path = match candidates.iter().find(|p| p.exists()) { | ||
| Some(p) => p.clone(), | ||
| None => return, // Bun not installed, skip | ||
| }; | ||
| let data = match std::fs::read(&path) { | ||
| Ok(d) => d, | ||
| Err(_) => return, | ||
| }; | ||
| let obj = match object::File::parse(&*data) { | ||
| Ok(o) => o, | ||
| Err(_) => return, | ||
| }; | ||
| let result = detect_zig_start_address(&obj); | ||
| // Bun is compiled with Zig so it should have __zig* symbols | ||
| // and _start. However, Bun is stripped so _start may not have size > 0. | ||
| // We just verify the function doesn't crash on a real Zig binary. | ||
| if result.is_some() { | ||
| assert!(result.unwrap() > 0); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make the Bun test assert the detection contract or rename it.
As written, this passes when Bun is present but detect_zig_start_address returns None, so it only checks “does not crash” rather than Bun/Zig detection. If detection is expected for Bun, assert Some; otherwise rename the test to reflect the weaker coverage.
🤖 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 `@profile-bee/src/dwarf_unwind.rs` around lines 1038 - 1065, The Bun test in
test_detect_zig_start_address_bun currently only verifies that
detect_zig_start_address does not crash because it allows None to pass; either
change the assertion to require a successful Zig start address detection for Bun
(and validate the returned value), or rename the test to explicitly describe
that it is only a smoke test for a real Zig binary.
| .range(..=addr) | ||
| .next_back() | ||
| .map(|(_, sym)| sym) | ||
| .filter(|sym| addr < sym.code_addr + sym.code_size) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Avoid wrapping the JIT range end calculation.
Line 69 can overflow on malformed input when code_addr + code_size exceeds u64::MAX, causing a debug panic or wrapped release-mode lookup. Compare via subtraction instead.
Proposed fix
- .filter(|sym| addr < sym.code_addr + sym.code_size)
+ .filter(|sym| addr >= sym.code_addr && addr - sym.code_addr < sym.code_size)📝 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
| .filter(|sym| addr < sym.code_addr + sym.code_size) | |
| .filter(|sym| addr >= sym.code_addr && addr - sym.code_addr < sym.code_size) |
🤖 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 `@profile-bee/src/jitdump.rs` at line 69, The JIT symbol range check in the
`jitdump.rs` lookup logic can overflow when computing the end of a symbol range
with `code_addr + code_size`. Update the filter in the JIT symbol matching path
to avoid addition overflow by comparing against a subtraction-based bound
instead, using the existing `sym.code_addr`, `sym.code_size`, and `addr` values
in the same lookup function.
Comment on lines
+214
to
+217
| _ => { | ||
| // Unknown record type — skip | ||
| skip_bytes(reader, payload_size)?; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Treat truncated unknown records like truncated known records.
The parser tolerates partial known records, but a truncated unknown record makes skip_bytes return an error and can discard an otherwise usable table. Keep last_read_offset = record_start and break on UnexpectedEof.
Proposed fix
_ => {
// Unknown record type — skip
- skip_bytes(reader, payload_size)?;
+ if let Err(e) = skip_bytes(reader, payload_size) {
+ self.last_read_offset = record_start;
+ if e.kind() != io::ErrorKind::UnexpectedEof {
+ tracing::debug!("JITDump unknown record skip error: {}", e);
+ }
+ break;
+ }
}📝 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
| _ => { | |
| // Unknown record type — skip | |
| skip_bytes(reader, payload_size)?; | |
| } | |
| _ => { | |
| // Unknown record type — skip | |
| if let Err(e) = skip_bytes(reader, payload_size) { | |
| self.last_read_offset = record_start; | |
| if e.kind() != io::ErrorKind::UnexpectedEof { | |
| tracing::debug!("JITDump unknown record skip error: {}", e); | |
| } | |
| break; | |
| } | |
| } |
🤖 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 `@profile-bee/src/jitdump.rs` around lines 214 - 217, The unknown-record branch
in jitdump parsing should handle truncation the same way as the known-record
paths do. Update the record processing logic around the unknown-type fallback in
jitdump.rs so that when skip_bytes(reader, payload_size) returns UnexpectedEof,
you preserve last_read_offset = record_start and break out of the loop instead
of treating it as a hard error; keep the behavior for other I/O errors
unchanged. Locate the fix in the record-dispatch code that handles unknown
record types and the same truncation handling used by the existing known-record
parsing branches.
Comment on lines
+249
to
+251
| let remaining = payload_size - FIXED_SIZE; | ||
| let mut name_and_code = vec![0u8; remaining as usize]; | ||
| reader.read_exact(&mut name_and_code)?; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Do not allocate record-sized buffers from untrusted payload lengths.
total_size comes from the JITDump file; a malformed /tmp/jit-* file can advertise a huge payload and force multi-GB allocations here before read_exact fails. Read the NUL-terminated name/filename into a capped buffer and stream-skip the remaining payload instead.
Also applies to: 341-343
🤖 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 `@profile-bee/src/jitdump.rs` around lines 249 - 251, The JITDump record
parsing in the name/filename handling path is allocating a buffer directly from
the untrusted payload length, which can trigger huge allocations before
validation. Update the parsing logic in the relevant reader flow around the
record-name handling (including the equivalent code path at the later matching
site) to read the NUL-terminated string into a capped, fixed-size buffer and
then stream-skip any remaining bytes instead of sizing a Vec from payload_size.
Keep the fix localized to the record parsing functions that currently use
remaining and name_and_code so the allocation is no longer driven by
attacker-controlled sizes.
Comment on lines
+281
to
+293
| fn parse_code_move<R: Read>(&mut self, reader: &mut R, payload_size: u64) -> io::Result<()> { | ||
| const FIXED_SIZE: u64 = 32; | ||
| if payload_size < FIXED_SIZE { | ||
| return Err(io::Error::new( | ||
| io::ErrorKind::InvalidData, | ||
| "JIT_CODE_MOVE too small", | ||
| )); | ||
| } | ||
|
|
||
| let old_code_addr = read_u64(reader)?; | ||
| let new_code_addr = read_u64(reader)?; | ||
| let new_code_size = read_u64(reader)?; | ||
| let _code_index = read_u64(reader)?; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Parse JIT_CODE_MOVE with the full record layout.
Line 290 currently reads the leading pid/tid bytes as old_code_addr. The Linux perf jitdump definition includes pid, tid, and vma before old_code_addr, so real MOVE records will not relocate symbols correctly. (codebrowser.dev)
Proposed fix
- const FIXED_SIZE: u64 = 32;
+ const FIXED_SIZE: u64 = 48;
if payload_size < FIXED_SIZE {
return Err(io::Error::new(
io::ErrorKind::InvalidData,
"JIT_CODE_MOVE too small",
));
}
+ let _pid = read_u32(reader)?;
+ let _tid = read_u32(reader)?;
+ let _vma = read_u64(reader)?;
let old_code_addr = read_u64(reader)?;
let new_code_addr = read_u64(reader)?;
let new_code_size = read_u64(reader)?;
let _code_index = read_u64(reader)?;Also update code_move_record in the tests to emit the same 48-byte fixed payload.
📝 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
| fn parse_code_move<R: Read>(&mut self, reader: &mut R, payload_size: u64) -> io::Result<()> { | |
| const FIXED_SIZE: u64 = 32; | |
| if payload_size < FIXED_SIZE { | |
| return Err(io::Error::new( | |
| io::ErrorKind::InvalidData, | |
| "JIT_CODE_MOVE too small", | |
| )); | |
| } | |
| let old_code_addr = read_u64(reader)?; | |
| let new_code_addr = read_u64(reader)?; | |
| let new_code_size = read_u64(reader)?; | |
| let _code_index = read_u64(reader)?; | |
| fn parse_code_move<R: Read>(&mut self, reader: &mut R, payload_size: u64) -> io::Result<()> { | |
| const FIXED_SIZE: u64 = 48; | |
| if payload_size < FIXED_SIZE { | |
| return Err(io::Error::new( | |
| io::ErrorKind::InvalidData, | |
| "JIT_CODE_MOVE too small", | |
| )); | |
| } | |
| let _pid = read_u32(reader)?; | |
| let _tid = read_u32(reader)?; | |
| let _vma = read_u64(reader)?; | |
| let old_code_addr = read_u64(reader)?; | |
| let new_code_addr = read_u64(reader)?; | |
| let new_code_size = read_u64(reader)?; | |
| let _code_index = read_u64(reader)?; |
🤖 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 `@profile-bee/src/jitdump.rs` around lines 281 - 293, The JIT_CODE_MOVE parser
in parse_code_move is reading the record with the wrong fixed layout, so it
treats the leading pid/tid/vma fields as old_code_addr and mis-parses real move
records. Update parse_code_move to consume the full perf jitdump JIT_CODE_MOVE
header in the correct order before reading old_code_addr, new_code_addr,
new_code_size, and code_index, and adjust the FIXED_SIZE check accordingly. Also
update the code_move_record test helper to emit the same full 48-byte payload so
the parser and tests match the jitdump format.
Comment on lines
+388
to
+394
| pub fn register_jit_table(&mut self, tgid: u32, table: JitSymbolTable) { | ||
| tracing::info!( | ||
| "registered JITDump symbol table for pid {} ({} symbols)", | ||
| tgid, | ||
| table.len(), | ||
| ); | ||
| self.jit_tables.insert(tgid, table); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Invalidate cached unknown stacks when registering a fresh JIT table.
reload_jitdump_tables() invalidates after incremental reloads, but its fresh-load branch calls register_jit_table() after stacks may already be cached as [unknown]. Clear this PID’s symbol cache when inserting the table so later output can use the new JIT symbols.
Proposed fix
);
self.jit_tables.insert(tgid, table);
+ self.cache.invalidate_pid(tgid);
}📝 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
| pub fn register_jit_table(&mut self, tgid: u32, table: JitSymbolTable) { | |
| tracing::info!( | |
| "registered JITDump symbol table for pid {} ({} symbols)", | |
| tgid, | |
| table.len(), | |
| ); | |
| self.jit_tables.insert(tgid, table); | |
| pub fn register_jit_table(&mut self, tgid: u32, table: JitSymbolTable) { | |
| tracing::info!( | |
| "registered JITDump symbol table for pid {} ({} symbols)", | |
| tgid, | |
| table.len(), | |
| ); | |
| self.jit_tables.insert(tgid, table); | |
| self.cache.invalidate_pid(tgid); | |
| } |
🤖 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 `@profile-bee/src/trace_handler.rs` around lines 388 - 394, The
register_jit_table() path in trace_handler.rs should also invalidate any cached
stack entries for the same PID, since a fresh JIT table can arrive after
“[unknown]” stacks have already been cached. Update register_jit_table() to
clear the symbol cache for the given tgid before or when inserting into
self.jit_tables, so subsequent lookups can resolve with the new JIT symbols. Use
the existing cache and JitSymbolTable-related symbols in TraceHandler to keep
the fix localized.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Given that I'm using bunjs more often than nodejs makes sense to have this supported...
Add a JITDump binary format parser that resolves JIT-compiled JavaScript function names from runtimes like Bun (JavaScriptCore), Java HotSpot, and LuaJIT. Without this, JIT code in anonymous mmap'd pages shows as [unknown].
Implementation:
Summary by CodeRabbit
BUN_JSC_useJITDump=1when symbol support is missing.