RFR (S) 8074292: nsk/jdb/kill/kill001: generateOopMap.cpp assert(bb->is_reachable()) failed

[David Holmes]

Hi Coleen,

On 14/04/2020 12:34 am, coleen.phillimore at oracle.com wrote:
> Summary: Do not install async exceptions at_safepoint for each bytecode.

I'm still not certain that we have to go this far to solve this problem, 
but it does sound like a relatively simple solution provided there are 
no unintended consequences.

> See CR for a lot more details.  This change calls a new 
> InterpreterRuntime::at_safepoint_async_safe() which installs the async 
> exception in the interpreter at backward branches and returns.  This 
> uses safepoint polling code in the interpreter for each platform.  These 
> changes (cross) compile on platforms that Oracle doesn't support but I 
> don't know if they work.
> 
> I'm not convinced the platform specific changes are necessary, because 
> calls to the runtime from many bytecodes will install the async 
> exception, so it's essentially installed "enough" for this deprecated 
> feature.  I tested the changes with *and* without the platform specific 
> changes with no failure, which included the jdb, jdi and jvmti 
> serviceability tests.

I don't understand what you mean here. If the whole basis of this fix is 
"don't install async exceptions other than at backward branches and 
returns" then how is that implemented without the changes in the 
interpreter code?

If this can be fixed just by adjusting the actual monitor code then I 
would much prefer that. It took me a while to get my head around the 
dispatch changes in interpreter code and even then I don't see how those 
changes only impact backward branches and returns ??

> 
> This change also makes InterpreterRuntime::monitorexit a JRT_LEAF 
> bytecode. The code to check for exceptions is outside the runtime call. 
> I ran the JCK vm and lang tests on this change with no failure.
> 
> Tested with tier1-6.
> 
> open webrev at http://cr.openjdk.java.net/~coleenp/2020/8074292.01/webrev
> bug link https://bugs.openjdk.java.net/browse/JDK-8074292

./cpu/x86/interp_masm_x86.cpp

It took me a long time to figure out how the new logic worked compared 
to the old logic. Even then I'm unclear about the effective recursive 
dispatch path: dispatch_base(generate_poll=true) -> dispatch_via -> 
dispatch_base(generate_poll=false) - does it work okay with 
VerifyActivationFrameSize? It seems a rather convoluted way to 
effectively just execute:

  858   lea(rscratch1, ExternalAddress((address)table));
  859   jmp(Address(rscratch1, rbx, Address::times_8));

---

src/hotspot/share/interpreter/interpreterRuntime.cpp

How were you able to drop this code:

791   if (elem == NULL || h_obj()->is_unlocked()) {
792     THROW(vmSymbols::java_lang_IllegalMonitorStateException());
793   }

?
  and this:

798 #ifdef ASSERT
799   thread->last_frame().interpreter_frame_verify_monitor(elem);
800 #endif

?

Thanks,
David

> Thanks,
> Coleen
>