[fault-injection] Profiler::recordSample() stack walk is unprotected - #687
Draft
zhengyu123 wants to merge 33 commits into
Draft
[fault-injection] Profiler::recordSample() stack walk is unprotected#687zhengyu123 wants to merge 33 commits into
zhengyu123 wants to merge 33 commits into
Conversation
zhengyu123
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors and extends crash-protection for native stack walking by moving fault recovery from HotspotSupport::checkFault() into Profiler::checkFault(), adding an outer sigsetjmp/siglongjmp protection region around Profiler::recordSample()’s unwind path, and updating fault-injection tests/counters accordingly.
Changes:
- Introduces
Profiler::checkFault(ProfiledThread*, siginfo_t*, void*)and wires it intoProfiler::crashHandlerInternal(), removing the old HotSpot-specificcheckFault. - Adds an outer crash-protection region in
Profiler::recordSample()to recover from faults in “unprotected” metadata reads around stack walking. - Renames the longjmp recovery counter and expands fault-injection tests to validate chaining/restoration behavior.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| ddprof-lib/src/test/cpp/hotspot_crash_protection_ut.cpp | Switches the guard-clause test to call Profiler::checkFault() instead of the removed HotSpot helper. |
| ddprof-lib/src/test/cpp/faultInjection_ut.cpp | Updates fault injection signal wrapper to use Profiler::checkFault() and adds a new regression test for recordSample’s outer setjmp/restore protocol. |
| ddprof-lib/src/main/cpp/vmEntry.cpp | Adds an assertion ensuring the JVM library lookup succeeded before publishing _libjvm. |
| ddprof-lib/src/main/cpp/threadLocalData.h | Changes the protected-jump context type stored in TLS from jmp_buf* to sigjmp_buf*. |
| ddprof-lib/src/main/cpp/profiler.h | Declares Profiler::checkFault(ProfiledThread*, siginfo_t*, void*). |
| ddprof-lib/src/main/cpp/profiler.cpp | Adds outer unwind crash protection in recordSample(), computes profiler address range at signal-handler setup, and implements Profiler::checkFault(). |
| ddprof-lib/src/main/cpp/hotspot/hotspotSupport.h | Removes the HotspotSupport::checkFault() declaration. |
| ddprof-lib/src/main/cpp/hotspot/hotspotSupport.cpp | Switches walkVM crash protection to sigsetjmp/sigjmp_buf and removes the old HotSpot-specific checkFault() implementation. |
| ddprof-lib/src/main/cpp/counters.h | Renames the recovery counter from WALKVM_LONGJMP_RECOVERED to STACKWALK_LONGJMP_RECOVERED. |
Comments suppressed due to low confidence (2)
ddprof-lib/src/test/cpp/faultInjection_ut.cpp:64
- Profiler::checkFault increments Counters from the signal handler; without pre-initializing Counters in SetUp(), the first injected fault can trigger Counters' lazy aligned_alloc/memset from inside the signal path, which is not async-signal-safe.
Profiler::checkFault(ProfiledThread::current()); // longjmp if protected
ddprof-lib/src/test/cpp/faultInjection_ut.cpp:236
- This test uses setjmp/jmp_buf but the recovery path uses siglongjmp (via Profiler::checkFault). Mixing setjmp with siglongjmp is undefined; use sigsetjmp/sigjmp_buf consistently.
jmp_buf unwind_ctx;
jmp_buf* prev_jmp_buf = t->getJmpCtx();
ASSERT_EQ(&grandparent_ctx, prev_jmp_buf);
t->setFiRng(0xC0FFEEC0FFEEC0FFULL);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
Scan-Build Report
Bug Summary
Reports
|
||||||||||||||||||||||||||||||||||||
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (4)
ddprof-lib/src/test/cpp/faultInjection_ut.cpp:214
ProfiledThread::setJmpCtxnow usessigjmp_buf*, so the test's outer context needs to besigjmp_bufas well (otherwise this won't compile).
jmp_buf grandparent_ctx;
t->setJmpCtx(&grandparent_ctx);
ddprof-lib/src/test/cpp/faultInjection_ut.cpp:238
- This test still uses
jmp_buf/setjmp, but the recovery path usessiglongjmpviaProfiler::checkFault. Usesigjmp_buf+sigsetjmp(..., 1)so the types and semantics match.
jmp_buf unwind_ctx;
jmp_buf* prev_jmp_buf = t->getJmpCtx();
ASSERT_EQ(&grandparent_ctx, prev_jmp_buf);
t->setFiRng(0xC0FFEEC0FFEEC0FFULL);
int jmp_rc = setjmp(unwind_ctx);
ddprof-lib/src/main/cpp/threadLocalData.h:66
- The comment above
_jmp_bufsays it's "Used by hotspot only", but the newProfiler::recordSample()crash-protection also installs a jump context. Updating the comment helps keep the contract accurate for future maintainers.
std::atomic<sigjmp_buf*> _jmp_buf;
ddprof-lib/src/test/cpp/faultInjection_ut.cpp:172
- This test uses
Profiler::checkFault()(via the installed signal handler), which now recovers viasiglongjmp, but the unwind context installed withProfiledThread::setJmpCtxis still ajmp_bufcreated withsetjmp. This mismatch will not compile after switchingProfiledThreadtosigjmp_buf*and should be updated tosigjmp_buf+sigsetjmp(..., 1).
for (int i = 0; i < 5000 && faults == 0; i++) {
// Raw deref of the (possibly poisoned) base — mirrors walkVM's raw reads.
uintptr_t v = *(uintptr_t*)INJECT_FAULT_ADDRESS_LIKELY(base);
// Optimization barrier: tell the compiler `v` is read/write and clobber memory to prevent
// reordering/optimizing away the load.
Contributor
CI Test ResultsRun: #30412258408 | Commit:
Status Overview
Legend: ✅ passed | ❌ failed | ⚪ skipped | 🚫 cancelled Summary: Total: 32 | Passed: 32 | Failed: 0 Updated: 2026-07-29 01:07:43 UTC |
Contributor
Benchmark Results (commit bbed591)Pipeline: https://gitlab.ddbuild.io/DataDog/apm-reliability/benchmarking-platform/-/pipelines/126986264 Commit: ✅ Within expected boundariesNo significant runtime deltas (all within run-to-run noise) and no internal-counter outliers. Runtime details (per benchmark × JDK)
Internal counter details (ddprof)ddprof internal counters, latest / dev (✅ = 0, · = unavailable):
|
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (3)
ddprof-lib/src/main/cpp/profiler.cpp:688
- The
sigsetjmpresult is assigned twice (int jmp_rc = jmp_rc = ...), which is almost certainly a typo and can trigger warnings or confusion. It should be a single assignment.
sigjmp_buf unwind_ctx;
sigjmp_buf *prev_jmp_buf = walk_thread->getJmpCtx();
int jmp_rc = jmp_rc = sigsetjmp(unwind_ctx, 1);
if (jmp_rc != 0) {
ddprof-lib/src/test/cpp/faultInjection_ut.cpp:215
- This test sets a
jmp_bufas the thread’s jump context, butProfiledThread::setJmpCtxnow expects asigjmp_buf*. Usingjmp_bufhere will fail to compile (and is inconsistent withProfiler::checkFaultusingsiglongjmp).
// Simulate a pre-existing outer protection context, as recordSample must
// support when nested inside another sampler's own protected region.
jmp_buf grandparent_ctx;
t->setJmpCtx(&grandparent_ctx);
ddprof-lib/src/test/cpp/faultInjection_ut.cpp:239
- The unwind context in this test uses
jmp_buf/setjmp, but the signal handler recovery path usessiglongjmp.siglongjmpmust return to asigsetjmpsite, so this should usesigjmp_buf+sigsetjmpto avoid undefined behavior.
jmp_buf unwind_ctx;
jmp_buf* prev_jmp_buf = t->getJmpCtx();
ASSERT_EQ(&grandparent_ctx, prev_jmp_buf);
t->setFiRng(0xC0FFEEC0FFEEC0FFULL);
int jmp_rc = setjmp(unwind_ctx);
if (jmp_rc != 0) {
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (4)
ddprof-lib/src/test/cpp/faultInjection_ut.cpp:64
Profiler::checkFaultis a private method, so calling it directly from this test helper will not compile. Use the existingfriend class ProfilerTestAccessorhook to expose a small test-only wrapper.
Profiler::checkFault(ProfiledThread::current(), siginfo, context); // siglongjmp if protected
ddprof-lib/src/test/cpp/faultInjection_ut.cpp:233
- This test mixes
jmp_bufwithsigsetjmp(), and also usesjmp_buf*whereProfiledThread::getJmpCtx()now returnssigjmp_buf*. This will fail to compile; usesigjmp_bufconsistently.
jmp_buf unwind_ctx;
jmp_buf* prev_jmp_buf = t->getJmpCtx();
ASSERT_EQ(&grandparent_ctx, prev_jmp_buf);
ddprof-lib/src/test/cpp/hotspot_crash_protection_ut.cpp:337
Profiler::checkFaultis private, so this direct call from the test will not compile. Add a localProfilerTestAccessor(friend ofProfiler) and call through it, as done in other gtests (e.g.,jvmSupport_ut.cpp).
Profiler::checkFault(nullptr, nullptr, nullptr); // must not crash
ddprof-lib/src/test/cpp/hotspot_crash_protection_ut.cpp:310
- This comment says "longjmp" but the code below uses
siglongjmp(). Update the wording to match the mechanism being exercised.
// Simulate checkFault(): longjmp through whatever is currently
// installed — this must hit the inner frame, not the outer.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (4)
ddprof-lib/src/test/cpp/faultInjection_ut.cpp:233
unwind_ctxandprev_jmp_bufare declared asjmp_buf, but this test usessigsetjmp/siglongjmpandProfiledThread::setJmpCtxnow storessigjmp_buf*. As written, this will not compile (type mismatch) and also doesn’t match the behavior being tested.
jmp_buf unwind_ctx;
jmp_buf* prev_jmp_buf = t->getJmpCtx();
ASSERT_EQ(&grandparent_ctx, prev_jmp_buf);
ddprof-lib/src/main/cpp/profiler.cpp:1118
profiler_min_address/profiler_max_addressarestd::atomic<uintptr_t>, butCodeCache::minAddress()/maxAddress()return pointers. Assigning a pointer to an integer atomic without an explicit cast will not compile in C++.
CodeCache* prof_lib = libs->findLibraryByAddress((const void*)&Profiler::setupSignalHandlers);
if (prof_lib != nullptr) {
profiler_min_address = prof_lib->minAddress();
profiler_max_address = prof_lib->maxAddress();
}
ddprof-lib/src/test/cpp/hotspot_crash_protection_ut.cpp:17
- The header comment was partially updated to sigsetjmp/siglongjmp, but steps 1–2 still describe
jmp_buf*andlongjmp(). This is now inaccurate and can mislead future readers when debugging crash-protection behavior.
* Crash recovery inside walkVM relies on sigsetjmp/siglongjmp:
* 1. walkVM stores a jmp_buf* on ProfiledThread (setJmpCtx/getJmpCtx),
* chaining it with whatever context was already installed so a
* signal-based sampler interrupting a non-signal-based sampler's own
* in-flight walkVM() call doesn't clobber the outer call's context.
ddprof-lib/src/test/cpp/hotspot_crash_protection_ut.cpp:286
- This comment describes the fault path as "longjmps", but the implementation and test now use sigsetjmp/siglongjmp. Keeping terminology consistent here helps avoid confusion when debugging jump-buffer chaining.
// End-to-end with real sigsetjmp/siglongjmp: a fault inside the inner frame must
// land in the inner frame's own recovery branch — checkFault() always
// longjmps through whatever is currently installed — and once the inner
// frame has recovered and restored the outer's context, the outer frame must
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (5)
ddprof-lib/src/test/cpp/faultInjection_ut.cpp:233
unwind_ctx/prev_jmp_bufare still declared asjmp_bufhere, butsigsetjmp()requires asigjmp_buf, andProfiledThread::getJmpCtx()now returnssigjmp_buf*. As written this should not compile (type mismatch) and also defeats the intent of testingsigsetjmpchaining.
jmp_buf unwind_ctx;
jmp_buf* prev_jmp_buf = t->getJmpCtx();
ASSERT_EQ(&grandparent_ctx, prev_jmp_buf);
ddprof-lib/src/main/cpp/profiler.cpp:2040
siginfois unused, butucontextis used below. The current(void)ucontext;line is therefore misleading and does not suppress the actual unused-parameter warning forsiginfo(which may be fatal under -Werror).
(void)ucontext;
ddprof-lib/src/test/cpp/hotspot_crash_protection_ut.cpp:310
- This comment says
longjmp, but the test usessiglongjmp(and the production code now uses sigsetjmp/siglongjmp). Keeping terminology consistent makes the test easier to follow.
// Simulate checkFault(): longjmp through whatever is currently
// installed — this must hit the inner frame, not the outer.
ddprof-lib/src/main/cpp/profiler.cpp:704
- This comment is inconsistent with the actual recovery path: on a siglongjmp landing the code explicitly sets
truncated = true. The current wording (“outertruncatedstays false on the recovery path”) is misleading for future maintenance.
// truncated_local is never read after a siglongjmp landing (only on the
// clean path below), so it need not be volatile; the outer `truncated`
// stays false on the recovery path.
bool truncated_local = false;
ddprof-lib/src/main/cpp/profiler.h:502
- Removing the DEBUG crash injection from
findLibraryByAddress()makes theDDPROF_FORCE_STACKWALK_CRASHintegration test (vmStackwalkerCrashRecoveryTest) ineffective and leavesforce_stackwalk_crash_envunused. Also, with the new address-range filter inProfiler::checkFault,raise(SIGSEGV)would no longer be recovered because the faulting PC is in libc — the forced crash needs to fault inside the profiler binary to remain recoverable.
// Keep backward compatibility with the upstream async-profiler
inline CodeCache* findLibraryByAddress(const void *address) {
return Libraries::instance()->findLibraryByAddress(address);
}
Contributor
Reliability & Chaos Results✅ All reliability & chaos checks passed Pipeline: https://gitlab.ddbuild.io/DataDog/java-profiler/-/pipelines/127560367 |
Contributor
Benchmark Results (commit 2b47e15)Pipeline: https://gitlab.ddbuild.io/DataDog/apm-reliability/benchmarking-platform/-/pipelines/127560407 Commit:
|
| Benchmark | JDK | Latest | Dev | Δ (dev vs latest) | Issues L/D |
|---|---|---|---|---|---|
| akka-uct | 21 | ✅ 10277 ms (21 iters) | ✅ 10241 ms (21 iters) | ≈ -0.4% (±11.2%) | — / — |
| akka-uct | 25 | ✅ 8936 ms (24 iters) | ✅ 8808 ms (24 iters) | ≈ -1.4% (±10.5%) | — / — |
| finagle-chirper | 21 | ✅ 5821 ms (33 iters) | ✅ 5847 ms (33 iters) | ≈ +0.4% (±24.6%) | |
| finagle-chirper | 25 | ✅ 5482 ms (36 iters) | ✅ 5463 ms (36 iters) | ≈ -0.3% (±24.7%) | |
| fj-kmeans | 21 | ✅ 2692 ms (70 iters) | ✅ 2638 ms (72 iters) | ≈ -2% (±2.6%) | — / — |
| fj-kmeans | 25 | ✅ 2820 ms (66 iters) | ✅ 2816 ms (66 iters) | ≈ -0.1% (±2.6%) | — / — |
| future-genetic | 21 | ✅ 2074 ms (90 iters) | ✅ 2072 ms (89 iters) | ≈ -0.1% (±2.7%) | — / — |
| future-genetic | 25 | ✅ 2092 ms (88 iters) | ✅ 2000 ms (92 iters) | 🟢 -4.4% | — / — |
| naive-bayes | 21 | ✅ 1278 ms (134 iters) | ✅ 1334 ms (129 iters) | ≈ +4.4% (±33.1%) | — / — |
| naive-bayes | 25 | ✅ 1024 ms (167 iters) | ✅ 1020 ms (168 iters) | ≈ -0.4% (±31.8%) | — / — |
| reactors | 21 | ✅ 16555 ms (15 iters) | ✅ 16290 ms (15 iters) | ≈ -1.6% (±7.1%) | — / — |
| reactors | 25 | ✅ 18453 ms (15 iters) | ✅ 17956 ms (15 iters) | ≈ -2.7% (±5.2%) | — / — |
Internal counter details (ddprof)
ddprof internal counters, latest / dev (✅ = 0, · = unavailable):
| Benchmark | JDK | Dropped rec | Dropped jvmti | Dropped trace | Skipped WC | AGCT fail | Unwind fail |
|---|---|---|---|---|---|---|---|
| akka-uct | 21 | ✅ / ✅ | ✅ / ✅ | 3 / 3 | 1936 / 1980 | ✅ / ✅ | ✅ / ✅ |
| akka-uct | 25 | ✅ / ✅ | ✅ / ✅ | 3 / 1 | 2005 / 2048 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 21 | ✅ / ✅ | ✅ / ✅ | 6 / 4 | 8390 / 8399 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 25 | ✅ / ✅ | ✅ / ✅ | 8 / ✅ | 8318 / 8282 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 21 | ✅ / ✅ | ✅ / ✅ | 2 / 1 | 1269 / 1303 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 25 | ✅ / ✅ | ✅ / ✅ | 2 / 4 | 1298 / 1260 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 21 | ✅ / ✅ | ✅ / ✅ | 3 / 1 | 3042 / 2914 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 25 | ✅ / ✅ | ✅ / ✅ | 1 / 2 | 2913 / 2802 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 21 | ✅ / ✅ | ✅ / ✅ | 2 / 7 | 3552 / 3548 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 25 | ✅ / ✅ | ✅ / ✅ | 5 / 5 | 3511 / 3483 | ✅ / ✅ | ✅ / ✅ |
| reactors | 21 | ✅ / ✅ | ✅ / ✅ | 1 / 2 | 1809 / 1744 | ✅ / ✅ | ✅ / ✅ |
| reactors | 25 | ✅ / ✅ | ✅ / ✅ | 1 / 1 | 1889 / 1830 | ✅ / ✅ | ✅ / ✅ |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Bumps the codeql-action group with 2 updates: [github/codeql-action/init](https://github.com/github/codeql-action) and [github/codeql-action/analyze](https://github.com/github/codeql-action). Updates `github/codeql-action/init` from 4.37.1 to 4.37.3 - [Release notes](https://github.com/github/codeql-action/releases) - [Changelog](https://github.com/github/codeql-action/blob/main/CHANGELOG.md) - [Commits](github/codeql-action@7188fc3...e4fba86) Updates `github/codeql-action/analyze` from 4.37.1 to 4.37.3 - [Release notes](https://github.com/github/codeql-action/releases) - [Changelog](https://github.com/github/codeql-action/blob/main/CHANGELOG.md) - [Commits](github/codeql-action@7188fc3...e4fba86) --- updated-dependencies: - dependency-name: github/codeql-action/init dependency-version: 4.37.3 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: codeql-action - dependency-name: github/codeql-action/analyze dependency-version: 4.37.3 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: codeql-action ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (4)
ddprof-lib/src/test/cpp/faultInjection_ut.cpp:232
RecordSampleOuterSetjmpRecoversAndRestoresChainusesjmp_buf/jmp_buf*while the rest of the crash-protection path has been migrated tosigjmp_bufandProfiledThread::{set,get}JmpCtx()now usesigjmp_buf*. As written, this won’t type-check cleanly in C++ and also passes the wrong env type tosigsetjmp.
jmp_buf unwind_ctx;
jmp_buf* prev_jmp_buf = t->getJmpCtx();
ASSERT_EQ(&grandparent_ctx, prev_jmp_buf);
ddprof-lib/src/test/cpp/hotspot_crash_protection_ut.cpp:310
- This comment still refers to
longjmpeven though the code and surrounding tests have switched tosigsetjmp/siglongjmp, which can confuse readers about which API is being exercised.
// Simulate checkFault(): longjmp through whatever is currently
// installed — this must hit the inner frame, not the outer.
ddprof-lib/src/main/cpp/vmEntry.cpp:223
VM::openJvmLibrary()already has callers that handle anullptrreturn (e.g.VM::initVM(),VM::initProfilerBridge()). Turning a lookup failure into anassert()changes the function’s contract and can abort debug/asan builds in scenarios where returningnullptris the intended behavior.
// The library must have been loaded. Otherwise, we cannot get to here due
// to JVM initialization
assert(lib != nullptr && "JVM library must be loaded");
__atomic_store_n(&_libjvm, lib, __ATOMIC_RELEASE);
ddprof-lib/src/main/cpp/profiler.h:502
- The
DDPROF_FORCE_STACKWALK_CRASHdebug hook (force_stackwalk_crash_env) is still defined, but its only use was removed fromfindLibraryByAddress(). This leaves an unused header-level variable (and an extragetenv()per TU) in DEBUG builds. Either remove the hook entirely or keep using it here.
// Keep backward compatibility with the upstream async-profiler
inline CodeCache* findLibraryByAddress(const void *address) {
return Libraries::instance()->findLibraryByAddress(address);
}
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
ddprof-lib/src/main/cpp/profiler.cpp:1073
- setupSignalHandlers() dereferences prof_lib unconditionally after an assert. In release builds (NDEBUG) the assert is compiled out, so if findLibraryByAddress() returns nullptr this becomes a null-deref during initialization. Since Profiler::checkFault() already has a fallback for an uninitialized address range, prefer a real runtime null-check and keep the atomics at 0 when the library can't be resolved.
orig_segvHandler = OS::replaceSigsegvHandler(segvHandler);
orig_busHandler = OS::replaceSigbusHandler(busHandler);
// Patch sigaction GOT in libraries with broken signal handlers (already loaded)
LibraryPatcher::patch_sigaction();
}
ddprof-lib/src/main/cpp/profiler.cpp:2001
- Profiler::checkFault() currently suppresses ucontext usage with (void)ucontext but then uses it, and leaves siginfo unused (which can trip -Wunused-parameter under -Wextra/-Werror). Also, if checkFault() is ever called with a protected thread but a null ucontext (e.g., from future tests), StackFrame(ucontext) would crash. It’s safer and clearer to explicitly guard ucontext and silence/consume siginfo.
" CPU Engine : %s\n"
" WallClock Engine : %s\n"
" Allocations : %s\n",
state() == RUNNING ? "true" : "false",
_cpu_engine != nullptr ? _cpu_engine->name() : "None",
ddprof-lib/src/main/cpp/profiler.h:502
- The DEBUG-only DDPROF_FORCE_STACKWALK_CRASH hook was removed from findLibraryByAddress(), but the env var is still defined (force_stackwalk_crash_env) and is used by JavaProfilerTest.vmStackwalkerCrashRecoveryTest. With the hook removed, that test no longer actually forces a crash/recovery path and the env var becomes dead code (potential -Wunused-variable noise). Consider restoring the hook here so the integration test still exercises the recovery behavior.
// Keep backward compatibility with the upstream async-profiler
inline CodeCache* findLibraryByAddress(const void *address) {
return Libraries::instance()->findLibraryByAddress(address);
}
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (2)
ddprof-lib/src/main/cpp/profiler.cpp:2030
Profiler::checkFaultcurrently has a bogus(void)ucontext;(it then usesucontext), readsstd::atomicvalues via implicit casts, and has indentation inconsistent with the surrounding file. This is likely to trigger warnings (and potentially Werror) and makes the signal-path logic harder to audit.
void Profiler::checkFault(ProfiledThread* thrd, siginfo_t *siginfo, void *ucontext) {
(void)ucontext;
// Check if siglongjmp is setup for this thread
if (thrd == nullptr || !thrd->isProtected()) {
return;
ddprof-lib/src/main/cpp/profiler.h:502
- The debug-only stackwalk crash injection block was removed from
findLibraryByAddress, butforce_stackwalk_crash_envremains declared at the top of this header. That leaves astaticheader-level variable unused in every translation unit includingprofiler.h, which can trigger-Wunused-variable(often promoted to an error). Either remove/annotate the variable or keep the injection logic somewhere that uses it.
// Keep backward compatibility with the upstream async-profiler
inline CodeCache* findLibraryByAddress(const void *address) {
return Libraries::instance()->findLibraryByAddress(address);
}
Comment on lines
463
to
+466
| static void segvHandler(int signo, siginfo_t *siginfo, void *ucontext); | ||
| static void busHandler(int signo, siginfo_t *siginfo, void *ucontext); | ||
| static void setupSignalHandlers(); | ||
| static void checkFault(ProfiledThread* thrd, siginfo_t *siginfo, void *ucontext); |
Comment on lines
+74
to
+81
| class LongjmpProtectionLeaver { | ||
| private: | ||
| sigjmp_buf* _jmp_buf; | ||
| ProfiledThread* const _thread; | ||
| public: | ||
| LongjmpProtectionLeaver(ProfiledThread* const thread); | ||
| ~LongjmpProtectionLeaver(); | ||
| }; |
Contributor
Benchmark Results (commit a99b910)Pipeline: https://gitlab.ddbuild.io/DataDog/apm-reliability/benchmarking-platform/-/pipelines/127598468 Commit:
|
| Benchmark | JDK | Latest | Dev | Δ (dev vs latest) | Issues L/D |
|---|---|---|---|---|---|
| akka-uct | 21 | ✅ 10308 ms (21 iters) | ✅ 10443 ms (21 iters) | ≈ +1.3% (±12.1%) | — / — |
| finagle-chirper | 21 | ✅ 5990 ms (33 iters) | ✅ 5977 ms (33 iters) | ≈ -0.2% (±25.6%) | |
| finagle-chirper | 25 | ✅ 5446 ms (36 iters) | ✅ 5468 ms (36 iters) | ≈ +0.4% (±24.2%) | |
| fj-kmeans | 25 | ✅ 2768 ms (68 iters) | ✅ 2843 ms (66 iters) | ≈ +2.7% (±2.7%) | — / — |
| future-genetic | 21 | ✅ 2058 ms (90 iters) | ✅ 2171 ms (86 iters) | 🔴 +5.5% | — / — |
| future-genetic | 25 | ✅ 2081 ms (89 iters) | ✅ 2076 ms (89 iters) | ≈ -0.2% (±2.6%) | — / — |
| naive-bayes | 25 | ✅ 1021 ms (167 iters) | ✅ 1006 ms (170 iters) | ≈ -1.5% (±31.6%) | — / — |
| reactors | 21 | ✅ 16209 ms (15 iters) | ✅ 17213 ms (15 iters) | ≈ +6.2% (±7.1%) | — / — |
| reactors | 25 | ✅ 18399 ms (15 iters) | ✅ 18259 ms (15 iters) | ≈ -0.8% (±4.6%) | — / — |
Internal counter details (ddprof)
ddprof internal counters, latest / dev (✅ = 0, · = unavailable):
| Benchmark | JDK | Dropped rec | Dropped jvmti | Dropped trace | Skipped WC | AGCT fail | Unwind fail |
|---|---|---|---|---|---|---|---|
| akka-uct | 21 | ✅ / ✅ | ✅ / ✅ | 1 / 1 | 1973 / 1954 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 21 | ✅ / ✅ | ✅ / ✅ | 4 / 6 | 8179 / 8407 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 25 | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ | 8411 / 8261 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 21 | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 25 | ✅ / ✅ | ✅ / ✅ | 4 / 1 | 1287 / 1283 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 21 | ✅ / ✅ | ✅ / ✅ | 1 / ✅ | 2947 / 2974 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 25 | ✅ / ✅ | ✅ / ✅ | ✅ / 3 | 2850 / 2844 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 21 | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 25 | ✅ / ✅ | ✅ / ✅ | 1 / 2 | 3489 / 3507 | ✅ / ✅ | ✅ / ✅ |
| reactors | 21 | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ | 1602 / 1638 | ✅ / ✅ | ✅ / ✅ |
| reactors | 25 | ✅ / ✅ | ✅ / ✅ | 1 / ✅ | 1946 / 1849 | ✅ / ✅ | ✅ / ✅ |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?:
Motivation:
Additional Notes:
How to test the change?:
For Datadog employees:
credentials of any kind, I've requested a security review (run the
dd:platform-security-reviewskill, or file a request via the PSEC review form).
bewairealso runs automatically on every PR.Unsure? Have a question? Request a review!