Review Request: 6908267: Zero fails to unlock synchronized native methods on exception

[Gary Benson]

The original code in Zero was based on the x86 C++ interpreter, which
is where the methodology (and the comment) came from.  I was
suspicious when I first wrote it, and again while writing the
corresponding part of Shark, but by that time the Zero implementation
had passed the TCK and I didn't want to mess with it!

But, the deal is that you can't jump in C++ like you can in assembler,
so Zero never jumps -- all blocks of code are C++ functions, and all
control flow between them is call/return.  The only place in Zero that
would unlock that monitor is the bit I patched.

I can't be sure for the other C++ interpreters, but it looks like they
are wrong too.  If you grep for "remove_activation_entry" in the
HotSpot sources the only file it shows up in that doesn't have
"templateInterpreter" in its name is interpreterRuntime.cpp, and in
there the two references to it are both surrounded by "#ifndef
CC_INTERP".

Cheers,
Gary

Tom Rodriguez wrote:
> From what I can tell the unlocking for synchronized methods is
> always performed in Interpreter::_remove_activation_entry which
> tears down an interpreter frame in the event of an exception.
> We get there through
> InterpreterRuntime::exception_handler_for_exception.  I think
> the other cppInterpeter's have the same problem as zero.  In
> their case I believe the unlock code should be moved above the
> check for a pending exception though I'm unsure how that
> interacts with the notify_method_exit logic for JVMTI.
> 
> tom
> 
> On Dec 8, 2009, at 3:43 PM, David Holmes - Sun Microsystems wrote:
> > Hi Gary,
> > 
> > Gary Benson said the following on 12/09/09 00:58:
> > > Hi all,
> > > 
> > > If a synchronized native method throws an exception, Zero does not
> > > unlock it.  This leaves the receiver/mirror locked, with a stale
> > > monitor, which can cause the VM will crash at some unspecified time
> > > in the future.  This webrev fixes it.
> > > 
> > >  http://cr.openjdk.java.net/~gbenson/zero-fix-6908267/
> > 
> > Your original code indicates that it emulated the template
> > interpreter by not unlocking upon an exception. Have you
> > determined why this was wrong? The comments in the sparc and x86
> > interpreter have me concerned eg:
> > 
> > // With c++ interpreter we just leave it pending caller will do
> > // the correct thing. However...  Like x86 we ignore the result of
> > // the native call and leave the method locked. This seems wrong
> > // to leave things locked.
> > 
> > Though an earlier comment states the exception handling will
> > handle unlocking.
> > 
> > Just curious ...
> > 
> > David Holmes

--
http://gbenson.net/