[fault-injection] Profiler::recordSample() stack walk is unprotected#680
[fault-injection] Profiler::recordSample() stack walk is unprotected#680zhengyu123 wants to merge 49 commits into
Conversation
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>
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>
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>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
…iler into zgu/unprotected_vmflag
There was a problem hiding this comment.
Pull request overview
This PR hardens the native profiler’s initialization and stack-walking paths against startup-ordering issues (Agent_OnLoad) and fault/crash scenarios during sampling, and adds a regression test to prevent reintroducing the initialization regression.
Changes:
- Add setjmp/longjmp crash protection around
Profiler::recordSample()’s stack unwind to avoid unrecoverable faults during metadata reads. - Ensure HotSpot
VMStructsinitialization happens early enough for the-agentpath:(Agent_OnLoad) path by callingVM::ready()fromVM::initProfilerBridge(), and add debug test logging for the decision points. - Add a Java regression test that launches a child JVM with
-agentpath:and asserts theDebugNonSafepointsflag lookup path is taken (not the CompiledMethodLoad workaround).
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| ddprof-test/src/test/java/com/datadoghq/profiler/JVMAccessTest.java | Adds regression test covering Agent_OnLoad initialization ordering and VM flag detection. |
| ddprof-lib/src/main/java/com/datadoghq/profiler/LibraryLoader.java | Exposes a package-visible library-path resolver for tests to supply a real -agentpath: without loading in-process. |
| ddprof-lib/src/main/cpp/vmEntry.h | Reworks _libjvm handling to a CodeCache* with an accessor using acquire semantics. |
| ddprof-lib/src/main/cpp/vmEntry.cpp | Makes VM::ready() idempotently initialize VMStructs once; calls ready() from initProfilerBridge() for Agent_OnLoad; adds test logs. |
| ddprof-lib/src/main/cpp/profiler.cpp | Wraps native+Java stack walking in recordSample() with crash protection and records an error frame on recovery; switches debug-symbol query to VM::libjvm(). |
| ddprof-lib/src/main/cpp/hotspot/vmStructs.inline.h | Adds safe offset-load helpers and uses SafeAccess for VMFlag name reads; introduces fault-injection point for offset access. |
| ddprof-lib/src/main/cpp/hotspot/vmStructs.h | Moves offset accessors to inline definitions and adds a safe-load template; updates VMFlag::name() signature. |
| ddprof-lib/src/main/cpp/hotspot/vmStructs.cpp | Avoids repeated unsafe dereferences by caching VMFlag::name() into a local before strcmp. |
| ddprof-lib/src/main/cpp/hotspot/hotspotSupport.cpp | Adds a null-check for scope.method() and renames the recovered-longjmp counter to be stackwalk-generic. |
| ddprof-lib/src/main/cpp/flightRecorder.cpp | Switches debug-symbol setting emission to use VM::libjvm(). |
| ddprof-lib/src/main/cpp/counters.h | Renames WALKVM_LONGJMP_RECOVERED to STACKWALK_LONGJMP_RECOVERED. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
CI Test ResultsRun: #30093470859 | Commit:
Status Overview
Legend: ✅ passed | ❌ failed | ⚪ skipped | 🚫 cancelled Failed Testsglibc-aarch64/debug / 8-j9Job: View logs No detailed failure information available. Check the job logs. glibc-aarch64/debug / 11-j9Job: View logs No detailed failure information available. Check the job logs. glibc-aarch64/debug / 17-j9Job: View logs No detailed failure information available. Check the job logs. glibc-amd64/debug / 8-j9Job: View logs No detailed failure information available. Check the job logs. glibc-amd64/debug / 11-j9Job: View logs No detailed failure information available. Check the job logs. glibc-amd64/debug / 8-ibmJob: View logs No detailed failure information available. Check the job logs. glibc-amd64/debug / 17-j9Job: View logs No detailed failure information available. Check the job logs. Summary: Total: 32 | Passed: 25 | Failed: 7 Updated: 2026-07-24 13:04:32 UTC |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Benchmark Results (commit 86238c0)Pipeline: https://gitlab.ddbuild.io/DataDog/apm-reliability/benchmarking-platform/-/pipelines/126639928 Commit:
|
| Benchmark | JDK | Latest | Dev | Δ (dev vs latest) | Issues L/D |
|---|---|---|---|---|---|
| akka-uct | 21 | ✅ 10301 ms (21 iters) | ✅ 10332 ms (21 iters) | ≈ +0.3% (±11.6%) | — / — |
| akka-uct | 25 | ✅ 8814 ms (24 iters) | ✅ 8767 ms (24 iters) | ≈ -0.5% (±10.1%) | — / — |
| finagle-chirper | 21 | ✅ 5965 ms (33 iters) | ✅ 5977 ms (33 iters) | ≈ +0.2% (±25.4%) | |
| finagle-chirper | 25 | ✅ 5497 ms (36 iters) | ✅ 5398 ms (36 iters) | ≈ -1.8% (±24%) | |
| fj-kmeans | 21 | ✅ 2650 ms (71 iters) | ✅ 2763 ms (68 iters) | 🔴 +4.3% | — / — |
| fj-kmeans | 25 | ✅ 2827 ms (66 iters) | ✅ 2812 ms (66 iters) | ≈ -0.5% (±2.6%) | — / — |
| future-genetic | 21 | ✅ 2048 ms (90 iters) | ✅ 2071 ms (89 iters) | ≈ +1.1% (±2.6%) | — / — |
| future-genetic | 25 | ✅ 2098 ms (89 iters) | ✅ 2128 ms (87 iters) | ≈ +1.4% (±2.5%) | — / — |
| naive-bayes | 21 | ✅ 1266 ms (135 iters) | ✅ 1237 ms (139 iters) | ≈ -2.3% (±31.9%) | — / — |
| naive-bayes | 25 | ✅ 1018 ms (168 iters) | ✅ 1008 ms (170 iters) | ≈ -1% (±31.3%) | — / — |
| reactors | 21 | ✅ 16486 ms (15 iters) | ✅ 16402 ms (15 iters) | ≈ -0.5% (±7.8%) | — / — |
| reactors | 25 | ✅ 18983 ms (15 iters) | ✅ 18298 ms (15 iters) | ≈ -3.6% (±4.1%) | — / — |
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 | ✅ / ✅ | ✅ / ✅ | 2 / 2 | 2062 / 1958 | ✅ / ✅ | ✅ / ✅ |
| akka-uct | 25 | ✅ / ✅ | ✅ / ✅ | 1 / 2 | 2047 / 2212 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 21 | ✅ / ✅ | ✅ / ✅ | 4 / 2 | 7979 / 8465 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 25 | ✅ / ✅ | ✅ / ✅ | 2 / ✅ | 8174 / 8614 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 21 | ✅ / ✅ | ✅ / ✅ | 4 / 3 | 1292 / 1267 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 25 | ✅ / ✅ | ✅ / ✅ | 2 / 1 | 1254 / 1267 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 21 | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ | 2964 / 2905 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 25 | ✅ / ✅ | ✅ / ✅ | 3 / 4 | 2947 / 2881 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 21 | ✅ / ✅ | ✅ / ✅ | 4 / ✅ | 3535 / 3561 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 25 | ✅ / ✅ | ✅ / ✅ | 2 / 1 | 3499 / 3501 | ✅ / ✅ | ✅ / ✅ |
| reactors | 21 | ✅ / ✅ | ✅ / ✅ | ✅ / 3 | 1704 / 1657 | ✅ / ✅ | ✅ / ✅ |
| reactors | 25 | ✅ / ✅ | ✅ / ✅ | ✅ / 3 | 1993 / 1778 | ✅ / ✅ | ✅ / ✅ |
Benchmark Results (commit 490b055)Pipeline: https://gitlab.ddbuild.io/DataDog/apm-reliability/benchmarking-platform/-/pipelines/126673593 Commit:
|
| Benchmark | JDK | Latest | Dev | Δ (dev vs latest) | Issues L/D |
|---|---|---|---|---|---|
| akka-uct | 21 | ✅ 10354 ms (21 iters) | ✅ 10192 ms (21 iters) | ≈ -1.6% (±11.2%) | — / — |
| akka-uct | 25 | ✅ 8881 ms (24 iters) | ✅ 8835 ms (24 iters) | ≈ -0.5% (±9.9%) | — / — |
| finagle-chirper | 21 | ✅ 6005 ms (33 iters) | ✅ 5958 ms (33 iters) | ≈ -0.8% (±24.7%) | |
| finagle-chirper | 25 | ✅ 5462 ms (36 iters) | ✅ 5459 ms (36 iters) | ≈ -0.1% (±24.2%) | |
| fj-kmeans | 21 | ✅ 2768 ms (68 iters) | ✅ 2659 ms (70 iters) | 🟢 -3.9% | — / — |
| fj-kmeans | 25 | ✅ 2827 ms (66 iters) | ✅ 2842 ms (66 iters) | ≈ +0.5% (±2.6%) | — / — |
| future-genetic | 21 | ✅ 2049 ms (91 iters) | ✅ 2115 ms (87 iters) | 🔴 +3.2% | — / — |
| future-genetic | 25 | ✅ 2049 ms (90 iters) | ✅ 2109 ms (88 iters) | 🔴 +2.9% | — / — |
| naive-bayes | 21 | ✅ 1263 ms (136 iters) | ✅ 1255 ms (136 iters) | ≈ -0.6% (±32.5%) | — / — |
| naive-bayes | 25 | ✅ 1019 ms (168 iters) | ✅ 1019 ms (168 iters) | ≈ 0% (±31.7%) | — / — |
| reactors | 21 | ✅ 16762 ms (15 iters) | ✅ 17039 ms (15 iters) | ≈ +1.7% (±7.9%) | — / — |
| reactors | 25 | ✅ 18327 ms (15 iters) | ✅ 18649 ms (15 iters) | ≈ +1.8% (±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 | 1960 / 2024 | ✅ / ✅ | ✅ / ✅ |
| akka-uct | 25 | ✅ / ✅ | ✅ / ✅ | 3 / 1 | 2257 / 2267 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 21 | ✅ / ✅ | ✅ / ✅ | 4 / 4 | 8586 / 8261 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 25 | ✅ / ✅ | ✅ / ✅ | 1 / 2 | 8451 / 8698 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 21 | ✅ / ✅ | ✅ / ✅ | ✅ / 1 | 1282 / 1249 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 25 | ✅ / ✅ | ✅ / ✅ | 4 / 1 | 1305 / 1277 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 21 | ✅ / ✅ | ✅ / ✅ | ✅ / 2 | 3012 / 2866 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 25 | ✅ / ✅ | ✅ / ✅ | 3 / 2 | 2771 / 2856 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 21 | ✅ / ✅ | ✅ / ✅ | 3 / 8 | 3585 / 3500 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 25 | ✅ / ✅ | ✅ / ✅ | 2 / 1 | 3525 / 3516 | ✅ / ✅ | ✅ / ✅ |
| reactors | 21 | ✅ / ✅ | ✅ / ✅ | 1 / 1 | 1752 / 1584 | ✅ / ✅ | ✅ / ✅ |
| reactors | 25 | ✅ / ✅ | ✅ / ✅ | 3 / ✅ | 1862 / 1859 | ✅ / ✅ | ✅ / ✅ |
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 (1)
ddprof-lib/src/main/cpp/vmEntry.cpp:581
- This header comment says the “VMStructs init below only ever runs once”, but
VM::ready()is called from initLibrary(), initProfilerBridge(), and the VMInit callback, and there is no guard aroundVMStructs::init(lib)/VMStructs::ready(). Either add a guard, or adjust the comment so it doesn’t claim single execution.
// Run late initialization when JVM is ready. May be called more than once (from
// initProfilerBridge() directly, and later from the VMInit JVMTI callback, or from
// initLibrary() followed by a JNI-triggered attach) -- the VMStructs init below only
// ever runs once.
Benchmark Results (commit 90fadfe)Pipeline: https://gitlab.ddbuild.io/DataDog/apm-reliability/benchmarking-platform/-/pipelines/126772742 Commit:
|
| Benchmark | JDK | Latest | Dev | Δ (dev vs latest) | Issues L/D |
|---|---|---|---|---|---|
| akka-uct | 21 | ✅ 10324 ms (21 iters) | ✅ 10431 ms (21 iters) | ≈ +1% (±11.8%) | — / — |
| akka-uct | 25 | ✅ 8786 ms (24 iters) | ✅ 8931 ms (24 iters) | ≈ +1.7% (±9.9%) | — / — |
| finagle-chirper | 21 | ✅ 6030 ms (33 iters) | ✅ 6016 ms (33 iters) | ≈ -0.2% (±26.1%) | |
| finagle-chirper | 25 | ✅ 5449 ms (36 iters) | ✅ 5448 ms (36 iters) | ≈ -0% (±24.6%) | |
| fj-kmeans | 21 | ✅ 2751 ms (68 iters) | ✅ 2683 ms (70 iters) | ≈ -2.5% (±2.7%) | — / — |
| fj-kmeans | 25 | ✅ 2842 ms (66 iters) | ✅ 2814 ms (66 iters) | ≈ -1% (±2.5%) | — / — |
| future-genetic | 21 | ✅ 2076 ms (89 iters) | ✅ 2101 ms (88 iters) | ≈ +1.2% (±2.7%) | — / — |
| future-genetic | 25 | ✅ 2077 ms (89 iters) | ✅ 2011 ms (93 iters) | 🟢 -3.2% | — / — |
| naive-bayes | 21 | ✅ 1264 ms (135 iters) | ✅ 1280 ms (133 iters) | ≈ +1.3% (±32.7%) | — / — |
| naive-bayes | 25 | ✅ 1006 ms (169 iters) | ✅ 976 ms (174 iters) | ≈ -3% (±31.7%) | — / — |
| reactors | 21 | ✅ 16957 ms (15 iters) | ✅ 16446 ms (15 iters) | ≈ -3% (±6.6%) | — / — |
| reactors | 25 | ✅ 18310 ms (15 iters) | ✅ 18574 ms (15 iters) | ≈ +1.4% (±5%) | — / — |
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 / ✅ | 2007 / 1989 | ✅ / ✅ | ✅ / ✅ |
| akka-uct | 25 | ✅ / ✅ | ✅ / ✅ | 1 / 2 | 2249 / 2195 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 21 | ✅ / ✅ | ✅ / ✅ | 6 / 2 | 8633 / 8540 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 25 | ✅ / ✅ | ✅ / ✅ | 1 / ✅ | 8691 / 8542 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 21 | ✅ / ✅ | ✅ / ✅ | 3 / ✅ | 1270 / 1287 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 25 | ✅ / ✅ | ✅ / ✅ | 3 / 2 | 1281 / 1276 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 21 | ✅ / ✅ | ✅ / ✅ | 3 / 3 | 2939 / 2945 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 25 | ✅ / ✅ | ✅ / ✅ | 1 / ✅ | 2878 / 2858 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 21 | ✅ / ✅ | ✅ / ✅ | ✅ / 6 | 3515 / 3506 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 25 | ✅ / ✅ | ✅ / ✅ | 5 / 3 | 3447 / 3501 | ✅ / ✅ | ✅ / ✅ |
| reactors | 21 | ✅ / ✅ | ✅ / ✅ | ✅ / 1 | 1709 / 1603 | ✅ / ✅ | ✅ / ✅ |
| reactors | 25 | ✅ / ✅ | ✅ / ✅ | 1 / ✅ | 1936 / 1940 | ✅ / ✅ | ✅ / ✅ |
| static void fi_signal_wrapper(int signo, siginfo_t* siginfo, void* context) { | ||
| if (SafeAccess::handle_safefetch(signo, context)) { | ||
| return; // safefetch load recovered; PC already rewritten to _cont. | ||
| } | ||
| HotspotSupport::checkFault(ProfiledThread::current()); // longjmp if protected | ||
| Profiler::checkFault(ProfiledThread::current()); // longjmp if protected | ||
| // Not protected and not a safefetch fault — real crash. |
Benchmark Results (commit e3556f8)Pipeline: https://gitlab.ddbuild.io/DataDog/apm-reliability/benchmarking-platform/-/pipelines/126938656 Commit:
|
| Benchmark | JDK | Latest | Dev | Δ (dev vs latest) | Issues L/D |
|---|---|---|---|---|---|
| akka-uct | 21 | ✅ 10324 ms (21 iters) | ✅ 10273 ms (21 iters) | ≈ -0.5% (±11.3%) | — / — |
| akka-uct | 25 | ✅ 8845 ms (24 iters) | ✅ 8832 ms (24 iters) | ≈ -0.1% (±9.8%) | — / — |
| finagle-chirper | 21 | ✅ 6012 ms (33 iters) | ✅ 5977 ms (33 iters) | ≈ -0.6% (±25%) | |
| finagle-chirper | 25 | ✅ 5418 ms (36 iters) | ✅ 5432 ms (36 iters) | ≈ +0.3% (±24.1%) | |
| fj-kmeans | 21 | ✅ 2733 ms (68 iters) | ✅ 2729 ms (68 iters) | ≈ -0.1% (±2.7%) | — / — |
| fj-kmeans | 25 | ✅ 2835 ms (66 iters) | ✅ 2829 ms (66 iters) | ≈ -0.2% (±2.6%) | — / — |
| future-genetic | 21 | ✅ 2062 ms (91 iters) | ✅ 2041 ms (90 iters) | ≈ -1% (±2.5%) | — / — |
| future-genetic | 25 | ✅ 2111 ms (88 iters) | ✅ 2045 ms (91 iters) | 🟢 -3.1% | — / — |
| naive-bayes | 21 | ✅ 1279 ms (134 iters) | ✅ 1232 ms (138 iters) | ≈ -3.7% (±32.3%) | — / — |
| naive-bayes | 25 | ✅ 1007 ms (170 iters) | ✅ 1012 ms (169 iters) | ≈ +0.5% (±31.6%) | — / — |
| reactors | 21 | ✅ 15868 ms (15 iters) | ✅ 16735 ms (15 iters) | ≈ +5.5% (±8.4%) | — / — |
| reactors | 25 | ✅ 18780 ms (15 iters) | ✅ 18600 ms (15 iters) | ≈ -1% (±3.5%) | — / — |
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 | ✅ / ✅ | ✅ / ✅ | 2 / 3 | 1966 / 2035 | ✅ / ✅ | ✅ / ✅ |
| akka-uct | 25 | ✅ / ✅ | ✅ / ✅ | 1 / ✅ | 2136 / 2349 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 21 | ✅ / ✅ | ✅ / ✅ | 1 / 3 | 8760 / 8482 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 25 | ✅ / ✅ | ✅ / ✅ | 4 / 1 | 8820 / 8102 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 21 | ✅ / ✅ | ✅ / ✅ | ✅ / 4 | 1241 / 1269 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 25 | ✅ / ✅ | ✅ / ✅ | 3 / 1 | 1258 / 1254 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 21 | ✅ / ✅ | ✅ / ✅ | ✅ / 1 | 2981 / 2900 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 25 | ✅ / ✅ | ✅ / ✅ | 2 / 1 | 2864 / 2926 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 21 | ✅ / ✅ | ✅ / ✅ | 4 / 3 | 3480 / 3499 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 25 | ✅ / ✅ | ✅ / ✅ | 11 / 4 | 3495 / 3518 | ✅ / ✅ | ✅ / ✅ |
| reactors | 21 | ✅ / ✅ | ✅ / ✅ | 4 / 2 | 1630 / 1779 | ✅ / ✅ | ✅ / ✅ |
| reactors | 25 | ✅ / ✅ | ✅ / ✅ | ✅ / 1 | 1969 / 1853 | ✅ / ✅ | ✅ / ✅ |
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>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
ddprof-lib/src/test/cpp/faultInjection_ut.cpp:66
- fi_signal_wrapper calls Profiler::checkFault with the old one-argument signature, but Profiler::checkFault now requires (ProfiledThread*, siginfo_t*, void*). This will fail to compile and also prevents checkFault from inspecting the ucontext/siginfo when deciding whether to longjmp.
static void fi_signal_wrapper(int signo, siginfo_t* siginfo, void* context) {
if (SafeAccess::handle_safefetch(signo, context)) {
return; // safefetch load recovered; PC already rewritten to _cont.
}
Profiler::checkFault(ProfiledThread::current()); // longjmp if protected
// Not protected and not a safefetch fault — real crash.
ddprof-lib/src/main/cpp/profiler.cpp:1034
- Profiler::checkFault is using siginfo->si_addr to decide whether to longjmp and also asserts that profiler{min,max}_address are non-null. For unwind faults, si_addr is typically the bad dereferenced metadata pointer (not an address inside libjavaProfiler), so this will skip recovery and crash. It also makes gtests that call checkFault without having run setupSignalHandlers() hit the assert. Use the instruction PC from ucontext for the range check (or fall back to unconditional longjmp when the range isn’t initialized), and compare addresses as uintptr_t.
const uintptr_t min = (uintptr_t)_profiler_min_address;
const uintptr_t max = (uintptr_t)_profiler_max_address;
// If the profiler address range is not initialized (e.g. unit tests), fall back
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
ddprof-lib/src/test/cpp/faultInjection_ut.cpp:65
Profiler::checkFaultnow requiressiginfo_t*anducontext, but the fault-injection signal wrapper calls it with a single argument. This will not compile, and it also preventscheckFaultfrom reading the faulting PC from the provideducontextfor origin filtering.
static void fi_signal_wrapper(int signo, siginfo_t* siginfo, void* context) {
if (SafeAccess::handle_safefetch(signo, context)) {
return; // safefetch load recovered; PC already rewritten to _cont.
}
Profiler::checkFault(ProfiledThread::current()); // longjmp if protected
ddprof-lib/src/main/cpp/vmEntry.cpp:581
- The comment says the VMStructs init path "only ever runs once", but
VM::ready()is invoked from bothinitLibrary()andinitProfilerBridge()and currently callsVMStructs::init()on every invocation. Either guard the init, or adjust the comment to match the actual behavior/assumptions.
// Run late initialization when JVM is ready. May be called more than once (from
// initProfilerBridge() directly, and later from the VMInit JVMTI callback, or from
// initLibrary() followed by a JNI-triggered attach) -- the VMStructs init below only
// ever runs once.
ddprof-lib/src/test/cpp/hotspot_crash_protection_ut.cpp:337
- The section header above still mentions
HotspotSupport::checkFault(), but the test now callsProfiler::checkFault(). Updating the header/comment avoids confusion when navigating the crash-protection tests.
// ---------------------------------------------------------------------------
TEST(CheckFaultGuardTest, NullThreadIsNoop) {
Profiler::checkFault(nullptr, nullptr, nullptr); // must not crash
Benchmark Results (commit b128b86)Pipeline: https://gitlab.ddbuild.io/DataDog/apm-reliability/benchmarking-platform/-/pipelines/126984072 Commit:
|
| Benchmark | JDK | Latest | Dev | Δ (dev vs latest) | Issues L/D |
|---|---|---|---|---|---|
| akka-uct | 21 | ✅ 10325 ms (21 iters) | ✅ 10466 ms (21 iters) | ≈ +1.4% (±11.3%) | — / — |
| akka-uct | 25 | ✅ 8806 ms (24 iters) | ✅ 8923 ms (24 iters) | ≈ +1.3% (±10.6%) | — / — |
| finagle-chirper | 21 | ✅ 5971 ms (33 iters) | ✅ 6034 ms (33 iters) | ≈ +1.1% (±25.3%) | |
| finagle-chirper | 25 | ✅ 5453 ms (36 iters) | ✅ 5488 ms (36 iters) | ≈ +0.6% (±24.5%) | |
| fj-kmeans | 21 | ✅ 2821 ms (66 iters) | ✅ 2748 ms (68 iters) | 🟢 -2.6% | — / — |
| fj-kmeans | 25 | ✅ 2839 ms (66 iters) | ✅ 2827 ms (66 iters) | ≈ -0.4% (±2.6%) | — / — |
| future-genetic | 21 | ✅ 2119 ms (87 iters) | ✅ 2100 ms (88 iters) | ≈ -0.9% (±2.8%) | — / — |
| future-genetic | 25 | ✅ 2056 ms (90 iters) | ✅ 2093 ms (90 iters) | ≈ +1.8% (±2.6%) | — / — |
| naive-bayes | 21 | ✅ 1244 ms (137 iters) | ✅ 1306 ms (131 iters) | ≈ +5% (±33.3%) | — / — |
| naive-bayes | 25 | ✅ 986 ms (173 iters) | ✅ 1014 ms (169 iters) | ≈ +2.8% (±32.1%) | — / — |
| reactors | 21 | ✅ 16355 ms (15 iters) | ✅ 16274 ms (15 iters) | ≈ -0.5% (±8.8%) | — / — |
| reactors | 25 | ✅ 18339 ms (15 iters) | ✅ 18305 ms (15 iters) | ≈ -0.2% (±4.7%) | — / — |
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 / ✅ | 1902 / 2042 | ✅ / ✅ | ✅ / ✅ |
| akka-uct | 25 | ✅ / ✅ | ✅ / ✅ | 3 / 1 | 2364 / 2260 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 21 | ✅ / ✅ | ✅ / ✅ | 3 / 4 | 8493 / 8807 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 25 | ✅ / ✅ | ✅ / ✅ | 1 / 1 | 8606 / 8299 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 21 | ✅ / ✅ | ✅ / ✅ | 1 / 3 | 1249 / 1270 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 25 | ✅ / ✅ | ✅ / ✅ | 2 / 1 | 1282 / 1292 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 21 | ✅ / ✅ | ✅ / ✅ | 2 / ✅ | 2947 / 2977 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 25 | ✅ / ✅ | ✅ / ✅ | 3 / ✅ | 2881 / 2914 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 21 | ✅ / ✅ | ✅ / ✅ | 2 / 5 | 3513 / 3471 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 25 | ✅ / ✅ | ✅ / ✅ | 4 / 4 | 3475 / 3473 | ✅ / ✅ | ✅ / ✅ |
| reactors | 21 | ✅ / ✅ | ✅ / ✅ | ✅ / 3 | 1664 / 1621 | ✅ / ✅ | ✅ / ✅ |
| reactors | 25 | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ | 1825 / 1843 | ✅ / ✅ | ✅ / ✅ |
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!