Skip to content

Missing libgcc_s.so.1 on the host should not result in crash - #689

Merged
gh-worker-dd-mergequeue-cf854d[bot] merged 23 commits into
mainfrom
zgu/missing_libgcc
Jul 28, 2026
Merged

Missing libgcc_s.so.1 on the host should not result in crash#689
gh-worker-dd-mergequeue-cf854d[bot] merged 23 commits into
mainfrom
zgu/missing_libgcc

Conversation

@zhengyu123

@zhengyu123zhengyu123 commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?:
Gracefully disable profiler if libgcc_s.so is 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:

  • If this PR touches code that signs or publishes builds or packages, or handles
    credentials of any kind, I've requested a security review (run the dd:platform-security-review
    skill, or file a request via the PSEC review form).
    bewaire also runs automatically on every PR.
  • This PR doesn't touch any of that.
  • JIRA: PROF-15395

Unsure? Have a question? Request a review!

CopilotAI review requested due to automatic review settings July 27, 2026 13:20

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 return bool indicating whether libgcc_s.so.1 was successfully loaded.
  • Added an early failure path intended to disable profiling when libgcc_s cannot 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.

FileDescription
ddprof-lib/src/main/cpp/profiler.hUpdates prewarmUnwinder() signature to return success/failure.
ddprof-lib/src/main/cpp/profiler.cppImplements 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 prewarms libgcc_s.so.1, but the agent startup path calls Profiler::runInternal(...)->start() from VM::VMInit() without going through Profiler::init(). This means the original lazy-load-in-signal-context crash scenario can still happen, and missing libgcc_s will not be detected/handled for the agent path. Reintroduce the prewarm here and return an error (disabling the profiler) when libgcc_s.so.1 is 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.

Comment threadddprof-lib/src/main/cpp/profiler.cpp Outdated
@dd-octo-sts

dd-octo-stsBot commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

CI Test Results

Run:#30406064723 | Commit:a4a9cac | Duration: 16m 7s (longest job)

All 32 test jobs passed

Status Overview

JDKglibc-aarch64/debugglibc-amd64/debugmusl-aarch64/debugmusl-amd64/debug
8---
8-ibm---
8-j9--
8-librca--
8-orcl---
11---
11-j9--
11-librca--
17--
17-graal--
17-j9--
17-librca--
21--
21-graal--
21-librca--
25--
25-graal--
25-librca--

Legend: ✅ passed | ❌ failed | ⚪ skipped | 🚫 cancelled

Summary: Total: 32 | Passed: 32 | Failed: 0


Updated: 2026-07-28 23:11:52 UTC

CopilotAI review requested due to automatic review settings July 27, 2026 14:46

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Comment threadddprof-lib/src/test/cpp/faultInjection_ut.cpp
Comment threadddprof-lib/src/main/cpp/profiler.cpp Outdated
Comment threadddprof-lib/src/main/cpp/faultInjection.h
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
CopilotAI review requested due to automatic review settings July 27, 2026 14:53

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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);

Comment threadddprof-lib/src/test/cpp/faultInjection_ut.cpp
@zhengyu123
zhengyu123 marked this pull request as ready for review July 27, 2026 15:01
@zhengyu123
zhengyu123 requested a review from a team as a code ownerJuly 27, 2026 15:01
CopilotAI review requested due to automatic review settings July 27, 2026 15:02

@chatgpt-codex-connectorchatgpt-codex-connectorBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 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".

Comment threadddprof-lib/src/main/cpp/profiler.cpp Outdated
Comment threadddprof-lib/src/test/cpp/faultInjection_ut.cpp

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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() calls prewarmUnwinder() 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 _state first 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");
}

Comment threadddprof-lib/src/main/cpp/profiler.cpp
@dd-octo-sts

Copy link
Copy Markdown
Contributor

Benchmark Results (commit 413bbce)

Pipeline: https://gitlab.ddbuild.io/DataDog/apm-reliability/benchmarking-platform/-/pipelines/127192661 Commit: 413bbce353af9ab43b545545008435a65b750d96

