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

[David Holmes]

On 14/04/2020 10:36 pm, coleen.phillimore at oracle.com wrote:
> On 4/13/20 10:49 PM, David Holmes wrote:
>> 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 ??
> 
> You have to read the comments in the bug again.  There *is* special code 
> to not install the async exception in the monitorexit code. That is not 
> enough to prevent this bug.  You can also read the old bug report you 
> linked to this one.

I know there is some special handling in monitorexit already, I was 
referring to additional special handling around the monitorexit to 
disable whatever piece of code is currently installing the async exception.

> The interpreter code dispatch_next passes "true" if it's a backwards 
> branch, that's how it can tell.

Okay - I see dispatch_next is passed generate_poll=true for returns but 
I can't see where backward branches come into play. Your changes cause 
the at_safepoint_async_safe to be called in that case, whereas any other 
code paths that lead to at_safepoint no longer install the async 
exception. So things are a bit clearer in that regard.

> My point was that there are enough code paths that install async 
> exceptions *other than monitorenter and monitorexit* that maybe it's not 
> necessary to install them at backwards branches and returns.  I suppose 
> someone could construct a test case to show otherwise.

Yes I understood that was the query, but without knowing exactly where 
those code paths are it is hard to comment on whether there is adequate 
coverage. Even with installing on backward branches there is a risk that 
small loops can be unrolled and so lose the check (indeed that is what 
"counted loops" in the JIT does) and thus we rely on these other code 
paths anyway.

>>> 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));
> 
> I could test it with VerifyActivationFrameSize.  Or I could just add the 
> two instructions per platform.  I mostly copied the code in 
> generate_safept_entry_for.  It might be better to just copy the 
> instructions.
> 
>>
>> ---
>>
>> 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:
> 
> The caller throws the exception for these cases.

Sorry but I don't see that in the caller e.g. 
InterpreterMacroAssembler::unlock_object. Further you are not allowing 
for elem to be NULL which will lead to a crash when you dereference it.

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

Possibly, but I assume the pairing in monitorenter and monitorexit 
around the actual monitor operation is to ensure that the stack frame 
info is not unexpectedly messed up. Maybe it is redundant to call before 
and after, but then maybe the same is true in monitorenter.

David
-----

> Coleen
>> ?
>>
>> Thanks,
>> David
>>
>>> Thanks,
>>> Coleen
>>>
>