8365192: post_meth_exit should be in vm state when calling get_jvmti_thread_state

[lmesnik]

The method
get_jvmti_thread_state()
should be called only while thread is in vm state.

The post_method_exit is doing some preparation before switching to vm state. This cause issues if thread is needed to initialize jvmti thread state.

The fix was found using jvmti stress agent and thus no additional regression test is required.

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• JDK-8365192: post_meth_exit should be in vm state when calling get_jvmti_thread_state (Bug - P4)

Reviewers

• Serguei Spitsyn (@sspitsyn - Reviewer) 🔄 Re-review required (review applies to 255c0ba8)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/26713/head:pull/26713
$ git checkout pull/26713

Update a local copy of the PR:
$ git checkout pull/26713
$ git pull https://git.openjdk.org/jdk.git pull/26713/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 26713

View PR using the GUI difftool:
$ git pr show -t 26713

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/26713.diff

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 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.

[openjdk]

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

[openjdk]

@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.

[mlbridge]

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)

[sspitsyn]

Thank you for catching and addressing this! How was the fix tested?
It looks okay at a glance but may give surprises.

[lmesnik]

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

[sspitsyn]

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.

[sspitsyn]

looks good

[dholmes-ora]

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 ??

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. ??

[lmesnik]

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.

[dholmes-ora]

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?

[lmesnik]

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.

[dholmes-ora]

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.

[dholmes-ora]

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?

[lmesnik]

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

[pchilano]

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.

[lmesnik]

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

[dholmes-ora]

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.

[lmesnik]

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.

[openjdk]

@lmesnik this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout 8365192
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push