⚠️ Significant outliers

  • 🔴 future-genetic (JDK 21): runtime +3.6% (2035→2108 ms)
  • 🟢 reactors (JDK 21): runtime -7.2% (16956→15727 ms)
Runtime details (per benchmark × JDK)
BenchmarkJDKLatestDevΔ (dev vs latest)Issues L/D
akka-uct21✅ 10389 ms (21 iters)✅ 10301 ms (21 iters)≈ -0.8% (±11.6%)— / —
akka-uct25✅ 8931 ms (24 iters)✅ 8862 ms (24 iters)≈ -0.8% (±10.2%)— / —
finagle-chirper21✅ 5977 ms (33 iters)✅ 6024 ms (33 iters)≈ +0.8% (±25.7%)⚠️ W:3 / ⚠️ W:3
finagle-chirper25✅ 5471 ms (36 iters)✅ 5496 ms (36 iters)≈ +0.5% (±24%)⚠️ W:3 / ⚠️ W:4
fj-kmeans21✅ 2717 ms (68 iters)✅ 2777 ms (68 iters)≈ +2.2% (±2.8%)— / —
fj-kmeans25✅ 2801 ms (67 iters)✅ 2825 ms (66 iters)≈ +0.9% (±2.6%)— / —
future-genetic21✅ 2035 ms (92 iters)✅ 2108 ms (88 iters)🔴 +3.6%— / —
future-genetic25✅ 2084 ms (88 iters)✅ 2032 ms (91 iters)≈ -2.5% (±2.6%)— / —
naive-bayes21✅ 1275 ms (134 iters)✅ 1315 ms (131 iters)≈ +3.1% (±32.7%)— / —
naive-bayes25✅ 1019 ms (168 iters)✅ 1009 ms (169 iters)≈ -1% (±31.9%)— / —
reactors21✅ 16956 ms (15 iters)✅ 15727 ms (15 iters)🟢 -7.2%— / —
reactors25✅ 18030 ms (15 iters)✅ 18354 ms (15 iters)≈ +1.8% (±4.7%)— / —
Internal counter details (ddprof)

ddprof internal counters, latest / dev (✅ = 0, · = unavailable):

BenchmarkJDKDropped recDropped jvmtiDropped traceSkipped WCAGCT failUnwind fail
akka-uct21✅ / ✅✅ / ✅2 / 31989 / 1961✅ / ✅✅ / ✅
akka-uct25✅ / ✅✅ / ✅3 / ✅2157 / 2135✅ / ✅✅ / ✅
finagle-chirper21✅ / ✅✅ / ✅3 / 48321 / 8782✅ / ✅✅ / ✅
finagle-chirper25✅ / ✅✅ / ✅1 / 18196 / 8508✅ / ✅✅ / ✅
fj-kmeans21✅ / ✅✅ / ✅3 / 21244 / 1276✅ / ✅✅ / ✅
fj-kmeans25✅ / ✅✅ / ✅3 / 31319 / 1276✅ / ✅✅ / ✅
future-genetic21✅ / ✅✅ / ✅2 / 13007 / 2921✅ / ✅✅ / ✅
future-genetic25✅ / ✅✅ / ✅1 / 12789 / 2782✅ / ✅✅ / ✅
naive-bayes21✅ / ✅✅ / ✅6 / 23525 / 3550✅ / ✅✅ / ✅
naive-bayes25✅ / ✅✅ / ✅3 / ✅3473 / 3485✅ / ✅✅ / ✅
reactors21✅ / ✅✅ / ✅✅ / ✅1672 / 1566✅ / ✅✅ / ✅
reactors25✅ / ✅✅ / ✅✅ / 11731 / 1906✅ / ✅✅ / ✅

CopilotAI review requested due to automatic review settings July 27, 2026 15:53

@chatgpt-codex-connectorchatgpt-codex-connectorBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 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".

Comment threadddprof-lib/src/test/cpp/faultInjection_ut.cpp Outdated

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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);

Comment threadddprof-lib/src/main/cpp/profiler.cpp Outdated
CopilotAI review requested due to automatic review settings July 27, 2026 16:04
@dd-octo-sts

