RFR: 8316397: StackTrace/Suspended/GetStackTraceSuspendedStressTest.java failed with: SingleStep event is NOT expected [v4]

[Patricio Chilano Mateo]

On Tue, 25 Feb 2025 10:54:14 GMT, Serguei Spitsyn <sspitsyn at openjdk.org> wrote:

>>> But note this same issue can happen with other events, the test is just checking for single step.
>> 
>> Does that mean we need the suspension check (and suspend) at other locations?
>> 
>> I am looking at `JRT_ENTRY` and it uses `ThreadInVMfromJava` - so why are we not checking for suspension in that transition somewhere, or else somewhere directly in the JRT_ENTRY? I guess maybe not all JRT_ENTRY points are safe for suspension? But then how do we know all the callers of `get_jvmti_thread_state` are safe for suspension?
>
>> What does the actual call-stack look like when we get here? We are obviously missing a suspension check for the current thread, but I still feel that should be higher-up the call-stack.
> 
> As Patricio pointed out (thanks, Patricio), with this particular test `GetStackTraceSuspendedStressTest.java` the `JvmtiExport::get_jvmti_thread_state` is called from `JvmtiExport::at_single_stepping_point()` which is called from `InterpreterRuntime::at_safepoint()`. There are no other points before to place the `ThreadBlockInVM`. It can be different for other events.
> 
> As an experiment I've added the following assert at the beginning of the `JvmtiExport::get_jvmti_thread_state()` and ran the mach5 tiers 1-6:
> 
> +  assert(!thread->owns_locks(), "all locks must be released before suspension");
> 
> and got many failures of the following tests:
> 
> applications/renaissance/RenaissanceStressTest.java
> applications/runthese/RunThese30M.java
> applications/skynet/SkyNet.java
> serviceability/jvmti/DynamicCodeGenerated/DynamicCodeGeneratedTest.java
> vmTestbase/nsk/jvmti/DynamicCodeGenerated/dyncodgen001/TestDescription.java
> vmTestbase/nsk/jvmti/scenarios/events/EM04/em04t001/TestDescription.java
> 
> 
> The assert was only triggered in the context of `post_dynamic_code_generated_while_holding_locks()` which make a direct call to `JvmtiExport::get_jvmti_thread_state()`.
> 
> A typical stack trace is:
> 
> Current thread (0x00007ff468008290):  JavaThread "ForkJoinPool-1-worker-4" daemon [_thread_in_vm, id=437394, stack(0x00007ff4eb6ea000,0x00007ff4eb7ea000) (1024K)]
> 
> Stack: [0x00007ff4eb6ea000,0x00007ff4eb7ea000],  sp=0x00007ff4eb7e5190,  free space=1004k
> Native frames: (J=compiled Java code, j=interpreted, Vv=VM code, C=native code)
> V  [libjvm.so+0x1223114]  JvmtiExport::get_jvmti_thread_state(JavaThread*)+0x94  (jvmtiExport.cpp:423)
> V  [libjvm.so+0x122628c]  JvmtiExport::post_dynamic_code_generated_while_holding_locks(char const*, unsigned char*, unsigned char*)+0x3c  (jvmtiExport.cpp:2636)
> V  [libjvm.so+0x19ad47e]  VtableStubs::find_stub(bool, int)+0x1ce  (vtableStubs.cpp:238)
> V  [libjvm.so+0xa81b0b]  CompiledIC::set_to_megamorphic(CallInfo*)+0x4b  (vtableStubs.hpp:107)
> V  [libjvm.so+0x17005a4]  SharedRuntime::handle_ic_miss_helper(JavaThread*)+0x2d4  (sharedRuntime.cpp:1637)
> V  [libjvm.so+0x1700a08]  SharedRuntime::handle_wrong_method_ic_miss(JavaThread*)+0xf8  (sharedRuntime.cpp:1441)
> v  ~RuntimeStub::Shared Runtime ic_miss_blob 0x00007ff50be5f449
> J 378 c1 java.util.concurrent.ForkJoinPool$WorkQueue.top...

> Does that mean we need the suspension check (and suspend) at other locations?
>
Yes, basically before posting any event. That’s why Serguei moved the check to `JvmtiExport::get_jvmti_thread_state()` to cover other events too. But I agree this method should not have side-effects. Also I don't see it is used for all events. I think we should probably use a new method.

> I am looking at JRT_ENTRY and it uses ThreadInVMfromJava - so why are we not checking for suspension in that transition somewhere, or else somewhere directly in the JRT_ENTRY? I guess maybe not all JRT_ENTRY points are safe for suspension? But then how do we know all the callers of get_jvmti_thread_state are safe for suspension?
>
We are checking for suspension in ~ThreadInVMfromJava(), the problem is that the check there is too late, and we have already posted the event. The fix aims to prevent the case where we suspend a thread, then we enable an event, and then the suspended thread posts that new enabled event without being resumed.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/23490#discussion_r1970548765