Conversation
CI Test ResultsRun: #27331660222 | Commit:
Status Overview
Legend: ✅ passed | ❌ failed | ⚪ skipped | 🚫 cancelled Summary: Total: 32 | Passed: 32 | Failed: 0 Updated: 2026-06-11 07:58:02 UTC |
jbachorik
changed the title
This comment has been minimized.
This comment has been minimized.
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 64163413cd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
This PR completes the Linux/ELF SFrame V2 parsing path and wires it back into library load-time unwind table construction, preferring .sframe (via PT_GNU_SFRAME) before falling back to the existing .eh_frame DWARF parser.
Changes:
- Implement full SFrame V2 FDE/FRE parsing in
SFrameParserand add additional validation/hardening. - Restore the “SFrame-first” fast path in
ElfParser::parseDwarfInfo()on Linux. - Add extensive unit test coverage for SFrame parsing edge cases and validation.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
ddprof-lib/src/test/cpp/sframe_ut.cpp |
Adds broad SFrame parser unit tests (header/FDE/FRE validation, bounds checks, sorting, arch behaviors). |
ddprof-lib/src/main/cpp/symbols_linux.cpp |
Tries PT_GNU_SFRAME first and uses SFrameParser to populate CodeCache unwind tables before DWARF fallback. |
ddprof-lib/src/main/cpp/sframe.h |
Adds may_alias packed structs and OOM tracking state to SFrameParser. |
ddprof-lib/src/main/cpp/sframe.cpp |
Replaces the parseFDE stub with a full implementation and expands validation/sorting logic. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 07dfb1908c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Replaces the stub parseFDE with the full implementation and restores the SFrame-before-DWARF integration in parseDwarfInfo. Includes fixes from sphinx review: OOM handling, fre_off bounds check, cfa_fixed_fp_offset, unsigned loc arithmetic, CFA overflow guard, may_alias annotation, qsort flag guard, and expanded tests.
…uard
- parseFDE: read RA offset before FP offset per SFrame V2 spec (CFA, RA, FP)
- parse: save/restore _count and _linked_frame_size on non-OOM parseFDE failure
- parseDwarfInfo: guard section_offset_full > UINT32_MAX before u32 cast
- sframe_ut: fix buildFRE_{1B,2B,4B} wire order (RA before FP); add tests
for both-tracked offset assignment, partial-FDE rollback (count and
_linked_frame_size), and section-offset overflow guard documentation
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
What does this PR do?:
Completes the SFrame V2 parser that was (mistakenly) partially landed in #491 (stub
parseFDEreturningtrue, integration removed fromparseDwarfInfo). This PR replaces the stub with the full implementation and restores the SFrame-before-DWARF fast path.parseDwarfInfo()now checks for a.sframesection first viaPT_GNU_SFRAME. If present and valid, it parses the SFrame data directly into the existingFrameDesc*table and returns. If absent or invalid, it falls back to the existing.eh_frameDWARF path. Walk-time code (StackWalker::walkDwarf(),CodeCache::findFrameDesc()) is completely unchanged.Motivation:
SFrame is a simpler ELF section format that directly encodes CFA/FP/RA offsets in flat, binary-searchable tables without requiring a DWARF CFI bytecode interpreter. On modern Linux distros (Fedora 43+, glibc 2.42+, binutils 2.40+), libraries ship
.sframesections alongside.eh_frame. Parsing SFrame is faster than interpreting DWARF opcodes at library load time.SFrame V2 reference material:
func_infobyte (bit 0 = FDE type, bits 1–2 = FRE addr size), FREinfobyte (bit 0 = CFA base, bits 1–2 = offset width, bit 3 = RA tracked, bit 4 = FP tracked), and per-FRE offset stream order (CFA, then RA if tracked, then FP if tracked).include/sframe.h— authoritative C header with all magic constants, struct definitions, and accessor macros.libsframesource — reference reader/writer implementation..eh_frame.Additional Notes:
PT_GNU_SFRAMEis never found on Mach-O).SFrameParseris self-contained (sframe.h/sframe.cpp), depends only ondwarf.h.addRecord;fde->fre_offpre-check before pointer arithmetic.may_aliason packed structs;qsortskipped whenSFRAME_F_FDE_SORTEDis set.section_offset_full > UINT32_MAXguard beforeu32cast inparseDwarfInfo; falls back to DWARF with a warning._countand_linked_frame_sizeare restored whenparseFDEfails mid-FDE for non-OOM reasons, preventing corrupt partial entries from surviving in the unwind table.How to test the change?:
sframe_ut.cppcovering: header validation, FDE loop (empty/PCMASK/empty-FRE skip), FRE parsing (SP/FP-based CFA, fixed/per-FRE RA, leaf functions), offset encodings (1B, 2B, 4B), multiple FDEs with sort verification (including reverse-order input), address translation with non-zerosection_offset, FRE bounds overrun recovery, destructor memory safety, aarch64 default frame detection,auxhdr_lenpositive path, reserved encoding rejection, individual bounds guard coverage, both-RA-and-FP-tracked offset assignment, partial-FDE rollback (count and_linked_frame_size)../gradlew :ddprof-lib:gtestDebug— all SFrame tests pass (Linux); existing DWARF tests unaffected../gradlew :ddprof-lib:buildRelease— builds cleanly on both Linux and macOS.cstack=dwarf, verify native stacks are collected via the SFrame fast path.For Datadog employees:
credentials of any kind, I've requested a review from
@DataDog/security-design-and-guidance.Unsure? Have a question? Request a review!