review for 7009361: JSR 292 Invalid value on stack on solaris-sparc with -Xcomp

[Vladimir Kozlov]

Yes, it is good enough.

Thanks,
Vladimir

Tom Rodriguez wrote:
> On May 2, 2011, at 10:24 AM, Vladimir Kozlov wrote:
> 
>> Tom Rodriguez wrote:
>>> On Apr 29, 2011, at 11:05 PM, Vladimir Kozlov wrote:
>>>> Looks fine as far as I understand it and thank you for fixing loop predicates output.
>>>> One thing which was not clear is registers usage in 32-bit code in trace_method_handle() in methodHandles_x86.cpp. Yes, from caller of trace_method_handle() you can find it out but in trace_method_handle() it looks like mess.
>>> What would you like to see?
>> Register naming something like r_method_handle and r_saved_sp to have the same call code for 32- and 64-bit VM. Also why you move values to c_rarg? super_call_VM_leaf() will move them again.
> 
> It won't move them if they are already in the right location.  One of the registers I'm passing, rcx, is part of the calling convention and its order in the calling convention changes depending on platform, so I'm passing them by hand to make sure they aren't clobbered.  I added some comments:
> 
> #ifdef _LP64
>   // Pass arguments carefully since the registers overlap with the calling convention.                                                               
>   // rcx: method handle                                                                                                                               
>   // r13: saved sp                                                                                                                                   
>   __ mov(c_rarg2, rcx); // mh                                                                                                                         
>   __ mov(c_rarg1, r13); // saved sp                                                                                                                   
>   __ mov(c_rarg3, rsp); // sp                                                                                                                         
>   __ movptr(c_rarg0, (intptr_t) adaptername);
>   __ super_call_VM_leaf(CAST_FROM_FN_PTR(address, trace_method_handle_stub), c_rarg0, c_rarg1, c_rarg2, c_rarg3);
> #else
>   // arguments:                                                                                                                                       
>   // rcx: method handle                                                                                                                               
>   // rsi: saved sp                                                                                                                                   
>   __ movptr(rbx, (intptr_t) adaptername);
>   __ super_call_VM_leaf(CAST_FROM_FN_PTR(address, trace_method_handle_stub), rbx, rsi, rcx, rsp);
> #endif
> 
> Is that good enough?
> 
> tom
> 
> 
>> Vladimir
>>
>>> tom
>>>> Vladimir
>>>>
>>>> On 4/29/11 3:29 PM, Tom Rodriguez wrote:
>>>>> http://cr.openjdk.java.net/~never/7009361
>>>>>
>>>>> 7009361: JSR 292 Invalid value on stack on solaris-sparc with -Xcomp
>>>>> Reviewed-by:
>>>>>
>>>>> In the invokedynamic world the signature of the method at an invoke
>>>>> bytecode might note be the same as the signature of the callee you
>>>>> finally end up in.  This all works correctly during normal execution
>>>>> but it can break the logic that deopt uses to layout the frames.  In
>>>>> particular on sparc we attempt to place the locals in the same
>>>>> location that the interpreter execution would have produced by using
>>>>> the callers expression stack address and callee->size_of_parameters()
>>>>> to figure out where the top of the live stack is.  If the size of the
>>>>> parameters at the original call site is less than the size of the
>>>>> parameters of the actual callee then the locals will end up top of the
>>>>> callers live expression stack.  The x86 version of this code places
>>>>> the locals at the bottom of the frame which keeps this from happening
>>>>> and I've modified sparc to do the same.  There's no reason they need
>>>>> to be in the same location.
>>>>>
>>>>> The other potential problem is that deopt also has logic that makes
>>>>> sure the existing caller is enlarged enough to account for the
>>>>> difference between the callee parameters and the callee locals.  In
>>>>> the current world we don't really need to enlarge this space because
>>>>> the method handle machinery also operates like the interpreter so it
>>>>> extends the caller frame when injecting extra arguments, thus making
>>>>> sure there's really enough space when we deopt.  Since it doesn't have
>>>>> to work this way I decided to fix this logic to grab the caller notion
>>>>> of the number of arguments and correct the last frame adjust using
>>>>> this value.
>>>>>
>>>>> Additionally the TraceMethodHandles logic was very broken in x86 so I
>>>>> fixed it.  It was mainly broken because some of the
>>>>> trace_method_handle calls are generated using an
>>>>> InterpreterMacroAssembler which has extra asserts in call_VM_leaf_base
>>>>> that don't really apply for the method handle adapters.  There were
>>>>> also problems with the number of arguments being passed in 64 bit.  I
>>>>> ended up moving super_call_VM_leaf up into MacroAssembler since
>>>>> there's no way to figure out that you are using an
>>>>> InterpreterMacroAssembler versus a MacroAssembler.
>>>>>
>>>>> To debug this I added a new printing function,
>>>>> JavaThread::print_frame_layout, that prints an annotated view of the
>>>>> stack memory similar to what the SA's memory viewer does.  It also
>>>>> includes some logic to check that the space owned by a frame isn't
>>>>> claimed by another frame.  That actually detects the original bug
>>>>> immediately.  It's not as full featured as it might be but it reports
>>>>> everything you need to know about interpreter frames.
>>>>>
>>>>> I also made a small change in loopPredicate because the ttyLocker was
>>>>> producing spurious output in the log all the time.
>>>>>
>>>>> Tested with original failing test from test and DeoptimizeALot testing
>>>>> on sparc.
>>>>>
>