From 29fd72840c2c6cc1fc586a227b982dc2070eca65 Mon Sep 17 00:00:00 2001 From: Jaroslav Bachorik Date: Mon, 10 Mar 2025 18:08:29 +0100 Subject: [PATCH 01/11] Use heuristic to get frame layout for aarch64 WIP --- ddprof-lib/src/main/cpp/codeCache.h | 3 + ddprof-lib/src/main/cpp/dwarf.cpp | 14 ++- ddprof-lib/src/main/cpp/dwarf.h | 14 +-- ddprof-lib/src/main/cpp/stackWalker.cpp | 17 +++- ddprof-lib/src/main/cpp/symbols_linux.cpp | 105 ++++++++++++++++++---- ddprof-lib/src/main/cpp/symbols_linux.h | 4 + 6 files changed, 125 insertions(+), 32 deletions(-) diff --git a/ddprof-lib/src/main/cpp/codeCache.h b/ddprof-lib/src/main/cpp/codeCache.h index 2f4d4d196..c8e551306 100644 --- a/ddprof-lib/src/main/cpp/codeCache.h +++ b/ddprof-lib/src/main/cpp/codeCache.h @@ -177,6 +177,9 @@ class CodeCache { } int count() { return _count; } + CodeBlob* blob(int idx) { + return &_blobs[idx]; + } }; class CodeCacheArray { diff --git a/ddprof-lib/src/main/cpp/dwarf.cpp b/ddprof-lib/src/main/cpp/dwarf.cpp index 8ef79b833..693915335 100644 --- a/ddprof-lib/src/main/cpp/dwarf.cpp +++ b/ddprof-lib/src/main/cpp/dwarf.cpp @@ -89,6 +89,7 @@ FrameDesc FrameDesc::empty_frame = {0, DW_REG_SP | EMPTY_FRAME_SIZE << 8, FrameDesc FrameDesc::default_frame = {0, DW_REG_FP | LINKED_FRAME_SIZE << 8, -LINKED_FRAME_SIZE, -LINKED_FRAME_SIZE + DW_STACK_SLOT}; +FrameDesc FrameDesc::default_clang_frame = {0, DW_REG_FP | LINKED_FRAME_CLANG_SIZE << 8, -LINKED_FRAME_CLANG_SIZE, -LINKED_FRAME_CLANG_SIZE + DW_STACK_SLOT}; DwarfParser::DwarfParser(const char *name, const char *image_base, const char *eh_frame_hdr) { @@ -398,12 +399,17 @@ int DwarfParser::parseExpression() { void DwarfParser::addRecord(u32 loc, u32 cfa_reg, int cfa_off, int fp_off, int pc_off) { - // sanity asserts for the values fitting into 16bit - assert(cfa_reg <= 0xffffffff); - assert(static_cast(cfa_off) <= 0xffffffff); + // Sanity asserts to be able to pack those two values into one u32 vq + // Assert that the cfa_reg fits in 8 bits (0 to 255) + assert(cfa_reg <= 0xFF); + + // Assert that the cfa_off fits in a 24-bit signed range. + // Signed 24-bit integer range: -2^23 (-8,388,608) to 2^23 - 1 (8,388,607) + assert(cfa_off >= -8388608 && cfa_off <= 8388607); // cfa_reg and cfa_off can be encoded to a single 32 bit value, considering the existing and supported systems - u32 cfa = static_cast(cfa_off) << 16 | static_cast(cfa_reg); + u32 cfa = static_cast(cfa_off) << 8 | static_cast(cfa_reg & 0xff); + if (_prev == NULL || (_prev->loc == loc && --_count >= 0) || _prev->cfa != cfa || _prev->fp_off != fp_off || _prev->pc_off != pc_off) { _prev = addRecordRaw(loc, cfa, fp_off, pc_off); diff --git a/ddprof-lib/src/main/cpp/dwarf.h b/ddprof-lib/src/main/cpp/dwarf.h index 70d73ebdf..66c9bbf11 100644 --- a/ddprof-lib/src/main/cpp/dwarf.h +++ b/ddprof-lib/src/main/cpp/dwarf.h @@ -36,6 +36,7 @@ const int DW_REG_SP = 7; const int DW_REG_PC = 16; const int EMPTY_FRAME_SIZE = DW_STACK_SLOT; const int LINKED_FRAME_SIZE = 2 * DW_STACK_SLOT; +const int LINKED_FRAME_CLANG_SIZE = LINKED_FRAME_SIZE; #elif defined(__i386__) @@ -46,6 +47,7 @@ const int DW_REG_SP = 4; const int DW_REG_PC = 8; const int EMPTY_FRAME_SIZE = DW_STACK_SLOT; const int LINKED_FRAME_SIZE = 2 * DW_STACK_SLOT; +const int LINKED_FRAME_CLANG_SIZE = LINKED_FRAME_SIZE; #elif defined(__aarch64__) @@ -55,13 +57,9 @@ const int DW_REG_FP = 29; const int DW_REG_SP = 31; const int DW_REG_PC = 30; const int EMPTY_FRAME_SIZE = 0; - -// aarch64 function prologue looks like this (if frame pointer is used): -// stp x29, x30, [sp, -16]! // Save FP (x29) and LR (x30) -// mov x29, sp // Set FP to SP -// --- -// LINKED_FRAME_SIZE should be 16 -const int LINKED_FRAME_SIZE = 2 * DW_STACK_SLOT; +const int LINKED_FRAME_SIZE = 0; +// clang compiler uses different frame layout than GCC +const int LINKED_FRAME_CLANG_SIZE = 2 * DW_STACK_SLOT; #else @@ -72,6 +70,7 @@ const int DW_REG_SP = 1; const int DW_REG_PC = 2; const int EMPTY_FRAME_SIZE = 0; const int LINKED_FRAME_SIZE = 0; +const int LINKED_FRAME_CLANG_SIZE = LINKED_FRAME_SIZE; #endif @@ -83,6 +82,7 @@ struct FrameDesc { static FrameDesc empty_frame; static FrameDesc default_frame; + static FrameDesc default_clang_frame; static int comparator(const void *p1, const void *p2) { FrameDesc *fd1 = (FrameDesc *)p1; diff --git a/ddprof-lib/src/main/cpp/stackWalker.cpp b/ddprof-lib/src/main/cpp/stackWalker.cpp index d614da28d..7786391fc 100644 --- a/ddprof-lib/src/main/cpp/stackWalker.cpp +++ b/ddprof-lib/src/main/cpp/stackWalker.cpp @@ -172,7 +172,7 @@ int StackWalker::walkDwarf(void *ucontext, const void **callchain, cc != NULL ? cc->findFrameDesc(pc) : &FrameDesc::default_frame; u8 cfa_reg = (u8)f->cfa; - int cfa_off = f->cfa >> 16; // cfa is encoded in the upper 16 bits of the CFA value + int cfa_off = f->cfa >> 8; // cfa is encoded in the upper 8 bits of the CFA value if (cfa_reg == DW_REG_SP) { sp = sp + cfa_off; @@ -262,6 +262,9 @@ int StackWalker::walkVM(void* ucontext, ASGCT_CallFrame* frames, int max_depth, NMethod *nm = CodeHeap::findNMethod(pc); if (nm == NULL) { fillFrame(frames[depth++], BCI_ERROR, "unknown_nmethod"); + // we are somewhere in JVM code heap and have no idea what is this frame + // might be good to bail out since it would be a challenge to unwind properly? + break; } else if (nm->isNMethod()) { int level = nm->level(); FrameTypeId type = detail != VM_BASIC && @@ -399,8 +402,10 @@ int StackWalker::walkVM(void* ucontext, ASGCT_CallFrame* frames, int max_depth, } const char *name = cc == NULL ? NULL : cc->binarySearch(pc); - if (detail != VM_BASIC) { - fillFrame(frames[depth++], BCI_NATIVE_FRAME, name); + fillFrame(frames[depth++], BCI_NATIVE_FRAME, name); + // _start should be the process entry point; any attempt to unwind from there will fail + if (name && !strcmp("_start", name)) { + break; } } @@ -412,11 +417,15 @@ int StackWalker::walkVM(void* ucontext, ASGCT_CallFrame* frames, int max_depth, if (cc == NULL || !cc->contains(pc)) { cc = libraries->findLibraryByAddress(pc); } + // It may happen that the current PC can not be resolved as belonging to any known library. + // In that case we just use the default_frame which is a bit of gamble on aarch64 due to + // no standard in the function epilogue - but the bet is that the GCC compiled code is + // seen more often than the clang one. FrameDesc *f = cc != NULL ? cc->findFrameDesc(pc) : &FrameDesc::default_frame; u8 cfa_reg = (u8)f->cfa; - int cfa_off = f->cfa >> 16; + int cfa_off = f->cfa >> 8; if (cfa_reg == DW_REG_SP) { sp = sp + cfa_off; } else if (cfa_reg == DW_REG_FP) { diff --git a/ddprof-lib/src/main/cpp/symbols_linux.cpp b/ddprof-lib/src/main/cpp/symbols_linux.cpp index 3cc711606..1171b4a35 100644 --- a/ddprof-lib/src/main/cpp/symbols_linux.cpp +++ b/ddprof-lib/src/main/cpp/symbols_linux.cpp @@ -30,17 +30,36 @@ #include ElfSection *ElfParser::findSection(uint32_t type, const char *name) { - const char *strtab = at(section(_header->e_shstrndx)); - - for (int i = 0; i < _header->e_shnum; i++) { - ElfSection *section = this->section(i); - if (section->sh_type == type && section->sh_name != 0) { - if (strcmp(strtab + section->sh_name, name) == 0) { - return section; + if (_header == NULL) { + return NULL; + } + if (_header->e_shoff + (_header->e_shnum * sizeof(Elf64_Shdr)) > _length) { + return NULL; + } + int idx = _header->e_shstrndx; + ElfSection* e_section = section(idx); + if (e_section) { + if (e_section->sh_offset >= _length) { + return NULL; + } + if (e_section->sh_offset + e_section->sh_size > _length) { + return NULL; + } + if (e_section->sh_offset < _header->e_ehsize) { + return NULL; + } + const char *strtab = at(e_section); + if (strtab) { + for (int i = 0; i < _header->e_shnum; i++) { + ElfSection *section = this->section(i); + if (section->sh_type == type && section->sh_name != 0) { + if (strcmp(strtab + section->sh_name, name) == 0) { + return section; + } + } } } } - return NULL; } @@ -91,12 +110,14 @@ bool ElfParser::parseFile(CodeCache *cc, const char *base, void ElfParser::parseProgramHeaders(CodeCache *cc, const char *base, const char *end, bool relocate_dyn) { - ElfParser elf(cc, base, base, NULL, 0, relocate_dyn); + ElfParser elf(cc, base, base, NULL, (size_t)(end - base), relocate_dyn); if (elf.validHeader() && base + elf._header->e_phoff < end) { cc->setTextBase(base); elf.calcVirtualLoadAddress(); elf.parseDynamicSection(); elf.parseDwarfInfo(); + } else { + Log::warn("Invalid ELF header for %s: %p-%p", cc->name(), base, end); } } @@ -216,20 +237,70 @@ void ElfParser::parseDynamicSection() { } void ElfParser::parseDwarfInfo() { - if (!DWARF_SUPPORTED) - return; + if (!DWARF_SUPPORTED) return; - ElfProgramHeader *eh_frame_hdr = findProgramHeader(PT_GNU_EH_FRAME); + ElfProgramHeader* eh_frame_hdr = findProgramHeader(PT_GNU_EH_FRAME); if (eh_frame_hdr != NULL) { if (eh_frame_hdr->p_vaddr != 0) { + // found valid eh_frame_hdr + TEST_LOG("Found eh_frame_hdr for %s: %p", _cc->name(), at(eh_frame_hdr)); DwarfParser dwarf(_cc->name(), _base, at(eh_frame_hdr)); - _cc->setDwarfTable(dwarf.table(), dwarf.count()); - } else if (strcmp(_cc->name(), "[vdso]") == 0) { - FrameDesc *table = (FrameDesc *)malloc(sizeof(FrameDesc)); - *table = FrameDesc::empty_frame; - _cc->setDwarfTable(table, 1); + if (dwarf.count() > 0 && strcmp(_cc->name(), "[vdso]") != 0) { + TEST_LOG("Setting dwarf table for %s: %p", _cc->name(), dwarf.table()); + _cc->setDwarfTable(dwarf.table(), dwarf.count()); + return; + } + } + } + // no valid eh_frame_hdr found; need to rely on the default linked frame descriptor + FrameDesc *table = (FrameDesc *)malloc(sizeof(FrameDesc)); +#if defined(__aarch64__) + // default to clang frame layout - if we have gcc binary it will have the .comment section + *table = FrameDesc::default_frame; + Elf64_Shdr* commentSection = findSection(SHT_PROGBITS, ".comment"); + bool frame_layout_resolved = false; + if (commentSection) { + if (commentSection->sh_size >= 4) { // "GCC" + NULL terminator needs at least 4 bytes + char* commentData = (char*)at(commentSection); + if (strstr(commentData, "GCC") != 0) { + frame_layout_resolved = true; + TEST_LOG(".comment section for %s :: %s, using gcc frame layout", _cc->name(), commentData); + } else { + TEST_LOG(".comment section for %s :: %s, using clang frame layout", _cc->name(), commentData); + } } + } else { + TEST_LOG("No .comment section found for %s, will probe pre-amble", _cc->name()); + } + if (!frame_layout_resolved) { + for (int b = 0; b < _cc->count(); b++) { + CodeBlob* blob = _cc->blob(b); + if (blob) { + instruction_t* ptr = (instruction_t*)blob->_start; + instruction_t gcc_pattern = 0x910003fd; // mov x29, sp + instruction_t clang_pattern = 0xfd7b01a9; // stp x29, x30, [sp, #16] + if (*(ptr + 1) == gcc_pattern || *(ptr + 2) == gcc_pattern) { + *table = FrameDesc::default_frame; + TEST_LOG("Found GCC pattern in code blob for %s, using gcc frame layout", _cc->name()); + frame_layout_resolved = true; + break; + } else if (*(ptr + 1) == clang_pattern || *(ptr + 2) == clang_pattern) { + *table = FrameDesc::default_clang_frame; + TEST_LOG("Found Clang pattern in code blob for %s, using clang frame layout", _cc->name()); + frame_layout_resolved = true; + break; + } + } + } + } + if (!frame_layout_resolved) { + *table = FrameDesc::default_frame; + TEST_LOG("No frame layout found for %s, using gcc frame layout", _cc->name()); } +#else + *table = FrameDesc::default_frame; +#endif + _cc->setDwarfTable(table, 1); } uint32_t ElfParser::getSymbolCount(uint32_t *gnu_hash) { diff --git a/ddprof-lib/src/main/cpp/symbols_linux.h b/ddprof-lib/src/main/cpp/symbols_linux.h index 9dda9704f..270071c74 100644 --- a/ddprof-lib/src/main/cpp/symbols_linux.h +++ b/ddprof-lib/src/main/cpp/symbols_linux.h @@ -153,6 +153,10 @@ class ElfParser { } ElfSection *section(int index) { + if (index >= _header->e_shnum) { + // invalid section index + return NULL; + } return (ElfSection *)(_sections + index * _header->e_shentsize); } From 433670efdc33102d148ac2bc92dbcd77cef6cc83 Mon Sep 17 00:00:00 2001 From: Jaroslav Bachorik Date: Fri, 21 Mar 2025 19:49:07 -0700 Subject: [PATCH 02/11] Improve recovery from Java Anchor Frame --- ddprof-lib/src/main/cpp/codeCache.h | 2 +- ddprof-lib/src/main/cpp/profiler.cpp | 4 +- ddprof-lib/src/main/cpp/profiler.h | 2 +- ddprof-lib/src/main/cpp/stackWalker.cpp | 338 +++++++++++++++--------- ddprof-lib/src/main/cpp/vmStructs.h | 8 +- 5 files changed, 223 insertions(+), 131 deletions(-) diff --git a/ddprof-lib/src/main/cpp/codeCache.h b/ddprof-lib/src/main/cpp/codeCache.h index c8e551306..2c35b00b0 100644 --- a/ddprof-lib/src/main/cpp/codeCache.h +++ b/ddprof-lib/src/main/cpp/codeCache.h @@ -113,7 +113,7 @@ class CodeCache { void expand(); void makeImportsPatchable(); - void saveImport(ImportId id, void** entry); + void saveImport(ImportId id, void** entry); public: explicit CodeCache(const char *name, short lib_index = -1, diff --git a/ddprof-lib/src/main/cpp/profiler.cpp b/ddprof-lib/src/main/cpp/profiler.cpp index b35448c9c..4d83c73f4 100644 --- a/ddprof-lib/src/main/cpp/profiler.cpp +++ b/ddprof-lib/src/main/cpp/profiler.cpp @@ -252,10 +252,10 @@ CodeBlob *Profiler::findRuntimeStub(const void *address) { return _runtime_stubs.findBlobByAddress(address); } -bool Profiler::isAddressInCode(const void *pc) { +bool Profiler::isAddressInCode(const void *pc, bool include_stubs) { if (CodeHeap::contains(pc)) { return CodeHeap::findNMethod(pc) != NULL && - !(pc >= _call_stub_begin && pc < _call_stub_end); + (include_stubs || !(pc >= _call_stub_begin && pc < _call_stub_end)); } else { return _libs->findLibraryByAddress(pc) != NULL; } diff --git a/ddprof-lib/src/main/cpp/profiler.h b/ddprof-lib/src/main/cpp/profiler.h index 82926ec34..801057029 100644 --- a/ddprof-lib/src/main/cpp/profiler.h +++ b/ddprof-lib/src/main/cpp/profiler.h @@ -243,7 +243,7 @@ class Profiler { const char *getLibraryName(const char *native_symbol); const char *findNativeMethod(const void *address); CodeBlob *findRuntimeStub(const void *address); - bool isAddressInCode(const void *pc); + bool isAddressInCode(const void *pc, bool include_stubs = true); static void segvHandler(int signo, siginfo_t *siginfo, void *ucontext); static void busHandler(int signo, siginfo_t *siginfo, void *ucontext); diff --git a/ddprof-lib/src/main/cpp/stackWalker.cpp b/ddprof-lib/src/main/cpp/stackWalker.cpp index 7786391fc..d0e4952ae 100644 --- a/ddprof-lib/src/main/cpp/stackWalker.cpp +++ b/ddprof-lib/src/main/cpp/stackWalker.cpp @@ -22,6 +22,8 @@ #include "safeAccess.h" #include "stackFrame.h" #include "vmStructs.h" + +#include #include const uintptr_t SAME_STACK_DISTANCE = 8192; @@ -42,6 +44,16 @@ static inline bool sameStack(void *hi, void *lo) { return (uintptr_t)hi - (uintptr_t)lo < SAME_STACK_DISTANCE; } +// AArch64: on Linux, frame link is stored at the top of the frame, +// while on macOS, frame link is at the bottom. +static inline uintptr_t defaultSenderSP(uintptr_t sp, uintptr_t fp) { +#ifdef __APPLE__ + return sp + 2 * sizeof(void*); +#else + return fp; +#endif +} + static inline void fillFrame(ASGCT_CallFrame &frame, ASGCT_CallFrameType type, const char *name) { frame.bci = type; @@ -210,7 +222,7 @@ int StackWalker::walkDwarf(void *ucontext, const void **callchain, } else if (f->fp_off != DW_SAME_FP) { // AArch64 default_frame pc = stripPointer(SafeAccess::load((void **)(sp + f->pc_off))); - sp = fp; + sp = defaultSenderSP(sp, fp); } else if (depth <= 1) { pc = (const void *)frame.link(); } else { @@ -224,6 +236,7 @@ int StackWalker::walkDwarf(void *ucontext, const void **callchain, int StackWalker::walkVM(void* ucontext, ASGCT_CallFrame* frames, int max_depth, StackDetail detail, const void* pc, uintptr_t sp, uintptr_t fp, bool *truncated) { + // extract StackFrame from uncontext StackFrame frame(ucontext); uintptr_t bottom = (uintptr_t)&frame + MAX_WALK_SIZE; @@ -242,170 +255,242 @@ int StackWalker::walkVM(void* ucontext, ASGCT_CallFrame* frames, int max_depth, // Should be preserved across setjmp/longjmp volatile int depth = 0; + JavaFrameAnchor* anchor = nullptr; + bool recovered_from_anchor = false; if (vm_thread != NULL) { vm_thread->exception() = &crash_protection_ctx; if (setjmp(crash_protection_ctx) != 0) { vm_thread->exception() = saved_exception; if (depth < max_depth) { + TEST_LOG("Crash protection triggered"); fillFrame(frames[depth++], BCI_ERROR, "break_not_walkable"); } return depth; } + anchor = vm_thread->anchor(); } CodeCache *cc = NULL; + bool got_start_thread = false; + const void* prev_pc = 0; // Walk until the bottom of the stack or until the first Java frame while (depth < max_depth) { if (CodeHeap::contains(pc)) { // constant time NMethod *nm = CodeHeap::findNMethod(pc); if (nm == NULL) { + TEST_LOG("CodeHeap::findNMethod returned NULL for pc: %p", pc); fillFrame(frames[depth++], BCI_ERROR, "unknown_nmethod"); // we are somewhere in JVM code heap and have no idea what is this frame // might be good to bail out since it would be a challenge to unwind properly? break; - } else if (nm->isNMethod()) { - int level = nm->level(); - FrameTypeId type = detail != VM_BASIC && - level >= 1 && level <= 3 ? FRAME_C1_COMPILED : FRAME_JIT_COMPILED; - fillFrame(frames[depth++], type, 0, nm->method()->id()); - - if (nm->isFrameCompleteAt(pc)) { - int scope_offset = nm->findScopeOffset(pc); - if (scope_offset > 0) { - depth--; - ScopeDesc scope(nm); - do { - scope_offset = scope.decode(scope_offset); - if (scope.method() == nullptr) { - fillFrame(frames[depth++], BCI_ERROR, "unknown_scope"); + } else { + if (nm->isNMethod()) { + int level = nm->level(); + FrameTypeId type = detail != VM_BASIC && + level >= 1 && level <= 3 ? FRAME_C1_COMPILED : FRAME_JIT_COMPILED; + fillFrame(frames[depth++], type, 0, nm->method()->id()); + + if (nm->isFrameCompleteAt(pc)) { + int scope_offset = nm->findScopeOffset(pc); + if (scope_offset > 0) { + depth--; + ScopeDesc scope(nm); + do { + scope_offset = scope.decode(scope_offset); + if (scope.method() == nullptr) { + TEST_LOG("ScopeDesc::decode returned nullptr for pc: %p", pc); + fillFrame(frames[depth++], BCI_ERROR, "unknown_scope"); + break; + } + if (detail != VM_BASIC) { + type = scope_offset > 0 ? FRAME_INLINED : + level >= 1 && level <= 3 ? FRAME_C1_COMPILED : FRAME_JIT_COMPILED; + } + fillFrame(frames[depth++], type, scope.bci(), + scope.method()->id()); + } while (scope_offset > 0 && depth < max_depth); + } + + // Handle situations when sp is temporarily changed in the compiled + // code + frame.adjustSP(nm->entry(), pc, sp); + uintptr_t fpback = fp; + sp += nm->frameSize() * sizeof(void *); + fp = ((uintptr_t *)sp)[-FRAME_PC_SLOT - 1]; + pc = ((const void **)sp)[-FRAME_PC_SLOT]; + + if (!profiler->isAddressInCode(pc, true)) { + const void* newpc = stripPointer(*(const void **)(fpback + sizeof(void*))); + if (profiler->isAddressInCode(newpc, true)) { + TEST_LOG("Relinking via FP for entry: %lx, %s", nm->entry(), nm->name()); + fp = *(uintptr_t *)fpback; + pc = newpc; + sp = fp; + } else { + if (anchor && !recovered_from_anchor) { + TEST_LOG("Recovering from java frame anchor from entry: %lx, %s", nm->entry(), nm->name()); + recovered_from_anchor = true; + if (anchor->lastJavaPC() == nullptr || anchor->lastJavaSP() == 0) { + // End of Java stack + break; + } + pc = anchor->lastJavaPC(); + sp = anchor->lastJavaSP(); + fp = anchor->lastJavaFP(); + continue; + } + TEST_LOG("Failed to recover from entry: %lx, %s", nm->entry(), nm->name()); + fillFrame(frames[depth++], BCI_ERROR, "break_compiled"); break; } - if (detail != VM_BASIC) { - type = scope_offset > 0 ? FRAME_INLINED : - level >= 1 && level <= 3 ? FRAME_C1_COMPILED : FRAME_JIT_COMPILED; - } - fillFrame(frames[depth++], type, scope.bci(), - scope.method()->id()); - } while (scope_offset > 0 && depth < max_depth); + } + continue; + } else { + if (frame.unwindCompiled(nm, (uintptr_t &)pc, sp, fp) && profiler->isAddressInCode(pc)) { + continue; + } } - // Handle situations when sp is temporarily changed in the compiled - // code - frame.adjustSP(nm->entry(), pc, sp); - - sp += nm->frameSize() * sizeof(void *); - fp = ((uintptr_t *)sp)[-FRAME_PC_SLOT - 1]; - pc = ((const void **)sp)[-FRAME_PC_SLOT]; - continue; - } else if (frame.unwindCompiled(nm, (uintptr_t &)pc, sp, fp) && - profiler->isAddressInCode(pc)) { - continue; - } - - fillFrame(frames[depth++], BCI_ERROR, "break_compiled"); - break; - } else if (nm->isInterpreter()) { - if (vm_thread != NULL && vm_thread->inDeopt()) { - fillFrame(frames[depth++], BCI_ERROR, "break_deopt"); + TEST_LOG("Failed to unwind compiled frame"); + fillFrame(frames[depth++], BCI_ERROR, "break_compiled"); break; - } - - bool is_plausible_interpreter_frame = - !inDeadZone((const void *)fp) && aligned(fp) && - sp > fp - MAX_INTERPRETER_FRAME_SIZE && - sp < fp + bcp_offset * sizeof(void *); - - if (is_plausible_interpreter_frame) { - VMMethod *method = ((VMMethod **)fp)[InterpreterFrame::method_offset]; - jmethodID method_id = getMethodId(method); - if (method_id != NULL) { - const char *bytecode_start = method->bytecode(); - const char *bcp = ((const char **)fp)[bcp_offset]; - int bci = bytecode_start == NULL || bcp < bytecode_start - ? 0 - : bcp - bytecode_start; - fillFrame(frames[depth++], FRAME_INTERPRETED, bci, method_id); - - sp = ((uintptr_t *)fp)[InterpreterFrame::sender_sp_offset]; - pc = stripPointer(((void **)fp)[FRAME_PC_SLOT]); - fp = *(uintptr_t *)fp; - continue; + } else if (nm->isInterpreter()) { + if (vm_thread != NULL && vm_thread->inDeopt()) { + TEST_LOG("Failed to unwind deopt frame"); + fillFrame(frames[depth++], BCI_ERROR, "break_deopt"); + break; } - } - - if (depth == 0) { - VMMethod *method = (VMMethod *)frame.method(); - jmethodID method_id = getMethodId(method); - if (method_id != NULL) { - fillFrame(frames[depth++], FRAME_INTERPRETED, 0, method_id); - if (is_plausible_interpreter_frame) { + bool is_plausible_interpreter_frame = + !inDeadZone((const void *)fp) && aligned(fp) && + sp > fp - MAX_INTERPRETER_FRAME_SIZE && + sp < fp + bcp_offset * sizeof(void *); + + if (is_plausible_interpreter_frame) { + VMMethod *method = ((VMMethod **)fp)[InterpreterFrame::method_offset]; + jmethodID method_id = getMethodId(method); + if (method_id != NULL) { + const char *bytecode_start = method->bytecode(); + const char *bcp = ((const char **)fp)[bcp_offset]; + int bci = bytecode_start == NULL || bcp < bytecode_start + ? 0 + : bcp - bytecode_start; + fillFrame(frames[depth++], FRAME_INTERPRETED, bci, method_id); + + sp = ((uintptr_t *)fp)[InterpreterFrame::sender_sp_offset]; pc = stripPointer(((void **)fp)[FRAME_PC_SLOT]); - sp = frame.senderSP(); fp = *(uintptr_t *)fp; - } else { - pc = stripPointer(*(void **)sp); - sp = frame.senderSP(); + continue; } - continue; } - } - fillFrame(frames[depth++], BCI_ERROR, "break_interpreted"); - break; - } else if (detail < VM_EXPERT && nm->isEntryFrame(pc)) { - JavaFrameAnchor* anchor = JavaFrameAnchor::fromEntryFrame(fp); - if (anchor == NULL) { - fillFrame(frames[depth++], BCI_ERROR, "break_entry_frame"); - break; - } - uintptr_t prev_sp = sp; - sp = anchor->lastJavaSP(); - fp = anchor->lastJavaFP(); - pc = anchor->lastJavaPC(); - if (sp == 0 || pc == NULL) { - // End of Java stack - break; - } - if (sp < prev_sp || sp >= bottom || !aligned(sp)) { - fillFrame(frames[depth++], BCI_ERROR, "break_entry_frame"); - break; - } - continue; - } else { - // linear time in number of runtime stubs - CodeBlob *stub = profiler->findRuntimeStub(pc); - const void *start = stub != NULL ? stub->_start : nm->code(); - const char *name = stub != NULL ? stub->_name : nm->name(); + if (depth == 0) { + VMMethod *method = (VMMethod *)frame.method(); + jmethodID method_id = getMethodId(method); + if (method_id != NULL) { + fillFrame(frames[depth++], FRAME_INTERPRETED, 0, method_id); + + if (is_plausible_interpreter_frame) { + pc = stripPointer(((void **)fp)[FRAME_PC_SLOT]); + sp = frame.senderSP(); + fp = *(uintptr_t *)fp; + } else { + pc = stripPointer(*(void **)sp); + sp = frame.senderSP(); + } + continue; + } + } - if (detail != VM_BASIC) { - fillFrame(frames[depth++], BCI_NATIVE_FRAME, name); - } + if (!recovered_from_anchor && anchor && detail < VM_EXPERT) { + if (anchor->lastJavaPC() == nullptr || anchor->lastJavaSP() == 0) { + // End of Java stack + break; + } + fp = anchor->lastJavaFP(); + sp = anchor->lastJavaSP(); + pc = anchor->lastJavaPC(); + recovered_from_anchor = true; + continue; + } - if (frame.unwindStub((instruction_t *)start, name, (uintptr_t &)pc, sp, - fp)) { + TEST_LOG("Failed to unwind interpreter frame"); + fillFrame(frames[depth++], BCI_ERROR, "break_interpreted"); + break; + } else if (detail < VM_EXPERT && nm->isEntryFrame(pc)) { + JavaFrameAnchor* e_anchor = JavaFrameAnchor::fromEntryFrame(fp); + if (e_anchor == NULL) { + if (anchor != NULL && !recovered_from_anchor && anchor->lastJavaSP() != 0) { + TEST_LOG("Reusing thread anchor"); + e_anchor = anchor; + } else { + fillFrame(frames[depth++], BCI_ERROR, "break_entry_frame: no anchor"); + break; + } + } + uintptr_t prev_sp = sp; + sp = e_anchor->lastJavaSP(); + fp = e_anchor->lastJavaFP(); + pc = e_anchor->lastJavaPC(); + if (sp == 0 || pc == NULL) { + // End of Java stack + break; + } + if (sp < prev_sp || sp >= bottom || !aligned(sp)) { + TEST_LOG("Failed to unwind entry frame"); + fillFrame(frames[depth++], BCI_ERROR, "break_entry_frame: invalid anchor"); + break; + } continue; - } + } else { + // linear time in number of runtime stubs + CodeBlob *stub = profiler->findRuntimeStub(pc); + const void *start = stub != NULL ? stub->_start : nm->code(); + const char *name = stub != NULL ? stub->_name : nm->name(); + + if (detail != VM_BASIC) { + fillFrame(frames[depth++], BCI_NATIVE_FRAME, name); + } - if (depth > 1 && nm->frameSize() > 0) { - sp += nm->frameSize() * sizeof(void *); - fp = ((uintptr_t *)sp)[-FRAME_PC_SLOT - 1]; - pc = ((const void **)sp)[-FRAME_PC_SLOT]; - continue; + if (frame.unwindStub((instruction_t *)start, name, (uintptr_t &)pc, sp, + fp)) { + continue; + } + + if (nm->frameSize() == 0) { + // stubs with 0 frame size are real + pc = stripPointer(*(const void **)(fp +sizeof(void*))); + fp = *(uintptr_t *)fp; + sp = fp; + continue; + } + if (depth > 1 && nm->frameSize() > 0) { + sp += nm->frameSize() * sizeof(void *); + fp = ((uintptr_t *)sp)[-FRAME_PC_SLOT - 1]; + pc = ((const void **)sp)[-FRAME_PC_SLOT]; + continue; + } } } } else { - if (cc == NULL || !cc->contains(pc)) { - cc = libraries->findLibraryByAddress(pc); - } - const char *name = cc == NULL ? NULL : cc->binarySearch(pc); + // sometimes we seem to get into a long sequence of frames pointing to the same pc + // we just want to skip that over + if (prev_pc != pc) { + if (cc == NULL || !cc->contains(pc)) { + cc = libraries->findLibraryByAddress(pc); + } + const char *name = cc == NULL ? NULL : cc->binarySearch(pc); - fillFrame(frames[depth++], BCI_NATIVE_FRAME, name); - // _start should be the process entry point; any attempt to unwind from there will fail - if (name && !strcmp("_start", name)) { - break; + fillFrame(frames[depth++], BCI_NATIVE_FRAME, name); + // _start should be the process entry point; any attempt to unwind from there will fail + if (name) { + if (!strcmp("_start", name) || !strcmp("start_thread", name) || !strcmp("_ZL19thread_native_entryP6Thread", name) || !strcmp("_thread_start", name) || !strcmp("thread_start", name)) { + break; + } + } + prev_pc = pc; } } @@ -459,7 +544,7 @@ int StackWalker::walkVM(void* ucontext, ASGCT_CallFrame* frames, int max_depth, } else if (f->fp_off != DW_SAME_FP) { // AArch64 default_frame pc = stripPointer(*(void **)(sp + f->pc_off)); - sp = fp; + sp = defaultSenderSP(sp, fp); } else if (depth <= 1) { pc = (const void *)frame.link(); } else { @@ -468,6 +553,13 @@ int StackWalker::walkVM(void* ucontext, ASGCT_CallFrame* frames, int max_depth, } } + if (anchor && !recovered_from_anchor && !VMStructs::goodPtr((const void*)fp) && anchor->lastJavaSP() != 0) { + recovered_from_anchor = true; + sp = anchor->lastJavaSP(); + fp = anchor->lastJavaFP(); + pc = anchor->lastJavaPC(); + } + if (inDeadZone(pc)) { break; } diff --git a/ddprof-lib/src/main/cpp/vmStructs.h b/ddprof-lib/src/main/cpp/vmStructs.h index b57afa390..930678890 100644 --- a/ddprof-lib/src/main/cpp/vmStructs.h +++ b/ddprof-lib/src/main/cpp/vmStructs.h @@ -162,10 +162,6 @@ class VMStructs { static void initUnsafeFunctions(); static void initMemoryUsage(JNIEnv *env); - static bool goodPtr(const void* ptr) { - return (uintptr_t)ptr >= 0x1000 && ((uintptr_t)ptr & (sizeof(uintptr_t) - 1)) == 0; - } - const char *at(int offset) { return (const char *)this + offset; } static bool aligned(const void *ptr) { @@ -184,6 +180,10 @@ class VMStructs { static bool isFlagTrue(const char *name); public: + static bool goodPtr(const void* ptr) { + return (uintptr_t)ptr >= 0x1000 && ((uintptr_t)ptr & (sizeof(uintptr_t) - 1)) == 0; + } + static void init(CodeCache *libjvm); static void ready(); From 98f565dec019589228a0b38fd87049c6803572fc Mon Sep 17 00:00:00 2001 From: Jaroslav Bachorik Date: Fri, 21 Mar 2025 19:51:47 -0700 Subject: [PATCH 03/11] Add and extend tests for vmstructs based stackwalking --- .../datadoghq/profiler/cpu/IOBoundCode.java | 93 +++++++++++++++++++ .../datadoghq/profiler/cpu/SmokeCpuTest.java | 49 +++++++--- .../ContendedWallclockSamplesTest.java | 6 -- .../wallclock/ContextWallClockTest.java | 10 +- 4 files changed, 129 insertions(+), 29 deletions(-) create mode 100644 ddprof-test/src/test/java/com/datadoghq/profiler/cpu/IOBoundCode.java diff --git a/ddprof-test/src/test/java/com/datadoghq/profiler/cpu/IOBoundCode.java b/ddprof-test/src/test/java/com/datadoghq/profiler/cpu/IOBoundCode.java new file mode 100644 index 000000000..2e1b4acd4 --- /dev/null +++ b/ddprof-test/src/test/java/com/datadoghq/profiler/cpu/IOBoundCode.java @@ -0,0 +1,93 @@ +package com.datadoghq.profiler.cpu; + +import java.io.InputStream; +import java.io.OutputStream; +import java.net.ServerSocket; +import java.net.Socket; +import java.util.concurrent.ThreadLocalRandom; + +public class IOBoundCode { + private static final class IdleClient extends Thread { + private final String host; + private final int port; + + public IdleClient(String host, int port) { + this.host = host; + this.port = port; + setDaemon(true); + } + + @Override + public void run() { + try { + byte[] buf = new byte[4096]; + + try (Socket s = new Socket(host, port)) { + InputStream in = s.getInputStream(); + while (in.read(buf) >= 0) { + // keep reading + } + System.out.println(Thread.currentThread().getName() + " stopped"); + } + } catch (Exception e) { + e.printStackTrace(); + } + } + } + + private static final class BusyClient extends Thread { + private final String host; + private final int port; + + public BusyClient(String host, int port) { + this.host = host; + this.port = port; + setDaemon(true); + } + + @Override + public void run() { + try { + byte[] buf = new byte[4096]; + + try (Socket s = new Socket(host, port)) { + + InputStream in = s.getInputStream(); + while (in.read(buf) >= 0) { + // keep reading + } + System.out.println(Thread.currentThread().getName() + " stopped"); + } + } catch (Exception e) { + e.printStackTrace(); + } + } + } + + void run() throws Exception { + try (ServerSocket s = new ServerSocket(0)) { + String host = "localhost"; + int port = s.getLocalPort(); + new IdleClient(host, port).start(); + OutputStream idleClient = s.accept().getOutputStream(); + + new BusyClient(host, port).start(); + OutputStream busyClient = s.accept().getOutputStream(); + + byte[] buf = new byte[4096]; + ThreadLocalRandom.current().nextBytes(buf); + + long target = System.nanoTime() + 3_000_000_000L; + for (int i = 0; ; i++) { + if ((i % 10_000_000) == 0) { + idleClient.write(buf, 0, 1); + } else { + busyClient.write(buf); + } + if (System.nanoTime() >= target) { + break; + } + } + } + } +} diff --git a/ddprof-test/src/test/java/com/datadoghq/profiler/cpu/SmokeCpuTest.java b/ddprof-test/src/test/java/com/datadoghq/profiler/cpu/SmokeCpuTest.java index 01787d243..868531250 100644 --- a/ddprof-test/src/test/java/com/datadoghq/profiler/cpu/SmokeCpuTest.java +++ b/ddprof-test/src/test/java/com/datadoghq/profiler/cpu/SmokeCpuTest.java @@ -21,24 +21,48 @@ import com.datadoghq.profiler.Platform; public class SmokeCpuTest extends CStackAwareAbstractProfilerTest { - private ProfiledCode profiledCode; - public SmokeCpuTest(@CStack String cstack) { super(cstack); } - @Override - protected void before() { - profiledCode = new ProfiledCode(profiler); - } - @RetryTest(10) @TestTemplate @ValueSource(strings = {"fp", "dwarf", "vm", "vmx"}) - public void test(@CStack String cstack) throws ExecutionException, InterruptedException { - for (int i = 0, id = 1; i < 100; i++, id += 3) { - profiledCode.method1(id); + public void testComputations(@CStack String cstack) throws Exception { + try (ProfiledCode profiledCode = new ProfiledCode(profiler)) { + for (int i = 0, id = 1; i < 100; i++, id += 3) { + profiledCode.method1(id); + } + stopProfiler(); + + verifyCStackSettings(); + + IItemCollection events = verifyEvents("datadog.ExecutionSample"); + + // on mac the usage of itimer to drive the sampling provides very unreliable outputs + for (IItemIterable cpuSamples : events) { + IMemberAccessor frameAccessor = JdkAttributes.STACK_TRACE_STRING.getAccessor(cpuSamples.getType()); + for (IItem sample : cpuSamples) { + String stackTrace = frameAccessor.getMember(sample); + assertFalse(stackTrace.contains("jvmtiError")); + if ("vmx".equals(stackTrace)) { + // extra checks to make sure we see the mixed stacktraces + assertTrue(stackTrace.contains("JavaCalls::call_virtual()"), + "JavaCalls::call_virtual() (above call_stub) found in stack trace"); + assertTrue(stackTrace.contains("call_stub()"), + "call_stub() (sentinel value used to halt unwinding) found in stack trace"); + } + } + } } + } + + @RetryTest(10) + @TestTemplate + @ValueSource(strings = {"vm", "fp", "dwarf", "vmx"}) + public void testIOBound(@CStack String cstack) throws Exception { + new IOBoundCode().run(); + stopProfiler(); verifyCStackSettings(); @@ -62,11 +86,6 @@ public void test(@CStack String cstack) throws ExecutionException, InterruptedEx } } - @Override - protected void after() throws Exception { - profiledCode.close(); - } - @Override protected String getProfilerCommand() { return "cpu=10ms"; diff --git a/ddprof-test/src/test/java/com/datadoghq/profiler/wallclock/ContendedWallclockSamplesTest.java b/ddprof-test/src/test/java/com/datadoghq/profiler/wallclock/ContendedWallclockSamplesTest.java index 17d7dbbbf..038d09f70 100644 --- a/ddprof-test/src/test/java/com/datadoghq/profiler/wallclock/ContendedWallclockSamplesTest.java +++ b/ddprof-test/src/test/java/com/datadoghq/profiler/wallclock/ContendedWallclockSamplesTest.java @@ -52,12 +52,6 @@ public void after() { @ValueSource(strings = {"fp", "dwarf", "vm", "vmx"}) public void test(@CStack String cstack) { assumeFalse(Platform.isZing() || Platform.isJ9()); - // on aarch64 and JDK 8 the vmstructs unwinding for wallclock is extremely unreliable - // ; perhaps due to something missing in the unwinder but until we figure it out we will just not run the tests in CI - assumeTrue(!isInCI() || !Platform.isAarch64() || !cstack.startsWith("vm") || Platform.isJavaVersionAtLeast(11)); - // TODO: investigate why this test fails on musl - // on musl the missing fp unwinding makes the wallclock tests unreliable - assumeTrue(!Platform.isMusl() || !cstack.startsWith("vm")); long result = 0; for (int i = 0; i < 10; i++) { diff --git a/ddprof-test/src/test/java/com/datadoghq/profiler/wallclock/ContextWallClockTest.java b/ddprof-test/src/test/java/com/datadoghq/profiler/wallclock/ContextWallClockTest.java index 62bca3dec..cbcc8ad77 100644 --- a/ddprof-test/src/test/java/com/datadoghq/profiler/wallclock/ContextWallClockTest.java +++ b/ddprof-test/src/test/java/com/datadoghq/profiler/wallclock/ContextWallClockTest.java @@ -31,14 +31,8 @@ protected void after() throws InterruptedException { @RetryTest(5) @TestTemplate - @ValueSource(strings = {"fp", "dwarf", "vm", "vmx"}) - public void test(@CStack String cstack) throws ExecutionException, InterruptedException { - // on aarch64 and JDK < 17 the vmstructs unwinding for wallclock is extremely unreliable - // ; perhaps due to something missing in the unwinder but until we figure it out we will just not run the tests in CI - assumeTrue(!isInCI() || !Platform.isAarch64() || !cstack.startsWith("vm") || Platform.isJavaVersionAtLeast(17)); - // TODO: investigate why this test fails on musl - // on musl the missing fp unwinding makes the wallclock tests unreliable - assumeTrue(!Platform.isMusl() || !cstack.startsWith("vm")); + @ValueSource(strings = {"fp", "dwarf", "vmx", "vm"}) + public void test(@CStack String cstack) throws ExecutionException, InterruptedException, Exception { base.test(this); } From 9c2aea939f71ae47f5bdc6e6739c411c0c6d417b Mon Sep 17 00:00:00 2001 From: Jaroslav Bachorik Date: Sat, 22 Mar 2025 21:33:49 -0700 Subject: [PATCH 04/11] Adjust for Musl signal trampoline --- ddprof-lib/src/main/cpp/stackWalker.cpp | 44 ++++++++++++++++--------- 1 file changed, 29 insertions(+), 15 deletions(-) diff --git a/ddprof-lib/src/main/cpp/stackWalker.cpp b/ddprof-lib/src/main/cpp/stackWalker.cpp index d0e4952ae..3ee6432e6 100644 --- a/ddprof-lib/src/main/cpp/stackWalker.cpp +++ b/ddprof-lib/src/main/cpp/stackWalker.cpp @@ -66,6 +66,13 @@ static inline void fillFrame(ASGCT_CallFrame &frame, FrameTypeId type, int bci, frame.method_id = method; } + +static inline void fillErrorFrame(ASGCT_CallFrame& frame, const char *name, bool *truncated) { + fillFrame(frame, BCI_ERROR, name); + *truncated = true; + TEST_LOG("[stackwalk-error] %s", name); +} + static jmethodID getMethodId(VMMethod *method) { if (!inDeadZone(method) && aligned((uintptr_t)method)) { jmethodID method_id = method->id(); @@ -264,7 +271,7 @@ int StackWalker::walkVM(void* ucontext, ASGCT_CallFrame* frames, int max_depth, vm_thread->exception() = saved_exception; if (depth < max_depth) { TEST_LOG("Crash protection triggered"); - fillFrame(frames[depth++], BCI_ERROR, "break_not_walkable"); + fillErrorFrame(frames[depth++], "break_not_walkable", truncated); } return depth; } @@ -280,8 +287,7 @@ int StackWalker::walkVM(void* ucontext, ASGCT_CallFrame* frames, int max_depth, // constant time NMethod *nm = CodeHeap::findNMethod(pc); if (nm == NULL) { - TEST_LOG("CodeHeap::findNMethod returned NULL for pc: %p", pc); - fillFrame(frames[depth++], BCI_ERROR, "unknown_nmethod"); + fillErrorFrame(frames[depth++], "unknown_nmethod", truncated); // we are somewhere in JVM code heap and have no idea what is this frame // might be good to bail out since it would be a challenge to unwind properly? break; @@ -300,8 +306,7 @@ int StackWalker::walkVM(void* ucontext, ASGCT_CallFrame* frames, int max_depth, do { scope_offset = scope.decode(scope_offset); if (scope.method() == nullptr) { - TEST_LOG("ScopeDesc::decode returned nullptr for pc: %p", pc); - fillFrame(frames[depth++], BCI_ERROR, "unknown_scope"); + fillErrorFrame(frames[depth++], "unknown_scope", truncated); break; } if (detail != VM_BASIC) { @@ -341,8 +346,7 @@ int StackWalker::walkVM(void* ucontext, ASGCT_CallFrame* frames, int max_depth, fp = anchor->lastJavaFP(); continue; } - TEST_LOG("Failed to recover from entry: %lx, %s", nm->entry(), nm->name()); - fillFrame(frames[depth++], BCI_ERROR, "break_compiled"); + fillErrorFrame(frames[depth++], "break_compiled: no java anchor", truncated); break; } } @@ -353,13 +357,11 @@ int StackWalker::walkVM(void* ucontext, ASGCT_CallFrame* frames, int max_depth, } } - TEST_LOG("Failed to unwind compiled frame"); - fillFrame(frames[depth++], BCI_ERROR, "break_compiled"); + fillErrorFrame(frames[depth++], "break_compiled", truncated); break; } else if (nm->isInterpreter()) { if (vm_thread != NULL && vm_thread->inDeopt()) { - TEST_LOG("Failed to unwind deopt frame"); - fillFrame(frames[depth++], BCI_ERROR, "break_deopt"); + fillErrorFrame(frames[depth++], "break_deopt", truncated); break; } @@ -416,8 +418,7 @@ int StackWalker::walkVM(void* ucontext, ASGCT_CallFrame* frames, int max_depth, continue; } - TEST_LOG("Failed to unwind interpreter frame"); - fillFrame(frames[depth++], BCI_ERROR, "break_interpreted"); + fillErrorFrame(frames[depth++], "break_interpreted", truncated); break; } else if (detail < VM_EXPERT && nm->isEntryFrame(pc)) { JavaFrameAnchor* e_anchor = JavaFrameAnchor::fromEntryFrame(fp); @@ -426,7 +427,7 @@ int StackWalker::walkVM(void* ucontext, ASGCT_CallFrame* frames, int max_depth, TEST_LOG("Reusing thread anchor"); e_anchor = anchor; } else { - fillFrame(frames[depth++], BCI_ERROR, "break_entry_frame: no anchor"); + fillErrorFrame(frames[depth++], "break_entry_frame: no anchor", truncated); break; } } @@ -440,7 +441,7 @@ int StackWalker::walkVM(void* ucontext, ASGCT_CallFrame* frames, int max_depth, } if (sp < prev_sp || sp >= bottom || !aligned(sp)) { TEST_LOG("Failed to unwind entry frame"); - fillFrame(frames[depth++], BCI_ERROR, "break_entry_frame: invalid anchor"); + fillErrorFrame(frames[depth++], "break_entry_frame: invalid anchor", truncated); break; } continue; @@ -523,6 +524,19 @@ int StackWalker::walkVM(void* ucontext, ASGCT_CallFrame* frames, int max_depth, // Check if the next frame is below on the current stack if (sp < prev_sp || sp >= prev_sp + MAX_FRAME_SIZE || sp >= bottom) { + if (fp < 0xff && sp < 0xff && depth <= 1) { + // Most likely MUSL signal trampoline + // FP, SP, RET is completely garbled and unrecoverable + // Let's try to jump to the stored Java frame - + // the garbled frame is leaf so there is no danger we 'loop' back to the top Java frame + if (anchor && !recovered_from_anchor && anchor->lastJavaSP() != 0) { + recovered_from_anchor = true; + sp = anchor->lastJavaSP(); + fp = anchor->lastJavaFP(); + pc = anchor->lastJavaPC(); + continue; + } + } break; } From 06b2efe946f1c482bdf930243c5ce60bceec2003 Mon Sep 17 00:00:00 2001 From: Jaroslav Bachorik Date: Sat, 22 Mar 2025 21:57:00 -0700 Subject: [PATCH 05/11] Run non-crashy test configs first --- .../java/com/datadoghq/profiler/cpu/CTimerSamplerTest.java | 2 +- .../test/java/com/datadoghq/profiler/cpu/ContextCpuTest.java | 2 +- .../test/java/com/datadoghq/profiler/cpu/SmokeCpuTest.java | 4 ++-- .../profiler/wallclock/ContendedWallclockSamplesTest.java | 2 +- .../datadoghq/profiler/wallclock/ContextWallClockTest.java | 2 +- .../java/com/datadoghq/profiler/wallclock/SmokeWallTest.java | 2 +- 6 files changed, 7 insertions(+), 7 deletions(-) diff --git a/ddprof-test/src/test/java/com/datadoghq/profiler/cpu/CTimerSamplerTest.java b/ddprof-test/src/test/java/com/datadoghq/profiler/cpu/CTimerSamplerTest.java index aa276a4b3..812bedc7f 100644 --- a/ddprof-test/src/test/java/com/datadoghq/profiler/cpu/CTimerSamplerTest.java +++ b/ddprof-test/src/test/java/com/datadoghq/profiler/cpu/CTimerSamplerTest.java @@ -43,7 +43,7 @@ protected void before() { @RetryTest(10) @TestTemplate - @ValueSource(strings = {"fp", "dwarf", "vm", "vmx"}) + @ValueSource(strings = {"vm", "vmx", "fp", "dwarf"}) public void test(@CStack String cstack) throws ExecutionException, InterruptedException { // timer_create is available on Linux only assumeTrue(Platform.isLinux()); diff --git a/ddprof-test/src/test/java/com/datadoghq/profiler/cpu/ContextCpuTest.java b/ddprof-test/src/test/java/com/datadoghq/profiler/cpu/ContextCpuTest.java index a195e4d76..25aefba43 100644 --- a/ddprof-test/src/test/java/com/datadoghq/profiler/cpu/ContextCpuTest.java +++ b/ddprof-test/src/test/java/com/datadoghq/profiler/cpu/ContextCpuTest.java @@ -42,7 +42,7 @@ protected void before() { @RetryTest(10) @TestTemplate - @ValueSource(strings = {"fp", "dwarf", "vm", "vmx"}) + @ValueSource(strings = {"vm", "vmx", "fp", "dwarf"}) public void test(@CStack String cstack) throws ExecutionException, InterruptedException { Assumptions.assumeTrue(!Platform.isJ9()); for (int i = 0, id = 1; i < 100; i++, id += 3) { diff --git a/ddprof-test/src/test/java/com/datadoghq/profiler/cpu/SmokeCpuTest.java b/ddprof-test/src/test/java/com/datadoghq/profiler/cpu/SmokeCpuTest.java index 868531250..fd065bd14 100644 --- a/ddprof-test/src/test/java/com/datadoghq/profiler/cpu/SmokeCpuTest.java +++ b/ddprof-test/src/test/java/com/datadoghq/profiler/cpu/SmokeCpuTest.java @@ -27,7 +27,7 @@ public SmokeCpuTest(@CStack String cstack) { @RetryTest(10) @TestTemplate - @ValueSource(strings = {"fp", "dwarf", "vm", "vmx"}) + @ValueSource(strings = {"vm", "vmx", "fp", "dwarf"}) public void testComputations(@CStack String cstack) throws Exception { try (ProfiledCode profiledCode = new ProfiledCode(profiler)) { for (int i = 0, id = 1; i < 100; i++, id += 3) { @@ -59,7 +59,7 @@ public void testComputations(@CStack String cstack) throws Exception { @RetryTest(10) @TestTemplate - @ValueSource(strings = {"vm", "fp", "dwarf", "vmx"}) + @ValueSource(strings = {"vm", "vmx", "fp", "dwarf"}) public void testIOBound(@CStack String cstack) throws Exception { new IOBoundCode().run(); diff --git a/ddprof-test/src/test/java/com/datadoghq/profiler/wallclock/ContendedWallclockSamplesTest.java b/ddprof-test/src/test/java/com/datadoghq/profiler/wallclock/ContendedWallclockSamplesTest.java index 038d09f70..ef011406c 100644 --- a/ddprof-test/src/test/java/com/datadoghq/profiler/wallclock/ContendedWallclockSamplesTest.java +++ b/ddprof-test/src/test/java/com/datadoghq/profiler/wallclock/ContendedWallclockSamplesTest.java @@ -49,7 +49,7 @@ public void after() { @RetryTest(10) @TestTemplate - @ValueSource(strings = {"fp", "dwarf", "vm", "vmx"}) + @ValueSource(strings = {"vm", "vmx", "fp", "dwarf"}) public void test(@CStack String cstack) { assumeFalse(Platform.isZing() || Platform.isJ9()); diff --git a/ddprof-test/src/test/java/com/datadoghq/profiler/wallclock/ContextWallClockTest.java b/ddprof-test/src/test/java/com/datadoghq/profiler/wallclock/ContextWallClockTest.java index cbcc8ad77..d8e4082d2 100644 --- a/ddprof-test/src/test/java/com/datadoghq/profiler/wallclock/ContextWallClockTest.java +++ b/ddprof-test/src/test/java/com/datadoghq/profiler/wallclock/ContextWallClockTest.java @@ -31,7 +31,7 @@ protected void after() throws InterruptedException { @RetryTest(5) @TestTemplate - @ValueSource(strings = {"fp", "dwarf", "vmx", "vm"}) + @ValueSource(strings = {"vm", "vmx", "fp", "dwarf"}) public void test(@CStack String cstack) throws ExecutionException, InterruptedException, Exception { base.test(this); } diff --git a/ddprof-test/src/test/java/com/datadoghq/profiler/wallclock/SmokeWallTest.java b/ddprof-test/src/test/java/com/datadoghq/profiler/wallclock/SmokeWallTest.java index 55caf858c..cbcea4ea8 100644 --- a/ddprof-test/src/test/java/com/datadoghq/profiler/wallclock/SmokeWallTest.java +++ b/ddprof-test/src/test/java/com/datadoghq/profiler/wallclock/SmokeWallTest.java @@ -32,7 +32,7 @@ protected void before() { @RetryTest(10) @TestTemplate - @ValueSource(strings = {"fp", "dwarf", "vm", "vmx"}) + @ValueSource(strings = {"vm", "vmx", "fp", "dwarf"}) public void test(@CStack String cstack) throws ExecutionException, InterruptedException { for (int i = 0, id = 1; i < 100; i++, id += 3) { profiledCode.method1(id); From 0836db57a8e0f337eff3e45f22902082ff3fb85d Mon Sep 17 00:00:00 2001 From: Jaroslav Bachorik Date: Sat, 22 Mar 2025 23:15:44 -0700 Subject: [PATCH 06/11] Reduce crashiness and locking of tests --- .../java/com/datadoghq/profiler/cpu/IOBoundCode.java | 8 ++++++-- .../com/datadoghq/profiler/junit/CStackInjector.java | 12 +++++++++++- .../metadata/BoundMethodHandleMetadataSizeTest.java | 1 + 3 files changed, 18 insertions(+), 3 deletions(-) diff --git a/ddprof-test/src/test/java/com/datadoghq/profiler/cpu/IOBoundCode.java b/ddprof-test/src/test/java/com/datadoghq/profiler/cpu/IOBoundCode.java index 2e1b4acd4..2314036cb 100644 --- a/ddprof-test/src/test/java/com/datadoghq/profiler/cpu/IOBoundCode.java +++ b/ddprof-test/src/test/java/com/datadoghq/profiler/cpu/IOBoundCode.java @@ -68,10 +68,12 @@ void run() throws Exception { try (ServerSocket s = new ServerSocket(0)) { String host = "localhost"; int port = s.getLocalPort(); - new IdleClient(host, port).start(); + Thread t1 = new IdleClient(host, port); + t1.start(); OutputStream idleClient = s.accept().getOutputStream(); - new BusyClient(host, port).start(); + Thread t2 = new BusyClient(host, port); + t2.start(); OutputStream busyClient = s.accept().getOutputStream(); byte[] buf = new byte[4096]; @@ -85,6 +87,8 @@ void run() throws Exception { busyClient.write(buf); } if (System.nanoTime() >= target) { + t1.interrupt(); + t2.interrupt(); break; } } diff --git a/ddprof-test/src/test/java/com/datadoghq/profiler/junit/CStackInjector.java b/ddprof-test/src/test/java/com/datadoghq/profiler/junit/CStackInjector.java index 088eddee3..e58702dc9 100644 --- a/ddprof-test/src/test/java/com/datadoghq/profiler/junit/CStackInjector.java +++ b/ddprof-test/src/test/java/com/datadoghq/profiler/junit/CStackInjector.java @@ -51,11 +51,21 @@ public Stream provideTestTemplateInvocationContex return Stream.of(new ParameterizedTestContext("no", retryCount)); } else { return Stream.of(valueSource.strings()). - filter(param -> (!Platform.isJ9() || "dwarf".equals(param))). + filter(CStackInjector::isModeSafe). map(param -> new ParameterizedTestContext(param, retryCount)); } } + private static boolean isModeSafe(String mode) { + if (Platform.isJ9()) { + return "dwarf".equals(mode); + } + if (Platform.isAarch64() && !Platform.isJavaVersionAtLeast(17)) { + return mode.startsWith("vm"); + } + return true; + } + public static class TestInfoAdapter implements TestInfo { private final String displayName; private final Set tags; diff --git a/ddprof-test/src/test/java/com/datadoghq/profiler/metadata/BoundMethodHandleMetadataSizeTest.java b/ddprof-test/src/test/java/com/datadoghq/profiler/metadata/BoundMethodHandleMetadataSizeTest.java index 2581a1bfd..91cc00b98 100644 --- a/ddprof-test/src/test/java/com/datadoghq/profiler/metadata/BoundMethodHandleMetadataSizeTest.java +++ b/ddprof-test/src/test/java/com/datadoghq/profiler/metadata/BoundMethodHandleMetadataSizeTest.java @@ -22,6 +22,7 @@ protected String getProfilerCommand() { @Test public void test() throws Throwable { assumeFalse(Platform.isJ9() && Platform.isJavaVersion(17)); // JVMTI::GetClassSignature() is reliably crashing on a valid 'class' instance ¯\_(ツ)_/¯ + assumeFalse(Platform.isAarch64() && Platform.isMusl() && !Platform.isJavaVersionAtLeast(11)); // aarch64 + musl + jdk 8 will crash very often registerCurrentThreadForWallClockProfiling(); int numBoundMethodHandles = 10_000; int x = generateBoundMethodHandles(numBoundMethodHandles); From 4882ffd8e2545a1bd2c9f2dade0889f1178d397d Mon Sep 17 00:00:00 2001 From: Jaroslav Bachorik Date: Tue, 25 Mar 2025 13:13:19 -0400 Subject: [PATCH 07/11] Use root symbols --- ddprof-lib/src/main/cpp/stackWalker.cpp | 12 +++----- ddprof-lib/src/main/cpp/symbols.h | 3 ++ ddprof-lib/src/main/cpp/symbols_linux.cpp | 37 +++++++++++++++++++++-- ddprof-lib/src/main/cpp/symbols_linux.h | 10 ++++++ ddprof-lib/src/main/cpp/symbols_macos.cpp | 5 +++ 5 files changed, 56 insertions(+), 11 deletions(-) diff --git a/ddprof-lib/src/main/cpp/stackWalker.cpp b/ddprof-lib/src/main/cpp/stackWalker.cpp index 3ee6432e6..125a47f33 100644 --- a/ddprof-lib/src/main/cpp/stackWalker.cpp +++ b/ddprof-lib/src/main/cpp/stackWalker.cpp @@ -21,6 +21,7 @@ #include "profiler.h" #include "safeAccess.h" #include "stackFrame.h" +#include "symbols.h" #include "vmStructs.h" #include @@ -288,9 +289,6 @@ int StackWalker::walkVM(void* ucontext, ASGCT_CallFrame* frames, int max_depth, NMethod *nm = CodeHeap::findNMethod(pc); if (nm == NULL) { fillErrorFrame(frames[depth++], "unknown_nmethod", truncated); - // we are somewhere in JVM code heap and have no idea what is this frame - // might be good to bail out since it would be a challenge to unwind properly? - break; } else { if (nm->isNMethod()) { int level = nm->level(); @@ -485,11 +483,9 @@ int StackWalker::walkVM(void* ucontext, ASGCT_CallFrame* frames, int max_depth, const char *name = cc == NULL ? NULL : cc->binarySearch(pc); fillFrame(frames[depth++], BCI_NATIVE_FRAME, name); - // _start should be the process entry point; any attempt to unwind from there will fail - if (name) { - if (!strcmp("_start", name) || !strcmp("start_thread", name) || !strcmp("_ZL19thread_native_entryP6Thread", name) || !strcmp("_thread_start", name) || !strcmp("thread_start", name)) { - break; - } + + if (Symbols::isRootSymbol(pc)) { + break; } prev_pc = pc; } diff --git a/ddprof-lib/src/main/cpp/symbols.h b/ddprof-lib/src/main/cpp/symbols.h index fff9bacb0..bf0738310 100644 --- a/ddprof-lib/src/main/cpp/symbols.h +++ b/ddprof-lib/src/main/cpp/symbols.h @@ -32,6 +32,9 @@ class Symbols { // There are internal caches that are not associated to the array static void clearParsingCaches(); static bool haveKernelSymbols() { return _have_kernel_symbols; } + + // Some symbols are always roots - eg. no unwinding should be attempted once they are encountered + static bool isRootSymbol(const void* address); }; #endif // _SYMBOLS_H diff --git a/ddprof-lib/src/main/cpp/symbols_linux.cpp b/ddprof-lib/src/main/cpp/symbols_linux.cpp index 1171b4a35..fde22f499 100644 --- a/ddprof-lib/src/main/cpp/symbols_linux.cpp +++ b/ddprof-lib/src/main/cpp/symbols_linux.cpp @@ -29,6 +29,8 @@ #define _FILE_OFFSET_BITS 64 #include +uintptr_t ElfParser::_root_symbols[LAST_ROOT_SYMBOL_KIND] = {0}; + ElfSection *ElfParser::findSection(uint32_t type, const char *name) { if (_header == NULL) { return NULL; @@ -348,6 +350,26 @@ void ElfParser::loadSymbols(bool use_debug) { } } +void ElfParser::addSymbol(const void *start, int length, const char *name, bool update_bounds) { + _cc->add(start, length, name, update_bounds); + if (!strcmp("_start", name)) { + TEST_LOG("===> found _start"); + _root_symbols[_start] = (uintptr_t)start; + } else if (!strcmp("start_thread", name)) { + TEST_LOG("===> found start_thread"); + _root_symbols[start_thread] = (uintptr_t)start; + } else if (!strcmp("_ZL19thread_native_entryP6Thread", name)) { + TEST_LOG("===> found _ZL19thread_native_entryP6Thread"); + _root_symbols[_ZL19thread_native_entryP6Thread] = (uintptr_t)start; + } else if (!strcmp("_thread_start", name)) { + TEST_LOG("===> found _thread_start"); + _root_symbols[_thread_start] = (uintptr_t)start; + } else if (!strcmp("thread_start", name)) { + TEST_LOG("===> found thread_start"); + _root_symbols[thread_start] = (uintptr_t)start; + } +} + // Load symbols from /usr/lib/debug/.build-id/ab/cdef1234.debug, where // abcdef1234 is Build ID bool ElfParser::loadSymbolsUsingBuildId() { @@ -427,8 +449,7 @@ void ElfParser::loadSymbolTable(const char *symbols, size_t total_size, // Skip special AArch64 mapping symbols: $x and $d if (sym->st_size != 0 || sym->st_info != 0 || strings[sym->st_name] != '$') { - _cc->add(_base + sym->st_value, (int)sym->st_size, - strings + sym->st_name); + addSymbol(_base + sym->st_value, (int)sym->st_size, strings + sym->st_name); } } } @@ -458,7 +479,7 @@ void ElfParser::addRelocationSymbols(ElfSection *reltab, const char *plt) { name[sizeof(name) - 1] = 0; } - _cc->add(plt, PLT_ENTRY_SIZE, name); + addSymbol(plt, PLT_ENTRY_SIZE, name); plt += PLT_ENTRY_SIZE; } } @@ -612,6 +633,16 @@ void Symbols::parseLibraries(CodeCacheArray *array, bool kernel_symbols) { // concurrent loading and unloading of shared libraries. // Without it, we may access memory of a library that is being unloaded. dl_iterate_phdr(parseLibrariesCallback, array); + TEST_LOG("Parsed %d libraries", array->count()); +} + +bool Symbols::isRootSymbol(const void* address) { + for (int i = 0; i < LAST_ROOT_SYMBOL_KIND; i++) { + if (ElfParser::_root_symbols[i] == (uintptr_t)address) { + return true; + } + } + return false; } #endif // __linux__ diff --git a/ddprof-lib/src/main/cpp/symbols_linux.h b/ddprof-lib/src/main/cpp/symbols_linux.h index 270071c74..1be9721fd 100644 --- a/ddprof-lib/src/main/cpp/symbols_linux.h +++ b/ddprof-lib/src/main/cpp/symbols_linux.h @@ -122,8 +122,16 @@ static const bool MUSL = true; static const bool MUSL = false; #endif // __musl__ +enum RootSymbolKind { + _start, start_thread, _ZL19thread_native_entryP6Thread, _thread_start, thread_start, + LAST_ROOT_SYMBOL_KIND +}; + class ElfParser { +friend Symbols; private: + static uintptr_t _root_symbols[LAST_ROOT_SYMBOL_KIND]; + CodeCache *_cc; const char *_base; const char *_file_name; @@ -194,6 +202,8 @@ class ElfParser { const char *strings); void addRelocationSymbols(ElfSection *reltab, const char *plt); + void addSymbol(const void *start, int length, const char *name, bool update_bounds = false); + public: static void parseProgramHeaders(CodeCache *cc, const char *base, const char *end, bool relocate_dyn); diff --git a/ddprof-lib/src/main/cpp/symbols_macos.cpp b/ddprof-lib/src/main/cpp/symbols_macos.cpp index 3b465e50a..6b458f832 100644 --- a/ddprof-lib/src/main/cpp/symbols_macos.cpp +++ b/ddprof-lib/src/main/cpp/symbols_macos.cpp @@ -180,4 +180,9 @@ void Symbols::parseLibraries(CodeCacheArray *array, bool kernel_symbols) { } } +bool Symbols::isRootSymbol(const void* address) { + // no known 'always-root' symbols + return false; +} + #endif // __APPLE__ From 7a9a263b941ba0f26bd2749b7b95d035aac8251e Mon Sep 17 00:00:00 2001 From: Jaroslav Bachorik Date: Tue, 25 Mar 2025 13:18:08 -0400 Subject: [PATCH 08/11] Compensate for GH musl/x64 runner quirks --- .../java/com/datadoghq/profiler/AbstractProfilerTest.java | 2 +- .../java/com/datadoghq/profiler/junit/CStackInjector.java | 7 +++++++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/ddprof-test/src/test/java/com/datadoghq/profiler/AbstractProfilerTest.java b/ddprof-test/src/test/java/com/datadoghq/profiler/AbstractProfilerTest.java index b37ba6e3f..c00db08c0 100644 --- a/ddprof-test/src/test/java/com/datadoghq/profiler/AbstractProfilerTest.java +++ b/ddprof-test/src/test/java/com/datadoghq/profiler/AbstractProfilerTest.java @@ -208,7 +208,7 @@ protected void before() throws Exception { protected void after() throws Exception { } - protected final boolean isInCI() { + public static final boolean isInCI() { return Boolean.getBoolean("ddprof_test.ci"); } diff --git a/ddprof-test/src/test/java/com/datadoghq/profiler/junit/CStackInjector.java b/ddprof-test/src/test/java/com/datadoghq/profiler/junit/CStackInjector.java index e58702dc9..92331a80e 100644 --- a/ddprof-test/src/test/java/com/datadoghq/profiler/junit/CStackInjector.java +++ b/ddprof-test/src/test/java/com/datadoghq/profiler/junit/CStackInjector.java @@ -63,6 +63,13 @@ private static boolean isModeSafe(String mode) { if (Platform.isAarch64() && !Platform.isJavaVersionAtLeast(17)) { return mode.startsWith("vm"); } + if (AbstractProfilerTest.isInCI()) { + if (Platform.isMusl() && !Platform.isAarch64()) { + // our CI runner for musl on x64 is iffy and inexplicably locks up + // randomly when doing vm stackwalking + return !mode.startsWith("vm"); + } + } return true; } From c90f352a9bbe1798fecb1904493706bd034fb1b7 Mon Sep 17 00:00:00 2001 From: Jaroslav Bachorik Date: Tue, 25 Mar 2025 13:36:37 -0400 Subject: [PATCH 09/11] Emit 'skip_frames' error frame on java anchor fallback --- ddprof-lib/src/main/cpp/stackWalker.cpp | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/ddprof-lib/src/main/cpp/stackWalker.cpp b/ddprof-lib/src/main/cpp/stackWalker.cpp index 125a47f33..66fcd6b43 100644 --- a/ddprof-lib/src/main/cpp/stackWalker.cpp +++ b/ddprof-lib/src/main/cpp/stackWalker.cpp @@ -71,7 +71,10 @@ static inline void fillFrame(ASGCT_CallFrame &frame, FrameTypeId type, int bci, static inline void fillErrorFrame(ASGCT_CallFrame& frame, const char *name, bool *truncated) { fillFrame(frame, BCI_ERROR, name); *truncated = true; - TEST_LOG("[stackwalk-error] %s", name); +} + +static inline void fillSkipFrame(ASGCT_CallFrame& frame) { + fillFrame(frame, BCI_ERROR, "skipped_frames"); } static jmethodID getMethodId(VMMethod *method) { @@ -339,6 +342,7 @@ int StackWalker::walkVM(void* ucontext, ASGCT_CallFrame* frames, int max_depth, // End of Java stack break; } + fillSkipFrame(frames[depth++]); pc = anchor->lastJavaPC(); sp = anchor->lastJavaSP(); fp = anchor->lastJavaFP(); @@ -409,6 +413,7 @@ int StackWalker::walkVM(void* ucontext, ASGCT_CallFrame* frames, int max_depth, // End of Java stack break; } + fillSkipFrame(frames[depth++]); fp = anchor->lastJavaFP(); sp = anchor->lastJavaSP(); pc = anchor->lastJavaPC(); @@ -423,6 +428,7 @@ int StackWalker::walkVM(void* ucontext, ASGCT_CallFrame* frames, int max_depth, if (e_anchor == NULL) { if (anchor != NULL && !recovered_from_anchor && anchor->lastJavaSP() != 0) { TEST_LOG("Reusing thread anchor"); + fillSkipFrame(frames[depth++]); e_anchor = anchor; } else { fillErrorFrame(frames[depth++], "break_entry_frame: no anchor", truncated); @@ -527,6 +533,7 @@ int StackWalker::walkVM(void* ucontext, ASGCT_CallFrame* frames, int max_depth, // the garbled frame is leaf so there is no danger we 'loop' back to the top Java frame if (anchor && !recovered_from_anchor && anchor->lastJavaSP() != 0) { recovered_from_anchor = true; + fillSkipFrame(frames[depth++]); sp = anchor->lastJavaSP(); fp = anchor->lastJavaFP(); pc = anchor->lastJavaPC(); @@ -565,6 +572,7 @@ int StackWalker::walkVM(void* ucontext, ASGCT_CallFrame* frames, int max_depth, if (anchor && !recovered_from_anchor && !VMStructs::goodPtr((const void*)fp) && anchor->lastJavaSP() != 0) { recovered_from_anchor = true; + fillSkipFrame(frames[depth++]); sp = anchor->lastJavaSP(); fp = anchor->lastJavaFP(); pc = anchor->lastJavaPC(); From e8c04708e4d4d3ab5ae4a6fe771bf43274e181b1 Mon Sep 17 00:00:00 2001 From: Jaroslav Bachorik Date: Tue, 25 Mar 2025 14:37:23 -0400 Subject: [PATCH 10/11] Do not try to recover from invalid anchor frames --- ddprof-lib/src/main/cpp/stackWalker.cpp | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/ddprof-lib/src/main/cpp/stackWalker.cpp b/ddprof-lib/src/main/cpp/stackWalker.cpp index 66fcd6b43..0d93d1cbf 100644 --- a/ddprof-lib/src/main/cpp/stackWalker.cpp +++ b/ddprof-lib/src/main/cpp/stackWalker.cpp @@ -427,6 +427,10 @@ int StackWalker::walkVM(void* ucontext, ASGCT_CallFrame* frames, int max_depth, JavaFrameAnchor* e_anchor = JavaFrameAnchor::fromEntryFrame(fp); if (e_anchor == NULL) { if (anchor != NULL && !recovered_from_anchor && anchor->lastJavaSP() != 0) { + if (anchor->lastJavaPC() == nullptr || anchor->lastJavaSP() == 0) { + // end of Java stack + break; + } TEST_LOG("Reusing thread anchor"); fillSkipFrame(frames[depth++]); e_anchor = anchor; @@ -532,11 +536,16 @@ int StackWalker::walkVM(void* ucontext, ASGCT_CallFrame* frames, int max_depth, // Let's try to jump to the stored Java frame - // the garbled frame is leaf so there is no danger we 'loop' back to the top Java frame if (anchor && !recovered_from_anchor && anchor->lastJavaSP() != 0) { + if (anchor->lastJavaPC() == nullptr || anchor->lastJavaSP() == 0) { + // end of Java stack + break; + } recovered_from_anchor = true; fillSkipFrame(frames[depth++]); sp = anchor->lastJavaSP(); fp = anchor->lastJavaFP(); pc = anchor->lastJavaPC(); + continue; } } @@ -571,6 +580,10 @@ int StackWalker::walkVM(void* ucontext, ASGCT_CallFrame* frames, int max_depth, } if (anchor && !recovered_from_anchor && !VMStructs::goodPtr((const void*)fp) && anchor->lastJavaSP() != 0) { + if (anchor->lastJavaPC() == nullptr || anchor->lastJavaSP() == 0) { + // end of Java stack + break; + } recovered_from_anchor = true; fillSkipFrame(frames[depth++]); sp = anchor->lastJavaSP(); From e912ec8dc8a75a7fffe8f2be2daef1f6b5a9a299 Mon Sep 17 00:00:00 2001 From: Jaroslav Bachorik Date: Fri, 28 Mar 2025 15:50:26 -0400 Subject: [PATCH 11/11] Use X-enums for better code clarity --- ddprof-lib/src/main/cpp/symbols_linux.cpp | 22 +++++++------------- ddprof-lib/src/main/cpp/symbols_linux.h | 25 ++++++++++++++++++++--- 2 files changed, 29 insertions(+), 18 deletions(-) diff --git a/ddprof-lib/src/main/cpp/symbols_linux.cpp b/ddprof-lib/src/main/cpp/symbols_linux.cpp index fde22f499..11626fb94 100644 --- a/ddprof-lib/src/main/cpp/symbols_linux.cpp +++ b/ddprof-lib/src/main/cpp/symbols_linux.cpp @@ -281,6 +281,7 @@ void ElfParser::parseDwarfInfo() { instruction_t* ptr = (instruction_t*)blob->_start; instruction_t gcc_pattern = 0x910003fd; // mov x29, sp instruction_t clang_pattern = 0xfd7b01a9; // stp x29, x30, [sp, #16] + // first instruction may be noop so we are checking first 2 for the gcc pattern if (*(ptr + 1) == gcc_pattern || *(ptr + 2) == gcc_pattern) { *table = FrameDesc::default_frame; TEST_LOG("Found GCC pattern in code blob for %s, using gcc frame layout", _cc->name()); @@ -352,21 +353,12 @@ void ElfParser::loadSymbols(bool use_debug) { void ElfParser::addSymbol(const void *start, int length, const char *name, bool update_bounds) { _cc->add(start, length, name, update_bounds); - if (!strcmp("_start", name)) { - TEST_LOG("===> found _start"); - _root_symbols[_start] = (uintptr_t)start; - } else if (!strcmp("start_thread", name)) { - TEST_LOG("===> found start_thread"); - _root_symbols[start_thread] = (uintptr_t)start; - } else if (!strcmp("_ZL19thread_native_entryP6Thread", name)) { - TEST_LOG("===> found _ZL19thread_native_entryP6Thread"); - _root_symbols[_ZL19thread_native_entryP6Thread] = (uintptr_t)start; - } else if (!strcmp("_thread_start", name)) { - TEST_LOG("===> found _thread_start"); - _root_symbols[_thread_start] = (uintptr_t)start; - } else if (!strcmp("thread_start", name)) { - TEST_LOG("===> found thread_start"); - _root_symbols[thread_start] = (uintptr_t)start; + for (int i = 0; i < sizeof(root_symbol_table)/sizeof(root_symbol_table[0]); i++) { + if (!strcmp(root_symbol_table[i].name, name)) { + TEST_LOG("===> found %s", name); + _root_symbols[root_symbol_table[i].kind] = (uintptr_t)start; + break; + } } } diff --git a/ddprof-lib/src/main/cpp/symbols_linux.h b/ddprof-lib/src/main/cpp/symbols_linux.h index 1be9721fd..615348ab7 100644 --- a/ddprof-lib/src/main/cpp/symbols_linux.h +++ b/ddprof-lib/src/main/cpp/symbols_linux.h @@ -122,10 +122,29 @@ static const bool MUSL = true; static const bool MUSL = false; #endif // __musl__ -enum RootSymbolKind { - _start, start_thread, _ZL19thread_native_entryP6Thread, _thread_start, thread_start, - LAST_ROOT_SYMBOL_KIND +#define ROOT_SYMBOL_KIND(X) \ + X(_start, "_start") \ + X(start_thread, "start_thread") \ + X(_ZL19thread_native_entryP6Thread, "_ZL19thread_native_entryP6Thread") \ + X(_thread_start, "_thread_start") \ + X(thread_start, "thread_start") + +#define X_ENUM(a, b) a, +typedef enum RootSymbolKind : int { + ROOT_SYMBOL_KIND(X_ENUM) LAST_ROOT_SYMBOL_KIND +} RootSymbolKind; +#undef X_ENUM + +typedef struct { + const char* name; + RootSymbolKind kind; +} RootSymbolEntry; + +#define X_ENTRY(a, b) { b, a }, +static const RootSymbolEntry root_symbol_table[] = { + ROOT_SYMBOL_KIND(X_ENTRY) }; +#undef X_ENTRY class ElfParser { friend Symbols;