Repository navigation
Conversation
The musl t07_body only exits the cancellation loop via pthread_exit (PTHREAD_CANCELED), which runs the registered cleanup handler and terminates the thread, so the trailing ProfiledThread::release() and return were unreachable. Keep pthread_cleanup_pop(0) — it is lexically required to pair with pthread_cleanup_push — and mark the tail __builtin_unreachable() like the glibc variant.
podman aborts container commands locally because its credential-helper lookup for the Datadog registries (ddtool) cannot reach vault outside the CI network. Document colima as the macOS runtime in AGENTS.md, README.md and utils/README.md; the script default stays podman for CI.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![datadog-datadog-prod-us1-2[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
There was a problem hiding this comment.
More details
The musl change preserves the cancellation cleanup handler and required cleanup macro pairing while removing an unreachable tail. The completed static review identified no reportable regression.
🤖 Bits Code Review · Commit b412cd0
CI Test ResultsRun: #39666677249 | Commit:
Status Overview
Legend: ✅ passed | ❌ failed | ⚪ skipped | 🚫 cancelled Summary: Total: 32 | Passed: 32 | Failed: 0 Updated: 2026-10-21 20:29:00 UTC |
1 participant
What does this PR do?:
Removes the remaining instance of the unreachable dead code flagged in #592: the musl variant of
t07_bodyinddprof-lib/src/test/cpp/thread_teardown_safety_ut.cppstill ended withProfiledThread::release(); return nullptr;after thewhile (true) { pthread_testcancel(); usleep(100); }loop — statements that can never execute, because the only exit from the loop is cancellation, which callspthread_exit(PTHREAD_CANCELED)(muslsrc/thread/pthread_cancel.c::__cancel()), runs the registered cleanup handler and terminates the thread.pthread_cleanup_pop(0)is kept: musl'spthread_cleanup_push/popare paired macros that open/close ado { ... } while (0)block, so the pop is lexically required even though it is unreachable. The tail is now marked__builtin_unreachable(), mirroring the glibc variant.Also updates AGENTS.md / README.md / utils/README.md to recommend colima-backed Docker (
--container=docker) for container tests on macOS instead of podman: podman aborts container commands locally because its credential-helper lookup for the Datadog registries (ddtool) cannot reach vault outside the CI network. The script's default runtime is unchanged.Motivation:
Closes #592 (sphinx review finding). The glibc half of the issue — the
__pthread_unregister_cancel/sigactionrestore /ProfiledThread::release()dead tail in the same function — was already removed on main; this PR completes the fix by removing the analogous musl-path dead code that survived, so the issue is fully addressed.Additional Notes:
Reachability verified against primary sources:
pthread_testcancel→__testcancel→__cancel()→pthread_exit(PTHREAD_CANCELED)(declared_Noreturn);__pthread_exitwalks thecancelbufchain, running the cleanup handler.nptl/unwind.c:unwind_stop()longjmps to thecancel_jmp_bufregistered by__pthread_register_cancel.How to test the change?:
Behavior-preserving (test-only) change;
thread_teardown_safety_utpasses before and after:Both report
BUILD SUCCESSFUL; the executable's gtest output shows14 tests ... PASSEDon musl, includingForcedUnwindWithConcurrentSignalDoesNotCrash.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.