On 4/15/20 1:28 AM, David Holmes wrote:
On 14/04/2020 10:36 pm, coleen.phillim...@oracle.com wrote:
On 4/13/20 10:49 PM, David Holmes wrote:
Hi Coleen,
On 14/04/2020 12:34 am, coleen.phillim...@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
