Skip to content

fix: unwinding for binaries compiled without eh_frame_hdr#393

Merged
not-matthias merged 3 commits into
mainfrom
cod-2769-runner-fails-to-extract-symbols-unwind-data-for-valgrind
Jun 11, 2026
Merged

fix: unwinding for binaries compiled without eh_frame_hdr#393
not-matthias merged 3 commits into
mainfrom
cod-2769-runner-fails-to-extract-symbols-unwind-data-for-valgrind

Conversation

@not-matthias

Copy link
Copy Markdown
Member

No description provided.

@not-matthias not-matthias force-pushed the cod-2769-runner-fails-to-extract-symbols-unwind-data-for-valgrind branch from e43227c to 47c6606 Compare June 5, 2026 09:30
@codspeed-hq

codspeed-hq Bot commented Jun 5, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

⚠️ Unknown Walltime execution environment detected

Using the Walltime instrument on standard Hosted Runners will lead to inconsistent data.

For the most accurate results, we recommend using CodSpeed Macro Runners: bare-metal machines fine-tuned for performance measurement consistency.

✅ 7 untouched benchmarks


Comparing cod-2769-runner-fails-to-extract-symbols-unwind-data-for-valgrind (41f4db9) with main (7e86f11)

Open in CodSpeed

@not-matthias not-matthias force-pushed the cod-2769-runner-fails-to-extract-symbols-unwind-data-for-valgrind branch from 47c6606 to 3987253 Compare June 5, 2026 09:47
@not-matthias not-matthias marked this pull request as ready for review June 5, 2026 09:47
@greptile-apps

greptile-apps Bot commented Jun 5, 2026

Copy link
Copy Markdown

Greptile Summary

This PR introduces UnwindDataV4, which makes eh_frame_hdr and eh_frame_hdr_svma optional fields, fixing stack unwinding for binaries compiled without ld --eh-frame-hdr (e.g. Valgrind's statically-linked tools). The existing V3 binary artifacts are transparently upgraded via a From<V3> impl that wraps the previously-required header data in Some.

  • New data version (UnwindDataV4): eh_frame_hdr/eh_frame_hdr_svma are now Option; unwind_data_from_elf no longer bails when the .eh_frame_hdr section is absent, and serialized V3 files are automatically upgraded on read.
  • All call sites updated: jit_dump.rs wraps the always-present JIT header in Some, all five insta snapshot files are regenerated, and a new binary test artifact (unwind_data_v4.bin) plus a Valgrind LFS binary are added for integration tests.
  • Backward compatibility preserved: V1 and V2 remain rejected by UnwindData::parse, and UnwindDataV2::parse now also rejects V4, with roundtrip tests covering every combination.

Confidence Score: 5/5

Safe to merge. The change is additive: a required binary field becomes optional, V3 artifacts are automatically upgraded on read, and all existing call sites are updated.

The version migration path is correct and tested end-to-end. V3 artifacts are upgraded via From wrapping fields in Some, all snapshot tests are regenerated, and a new integration test against a real Valgrind binary validates the None path. No logic breakage is apparent.

No files require special attention.

Important Files Changed

Filename Overview
crates/runner-shared/src/unwind_data.rs Adds UnwindDataV4 with optional eh_frame_hdr/eh_frame_hdr_svma, bumps the UnwindData type alias to V4, updates parse/save_to, and adds comprehensive roundtrip tests.
src/executor/wall_time/profiler/perf/unwind_data.rs unwind_data_from_elf now silently accepts a missing .eh_frame_hdr by removing the .context() bail-out; adds a valgrind test that asserts None for the missing header.
src/executor/wall_time/profiler/perf/jit_dump.rs JIT path wraps eh_frame_hdr and eh_frame_hdr_svma in Some(...) to match the new optional struct fields.
crates/runner-shared/testdata/unwind_data_v4.bin New binary test artifact for V4 deserialization round-trip tests (generated by the #[ignore] generator test).
testdata/perf_map/valgrind Git-LFS pointer to a Valgrind statically-linked binary used by the new integration test for binaries without .eh_frame_hdr.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Read .unwind_data bytes] --> B[bincode::deserialize as UnwindDataCompat]
    B --> C{variant?}
    C -- V1 --> D[bail: cannot parse V1 as V4]
    C -- V2 --> E[bail: cannot parse V2 as V4]
    C -- V3 --> F[From<V3> for V4: wraps hdr fields in Some]
    C -- V4 --> G[return as-is]
    F --> G
    G --> H{eh_frame_hdr?}
    H -- Some --> I[use hdr as lookup index into .eh_frame]
    H -- None --> J[rebuild index directly from .eh_frame - e.g. Valgrind statically-linked tools]
Loading

Reviews (2): Last reviewed commit: "docs(runner-shared): add unwind data ver..." | Re-trigger Greptile

Comment thread .github/workflows/ci.yml

@GuillaumeLagrange GuillaumeLagrange left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

olgtm

Comment thread crates/runner-shared/src/unwind_data.rs
Add UnwindDataV4 where eh_frame_hdr and eh_frame_hdr_svma are optional.
The hdr is only a binary-search index into .eh_frame: binaries linked
without `ld --eh-frame-hdr` (e.g. Valgrind's statically-linked tools)
don't carry it, but can still be unwound since the parser rebuilds the
index from .eh_frame.

V3 data is transparently upgraded to V4 on parse, following the
existing V1->V2->V3 compat pattern. This must land in perf-parser
before runners start emitting V4.
unwind_data_from_elf bailed when .eh_frame_hdr was absent, leaving
modules like Valgrind's statically-linked tools without any unwind
data. Samples passing through their code could not be unwound,
producing broken stacks in the flamegraph.

Only require .eh_frame and emit the hdr fields as None when the
section is missing.
@not-matthias not-matthias force-pushed the cod-2769-runner-fails-to-extract-symbols-unwind-data-for-valgrind branch from 3987253 to 41f4db9 Compare June 11, 2026 10:02
@not-matthias not-matthias merged commit 41f4db9 into main Jun 11, 2026
21 checks passed
@not-matthias not-matthias deleted the cod-2769-runner-fails-to-extract-symbols-unwind-data-for-valgrind branch June 11, 2026 10:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants