Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 6 additions & 0 deletions ddprof-lib/src/main/cpp/profiler.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1609,6 +1609,12 @@ Error Profiler::start(Arguments &args, bool reset) {
}

args._cstack = _cstack;
// From here on, cstackMode() reflects this session's final, resolved mode.
// Anything parsed before this point (e.g. ElfParser::parseDwarfInfo() for
// the handful of libraries loaded at JVMTI Agent_OnLoad, before this
// start() call) must not trust cstackMode() yet -- see cstackResolved()'s
// declaration for why.
_cstack_resolved.store(true, std::memory_order_release);
// Prepare JVMSupport for execution
JVMSupport::initExecution(args, VM::jvmti(), VM::jni());

Expand Down
18 changes: 18 additions & 0 deletions ddprof-lib/src/main/cpp/profiler.h
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,15 @@ class alignas(alignof(SpinLock)) Profiler {
StackWalkFeatures _features;
int _safe_mode;
CStack _cstack;
// Set once, immediately after start() resolves _cstack (CSTACK_DEFAULT ->
// a concrete mode) and before any sampling begins. _cstack's constructor
// default is CSTACK_NO -- a real, user-selectable mode, not a
// distinguishable "not yet decided" sentinel -- so code that must tell
// "not yet resolved" apart from "resolved, and it's CSTACK_NO" (e.g.
// ElfParser::parseDwarfInfo() deciding whether a library parsed before
// start() might still need a DWARF table once resolution happens) needs
// this, not just cstackMode().
std::atomic<bool> _cstack_resolved;
bool _force_jmethodID;

volatile jvmtiEventMode _thread_events_state;
Expand Down Expand Up @@ -228,6 +237,7 @@ class alignas(alignof(SpinLock)) Profiler {
_start_time(0), _stop_time(0), _epoch(0), _timer_id(NULL),
_total_samples(0), _sample_seq(0), _failures(), _class_map_lock(),
_max_stack_depth(0), _features(), _safe_mode(0), _cstack(CSTACK_NO),
_cstack_resolved(false),
_force_jmethodID(true),
_thread_events_state(JVMTI_DISABLE), _libs(Libraries::instance()),
_num_context_attributes(0), _omit_stacktraces(false),
Expand Down Expand Up @@ -280,6 +290,14 @@ class alignas(alignof(SpinLock)) Profiler {
return _cstack;
}

// True once start() has resolved _cstack from a request (including
// CSTACK_DEFAULT) to the concrete mode it will use for this session. See
// the comment on _cstack_resolved for why cstackMode() alone can't tell
// "not yet decided" apart from "decided, and it's CSTACK_NO".
inline bool cstackResolved() const {
return _cstack_resolved.load(std::memory_order_acquire);
}

inline bool forceJmethodID() const {
return _force_jmethodID;
}
Expand Down
26 changes: 26 additions & 0 deletions ddprof-lib/src/main/cpp/symbols_linux.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
#include "fdtransferClient.h"
#include "log.h"
#include "os.h"
#include "profiler.h"
#include "symbols_linux.h"

// Simple address range
Expand Down Expand Up @@ -703,6 +704,31 @@ void ElfParser::parseDynamicSection() {
void ElfParser::parseDwarfInfo() {
if (!DWARF_SUPPORTED) return;

// Skip entirely when we are certain the table can never be read: it is
// consumed only by StackWalker::walkDwarf() (profiler.cpp), which runs
// only under CSTACK_DWARF. Gating on cstackMode() alone is NOT safe:
// Profiler::_cstack's constructor default is CSTACK_NO (a real,
// user-selectable mode, not a distinguishable "unresolved" sentinel), so
// a naive `cstackMode() != CSTACK_DWARF` check would also -- silently and
// incorrectly -- skip building the table for every library parsed before
// Profiler::start() has run and resolved it (the handful parsed at JVMTI
// Agent_OnLoad, vmEntry.cpp, before the later VM::VMInit()-triggered
// start() call): those libraries are parsed exactly once
// (Symbols::parseLibraries tracks _parsed_inodes) and never re-parsed, so
// getting this wrong would permanently and silently drop libjvm.so's own
// unwind info in a real CSTACK_DWARF session. cstackResolved() -- set
// once, immediately after start()'s resolution logic, before any
// sampling begins -- disambiguates "not yet decided" from "decided, and
// it wasn't DWARF". This is a real cost to skip once resolved-away: at
// 150,000 loaded classes this measured at 4.52 MiB across 22 allocations,
// unconditionally, even in the common case where the requested default
// resolves to CSTACK_VM (whenever HotSpot VMStructs are available)
// rather than CSTACK_DWARF.
if (Profiler::instance()->cstackResolved() &&
Profiler::instance()->cstackMode() != CSTACK_DWARF) {
return;
Comment on lines +727 to +729

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Rebuild skipped unwind tables before enabling DWARF

When an initial recording uses cstack=vm, fp, or no and a library is loaded during that session, this return creates its CodeCache without an unwind table, while Symbols::_parsed_inodes prevents that library from ever being parsed again. A later supported stop/start cycle with cstack=dwarf therefore walks frames from that library using the fallback descriptor instead of its SFrame/DWARF data, silently producing incorrect or truncated native stacks. Retain the unwind data or explicitly populate missing tables when transitioning to DWARF.

Useful? React with 👍 / 👎.

}

// 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) {
Expand Down
Loading