👋 Welcome back lmesnik! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@lmesnik This change is no longer ready for integration - check the PR body for details.

@lmesnik The following labels will be automatically applied to this pull request:

• hotspot
• serviceability

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

Webrevs

• 12: Full - Incremental (d56e3536)
• 11: Full - Incremental (ce132751)
• 10: Full - Incremental (106e5a1f)
• 09: Full - Incremental (67d87dbb)
• 08: Full - Incremental (6960b1aa)
• 07: Full - Incremental (320b93eb)
• 06: Full - Incremental (0008f725)
• 05: Full - Incremental (32b08587)
• 04: Full - Incremental (d9d21af6)
• 03: Full - Incremental (255c0ba8)
• 02: Full - Incremental (977c9eb4)
• 01: Full - Incremental (7cce73e3)
• 00: Full (47a32e8c)

I ran tier1-5 testing and separately verified that test pass with jvmti strass agent.

I ran tier1-5 testing and separately verified that test pass with jvmti strass agent.

Okay, thanks. I'd also run the tier 6 to be more safe.

I'm very confused by this issue. The description indicates we are calling get_jvmti_thread_state from the wrong state, but nowhere here or in JBS do I see anything showing me the actual failing code path. JBS has a part of a hs_err file that reports "# fatal error: LEAF method calling lock? " but I don't see how that relates to the stated problem of being in the wrong state ??

The comment in JBS shows the stacktrace from hs_err. I copied it here:
Stack: [0x00007f13537f9000,0x00007f13538f9000], sp=0x00007f13538f6b60, free space=1014k
Native frames: (J=compiled Java code, j=interpreted, Vv=VM code, C=native code)
V [libjvm.so+0x10c4458] JavaThread::check_for_valid_safepoint_state()+0xc8 (javaThread.cpp:401)
V [libjvm.so+0x16fe5e1] Mutex::lock()+0x41 (mutex.cpp:119)
V [libjvm.so+0x1403b7b] JvmtiEventControllerPrivate::thread_started(JavaThread*)+0x39b (mutexLocker.hpp:204)
V [libjvm.so+0x140b178] JvmtiExport::get_jvmti_thread_state(JavaThread*, bool) [clone .constprop.0]+0x98 (jvmtiExport.cpp:424)
V [libjvm.so+0x1415b57] JvmtiExport::post_method_exit(JavaThread*, Method*, frame)+0x77 (jvmtiExport.cpp:1834)
V [libjvm.so+0x1066b86] InterpreterRuntime::post_method_exit(JavaThread*)+0xe6 (interpreterRuntime.cpp:1278)

Also, I updated another comment to show how to reproduce the bug. The testing is not integrated in CI yet. However, it could be reproduced using our tests.

