Skip to content

feat(sframe): complete SFrame V2 parser — replace stub with full implementation #471

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
jbachorik wants to merge 4 commits into
base: main
Choose a base branch
from
+957 −30
Open

feat(sframe): complete SFrame V2 parser — replace stub with full implementation #471

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9a9862f
feat(sframe): complete SFrame V2 parser with review fixes
jbachorik Jun 10, 2026
b5200a5
fix(sframe): update DwarfParser call to 6-arg constructor
jbachorik Jun 10, 2026
7742578
fix(sframe): use main's symbols_linux.cpp as base, add SFrame fast-path
jbachorik Jun 10, 2026
d850da1
fix(sframe): correct RA/FP read order, rollback on partial FDE, u32 g…
jbachorik Jun 10, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
230 changes: 213 additions & 17 deletions ddprof-lib/src/main/cpp/sframe.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
*/

#include "sframe.h"
#include "log.h"
#include <climits>
#include <cstdlib>
#include <cstring>

Expand All @@ -16,7 +18,8 @@ SFrameParser::SFrameParser(const char* name, const char* section_base,
_capacity(128),
_count(0),
_table(static_cast<FrameDesc*>(malloc(128 * sizeof(FrameDesc)))),
_linked_frame_size(-1) {}
_linked_frame_size(-1),
_oom(false) {}

SFrameParser::~SFrameParser() {
free(_table); // safe: free(nullptr) is a no-op; table() nulls _table on success
Expand All @@ -37,10 +40,15 @@ const FrameDesc& SFrameParser::detectedDefaultFrame() const {
}

FrameDesc* SFrameParser::addRecord(u32 loc, u32 cfa, int fp_off, int pc_off) {
if (!_table) return nullptr; // constructor malloc failed
if (_count >= _capacity) {
if (_capacity > (INT_MAX / 2)) return nullptr; // overflow guard
FrameDesc* resized = static_cast<FrameDesc*>(
realloc(_table, _capacity * 2 * sizeof(FrameDesc)));
if (!resized) return nullptr;
if (!resized) {
_oom = true;
return nullptr;
}
_capacity *= 2;
_table = resized;
Comment thread
jbachorik marked this conversation as resolved.
}
Expand All @@ -52,54 +60,242 @@ FrameDesc* SFrameParser::addRecord(u32 loc, u32 cfa, int fp_off, int pc_off) {
return fd;
}

bool SFrameParser::parseFDE(const SFrameHeader* /*hdr*/, const SFrameFDE* /*fde*/,
const char* /*fre_section*/, const char* /*fre_end*/) {
return true; // stub — implemented in Task 4
bool SFrameParser::parseFDE(const SFrameHeader* hdr, const SFrameFDE* fde,
const char* fre_section, const char* fre_end) {
// Determine FRE start address size
int addr_size;
switch (SFRAME_FUNC_FRE_TYPE(fde->info)) {
case SFRAME_FRE_TYPE_ADDR1: addr_size = 1; break;
case SFRAME_FRE_TYPE_ADDR2: addr_size = 2; break;
case SFRAME_FRE_TYPE_ADDR4: addr_size = 4; break;
Comment thread
jbachorik marked this conversation as resolved.
default: return false;
}

// Bounds-check fre_off before pointer arithmetic
size_t fre_section_len = static_cast<size_t>(fre_end - fre_section);
if (fde->fre_off >= fre_section_len) return false;

const char* fre_ptr = fre_section + fde->fre_off;

for (uint32_t j = 0; j < fde->fre_num; j++) {
// (a) Entry-level bounds check
if (fre_ptr >= fre_end) return false;

// (b) Read FRE start address offset (unsigned)
if (fre_ptr + addr_size > fre_end) return false;
uint32_t fre_start = 0;
if (addr_size == 1) {
fre_start = *reinterpret_cast<const uint8_t*>(fre_ptr);
} else if (addr_size == 2) {
uint16_t v; memcpy(&v, fre_ptr, 2); fre_start = v;
} else {
memcpy(&fre_start, fre_ptr, 4);
}
fre_ptr += addr_size;

// (c) Read FRE info byte
if (fre_ptr + 1 > fre_end) return false;
uint8_t fre_info = *reinterpret_cast<const uint8_t*>(fre_ptr);
fre_ptr += 1;

// (d) Determine offset encoding size
int off_size;
switch (SFRAME_FRE_OFFSET_SIZE(fre_info)) {
case SFRAME_FRE_OFFSET_1B: off_size = 1; break;
case SFRAME_FRE_OFFSET_2B: off_size = 2; break;
case SFRAME_FRE_OFFSET_4B: off_size = 4; break;
default: return false;
Comment thread
jbachorik marked this conversation as resolved.
}

// Decide what to read from the stream (governed by FRE info byte alone)
bool fp_tracked = SFRAME_FRE_FP_TRACKED(fre_info);
bool ra_in_fre = SFRAME_FRE_RA_TRACKED(fre_info);

// (e) Bounds check all remaining reads for this FRE
int n_offsets = 1 + (fp_tracked ? 1 : 0) + (ra_in_fre ? 1 : 0);
if (fre_ptr + n_offsets * off_size > fre_end) return false;

// (f) Read CFA offset (signed)
int32_t cfa_offset = 0;
if (off_size == 1) {
cfa_offset = *reinterpret_cast<const int8_t*>(fre_ptr);
} else if (off_size == 2) {
int16_t v; memcpy(&v, fre_ptr, 2); cfa_offset = v;
} else {
memcpy(&cfa_offset, fre_ptr, 4);
}
fre_ptr += off_size;

// Guard: CFA offset must fit in the 24-bit field packed into FrameDesc::cfa
if (cfa_offset < -8388608 || cfa_offset > 8388607) return false;

@rkennke rkennke Jun 12, 2026

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.

Any per-FRE failure like the one here, and also in some other places makes parseFDE return false, which parse() treats as a corrupt FDE and rolls back all FREs already added for that function. DwarfParser::addRecord instead drops just the offending record and keeps going. So one out-of-range FRE here costs unwinding for the entire function. Defensible as a conservative choice, but consider continue-ing past the single FRE instead of failing the FDE, to match DWARF's coverage behavior. At minimum, worth a comment noting the intentional difference.


// (g) Read RA offset if tracked
int32_t ra_offset = 0;
if (ra_in_fre) {
if (off_size == 1) {
ra_offset = *reinterpret_cast<const int8_t*>(fre_ptr);
} else if (off_size == 2) {
int16_t v; memcpy(&v, fre_ptr, 2); ra_offset = v;
} else {
memcpy(&ra_offset, fre_ptr, 4);
}
fre_ptr += off_size;
}

// (h) Read FP offset if tracked
int32_t fp_offset = 0;
if (fp_tracked) {
if (off_size == 1) {
fp_offset = *reinterpret_cast<const int8_t*>(fre_ptr);
} else if (off_size == 2) {
int16_t v; memcpy(&v, fre_ptr, 2); fp_offset = v;
} else {
memcpy(&fp_offset, fre_ptr, 4);
}
fre_ptr += off_size;
}

// (i) Translate to FrameDesc fields
// Use unsigned arithmetic to avoid implementation-defined signed cast for large offsets
u32 loc = _section_offset + static_cast<u32>(fde->start_addr) + fre_start;

@rkennke rkennke Jun 12, 2026

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.

This code treats fde->start_addr as section-relative.
But SFrame V2 has a header flag (SFRAME_F_FDE_FUNC_START_PCREL = 0x4, a V2 erratum) that, when set, makes start_addr relative to the location of the field itself, not the section start.
If a producer sets this flag, every loc is computed wrong (off by the distance from the FDE field to the section start, which differs per FDE) → the unwind table is effectively scrambled → silent bad unwinds / lookup misses rather than a clean DWARF fallback. The Fedora-43 integration test presumably passed because those binaries currently use section-relative encoding, but this silently breaks the day the toolchain default flips.

Minimum fix: detect the flag and bail to DWARF:

  if (hdr->flags & SFRAME_F_FDE_FUNC_START_PCREL) {                                                                                              
      Log::warn("SFrame PCREL func-start encoding unsupported in %s; falling back to DWARF", _name);                                             
      return false;                                                                                                                              
  }

Or implement it.


u32 cfa_reg = SFRAME_FRE_BASE_REG(fre_info) ? static_cast<u32>(DW_REG_FP)
: static_cast<u32>(DW_REG_SP);
u32 cfa = (static_cast<u32>(cfa_offset) << 8) | cfa_reg;

Comment thread
jbachorik marked this conversation as resolved.
// aarch64 GCC vs Clang detection: first FP-based entry with cfa_offset > 0
if (_linked_frame_size < 0 && cfa_reg == static_cast<u32>(DW_REG_FP) && cfa_offset > 0) {
_linked_frame_size = cfa_offset;
}

// Determine fp_off: per-FRE value takes priority; fall back to header fixed offset
int fp_off;
if (fp_tracked) {
fp_off = static_cast<int>(fp_offset);
} else if (hdr->cfa_fixed_fp_offset != 0) {
fp_off = static_cast<int>(hdr->cfa_fixed_fp_offset);
} else {
fp_off = DW_SAME_FP;
}

// Header fixed RA offset takes priority over per-FRE value
int pc_off;
if (hdr->cfa_fixed_ra_offset != 0) {
pc_off = static_cast<int>(hdr->cfa_fixed_ra_offset);
} else if (ra_in_fre) {
pc_off = static_cast<int>(ra_offset);
} else {
pc_off = DW_LINK_REGISTER;
}

// (j) Append record
if (!addRecord(loc, cfa, fp_off, pc_off)) return false;
}

return true;
}

bool SFrameParser::parse() {
// 1. Size check
if (_section_size < sizeof(SFrameHeader)) return false;
if (_section_size < sizeof(SFrameHeader)) {
Log::warn("SFrame section too small in %s", _name);
return false;
}

const SFrameHeader* hdr = reinterpret_cast<const SFrameHeader*>(_section_base);

// 2-4. Header field validation
if (hdr->magic != SFRAME_MAGIC) return false;
if (hdr->version != SFRAME_VERSION_2) return false;
if (hdr->magic != SFRAME_MAGIC) {
Log::warn("SFrame bad magic in %s", _name);
return false;
}
if (hdr->version != SFRAME_VERSION_2) {
Log::warn("SFrame unsupported version %d in %s", (int)hdr->version, _name);
return false;
}

#if defined(__x86_64__)
if (hdr->abi_arch != SFRAME_ABI_AMD64_ENDIAN_LITTLE) return false;
if (hdr->abi_arch != SFRAME_ABI_AMD64_ENDIAN_LITTLE) {
Log::warn("SFrame wrong ABI 0x%x in %s", (int)hdr->abi_arch, _name);
return false;
}
#elif defined(__aarch64__)
if (hdr->abi_arch != SFRAME_ABI_AARCH64_ENDIAN_LITTLE) return false;
if (hdr->abi_arch != SFRAME_ABI_AARCH64_ENDIAN_LITTLE) {
Log::warn("SFrame wrong ABI 0x%x in %s", (int)hdr->abi_arch, _name);
return false;
}
#else
return false;
#endif

// 5. Bounds check auxhdr_len before computing data_start
if (sizeof(SFrameHeader) + hdr->auxhdr_len > _section_size) return false;
if (sizeof(SFrameHeader) + hdr->auxhdr_len > _section_size) {
Log::warn("SFrame auxhdr_len overflows section in %s", _name);
return false;
}

const char* data_start = _section_base + sizeof(SFrameHeader) + hdr->auxhdr_len;
const char* section_end = _section_base + _section_size;

// Bounds-check fdeoff, freoff, and fre_len before pointer arithmetic
size_t data_len = static_cast<size_t>(section_end - data_start);
if (hdr->fdeoff > data_len) {
Log::warn("SFrame fdeoff out of bounds in %s", _name);
return false;
}
if (hdr->freoff > data_len) {
Log::warn("SFrame freoff out of bounds in %s", _name);
return false;
}
if (hdr->fre_len > data_len - hdr->freoff) {
Log::warn("SFrame fre_len overflows section in %s", _name);
return false;
}

const SFrameFDE* fde_array = reinterpret_cast<const SFrameFDE*>(data_start + hdr->fdeoff);
const char* fre_section = data_start + hdr->freoff;
const char* fre_end = fre_section + hdr->fre_len;

// 6-7. Bounds checks for FDE array and FRE section
if (reinterpret_cast<const char*>(fde_array) +
(size_t)hdr->num_fdes * sizeof(SFrameFDE) > section_end) return false;
if (fre_end > section_end) return false;
(size_t)hdr->num_fdes * sizeof(SFrameFDE) > section_end) {
Log::warn("SFrame FDE array overflows section in %s", _name);
return false;
}
if (fre_end > section_end) {
Log::warn("SFrame FRE section overflows in %s", _name);
return false;
}

// 8. FDE array / FRE section overlap check
if (hdr->num_fdes > 0 && hdr->fre_len > 0) {
const char* fde_start_ptr = reinterpret_cast<const char*>(fde_array);
const char* fde_end_ptr = fde_start_ptr + (size_t)hdr->num_fdes * sizeof(SFrameFDE);
if (fde_end_ptr > fre_section && fde_start_ptr < fre_end) {
Log::warn("SFrame FDE array overlaps FRE section in %s", _name);
Comment thread
jbachorik marked this conversation as resolved.
return false;
}
}

// 8. Iterate FDEs
// 9. Iterate FDEs
for (uint32_t i = 0; i < hdr->num_fdes; i++) {
const SFrameFDE* fde = &fde_array[i];
if (SFRAME_FUNC_FDE_TYPE(fde->info) != 0) continue; // skip PCMASK
if (fde->fre_num == 0) continue; // empty FDE
parseFDE(hdr, fde, fre_section, fre_end); // ignore return; skip corrupt FDE
int saved_count = _count;
int saved_linked_frame_size = _linked_frame_size;
if (!parseFDE(hdr, fde, fre_section, fre_end)) {
if (_oom) return false; // OOM: partial table is not safe to use
// Corrupt FDE: roll back any partially-added records.
_count = saved_count;
_linked_frame_size = saved_linked_frame_size;
}
Comment thread
jbachorik marked this conversation as resolved.
}

// 9. Sort
qsort(_table, _count, sizeof(FrameDesc), FrameDesc::comparator);
// 10. Sort (skip if the section header guarantees sorted order)
if (_count > 0 && !(hdr->flags & SFRAME_F_FDE_SORTED)) {

@rkennke rkennke Jun 12, 2026

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.

findFrameDesc binary-searches by loc, so the flattened table must be globally sorted. That holds if FDEs are sorted by start address and FREs within each FDE are ascending and ranges don't overlap. A malformed file that sets the SORTED flag without honoring all three yields an unsorted table → silent lookup misses (not memory-unsafe). DWARF always sorts. Low severity, but a defensive note — or just always sorting, since qsort on an already-sorted array is cheap — would remove the producer-trust dependency.

qsort(_table, _count, sizeof(FrameDesc), FrameDesc::comparator);
}

return _count > 0;
}
5 changes: 3 additions & 2 deletions ddprof-lib/src/main/cpp/sframe.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ const int SFRAME_FRE_TYPE_ADDR1 = 0;
const int SFRAME_FRE_TYPE_ADDR2 = 1;
const int SFRAME_FRE_TYPE_ADDR4 = 2;

struct __attribute__((packed)) SFrameHeader { // 28 bytes
struct __attribute__((packed, may_alias)) SFrameHeader { // 28 bytes
uint16_t magic;
uint8_t version;
uint8_t flags;
Expand All @@ -57,7 +57,7 @@ struct __attribute__((packed)) SFrameHeader { // 28 bytes
uint32_t freoff;
};

struct __attribute__((packed)) SFrameFDE { // 20 bytes
struct __attribute__((packed, may_alias)) SFrameFDE { // 20 bytes
int32_t start_addr; // signed, relative to .sframe section start (V2)
uint32_t func_size;
uint32_t fre_off; // byte offset into FRE sub-section
Expand All @@ -78,6 +78,7 @@ class SFrameParser {
int _count;
FrameDesc* _table;
int _linked_frame_size; // for aarch64 GCC vs Clang detection; -1 = undetected
bool _oom; // set by addRecord on realloc failure

bool parseFDE(const SFrameHeader* hdr, const SFrameFDE* fde,
const char* fre_section, const char* fre_end);
Expand Down
22 changes: 22 additions & 0 deletions ddprof-lib/src/main/cpp/symbols_linux.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
#include "common.h"
#include "symbols.h"
#include "dwarf.h"
#include "sframe.h"
#include "fdtransferClient.h"
#include "log.h"
#include "os.h"
Expand Down Expand Up @@ -702,6 +703,27 @@ void ElfParser::parseDynamicSection() {
void ElfParser::parseDwarfInfo() {
if (!DWARF_SUPPORTED) return;

// Try SFrame first (simpler format, faster parsing, no opcode interpretation).
ElfProgramHeader* sframe_phdr = findProgramHeader(PT_GNU_SFRAME);
if (sframe_phdr != NULL && sframe_phdr->p_vaddr != 0) {
const char* section_base = at(sframe_phdr);
uintptr_t section_offset_full = static_cast<uintptr_t>(section_base - _base);
if (section_offset_full > static_cast<uintptr_t>(UINT32_MAX)) {
Log::warn("SFrame section offset too large for u32 in %s; falling back to DWARF", _cc->name());
goto dwarf_fallback;

@rkennke rkennke Jun 12, 2026

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.

Ewww. The goto seems relatively easily avoidable, turn around the guard and let the failure-path fall-through.

}
u32 section_offset = static_cast<u32>(section_offset_full);
SFrameParser sframe(_cc->name(), section_base,
static_cast<size_t>(sframe_phdr->p_filesz), section_offset);
if (sframe.parse()) {
_cc->setDwarfTable(sframe.table(), sframe.count(),
sframe.detectedDefaultFrame());
return;
}
// SFrame parse failed; fall through to DWARF
}

dwarf_fallback:
ElfProgramHeader* eh_frame_hdr = findProgramHeader(PT_GNU_EH_FRAME);
if (eh_frame_hdr != NULL) {
if (eh_frame_hdr->p_vaddr != 0) {
Expand Down
Loading
Loading