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

[Pushparaj M. <pus...@gm...>]

Hi,

Please ignore the previous mail. I have found the issue and fixed it. Now I
will start work on fix for the BaseLineCompiler.


Thank you

Pushparaj

On Tue, Apr 30, 2013 at 5:51 AM, Pushparaj Motamari
<pus...@gm...>wrote:

> Hi,
>
> When I build the Jikes after chanigng the GenerationContext.java as you
> suggested, the precommit run successful,but when I try to run a testcase
> which modifies the localvariable 0. It is failing. I am attaching the patch
> and the testcase with the mail.
>
> Thank You
>
> Pushparaj
>
> On Tue, Apr 30, 2013 at 3:48 AM, David P Grove <gr...@us...> wrote:
>
>> Pushparaj Motamari <pus...@gm...> 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
>> Jik...@li...
>> https://lists.sourceforge.net/lists/listinfo/jikesrvm-researchers
>>
>>
>