Skip to content

Return JMETHODID_NOT_WALKABLE instead of nullptr for unpopulated jmethodID slots - #733

Draft
jbachorik wants to merge 1 commit into
mainfrom
fix/jmethodid_final
Draft

Return JMETHODID_NOT_WALKABLE instead of nullptr for unpopulated jmethodID slots#733
jbachorik wants to merge 1 commit into
mainfrom
fix/jmethodid_final

Conversation

@jbachorik

Copy link
Copy Markdown
Collaborator

What does this PR do?:
Changes VMMethod::id() to return JMETHODID_NOT_WALKABLE instead of nullptr when the jmethodID cache is not yet populated for a klass. This prevents fillJavaFrame from taking the raw Method* pointer fallback path.

Motivation:
VMMethod::id() returned nullptr when a klass's jmethodID cache was not yet populated. fillJavaFrame treats nullptr as "store raw Method* pointer", deferring a dereference of that pointer to dump time (up to ~60s later per JFR chunk length). If HotSpot's redefinition cleanup reclaimed the underlying Method*/Symbol* metadata in that window, the unguarded memcpys in HotspotSupport::resolve() faulted on freed memory.

This fires on every RedefineClasses/RetransformClasses (routine, continuous — e.g. dd-trace-java agent retransformation): the cache is briefly empty between invalidation and repopulation, and a thread sampled in that window hits the nullptr path. Independent of the _force_jmethodID/fjmethodid config — that flag only controls the startup-time preload sweep, not this fallback.

Chain:

  1. Agent retransforms a class → jmethodID cache empty
  2. Concurrently-sampled frame → id() returns nullptrfillFrameRaw stores raw Method* + isRawPointer flag
  3. Dump time → resolveMethod sees isRawPointer(bci) → calls HotspotSupport::resolve() → unguarded memcpys fault on reclaimed metadata

Fix: Return JMETHODID_NOT_WALKABLE from all unpopulated-cache paths in VMMethod::id(). fillJavaFrame stores the sentinel without the raw flag; the dump thread maps it to the shared "unknown" method. No raw Method* dereference occurs. Covers 7 call sites (2 direct id() callers + 5 getMethodId()validatedId()id() callers).

Additional Notes:

  • fillFrameRaw and isRawPointer left in place as defensive dead code
  • HotspotSupport::resolve() still returns nullptr as the dump-time "unknown" signal — that contract with flightRecorder.cpp is unchanged
  • OpenJ9/Zing paths untouched — they don't use fillJavaFrame/fillFrameRaw/isRawPointer

How to test the change?:

  • HotspotMethodIdTest.RejectedMethodIdStaysNonRawAndResolvesToUnknown — passes (gtest)
  • JMethodIDInvalidationStressTest — passes (integration, slow suite)
  • All gtest suites pass; spotlessApply clean

For Datadog employees:

  • This PR doesn't touch any of that.
  • JIRA: PROF-15385

…hodID slots

VMMethod::id() returned nullptr when the jmethodID cache was not yet
populated for a klass. fillJavaFrame treats nullptr as 'store raw Method*',
which defers a dereference of potentially-reclaimed metadata to dump time.
This fires on every RedefineClasses/RetransformClasses: the cache is
briefly empty between invalidation and repopulation, and a thread sampled
in that window stores a raw Method* that may fault at dump time.

Return JMETHODID_NOT_WALKABLE instead so fillJavaFrame stores the sentinel
without the raw flag. The dump thread maps the sentinel to the shared
unknown method, avoiding any raw Method* dereference.
@jbachorik jbachorik added the AI label Aug 11, 2026
@dd-octo-sts

dd-octo-sts Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Scan-Build Report

User:runner@runnervmvrwv9
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:Tue Aug 11 15:22:45 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.hfindLibraryByAddress52313

@jbachorik

Copy link
Copy Markdown
Collaborator Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot 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: 465607fc9f

ℹ️ 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 on lines +152 to +154
// No jmethodID cache allocated for this klass yet; same reason
// as above — return the sentinel, not nullptr.
return JMETHODID_NOT_WALKABLE;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Preserve the raw fallback for intentionally unprimed classes

When profiling HotSpot with fjmethodid=false and VM/default C-stack mode, initExecution() enters Partial_loaded, and loadMethodIDsIfNeededImpl() deliberately leaves ordinary bootstrap, platform, and application-loader classes without a jmethodID cache because their stable Method* values are resolved through this raw-pointer fallback. Thus ids == NULL is an expected persistent state, not necessarily the brief retransformation race described here. Returning the sentinel makes compiled frames for those classes serialize as unknown, while interpreter paths that reject JMETHODID_NOT_WALKABLE can terminate the walk at the first such frame, substantially corrupting profiles in this supported configuration. Restrict the sentinel behavior to invalidation-sensitive cases while retaining the null/raw fallback for deliberately unprimed stable classes.

Useful? React with 👍 / 👎.

@dd-octo-sts

dd-octo-sts Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

CI Test Results

Run: #31506496226 | Commit: 495bef9 | Duration: 14m 54s (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: 29 | Failed: 0


Updated: 2026-08-11 15:44:33 UTC

@dd-octo-sts

dd-octo-sts Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

38 passed, 1 failed out of 39 configurations

Test Matrix

Platform JDK 8 JDK 11 JDK 17 JDK 21 JDK 25
glibc-x64-hotspot
glibc-x64-openj9
glibc-arm64-hotspot
glibc-arm64-openj9
musl-x64-hotspot
musl-x64-openj9
musl-arm64-hotspot
musl-arm64-openj9

Failure Details

musl-x64-openj9-jdk17

Profiler-only:

"  Skipped (tracer not expected in this configuration)"
""
"[7/8] Checking for unexpected events..."
"  ✓ No unexpected events found"
""
"[8/8] Scenario-specific validation (ddprof_only)..."
"  Validating profiler-only scenario..."
"  ✓ Profiler-only scenario checks passed"
""
"=== Validation Summary ==="
"ExecutionSample:            0.0 events (datadog.ExecutionSample)"
"Stack traces:               0.0 samples"
"Thread diversity:           0.0 threads"
"Allocation samples:         0 events (jdk.ObjectAllocationSample)"
"ThreadAllocationStatistics: 49 events"
""
"VALIDATION_FAILED: One or more checks did not pass"

Script executed successfully: 425 commands
VALIDATION_FAILED: Validation checks did not pass

Links

@datadog-prod-us1-6

Copy link
Copy Markdown

Pipelines

Unblock PR with BitsAI

⚠️ Warnings

🚦 3 Pipeline jobs failed

DataDog/java-profiler | integration-test-x64-musl: [openj9, 17]   View in Datadog   GitLab

DataDog/java-profiler | post-pr-comment   View in Datadog   GitLab

DataDog/java-profiler | integration-test-x64-glibc: [openj9, 8]   View in Datadog   GitLab

Useful? React with 👍 / 👎

This comment will be updated automatically if new data arrives.
🔗 Commit SHA: 465607f | Docs | Datadog PR Page | Give us feedback!

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant