[aarch64-port-dev ] TestStackBangRbp hangs

[Andrew Haley]

On 01/13/2014 01:37 PM, Andrew Dinn wrote:
> On 13/01/14 11:55, Andrew Haley wrote:
>> It turns out that there were three separate bugs.
>>
>> This one in shared code:
>>
>> http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2014-January/012938.html
> 
> Yes, that makes sense.
> 
>> And two others in AArch64-specific code.  The problem was that when we
>> returned from deoptimizing we were not properly removing the stack
>> frame of the deoptimized method.  All we were doing was:
>>
>>   // Pop deoptimized frame
>>   __ ldrw(r2, Address(r5, Deoptimization::UnrollBlock::size_of_deoptimized_frame_offset_in_bytes()));
>>   __ add(sp, sp, r2);
>>
>> Clearly, this does not work.  We much also restore SP and LR.  We need
>> to do this:
> 
> That doesn't make sense to me. Why do we need to restore SP and LR? They
> get reset later (see below).
> 
>>   // Pop deoptimized frame
>>   __ ldrw(r2, Address(r5, Deoptimization::UnrollBlock::size_of_deoptimized_frame_offset_in_bytes()));
>>   __ sub(r2, r2, 2 * wordSize);
>>   __ add(sp, sp, r2);
>>   __ ldp(rfp, lr, __ post(sp, 2 * wordSize));
>>
>> But -- amazingly -- there hardly ever was a failure caused by this
>> fundamental mistake.
> 
> In the deopt case rfp and lr are patched up after the interpreter frames
> have been pushed i.e. at line 2660:
> 
>   // Pick up the initial fp we should save
>   __ ldr(rfp,
> 	 Address(r4, Deoptimization::UnrollBlock::initial_info_offset_in_bytes()));
> 
> and at line 2694:
> 
>   __ ldr(lr, Address(r2, 0));	  // save final return address
>   // Re-push self-frame
>   __ enter();			  // & old rfp & set new rfp

It's too late by then: bang_stack_size() on Line 2670 might have
trapped, and it needs to see a valid stack when it throws the
StackOverflowException.

> Also, if you do want to load rfp and lr asd above then would it not be
> better to subtract r2 as is and then do the load pair with an immediate
> offset of -2 * wordsize?

I wrote it that way first, and I thought this one was clearer.

>   // Pop deoptimized frame
>   __ ldrw(r2, Address(r5,
> Deoptimization::UnrollBlock::size_of_deoptimized_frame_offset_in_bytes()));
>   __ add(sp, sp, r2);
>   __ ldp(rfp, lr, Address(sp, -2 * wordSize));
> 
>> This same bug was present in SharedRuntime::generate_deopt_blob() and
>> SharedRuntime::generate_uncommon_trap_blob().  I suppose the code was
>> copied from one to another, or maybe it was a common programmer
>> thinko.
> 
> In fact, they were all copied from the corresponding x86 code which also
> does not bother to do anything with the frame pointer or return address.

It doesn't need to because the return address is in memory, not on the
stack, and that's where a backtrace will pick it up.  AArch64 really
needs to restore at least the return address and (for sanity checking)
the FP.  See SPARC for the equivalent code.

Andrew.