Copy link
Copy Markdown
Contributor

Benchmark Results (commit a9dda01)

Pipeline: https://gitlab.ddbuild.io/DataDog/apm-reliability/benchmarking-platform/-/pipelines/127424714 Commit: a9dda011e83af947f9499f9f9d6173ed91af9c6b

⚠️ Significant outliers

  • 🔴 future-genetic (JDK 21): runtime +4.6% (2045→2139 ms)
  • 💥 naive-bayes (JDK 25): latest crashed
Runtime details (per benchmark × JDK)
BenchmarkJDKLatestDevΔ (dev vs latest)Issues L/D
akka-uct21✅ 10186 ms (21 iters)✅ 10303 ms (21 iters)≈ +1.1% (±11.2%)— / —
akka-uct25✅ 8759 ms (24 iters)✅ 8816 ms (24 iters)≈ +0.7% (±9.9%)— / —
finagle-chirper21✅ 5971 ms (33 iters)✅ 6013 ms (33 iters)≈ +0.7% (±25.6%)⚠️ W:3 / ⚠️ W:3
finagle-chirper25✅ 5479 ms (36 iters)✅ 5440 ms (36 iters)≈ -0.7% (±24.3%)⚠️ W:3 / ⚠️ W:3
fj-kmeans21✅ 2782 ms (67 iters)✅ 2715 ms (69 iters)≈ -2.4% (±2.7%)— / —
fj-kmeans25✅ 2771 ms (67 iters)✅ 2827 ms (66 iters)≈ +2% (±2.7%)— / —
future-genetic21✅ 2045 ms (90 iters)✅ 2139 ms (87 iters)🔴 +4.6%— / —
future-genetic25✅ 2089 ms (89 iters)✅ 2124 ms (87 iters)≈ +1.7% (±2.6%)— / —
naive-bayes21✅ 1224 ms (140 iters)✅ 1310 ms (131 iters)≈ +7% (±33.6%)— / —
naive-bayes25💥 0 ms (0 iters)💥 0 ms (0 iters)— / —
reactors21✅ 16638 ms (15 iters)✅ 16498 ms (15 iters)≈ -0.8% (±9%)— / —
reactors25✅ 18199 ms (15 iters)✅ 18597 ms (15 iters)≈ +2.2% (±5.3%)— / —
Internal counter details (ddprof)

ddprof internal counters, latest / dev (✅ = 0, · = unavailable):

BenchmarkJDKDropped recDropped jvmtiDropped traceSkipped WCAGCT failUnwind fail
akka-uct21✅ / ✅✅ / ✅1 / 21900 / 1916✅ / ✅✅ / ✅
akka-uct25✅ / ✅✅ / ✅✅ / ✅2277 / 2130✅ / ✅✅ / ✅
finagle-chirper21✅ / ✅✅ / ✅3 / 38663 / 8636✅ / ✅✅ / ✅
finagle-chirper25✅ / ✅✅ / ✅1 / 18524 / 8412✅ / ✅✅ / ✅
fj-kmeans21✅ / ✅✅ / ✅✅ / ✅✅ / ✅✅ / ✅✅ / ✅
fj-kmeans25✅ / ✅✅ / ✅4 / 31291 / 1258✅ / ✅✅ / ✅
future-genetic21✅ / ✅✅ / ✅✅ / ✅3003 / 2998✅ / ✅✅ / ✅
future-genetic25✅ / ✅✅ / ✅2 / 22890 / 2875✅ / ✅✅ / ✅
naive-bayes21✅ / ✅✅ / ✅3 / 73526 / 3476✅ / ✅✅ / ✅
reactors21✅ / ✅✅ / ✅1 / 21861 / 1777✅ / ✅✅ / ✅
reactors25✅ / ✅✅ / ✅1 / ✅1892 / 1869✅ / ✅✅ / ✅

@jbachorikjbachorik left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM, assuming that the naming issue from the sphinx review is addressed

CopilotAI review requested due to automatic review settings July 28, 2026 18:55

@chatgpt-codex-connectorchatgpt-codex-connectorBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 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".

