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

Hi,

I have fixed the issue for optimizing compiler, now I started working on
BaseLine Compiler. Please find the patch attached with the mail. The build
getting successful , but when I try to run the program  I am getting the
gollowing exceptions
JikesRVM: TROUBLE.  Got a signal (Segmentation fault; #11) from outside the
VM's address space in thread 0xb7404940.
JikesRVM: UNRECOVERABLE trapped signal 11 (Segmentation fault)
handler stack 0
si->si_addr   (nil)
cs            0x00000073
ds            0x0000007b
es            0x0000007b
fs            0x00000000
gs            0x00000033
ss            0x0000007b
edi           0x00000bc7
esi -- PR/VP  0x58064edc
ebp           0xbfba4ab8
esp -- SP     0x5807dd18
ebx           0x58064edc
edx           0x00000000
ecx           0x5807dd28
eax           0x00000001
eip           0x00000000
trapno        0x0000000e
err           0x00000014
eflags        0x00210202
fpregs        89e1c54
oldmask       0x00000000
cr2           0x00000000
fp0 0x00000000000000000000
fp1 0x00000000000000000000
fp2 0x00000000000000000000
fp3 0x00000000000000000000
fp4 0x00000000000000000000
fp5 0x00000000000000000000
fp6 0x00000000000000000000
fp7 0x00000000000000000000
Segmentation fault (core dumped)

Any suggestions here?

Thank you

Pushparaj


On Tue, Apr 30, 2013 at 7:09 AM, Pushparaj Motamari
<[email protected]>wrote:

> 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 <[email protected]
> > 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 <[email protected]> wrote:
>>
>>> 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
>>>
>>>
>>
>

------------------------------------------------------------------------------
Learn Graph Databases - Download FREE O'Reilly Book
"Graph Databases" is the definitive new guide to graph databases and 
their applications. This 200-page book is written by three acclaimed 
leaders in the field. The early access version is available now. 
Download your free book today! http://p.sf.net/sfu/neotech_d2d_may

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

diff -urB /home/pushparaj/New_UnMd2/rvm/src/org/jikesrvm/compilers/baseline/ia32/BaselineCompilerImpl.java ./rvm/src/org/jikesrvm/compilers/baseline/ia32/BaselineCompilerImpl.java
--- /home/pushparaj/New_UnMd2/rvm/src/org/jikesrvm/compilers/baseline/ia32/BaselineCompilerImpl.java	2013-03-14 15:01:46.000000000 -0700
+++ ./rvm/src/org/jikesrvm/compilers/baseline/ia32/BaselineCompilerImpl.java	2013-05-07 18:15:07.896851315 -0700
@@ -3253,7 +3253,8 @@
       JNICompiler.generateGlueCodeForJNIMethod(asm, method, compiledMethod.getId());
       // set some constants for the code generation of the rest of the method
       // firstLocalOffset is shifted down because more registers are saved
-      firstLocalOffset = STACKFRAME_BODY_OFFSET - (JNICompiler.SAVED_GPRS_FOR_JNI << LG_WORDSIZE);
+      asm.emitPUSH_Reg(T0);
+      firstLocalOffset = STACKFRAME_BODY_OFFSET - (JNICompiler.SAVED_GPRS_FOR_JNI << LG_WORDSIZE)-WORDSIZE;
     } else {
       /* paramaters are on the stack and/or in registers;  There is space
        * on the stack for all the paramaters;  Parameter slots in the
@@ -3331,8 +3332,9 @@
       }
 
       // copy registers to callee's stackframe
-      firstLocalOffset = STACKFRAME_BODY_OFFSET - savedRegistersSize;
-      Offset firstParameterOffset = Offset.fromIntSignExtend(savedRegistersSize + STACKFRAME_HEADER_SIZE + (parameterWords << LG_WORDSIZE) - WORDSIZE);
+      asm.emitPUSH_Reg(T0);
+      firstLocalOffset = STACKFRAME_BODY_OFFSET - savedRegistersSize - WORDSIZE;
+      Offset firstParameterOffset = Offset.fromIntSignExtend(savedRegistersSize + STACKFRAME_HEADER_SIZE + (parameterWords << LG_WORDSIZE) - WORDSIZE+WORDSIZE);
       genParameterCopy(firstParameterOffset);
       int emptyStackOffset = (method.getLocalWords() << LG_WORDSIZE) - (parameterWords << LG_WORDSIZE);
       if (emptyStackOffset != 0) {
@@ -3444,6 +3446,7 @@
         int spaceToRelease = fp2spOffset(NO_SLOT).toInt() - bytesPopped - (3 * WORDSIZE);
         adjustStack(spaceToRelease, true);
       }
+      adjustStack(WORDSIZE, true);//throw away our extra local variable
       if (VM.VerifyAssertions) VM._assert(EBX_SAVE_OFFSET.toInt() == -(3 * WORDSIZE));
       asm.emitPOP_Reg(EBX);  // restore non-volatile EBX register
       if (VM.VerifyAssertions) VM._assert(EDI_SAVE_OFFSET.toInt() == -(2 * WORDSIZE));
Only in ./rvm/src/org/jikesrvm/compilers/baseline/ia32: BaselineCompilerImpl.java~