Skip to content

Adopt JmpCtxScope for setting stack walk siglongjmp protection - #758

Merged
zhengyu123 merged 1 commit into
mainfrom
zgu/adopt_JmpCtxScope
Aug 27, 2026
Merged

Adopt JmpCtxScope for setting stack walk siglongjmp protection#758
zhengyu123 merged 1 commit into
mainfrom
zgu/adopt_JmpCtxScope

Conversation

@zhengyu123

@zhengyu123 zhengyu123 commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?:
Replaces the hand-rolled sigjmp_buf* prev = prof_thread->getJmpCtx(); ... prof_thread->setJmpCtx(prev); siglongjmp-chaining pattern with the RAII JmpCtxScope guard (introduced in #743 for HotspotSupport::resolve()) across the remaining stack-walking entry points:

  • HotspotSupport::walkVM()
  • HotspotSupport::walkJavaStack()
  • StackWalker::walkFP()
  • StackWalker::walkDwarf()

Per site:

  • sigjmp_buf* prev_jmp_buf = getJmpCtx();JmpCtxScope jmp_scope(prof_thread);
  • setJmpCtx(&crash_protection_ctx);jmp_scope.install(&crash_protection_ctx);
  • In the sigsetjmp fault-recovery branch: setJmpCtx(prev_jmp_buf);jmp_scope.restore(); (still an explicit disarm before touching anything else that could fault)
  • On the normal exit path, the trailing setJmpCtx(prev_jmp_buf); is dropped in walkFP/walkDwarf/walkJavaStack — nothing after the walk loop can fault, so the guard's destructor restoring at function exit is sufficient. In walkVM, the explicit jmp_scope.restore() at the done: label is kept, since post-walk work in that frame (leaf-drop, counters) doesn't need the landing pad held live.

guards.h is now included by stackWalker.cpp (hotspotSupport.cpp already had it from #743).

Motivation:
Consolidate the siglongjmp landing-pad save/restore/chain logic behind one RAII abstraction instead of four duplicated get/set call sites, so every exit path (normal return, fault recovery, goto) is guaranteed to reinstate the previous landing pad without relying on the author remembering to add the restore call.

Additional Notes:
No functional/behavioral change intended — this is a pure refactor onto the existing JmpCtxScope abstraction from #743. JmpCtxScope's constructor snapshots the previous landing pad before install() overwrites it, and restore()/the destructor always write that fixed snapshot, so it is safe to call restore() explicitly and then let the destructor run again at scope exit.

How to test the change?:
./gradlew buildDebug compiles clean. No behavior change, so existing stack-walking/crash-protection test coverage applies unchanged.

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-15845

Unsure? Have a question? Request a review!

@dd-octo-sts

dd-octo-sts Bot commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

Scan-Build Report

User:runner@runnervmgx7h7
Working Directory:/home/runner/work/java-profiler/java-profiler/ddprof-lib/src/test/make
Command Line:make -j4 all
Clang Version:Ubuntu clang version 18.1.3 (1ubuntu1)
Date:Thu Aug 27 14:23:29 2026

Bug Summary

Bug TypeQuantityDisplay?
All Bugs1
Logic error
Dereference of null pointer1

Reports

Bug Group Bug Type ▾ File Function/Method Line Path Length
Logic errorDereference of null pointerprofiler.hfindLibraryByAddress52928

@dd-octo-sts

dd-octo-sts Bot commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

All 40 integration tests passed

📊 Dashboard · 👷 Pipeline · 📦 fcbed9a4

@dd-octo-sts

dd-octo-sts Bot commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

CI Test Results

Run: #33081778630 | Commit: f4c6e06 | Duration: 26m 0s (longest job)

All 32 test jobs passed

Status Overview

JDK glibc-aarch64/debug glibc-amd64/debug musl-aarch64/debug musl-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-08-27 14:51:02 UTC

@zhengyu123
zhengyu123 marked this pull request as ready for review August 27, 2026 15:00
@zhengyu123
zhengyu123 requested a review from a team as a code owner August 27, 2026 15:00

@jbachorik jbachorik 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.

This looks good. Nice cleanup!

@zhengyu123
zhengyu123 merged commit be960ad into main Aug 27, 2026
109 checks passed
@zhengyu123
zhengyu123 deleted the zgu/adopt_JmpCtxScope branch August 27, 2026 16:38
@github-actions github-actions Bot added this to the 1.50.0 milestone Aug 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants