On Tue, 29 Sep 2026 10:38:42 GMT, Andrei Pangin <[email protected]> wrote:
>> This PR adds a new JFR frame type - for native (non-Java) frames. >> Native stack walking can be enabled with a new `FlightRecorderOption`. >> Only `jdk.CPUTimeSample` event can have native frames for now, but this can >> be extended to other events in the future. >> >> More details in https://bugs.openjdk.org/browse/JDK-8373791 >> >> Existing JFR tests pass. >> Added a new jtreg test that verifies sanity of collected native frames. >> >> --------- >> - [x] I confirm that I make this contribution in accordance with the >> [OpenJDK Interim AI Policy](https://openjdk.org/legal/ai). > > Andrei Pangin has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains two commits: > > - Re-implemented native frames support using variable-size requests > - 8392187: JFR: Use-after-free bug in CPU-time profiler can crash JVM src/hotspot/share/jfr/periodic/sampling/jfrCPUTimeThreadSampler.cpp line 165: > 163: u4 capacity = _capacity; > 164: if (capacity < CPU_TIME_QUEUE_MAX_CAPACITY) { > 165: float ratio = (float)(lost_samples_due_to_queue_full * > JfrCPUTimeSampleRequest::fixed_size()) / (float)capacity; but this doesn't reflect the demand of native frame part. If we observe lost_samples when native_stack is enabled, i think the real demand of space is bigger. src/hotspot/share/jfr/periodic/sampling/jfrCPUTimeThreadSampler.cpp line 609: > 607: u4 JfrCPUSamplerThread::walk_native_stack(const void* ucontext, > JavaThread* jt, address* pcs) { > 608: frame f = os::fetch_frame_from_context(ucontext); > 609: const intptr_t* const last_java_sp = jt->last_Java_sp(); Should we assert(jt->has_last_Java_frame) beforehand? It's very important that the loop terminates at the boundary (LJF). src/hotspot/share/jfr/periodic/sampling/jfrCPUTimeThreadSampler.cpp line 614: > 612: while (count < JfrCPUTimeSampleRequest::MAX_NATIVE_FRAMES) { > 613: const address pc = f.pc(); > 614: if (pc == nullptr || CodeCache::contains(pc)) { I think the cheap check should be LJF(last java frame). Until we encounter the LJF, everything are native frames. jt->_anchor is the invariant provided by hotspot. for pc check about here: 2. if pc is from interpreter but JVM is not using template interpreter(a corner case), pc is NOT in code cache! However, ucontext points to a java frame, not native frame. 3. Isn't CodeCache.contains() expensive? if we have multiple heaps in CodeCache. src/hotspot/share/jfr/periodic/sampling/jfrCPUTimeThreadSampler.cpp line 617: > 615: break; // reached generated Java code > 616: } > 617: pcs[count++] = pc; should we put this assignment after the sanity check at line 619? src/hotspot/share/jfr/periodic/sampling/jfrThreadSampling.cpp line 341: > 339: return; > 340: } > 341: traceid sid; we need to initialize sid = 0 here, or assert(sid != 0) at line 355 is undefined. src/hotspot/share/jfr/periodic/sampling/jfrThreadSampling.cpp line 390: > 388: } > 389: JfrCPUTimeTraceQueue& queue = tl->cpu_time_jfr_queue(); > 390: for (u4 offset = 0; offset < queue.size(); ) { I think we should use iterator pattern here. RequestIterator iter; while (iter.hasNext()) { auto& request = iter.next(); record_cpu_time_thread(request, now, tl, jt, current); } not only it's easier to read, it also hides variable-length queue and offset. src/hotspot/share/jfr/recorder/stacktrace/jfrNativeFunctionRepository.cpp line 28: > 26: #include "jfr/recorder/stacktrace/jfrNativeFunctionRepository.hpp" > 27: #include "jfr/utilities/jfrAllocation.hpp" > 28: #include "jvm_io.h" This header introduces the declaration of jio_ functions. Do we need that in this cpp file? src/hotspot/share/jfr/recorder/stacktrace/jfrNativeFunctionRepository.cpp line 192: > 190: } > 191: > 192: return _main_exe != nullptr ? _main_exe : ""; I understand that we can't return nullptr because path will pass to strcmp later. We could optimize the fast path here. _main_exec is nullptr only if os::strdup can't execute malloc (len+1) bytes. How about we use os::strdup_check_oom at line 189? Or we define static char s_zero_ = 0; then we have the following logic. if (_main_exe == nullptr) { _main_exe = &s_zero; } Because _main_exe is leaked anyway, we can assign it a non-allocated pointer. if so, we waive the null pointer comparison in library_call. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/32524#discussion_r4319430894 PR Review Comment: https://git.openjdk.org/jdk/pull/32524#discussion_r4319586724 PR Review Comment: https://git.openjdk.org/jdk/pull/32524#discussion_r4319668391 PR Review Comment: https://git.openjdk.org/jdk/pull/32524#discussion_r4319484320 PR Review Comment: https://git.openjdk.org/jdk/pull/32524#discussion_r4317296112 PR Review Comment: https://git.openjdk.org/jdk/pull/32524#discussion_r4318050411 PR Review Comment: https://git.openjdk.org/jdk/pull/32524#discussion_r4319087606 PR Review Comment: https://git.openjdk.org/jdk/pull/32524#discussion_r4319290115