Comment threadddprof-lib/src/test/cpp/faultInjection_ut.cpp Outdated

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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>

CopilotAI review requested due to automatic review settings July 28, 2026 19:06

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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);
}
};

@chatgpt-codex-connectorchatgpt-codex-connectorBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 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".

Comment threadddprof-lib/src/test/cpp/jvmSupport_ut.cpp Outdated
@dd-octo-sts

Copy link
Copy Markdown
Contributor

Benchmark Results (commit 78f4b2f)

Pipeline: https://gitlab.ddbuild.io/DataDog/apm-reliability/benchmarking-platform/-/pipelines/127536213 Commit: 78f4b2f19d36be398e57480d5931725934635487

⚠️ Significant outliers

  • 🟢 fj-kmeans (JDK 21): runtime -5.4% (2802→2652 ms)
  • 🟢 future-genetic (JDK 21): runtime -3.8% (2126→2045 ms)
Runtime details (per benchmark × JDK)
BenchmarkJDKLatestDevΔ (dev vs latest)Issues L/D
akka-uct25✅ 8746 ms (24 iters)✅ 8832 ms (24 iters)≈ +1% (±9.9%)— / —
finagle-chirper21✅ 5999 ms (33 iters)✅ 5948 ms (33 iters)≈ -0.9% (±24.8%)⚠️ W:3 / ⚠️ W:3
finagle-chirper25✅ 5454 ms (36 iters)✅ 5486 ms (36 iters)≈ +0.6% (±24.3%)⚠️ W:4 / ⚠️ W:3
fj-kmeans21✅ 2802 ms (67 iters)✅ 2652 ms (70 iters)🟢 -5.4%— / —
fj-kmeans25✅ 2739 ms (68 iters)✅ 2812 ms (66 iters)≈ +2.7% (±2.7%)— / —
future-genetic21✅ 2126 ms (87 iters)✅ 2045 ms (90 iters)🟢 -3.8%— / —
future-genetic25✅ 2102 ms (88 iters)✅ 2065 ms (89 iters)≈ -1.8% (±2.5%)— / —
naive-bayes21✅ 1237 ms (138 iters)✅ 1233 ms (138 iters)≈ -0.3% (±32.7%)— / —
naive-bayes25✅ 1011 ms (169 iters)✅ 1011 ms (169 iters)≈ 0% (±31.6%)— / —
reactors25✅ 18691 ms (15 iters)✅ 18790 ms (15 iters)≈ +0.5% (±6.2%)— / —
Internal counter details (ddprof)

ddprof internal counters, latest / dev (✅ = 0, · = unavailable):

BenchmarkJDKDropped recDropped jvmtiDropped traceSkipped WCAGCT failUnwind fail
akka-uct21✅ / ✅✅ / ✅2 / 22139 / 1944✅ / ✅✅ / ✅
akka-uct25✅ / ✅✅ / ✅2 / 51977 / 2128✅ / ✅✅ / ✅
finagle-chirper21✅ / ✅✅ / ✅2 / 38839 / 8612✅ / ✅✅ / ✅
finagle-chirper25✅ / ✅✅ / ✅2 / 18546 / 8881✅ / ✅✅ / ✅
fj-kmeans21✅ / ✅✅ / ✅2 / ✅1233 / 1244✅ / ✅✅ / ✅
fj-kmeans25✅ / ✅✅ / ✅3 / 21287 / 1296✅ / ✅✅ / ✅
future-genetic21✅ / ✅✅ / ✅2 / ✅2969 / 2912✅ / ✅✅ / ✅
future-genetic25✅ / ✅✅ / ✅1 / ✅2916 / 2930✅ / ✅✅ / ✅
naive-bayes21✅ / ✅✅ / ✅3 / 33526 / 3528✅ / ✅✅ / ✅
naive-bayes25✅ / ✅✅ / ✅2 / 13459 / 3531✅ / ✅✅ / ✅
reactors25✅ / ✅✅ / ✅✅ / 11980 / 1942✅ / ✅✅ / ✅

CopilotAI review requested due to automatic review settings July 28, 2026 19:51

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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();

@dd-octo-sts

