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

Thu Apr 16 02:40:13 UTC 2020

On 4/15/20 1:28 AM, David Holmes wrote:
> 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.

David pointed out to me offline that the bytecode after monitorexit was 
not part of the exception region (see bug comments for details).

I'm withdrawing this RFR.

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