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