diff --git a/src/hotspot/cpu/ppc/frame_ppc.cpp b/src/hotspot/cpu/ppc/frame_ppc.cpp index 1778c753ad4..a077c54d28e 100644 --- a/src/hotspot/cpu/ppc/frame_ppc.cpp +++ b/src/hotspot/cpu/ppc/frame_ppc.cpp @@ -48,7 +48,6 @@ void RegisterMap::check_location_valid() { #endif // ASSERT bool frame::safe_for_sender(JavaThread *thread) { - bool safe = false; address sp = (address)_sp; address fp = (address)_fp; address unextended_sp = (address)_unextended_sp; @@ -76,29 +75,23 @@ bool frame::safe_for_sender(JavaThread *thread) { // An fp must be within the stack and above (but not equal) sp. bool fp_safe = (fp <= thread->stack_base()) && (fp > sp); - // An interpreter fp must be within the stack and above (but not equal) sp. - // Moreover, it must be at least the size of the ijava_state structure. - bool fp_interp_safe = (fp <= thread->stack_base()) && (fp > sp) && - ((fp - sp) >= ijava_state_size); + // An interpreter fp must be fp_safe. + // Moreover, it must be at a distance at least the size of the ijava_state structure. + bool fp_interp_safe = fp_safe && ((fp - sp) >= ijava_state_size); // We know sp/unextended_sp are safe, only fp is questionable here // If the current frame is known to the code cache then we can attempt to - // to construct the sender and do some validation of it. This goes a long way + // construct the sender and do some validation of it. This goes a long way // toward eliminating issues when we get in frame construction code if (_cb != NULL ){ - // Entry frame checks - if (is_entry_frame()) { - // An entry frame must have a valid fp. - return fp_safe && is_entry_frame_valid(thread); - } - // Now check if the frame is complete and the test is - // reliable. Unfortunately we can only check frame completeness for - // runtime stubs and nmethods. Other generic buffer blobs are more - // problematic so we just assume they are OK. Adapter blobs never have a - // complete frame and are never OK + // First check if the frame is complete and the test is reliable. + // Unfortunately we can only check frame completeness for runtime stubs + // and nmethods. Other generic buffer blobs are more problematic + // so we just assume they are OK. + // Adapter blobs never have a complete frame and are never OK if (!_cb->is_frame_complete_at(_pc)) { if (_cb->is_compiled() || _cb->is_adapter_blob() || _cb->is_runtime_stub()) { return false; @@ -110,10 +103,23 @@ bool frame::safe_for_sender(JavaThread *thread) { return false; } + // Entry frame checks + if (is_entry_frame()) { + // An entry frame must have a valid fp. + return fp_safe && is_entry_frame_valid(thread); + } + if (is_interpreted_frame() && !fp_interp_safe) { return false; } + // At this point, there still is a chance that fp_safe is false. + // In particular, (fp == NULL) might be true. So let's check and + // bail out before we actually dereference from fp. + if (!fp_safe) { + return false; + } + abi_minframe* sender_abi = (abi_minframe*) fp; intptr_t* sender_sp = (intptr_t*) fp; address sender_pc = (address) sender_abi->lr;; diff --git a/src/hotspot/cpu/s390/frame_s390.cpp b/src/hotspot/cpu/s390/frame_s390.cpp index 9735171b340..b752e9dfedb 100644 --- a/src/hotspot/cpu/s390/frame_s390.cpp +++ b/src/hotspot/cpu/s390/frame_s390.cpp @@ -52,7 +52,6 @@ void RegisterMap::check_location_valid() { // Profiling/safepoint support bool frame::safe_for_sender(JavaThread *thread) { - bool safe = false; address sp = (address)_sp; address fp = (address)_fp; address unextended_sp = (address)_unextended_sp; @@ -80,29 +79,23 @@ bool frame::safe_for_sender(JavaThread *thread) { // An fp must be within the stack and above (but not equal) sp. bool fp_safe = (fp <= thread->stack_base()) && (fp > sp); - // An interpreter fp must be within the stack and above (but not equal) sp. - // Moreover, it must be at least the size of the z_ijava_state structure. - bool fp_interp_safe = (fp <= thread->stack_base()) && (fp > sp) && - ((fp - sp) >= z_ijava_state_size); + // An interpreter fp must be fp_safe. + // Moreover, it must be at a distance at least the size of the z_ijava_state structure. + bool fp_interp_safe = fp_safe && ((fp - sp) >= z_ijava_state_size); // We know sp/unextended_sp are safe, only fp is questionable here // If the current frame is known to the code cache then we can attempt to - // to construct the sender and do some validation of it. This goes a long way + // construct the sender and do some validation of it. This goes a long way // toward eliminating issues when we get in frame construction code if (_cb != NULL ) { - // Entry frame checks - if (is_entry_frame()) { - // An entry frame must have a valid fp. - return fp_safe && is_entry_frame_valid(thread); - } - // Now check if the frame is complete and the test is - // reliable. Unfortunately we can only check frame completeness for - // runtime stubs. Other generic buffer blobs are more - // problematic so we just assume they are OK. Adapter blobs never have a - // complete frame and are never OK. nmethods should be OK on s390. + // First check if the frame is complete and the test is reliable. + // Unfortunately we can only check frame completeness for runtime stubs. + // Other generic buffer blobs are more problematic so we just assume they are OK. + // Adapter blobs never have a complete frame and are never OK. + // nmethods should be OK on s390. if (!_cb->is_frame_complete_at(_pc)) { if (_cb->is_adapter_blob() || _cb->is_runtime_stub()) { return false; @@ -114,13 +107,26 @@ bool frame::safe_for_sender(JavaThread *thread) { return false; } + // Entry frame checks + if (is_entry_frame()) { + // An entry frame must have a valid fp. + return fp_safe && is_entry_frame_valid(thread); + } + if (is_interpreted_frame() && !fp_interp_safe) { return false; } + // At this point, there still is a chance that fp_safe is false. + // In particular, (fp == NULL) might be true. So let's check and + // bail out before we actually dereference from fp. + if (!fp_safe) { + return false; + } + z_abi_160* sender_abi = (z_abi_160*) fp; intptr_t* sender_sp = (intptr_t*) sender_abi->callers_sp; - address sender_pc = (address) sender_abi->return_pc; + address sender_pc = (address) sender_abi->return_pc; // We must always be able to find a recognizable pc. CodeBlob* sender_blob = CodeCache::find_blob_unsafe(sender_pc); diff --git a/src/hotspot/os_cpu/linux_ppc/thread_linux_ppc.cpp b/src/hotspot/os_cpu/linux_ppc/thread_linux_ppc.cpp index 3f49feddf5a..d25cab8c592 100644 --- a/src/hotspot/os_cpu/linux_ppc/thread_linux_ppc.cpp +++ b/src/hotspot/os_cpu/linux_ppc/thread_linux_ppc.cpp @@ -34,6 +34,8 @@ frame JavaThread::pd_last_frame() { address pc = _anchor.last_Java_pc(); // Last_Java_pc ist not set, if we come here from compiled code. + // Assume spill slot for link register contains a suitable pc. + // Should have been filled by method entry code. if (pc == NULL) { pc = (address) *(sp + 2); } @@ -64,6 +66,17 @@ bool JavaThread::pd_get_top_frame_for_profiling(frame* fr_addr, void* ucontext, return false; } + if (ret_frame.fp() == NULL) { + // The found frame does not have a valid frame pointer. + // Bail out because this will create big trouble later on, either + // - when using istate, calculated as (NULL - ijava_state_size) or + // - when using fp() directly in safe_for_sender() + // + // There is no conclusive description (yet) how this could happen, but it does. + // For more details on what was observed, see thread_linux_s390.cpp + return false; + } + if (ret_frame.is_interpreted_frame()) { frame::ijava_state *istate = ret_frame.get_ijava_state(); const Method *m = (const Method*)(istate->method); diff --git a/src/hotspot/os_cpu/linux_s390/thread_linux_s390.cpp b/src/hotspot/os_cpu/linux_s390/thread_linux_s390.cpp index c6b183fccc2..279e23fd34c 100644 --- a/src/hotspot/os_cpu/linux_s390/thread_linux_s390.cpp +++ b/src/hotspot/os_cpu/linux_s390/thread_linux_s390.cpp @@ -34,6 +34,8 @@ frame JavaThread::pd_last_frame() { address pc = _anchor.last_Java_pc(); // Last_Java_pc ist not set if we come here from compiled code. + // Assume spill slot for Z_R14 (return register) contains a suitable pc. + // Should have been filled by method entry code. if (pc == NULL) { pc = (address) *(sp + 14); } @@ -51,6 +53,9 @@ bool JavaThread::pd_get_top_frame_for_profiling(frame* fr_addr, void* ucontext, return true; } + // At this point, we don't have a last_Java_frame, so + // we try to glean some information out of the ucontext + // if we were running Java code when SIGPROF came in. if (isInJava) { ucontext_t* uc = (ucontext_t*) ucontext; frame ret_frame((intptr_t*)uc->uc_mcontext.gregs[15/*Z_SP*/], @@ -61,6 +66,38 @@ bool JavaThread::pd_get_top_frame_for_profiling(frame* fr_addr, void* ucontext, return false; } + if (ret_frame.fp() == NULL) { + // The found frame does not have a valid frame pointer. + // Bail out because this will create big trouble later on, either + // - when using istate, calculated as (NULL - z_ijava_state_size (= 0x70 (dbg) or 0x68 (rel)) or + // - when using fp() directly in safe_for_sender() + // + // There is no conclusive description (yet) how this could happen, but it does: + // + // We observed a SIGSEGV with the following stack trace (openjdk.jdk11u-dev, 2021-07-07, linuxs390x fastdebug) + // V [libjvm.so+0x12c8f12] JavaThread::pd_get_top_frame_for_profiling(frame*, void*, bool)+0x142 + // V [libjvm.so+0xb1020c] JfrGetCallTrace::get_topframe(void*, frame&)+0x3c + // V [libjvm.so+0xba0b08] OSThreadSampler::protected_task(os::SuspendedThreadTaskContext const&)+0x98 + // V [libjvm.so+0xff33c4] os::SuspendedThreadTask::internal_do_task()+0x14c + // V [libjvm.so+0xfe3c9c] os::SuspendedThreadTask::run()+0x24 + // V [libjvm.so+0xba0c66] JfrThreadSampleClosure::sample_thread_in_java(JavaThread*, JfrStackFrame*, unsigned int)+0x66 + // V [libjvm.so+0xba1718] JfrThreadSampleClosure::do_sample_thread(JavaThread*, JfrStackFrame*, unsigned int, JfrSampleType)+0x278 + // V [libjvm.so+0xba4f54] JfrThreadSampler::task_stacktrace(JfrSampleType, JavaThread**) [clone .constprop.62]+0x284 + // V [libjvm.so+0xba5e54] JfrThreadSampler::run()+0x2ec + // V [libjvm.so+0x12adc9c] Thread::call_run()+0x9c + // V [libjvm.so+0xff5ab0] thread_native_entry(Thread*)+0x128 + // siginfo: si_signo: 11 (SIGSEGV), si_code: 1 (SEGV_MAPERR), si_addr: 0xfffffffffffff000 + // failing instruction: e320 6008 0004 LG r2,8(r0,r6) + // contents of r6: 0xffffffffffffff90 + // + // Here is the sequence of what happens: + // - ret_frame is constructed with _fp == NULL (for whatever reason) + // - ijava_state_unchecked() calculates it's result as + // istate = fp() - z_ijava_state_size() = NULL - 0x68 DEBUG_ONLY(-8) + // - istate->method dereferences memory at offset 8 from istate + return false; + } + if (ret_frame.is_interpreted_frame()) { frame::z_ijava_state* istate = ret_frame.ijava_state_unchecked(); if (stack_base() >= (address)istate && (address)istate > stack_end()) {