Also, the problem could be quite easy reproduced if assertion from the patch is added to
JvmtiExport::get_jvmti_thread_state(JavaThread *thread, bool allow_suspend) {

Also you state post_method_exit is not called when the method terminates via an exception, but I can't see that as being the case. The MethodExit callback in JVMTI is called under all termination conditions and post_method_exit seems similarly unconditional. ??

The method
void JvmtiExport::post_method_exit_inner(JavaThread* thread, methodHandle& mh, JvmtiThreadState *state, bool exception_exit, frame current_frame, jvalue& value) {
is used by
void JvmtiExport::notice_unwind_due_to_exception(JavaThread *thread, Method* method, address location, oop exception, bool in_handler_frame) {
to post MethodExit events.
And
void JvmtiExport::post_method_exit(JavaThread* thread, Method* method, frame current_frame) {
that also calls JvmtiExport::post_method_exit_inner() is used only when method exit normally.

T JvmtiExport::post_method_exit() has the variable exception_exit. This variable is set to true while the thread has exception that has been thrown but haven't caught yet. It might happens if VM or JNI made upcall while thread has ES_DETECTED _exception_state. So current method is exit normally while thread is processing exception. Pretty rare case that is reproduced in the test 'ExceptionOccured'.
The method is called from MethodExit event for the method where exception has been thrown.

The comment in JBS shows the stacktrace from hs_err.

But nothing in that hs_err snippet indicates that the problem is we are in the wrong state.

void JvmtiExport::post_method_exit(JavaThread* thread, Method* method, frame current_frame) that also calls JvmtiExport::post_method_exit_inner() is used only when method exit normally.

Thanks for clarifying. I was looking at remove_activation in the interpreter and did not see any special exception processing path.

So current method is exit normally while thread is processing exception.

Still struggling with this part. So the method exited normally then as part of event processing a Java upcall can raise an exception? But according to the spec any such exceptions get dropped - so is the flag just to do the dropping?

Still struggling with this part. So the method exited normally then as part of event processing a Java upcall can raise an exception? But according to the spec any such exceptions get dropped - so is the flag just to do the dropping?

Vice versa.
The exception is thrown in "usual" method. Usually, the stack is unwinded until exception is caught. However, it is possible to call the method (using upCall) in the middle of this process. And this 'upCall' method doesn't throw or caught any exceptions. Should just work normally.

Vice versa. The exception is thrown in "usual" method. Usually, the stack is unwinded until exception is caught. However, it is possible to call the method (using upCall) in the middle of this process. And this 'upCall' method doesn't throw or caught any exceptions. Should just work normally.

So the callback does an upcall that invokes the same method we are still posting the "method exit" for. That is a distinct frame/activation so the exit mode of the original should be immaterial. Though not sure how we avoid infinite loop in such a recursive case.

To fix the actual, simple, problem of being in the wrong state, it seems to me that this earlier suggestion is far easier to understand:

  {
    ThreadInVMfromJava __tiv(thread);
    state = get_jvmti_thread_state(thread);
  }

With the proposed deferral of the get_jvmti_thread_state and the deferred check for is_interp_only_mode I am left wondering if it is okay to call:

current_frame.interpreter_frame_result(&oop_result, &value);
current_frame.interpreter_frame_expression_stack_size() > 0

if we may not be in interp_only mode?

If the concern is the overhead of ThreadInVMfromJava can't we cheat here and use a direct state change as we are going from one safepoint-unsafe state to another?

I see that that
post_method_exit in the
void InterpreterMacroAssembler::notify_method_exit(..)
is called only when thread is in interp_only mode. So it is ok to call
current_frame.interpreter_frame_result(&oop_result, &value);

I did this, and why I trying to understand what would be result in the case of exception_exit == true I realized that it is still safe, because this method always exit normally.

So let me split this fix into 2 separate issues and fix them separately. I filed
https://bugs.openjdk.org/browse/JDK-8365937

to don't change posting because of exception_exit

To fix the actual, simple, problem of being in the wrong state, it seems to me that this earlier suggestion is far easier to understand:

  {
    ThreadInVMfromJava __tiv(thread);
    state = get_jvmti_thread_state(thread);
  }

With the proposed deferral of the get_jvmti_thread_state and the deferred check for is_interp_only_mode I am left wondering if it is okay to call:

current_frame.interpreter_frame_result(&oop_result, &value);
current_frame.interpreter_frame_expression_stack_size() > 0

if we may not be in interp_only mode?

InterpreterRuntime::post_method_exit is only called from the interpreter, so the top frame should always be interpreted.

If the concern is the overhead of ThreadInVMfromJava can't we cheat here and use a direct state change as we are going from one safepoint-unsafe state to another?

I think that once we transition to vm there is no point in going back to Java to then possibly transition back to vm. The only reason to delay the transition was to have the result Handle be created outside of the JRT_BLOCK scope, so that we are able to restore the return oop (if any) after the last safepoint. That means we could move JRT_BLOCK even further up, right after declaring the locals.

Agree with @pchilano, the only oop handling should be done before JRT_BLOCK. And the part dealing with with exception_exit will be implemented separately in the:
#26886

InterpreterRuntime::post_method_exit is only called from the interpreter, so the top frame should always be interpreted.

How does that relate to interp_only_mode though? I can't find anything that actually describes what that mode is and when we transition in/out of it.

The interp_only_mode is set in the SetEventNotificationMode. When this mode is set the stack is deoptimized and it became interpreted only (see EnterInterpOnlyModeClosure). It is set for post_method* events and these events are only posted from interpreted code. So the top frame as well as any frame should be interpreted in the post_method_exit even if we are not in interp_only_mode.
However, the InterpreterMacroAssembler::notify_method_entry() actually call the post_method_entry and post_method_exit only when thread is in the interpreter mode.