Re: [rvm-research] Approach for fixing the RVM-1017(Implicit unlock in synchronized method relies on local variable 0 )

Pushparaj Motamari <[email protected]> wrote on 04/29/2013 04:32:44 PM:
>
> I am working on the fix for RVM-1017(Implicit unlock in synchronized
> method relies on local variable 0 ). I tried by reserving one extra
> localword in NormalMethod.java, now it adds extra word to every
> method, but it can be changed by putting "if(method.isSynchronized
> ())". I am attaching the patch with the mail. When I do precommit
> with this patch for development configuration (dacapo), some
> testcases are failing ..like eclipse. But with prototype
> onfiguration i.e (basic and opttests) it is successful. I would like
> to know other approaches to fixing this.
>

Hi,

	I'd think about it from the perspective of what parts of the system
need to know how to get to the this pointer.  In addition to the obvious
need of the compilers to generate code in the prologue/epilogue of a
synchronized instance method to lock/unlock this, we also need to be able
to unlock when unwinding the stack frame due to an exception being
processed.  The GC will also need to know about it in order to make sure it
is recognized as a root during GC and processed.

	I'd also go simple first, then try to optimize it after it was
working.

	I think the fix to the opt compiler is actually much simpler than the
baseline compilers.  For the opt compiler, I would just add a new temporary
"cachedThis" in the GenerationContext and use it instead of localO in
getLockObject.  In the prologue, add an assignment to cachedThis from
local0.  Since the opt compiler (in GenerationContext) wraps the real body
of the synchronized method in a generated try/catch that uses getLockObject
to unlock this and then rethrow the exception when we need to unwind the
method, just tweaking GenerationContext to stash away the original value of
this should fix everything.  The fix for the opt compiler should be
confined entirely to GenerationContext.

	In the baseline compiler, things are much more annoying.  You really
need an entry in the stackframe to hold the this pointer.  You'll need to
store it there in the prologue, make sure the GC maps know about it, and
use it to unlock in the method epilogue and the exception delivery code
(unwindStackFrame in BaselineExceptionDeliverer).

	You're trying to get yourself a stackslot in the baseline compiler by
modifying NormalMethod to add an extra local word.  I think I would not
approach it this way, because you will have to track down all the places in
the system that do some computation based on getLocalWords() and adjust
them.  Not pretty.  And you only need an explicit extra word in the
stackframe for the baseline compiler.   So, I'd probably go after it by
unconditionally making the baseline compiler's stackframe header have an
extra word (next to the CMID), stashing the this pointer there in the
prologue of a synchronized instance method (before the yieldpoint),
teaching the GC mapping code to include this word for a synchronized
instance method, and using this word to unlock in the exception handler and
method epilogue.   After this is working robustly, then you could try to
move it out of the fixed header and treat it as if there was an extra local
for synchronized instance methods to avoid adding a word to the stackframe
for all methods.  I doubt this is actually worth the complexity of doing,
but it probably could be done.

--dave

------------------------------------------------------------------------------
Try New Relic Now & We'll Send You this Cool Shirt
New Relic is the only SaaS-based application performance monitoring service 
that delivers powerful full stack analytics. Optimize and monitor your
browser, app, & servers with just a few lines of code. Try New Relic
and get this awesome Nerd Life shirt! http://p.sf.net/sfu/newrelic_d2d_apr

_______________________________________________
Jikesrvm-researchers mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/jikesrvm-researchers