RFR JDK-8194084: Obsolete FastTLABRefill and remove the related code

[JC Beyler]

Hi all,

I fixed this issue after Stuart + Andrew diagnosed the issue for aarch64.
After looking at the code and trying to get the spill/fills out of the way
when possible, x86 also had the possibility to do skip a case of spill/fill.

Let me know what you think:
http://cr.openjdk.java.net/~jcbeyler/8194084/webrev.05

I added a no_pop slowpath, fixed the str to stp for aarch64, and then did
the same no_pop slowpath for x86. I can also remove the no_pop label and
just fix the aarch64 code by moving the str/ldr to stp/ldp and moving the
stp before the jump to the slowpath.

Let me know and thanks!
Jc


On Mon, Feb 19, 2018 at 7:53 AM, Stuart Monteith <stuart.monteith at linaro.org
> wrote:

> Hi,
>    I've tried with:
>
> __ stp(r19, zr, Address(__ pre(sp, -wordSize*2)));
>
> and
>
>  __ ldp(r19, zr, Address(__ post(sp, wordSize*2)));
>
> To save and restore r19.  We now get:
>
> #
> # A fatal error has been detected by the Java Runtime Environment:
> #
> #  SIGSEGV (0xb) at pc=0x0000010008e1ce5c, pid=15063, tid=15091
> #
> # JRE version: OpenJDK Runtime Environment (11.0) (fastdebug build
> 11-internal+0-adhoc.stuart.hs)
> # Java VM: OpenJDK 64-Bit Server VM (fastdebug
> 11-internal+0-adhoc.stuart.hs, mixed mode, tiered, compressed oops,
> serial gc, linux-aarch64)
> # Problematic frame:
> # J 473 c1 java.io.BufferedReader.readLine(Z)Ljava/lang/String;
> java.base (304 bytes) @ 0x0000010008e1ce5c
> [0x0000010008e1cb40+0x000000000000031c]
> #
> siginfo: si_signo: 11 (SIGSEGV), si_code: 1 (SEGV_MAPERR), si_addr:
> 0x000000000000002c
>
> java/io/BufferedReader.readLine(Z)Ljava/lang/String;
> [0x0000010008e1cb40, 0x0000010008e1d9c0]  3712 bytes
> [Entry Point]
> [Constants]
>   # {method} {0x00000100025663b0} 'readLine' '(Z)Ljava/lang/String;'
> in 'java/io/BufferedReader'
>   # this:     c_rarg1:c_rarg1
>                         = 'java/io/BufferedReader'
>   # parm0:    c_rarg2   = boolean
>   #           [sp+0x120]  (sp of caller)
>  ;;  block B45 [0, 0]
>
>   0x0000010008e1cb40: ldr       w8, [x1,#8]
>   0x0000010008e1cb44: cmp       x9, x8, lsl #3
>   0x0000010008e1cb48: b.eq      0x0000010008e1cb80
>   0x0000010008e1cb4c: adrp      x8, 0x00000100080a5000
> [error occurred during error reporting (printing code blob if possible),
> id 0xb]
>
>
> It looks like more is wrong here than just the saving/restoration of
> r19. I'll review the changes a bit more closely.
>
> BR,
>    Stuart
>
>
> On 19 February 2018 at 15:15, Andrew Dinn <adinn at redhat.com> wrote:
> > On 19/02/18 14:24, Stuart Monteith wrote:
> >> I've tried building with Aleksey's patch
> >> (http://cr.openjdk.java.net/~shade/8198341/fixes.patch), but came
> >> across a JVM crash when building OpenJDK.
> >>
> >> I need to look a bit closer, but the patch "8194084: Obsolete
> >> FastTLABRefill and remove the related code" is causing SIGBUS
> >> BUS_ADRALN errors. The stack pointer is becoming unaligned, and so
> >> breaks on aarch64.
> >> For example, in your patch you do:
> >>
> >> -          __ ldp(r5, r19, Address(__ post(sp, 2 * wordSize)));
> >> +          __ ldr(r19, Address(__ post(sp, wordSize)));
> >>
> >> You can only have a 16-byte aligned stack pointer, and you replaced
> >> two loads with one, resulting in an unaligned SP.
> > Yes, I believe Stuart has diagnosed this correctly. The problem is in
> > the changes in c1_Runtime1_aarch64.cpp. The original stp with
> > pre-decrement instruction that save r19+r5 retained 16-byte alignment
> > for rsp. The replacement single str with pre-decrement instruction
> > misaligns sp -- and AArch64 hw gets /very/ unhappy when that happens.
> >
> > Three may be no need to save and restore r5, per se, but there is still
> > a need to push and restore 16 byte's worth of stack data. The str and
> > ldr instructions which currently save/restore r19 could simply be
> > reverted to stp and ldp of r5+r19 (it does no harm to save/restore r5).
> > However, it would be better to save and restore zr+r19. That would
> > better indicate the uselessness of the zr stack slot.
> >
> > regards,
> >
> >
> > Andrew Dinn
> > -----------
> >
>