JVM bugfix for failing to running JTreg test 'hotspot/test/compiler/7116216/StackOverflow.java'

[Joseph Joyce]

Hi Ed,
I believe the adjustment was added so that the saved fp and lr were at 
the same offsets for
Java interpreted code as compiled C. This was done in a misguided 
attempt to stop GDB from
giving up tracing the stack through interpreted code. It didn't fix the 
problem but as far as I
remember I'd updated other parts of the code to match so never reverted 
the change.

I can't remember any other reasons for the change.

Thanks
Joseph

On 27/01/16 12:00, Edward Nevill wrote:
> Hi Mingliang Yi,
>
> Thanks for finding this.
>
> The reason this code works on aarch64 and does not work on aarch32 is due to the differing implementations of enter and leave on aarch64 vs aarch32.
>
> On aarch64 enter() does
>
>    void enter()
>    {
>      stp(rfp, lr, Address(pre(sp, -2 * wordSize)));
>      mov(rfp, sp);
>    }
>
> whereas on aarch32 enter() does
>
>    void enter()
>    {
>      stmdb(sp, RegSet::of(rfp, lr).bits());
>      add(rfp, sp, wordSize);
>    }
>
> Joseph: Do you recall why aarch32 adjusts the frame pointer by 4 in enter and leave like this?
>
> So on aarch64 the line
>
>      __ sub(sp, rfp, ((unsigned)framesize-4) << LogBytesPerInt); // prolog
>
> works because it just moves the fp straight back to sp, whereas on aarch32 it would need to subtract 4 to account for the additional wordSize added in enter().
>
> This line of code is in fact just trying to create space for any callee save argument, so this line could be just replaced with
>
>      __ sub(sp, rfp, frame::arg_reg_save_area_bytes + wordSize); // prolog
>
> (the additional wordsize here to cater for the wordSize added in enter()).
>
> On aarch32/aarch64 frame::arg_reg_save_area_bytes is guaranteed to be 0 so this line is redundant. However if we ever decided to have callee saved arguments this would not be redundant.
>
> I have modified your patch to leave this line of code in, but commented out with an additional assertion that arg_reg_save_area_bytes == 0.
>
> I have also tidied up some redundant comments and fixed the enum_layout which was still based on aarch64.
>
> Webrev here: http://cr.openjdk.java.net/~enevill/8148355/webrev
>
> All the best,
> Ed.
>
>
> ==================================================
> Bug description:
>
>     JTreg test fail: hotspot/test/compiler/7116216/StackOverflow.java, with
> error "assert(false) failed: DEBUG MESSAGE: no r13 to peel back"
>
> The testcase aims to test when the callee have a lot of locals, if we've
> got enough room on the stack for it. In the function
> InterpreterGenerator::generate_stack_overflow_check in the source file
> templateInterpreter_aarch32.cpp, we can see how it is done. First check if
> the frame is greater than one page in size. If not, we finish checking, if
> so, then we will use max_pages * page_size to expand the stack, and check
> if the sp is out of the limit stack scope. If not, we finish checking, if
> so, we will go to generate_throw_exception to throw a stackoverflow
> exception. But at present, it left with “__ stop("no r13 to peel back");”
> before going to generate_throw_exception which will stop JVM and cause fail
> “assert(false)”.
>
> At the same time, we find a bug in the function generate_throw_exception in
> the source file stubGenerator_aarch32.cpp, where we generate the
> stackoverflow exception. The assembly it produces like this:
>
> push  {fp, lr}
>
> add  fp, sp, #4
>
> sub  sp, fp, #0
>
> add  r9, pc, #0
>
> str    r9, [r8, #324]
>
> …
>
> “add      fp, sp, #4” makes the fp point to lr in the stack. “sub  sp, fp,
> #0” makes the sp and fp point to the same place(lr). It would destroy the
> stack. The sp should point to fp in the stack.
>
>
>
> Bug solution:
>
>     For the first problem, We should delete the “__ stop("no r13 to peel
> back");”. And let it go on.
>
>     For the second, I delete the c++ code which generated the assembly “sub
> sp, fp, #0”. It will not have the stack trample problem.
>
>
>
> Bug patch:
>
> diff -r 06eed0568597 src/cpu/aarch32/vm/templateInterpreter_aarch32.cpp
>
> --- a/src/cpu/aarch32/vm/templateInterpreter_aarch32.cpp        Tue Jan 05
> 09:17:02 2016 +0000
>
> +++ b/src/cpu/aarch32/vm/templateInterpreter_aarch32.cpp        Mon Jan 11
> 11:09:16 2016 +0800
>
> @@ -518,8 +518,8 @@
>
>     // unnecessary because the sender SP in r13 is always aligned, but
>
>     // it doesn't hurt.
>
>     //__ bic(sp, r13, 7);
>
> -  __ stop("no r13 to peel back");
>
>     // Note: the restored frame is not necessarily interpreted.
>
>     // Use the shared runtime version of the StackOverflowError.
>
>     assert(StubRoutines::throw_StackOverflowError_entry() != NULL, "stub not
> yet generated");
>
>
>
> diff -r 06eed0568597 src/cpu/aarch32/vm/stubGenerator_aarch32.cpp
>
> --- a/src/cpu/aarch32/vm/stubGenerator_aarch32.cpp      Tue Jan 05 09:17:02
> 2016 +0000
>
> +++ b/src/cpu/aarch32/vm/stubGenerator_aarch32.cpp      Mon Jan 11 15:03:17
> 2016 +0800
>
> @@ -1640,7 +1640,7 @@
>
>       assert(is_even(framesize/2), "sp not 16-byte aligned");
>
>       // lr and fp are already in place
>
> -    __ sub(sp, rfp, ((unsigned)framesize-4) << LogBytesPerInt); // prolog
>
>       int frame_complete = __ pc() - start;
>
>