8340698: JVMTI FRAME_POP event is sometimes missed if NotifyFramePop …

…is called as a method is returning

Reviewed-by: cjplummer, amenkov

src/hotspot/share/prims/jvmtiEnvBase.cpp

@@ -1332,7 +1332,7 @@ JvmtiEnvBase::set_frame_pop(JvmtiThreadState* state, javaVFrame* jvf, jint depth
   if (jvf == nullptr) {
     return JVMTI_ERROR_NO_MORE_FRAMES;
   }
-  if (jvf->method()->is_native()) {
+  if (jvf->method()->is_native() || (depth == 0 && state->top_frame_is_exiting())) {
     return JVMTI_ERROR_OPAQUE_FRAME;
   }
   assert(jvf->frame_pointer() != nullptr, "frame pointer mustn't be null");

src/hotspot/share/prims/jvmtiExport.cpp

@@ -1874,6 +1874,10 @@ void JvmtiExport::post_method_exit(JavaThread* thread, Method* method, frame cur
     }
   }
 
+  // Do not allow NotifyFramePop to add new FramePop event request at
+  // depth 0 as it is already late in the method exiting dance.
+  state->set_top_frame_is_exiting();
+
   // Deferred transition to VM, so we can stash away the return oop before GC
   // Note that this transition is not needed when throwing an exception, because
   // there is no oop to retain.
@@ -1882,6 +1886,10 @@ void JvmtiExport::post_method_exit(JavaThread* thread, Method* method, frame cur
     post_method_exit_inner(thread, mh, state, exception_exit, current_frame, value);
   JRT_BLOCK_END
 
+  // The JRT_BLOCK_END can safepoint in ThreadInVMfromJava desctructor. Now it is safe to allow
+  // adding FramePop event requests as no safepoint can happen before removing activation.
+  state->clr_top_frame_is_exiting();
+
   if (result.not_null() && !mh->is_native()) {
     // We have to restore the oop on the stack for interpreter frames
     *(oop*)current_frame.interpreter_frame_tos_address() = result();

src/hotspot/share/prims/jvmtiThreadState.cpp

@@ -84,6 +84,7 @@ JvmtiThreadState::JvmtiThreadState(JavaThread* thread, oop thread_oop)
   _earlyret_value.j = 0L;
   _earlyret_oop = nullptr;
   _jvmti_event_queue = nullptr;
+  _top_frame_is_exiting = false;
   _is_virtual = false;
 
   _thread_oop_h = OopHandle(JvmtiExport::jvmti_oop_storage(), thread_oop);

src/hotspot/share/prims/jvmtiThreadState.hpp

@@ -190,6 +190,7 @@ class JvmtiThreadState : public CHeapObj<mtInternal> {
   bool              _pending_interp_only_mode;
   bool              _pending_step_for_popframe;
   bool              _pending_step_for_earlyret;
+  bool              _top_frame_is_exiting;
   int               _hide_level;
 
  public:
@@ -357,6 +358,11 @@ class JvmtiThreadState : public CHeapObj<mtInternal> {
   bool is_pending_step_for_earlyret()  { return _pending_step_for_earlyret;  }
   void process_pending_step_for_earlyret();
 
+  // For synchronization between NotifyFramePop and FramePop posting code.
+  void set_top_frame_is_exiting() { _top_frame_is_exiting = true;  }
+  void clr_top_frame_is_exiting() { _top_frame_is_exiting = false; }
+  bool top_frame_is_exiting()     { return _top_frame_is_exiting;  }
+
   // Setter and getter method is used to send redefined class info
   // when class file load hook event is posted.
   // It is set while loading redefined class and cleared before the

test/hotspot/jtreg/serviceability/jvmti/events/NotifyFramePopStressTest/NotifyFramePopStressTest.java

@@ -0,0 +1,130 @@
+/*
+ * Copyright (c) 2024, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+/**
+ * @test
+ * @bug 8340698
+ * @summary JVMTI FRAME_POP event is sometimes missed if NotifyFramePop is called as a method is returning
+ * @requires vm.jvmti
+ * @library /test/lib
+ * @compile NotifyFramePopStressTest.java
+ * @run main/othervm/native -agentlib:NotifyFramePopStressTest NotifyFramePopStressTest
+ */
+
+import jtreg.SkippedException;
+
+public class NotifyFramePopStressTest {
+    static volatile boolean done = false;
+
+    public static void main(String args[]) {
+        if (!canGenerateFramePopEvents()) {
+            throw new SkippedException("FramePop event is not supported");
+        }
+        Thread testThread = Thread.currentThread();
+        Thread controlThread = new Thread(() -> control(testThread), "Control Thread");
+
+        setFramePopNotificationMode(testThread);
+        controlThread.start();
+        sleep(10);
+
+        for (int i = 0; i < 10*1000; i++) {
+            foo();
+            bar();
+        }
+        done = true;
+
+        try {
+            controlThread.join();
+        } catch (InterruptedException e) {
+        }
+
+        if (failed()) {
+            throw new RuntimeException("Test FAILED: see log for details");
+        } else {
+            log("Test PASSED");
+        }
+    }
+
+    private static void sleep(long ms) {
+        try {
+            Thread.sleep(ms);
+        } catch (InterruptedException e) {
+        }
+    }
+
+    private static void control(Thread thread) {
+        int notifyCount = 0;
+
+        log("control has started");
+        while (!done) {
+            suspend(thread);
+            if (notifyFramePop(thread)) {
+                notifyCount++;
+                log("control incremented notifyCount to " + notifyCount);
+            }
+            resume(thread);
+            int waitCount = 0;
+            while (notifyCount != getPopCount()) {
+                sleep(1);
+                waitCount++;
+                if (waitCount > 1000) {
+                    break;
+                }
+            }
+            if (waitCount > 100) {
+                log("About to fail. notifyCount=" + notifyCount + " getPopCount()=" + getPopCount());
+                throw new RuntimeException("Test FAILED: Waited too long for notify: " + waitCount);
+            }
+        }
+        log("control has finished: " + notifyCount);
+    }
+
+    private native static void suspend(Thread thread);
+    private native static void resume(Thread thread);
+    private native static int getPopCount();
+    private native static boolean failed();
+    private native static boolean canGenerateFramePopEvents();
+    private native static void setFramePopNotificationMode(Thread thread);
+    private native static boolean notifyFramePop(Thread thread);
+
+    private static void log(String msg) {
+        System.out.println(msg);
+    }
+
+    private static int fetchIntFoo() {
+        return 13;
+    }
+
+    private static int fetchIntBar() {
+        return 33;
+    }
+
+    private static int foo() {
+        return fetchIntFoo();
+    }
+
+    private static int bar() {
+        return fetchIntBar();
+    }
+}
+

test/hotspot/jtreg/serviceability/jvmti/events/NotifyFramePopStressTest/libNotifyFramePopStressTest.cpp

@@ -0,0 +1,194 @@
+/*
+ * Copyright (c) 2024, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+#include <stdio.h>
+#include <string.h>
+#include "jvmti.h"
+#include "jvmti_common.hpp"
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+static jvmtiEnv *jvmti;
+static jvmtiCapabilities caps;
+static jvmtiEventCallbacks callbacks;
+static volatile jint pop_count;
+static char* volatile last_notify_method;
+static volatile jboolean failed = JNI_FALSE;
+static jboolean seenMain = JNI_FALSE;
+
+static jint Agent_Initialize(JavaVM *jvm, char *options, void *reserved);
+
+JNIEXPORT
+jint JNICALL Agent_OnLoad(JavaVM *jvm, char *options, void *reserved) {
+  return Agent_Initialize(jvm, options, reserved);
+}
+
+JNIEXPORT
+jint JNICALL Agent_OnAttach(JavaVM *jvm, char *options, void *reserved) {
+  return Agent_Initialize(jvm, options, reserved);
+}
+
+JNIEXPORT
+jint JNICALL JNI_OnLoad(JavaVM *jvm, void *reserved) {
+  return JNI_VERSION_9;
+}
+
+static void JNICALL
+FramePop(jvmtiEnv *jvmti, JNIEnv *jni, jthread thread,
+         jmethodID method, jboolean wasPoppedByException) {
+  jvmtiError err;
+  jclass cls = nullptr;
+  char* csig = nullptr;
+  char* name = nullptr;
+
+  err = jvmti->GetMethodDeclaringClass(method, &cls);
+  check_jvmti_status(jni, err, "FramePop: Failed in JVMTI GetMethodDeclaringClass");
+
+  err = jvmti->GetClassSignature(cls, &csig, nullptr);
+  check_jvmti_status(jni, err, "FramePop: Failed in JVMTI GetClassSignature");
+
+  name = get_method_name(jvmti, jni, method);
+  LOG("FramePop(%d) event from method: %s %s\n", pop_count + 1, csig, name);
+
+  if (strcmp(name, "main") != 0) { // ignore FRAME_POP for main that comes in as the test exits
+    if (strcmp(name, (char*)last_notify_method) != 0) {
+      LOG("ERROR: FramePop event is for wrong method: expected %s, got %s\n", last_notify_method, name);
+      failed = JNI_TRUE;
+    }
+  }
+  pop_count++;
+  deallocate(jvmti, jni, csig);
+  deallocate(jvmti, jni, name);
+}
+
+static
+jint Agent_Initialize(JavaVM *jvm, char *options, void *reserved) {
+  jint res;
+  jvmtiError err;
+
+  res = jvm->GetEnv((void **) &jvmti, JVMTI_VERSION_9);
+  if (res != JNI_OK || jvmti == nullptr) {
+    LOG("GetEnv(JVMTI_VERSION_9) failed error(%d)", res);
+    return JNI_ERR;
+  }
+  err = jvmti->GetPotentialCapabilities(&caps);
+  check_jvmti_error(err, "Agent: GetPotentialCapabilities failed");
+
+  err = jvmti->AddCapabilities(&caps);
+  check_jvmti_error(err, "Agent: AddCapabilities failed");
+
+  err = jvmti->GetCapabilities(&caps);
+  check_jvmti_error(err, "Agent: GetCapabilities failed");
+
+  if (caps.can_generate_frame_pop_events) {
+    callbacks.FramePop = &FramePop;
+    err = jvmti->SetEventCallbacks(&callbacks, sizeof(callbacks));
+    check_jvmti_error(err, "Agent: SetEventCallbacks failed");
+  }
+  return JNI_OK;
+}
+
+JNIEXPORT jboolean JNICALL
+Java_NotifyFramePopStressTest_canGenerateFramePopEvents(JNIEnv *env, jclass cls) {
+  return caps.can_generate_frame_pop_events ? JNI_TRUE : JNI_FALSE;
+}
+
+JNIEXPORT void JNICALL
+Java_NotifyFramePopStressTest_setFramePopNotificationMode(JNIEnv *env, jclass cl, jthread thread) {
+  set_event_notification_mode(jvmti, env, JVMTI_ENABLE, JVMTI_EVENT_FRAME_POP, thread);
+}
+
+/*
+ * Call NotifyFramePop on the current frame.
+ */
+JNIEXPORT jboolean JNICALL
+Java_NotifyFramePopStressTest_notifyFramePop(JNIEnv *jni, jclass cls, jthread thread) {
+  jmethodID method;
+  jlocation loc;
+  char* name;
+  jvmtiError err;
+  jboolean isMain;
+
+  err = jvmti->GetFrameLocation(thread, 0, &method, &loc);
+  check_jvmti_status(jni, err, "notifyFramePop: Failed in JVMTI GetFrameLocation");
+
+  name = get_method_name(jvmti, jni, method);
+
+  // We only want to do a NotifyFramePop once for the main method. The sole purpose is
+  // to force the thread into interpOnly mode, which seems to help the test's timing
+  // in a way that makes it more likely to reproduce the issue.
+  isMain = (strcmp(name, "main") == 0);
+  if (isMain) {
+    if (seenMain) {
+      deallocate(jvmti, jni, name);
+      return JNI_FALSE; // Only do NotifyFramePop once for main()
+    } else {
+      seenMain = JNI_TRUE;
+    }
+  }
+
+  err= jvmti->NotifyFramePop(thread, 0);
+  if (err == JVMTI_ERROR_OPAQUE_FRAME || err == JVMTI_ERROR_DUPLICATE) {
+    //LOG("\nNotifyFramePop for method %s returned acceptable error: %s\n", name, TranslateError(err));
+    deallocate(jvmti, jni, name);
+    return JNI_FALSE;
+  }
+  check_jvmti_status(jni, err, "notifyFramePop: Failed in JVMTI notifyFramePop");
+  LOG("\nNotifyFramePop called for method %s\n", name);
+
+  if (isMain) {
+    LOG("notifyFramePop not counting main method\n");
+    deallocate(jvmti, jni, name);
+    return JNI_FALSE;
+  } else {
+    deallocate(jvmti, jni, last_notify_method);
+    last_notify_method = name;
+    return JNI_TRUE;
+  }
+}
+
+JNIEXPORT void JNICALL
+Java_NotifyFramePopStressTest_suspend(JNIEnv *jni, jclass cls, jthread thread) {
+  suspend_thread(jvmti, jni, thread);
+}
+
+JNIEXPORT void JNICALL
+Java_NotifyFramePopStressTest_resume(JNIEnv *jni, jclass cls, jthread thread) {
+  resume_thread(jvmti, jni, thread);
+}
+
+JNIEXPORT jint JNICALL
+Java_NotifyFramePopStressTest_getPopCount(JNIEnv *env, jclass cls) {
+  return pop_count;
+}
+
+JNIEXPORT jboolean JNICALL
+Java_NotifyFramePopStressTest_failed(JNIEnv *env, jclass cls) {
+  return failed;
+}
+
+#ifdef __cplusplus
+}
+#endif