Copy link
Copy Markdown
Contributor

Benchmark Results (commit d71431e)

Pipeline: https://gitlab.ddbuild.io/DataDog/apm-reliability/benchmarking-platform/-/pipelines/127550270 Commit: d71431ed868dbaeb259f4e9837662e26ce432d93

⚠️ Significant outliers

  • 🔴 future-genetic (JDK 21): runtime +3.1% (2083→2147 ms)
  • 🔴 future-genetic (JDK 25): runtime +5.1% (2040→2145 ms)
Runtime details (per benchmark × JDK)
BenchmarkJDKLatestDevΔ (dev vs latest)Issues L/D
akka-uct21✅ 10369 ms (21 iters)✅ 10289 ms (21 iters)≈ -0.8% (±11.2%)— / —
akka-uct25✅ 8616 ms (24 iters)💥 0 ms (0 iters)— / —
finagle-chirper21✅ 5988 ms (33 iters)✅ 5922 ms (33 iters)≈ -1.1% (±24.9%)⚠️ W:3 / ⚠️ W:3
finagle-chirper25✅ 5452 ms (36 iters)✅ 5493 ms (36 iters)≈ +0.8% (±24.9%)⚠️ W:3 / ⚠️ W:3
fj-kmeans21✅ 2758 ms (67 iters)✅ 2687 ms (70 iters)≈ -2.6% (±2.6%)— / —
fj-kmeans25✅ 2766 ms (68 iters)✅ 2832 ms (66 iters)≈ +2.4% (±2.7%)— / —
future-genetic21✅ 2083 ms (89 iters)✅ 2147 ms (87 iters)🔴 +3.1%— / —
future-genetic25✅ 2040 ms (91 iters)✅ 2145 ms (87 iters)🔴 +5.1%— / —
naive-bayes21✅ 1269 ms (135 iters)✅ 1310 ms (131 iters)≈ +3.2% (±33.1%)— / —
naive-bayes25✅ 1013 ms (169 iters)✅ 1008 ms (170 iters)≈ -0.5% (±31.3%)— / —
reactors21✅ 16686 ms (15 iters)✅ 16624 ms (15 iters)≈ -0.4% (±7.3%)— / —
reactors25✅ 18344 ms (15 iters)✅ 18335 ms (15 iters)≈ -0% (±4.9%)— / —
Internal counter details (ddprof)

ddprof internal counters, latest / dev (✅ = 0, · = unavailable):

BenchmarkJDKDropped recDropped jvmtiDropped traceSkipped WCAGCT failUnwind fail
akka-uct21✅ / ✅✅ / ✅1 / ✅1949 / 2051✅ / ✅✅ / ✅
akka-uct25✅ / ·✅ / ·1 / ·2133 / ·✅ / ·✅ / ·
finagle-chirper21✅ / ✅✅ / ✅4 / 38797 / 8819✅ / ✅✅ / ✅
finagle-chirper25✅ / ✅✅ / ✅✅ / ✅8622 / 8567✅ / ✅✅ / ✅
fj-kmeans21✅ / ✅✅ / ✅2 / 21264 / 1284✅ / ✅✅ / ✅
fj-kmeans25✅ / ✅✅ / ✅3 / ✅1274 / 1284✅ / ✅✅ / ✅
future-genetic21✅ / ✅✅ / ✅✅ / 32901 / 2958✅ / ✅✅ / ✅
future-genetic25✅ / ✅✅ / ✅3 / 22888 / 2847✅ / ✅✅ / ✅
naive-bayes21✅ / ✅✅ / ✅3 / 33523 / 3549✅ / ✅✅ / ✅
naive-bayes25✅ / ✅✅ / ✅4 / 43470 / 3484✅ / ✅✅ / ✅
reactors21✅ / ✅✅ / ✅✅ / 11792 / 1762✅ / ✅✅ / ✅
reactors25✅ / ✅✅ / ✅✅ / ✅1858 / 1880✅ / ✅✅ / ✅

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

mergequeue-status: donesphinx:spotcheckSphinx: spot-check recommended

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@zhengyu123@jbachorik