[aarch64-port-dev ] RFR: 8215100: AArch64: fix compareTo intrinsic with four-character Latin/Unicode

[Nick Gasson (Arm Technology China)]

Hi Andrew,

> ... because the code is already difficult to understand, which is what caused the
> bug
> in the first place. But if you strongly prefer the former I'll accept it.

I did it that way originally to avoid having any effect on the
common case where the encodings match, but I agree it's much
simpler if the test is the same in all cases. I've put an updated
webrev here:

http://cr.openjdk.java.net/~njian/8215100/webrev.1/

If you're happy with this version could you help me push it?

Thanks,
Nick

> -----Original Message-----
> From: Andrew Haley <aph at redhat.com>
> Sent: 10 December 2018 19:20
> To: Nick Gasson (Arm Technology China) <Nick.Gasson at arm.com>; hotspot-
> compiler-dev at openjdk.java.net
> Cc: nd <nd at arm.com>; aarch64-port-dev at openjdk.java.net
> Subject: Re: RFR: 8215100: AArch64: fix compareTo intrinsic with four-character
> Latin/Unicode
> 
> On 12/10/18 8:41 AM, Nick Gasson (Arm Technology China) wrote:
> > Please help me review this patch to fix an intermittent failure of
> > java/util/Calendar/Bug8167273.java on AArch64.
> >
> > Webrev: http://cr.openjdk.java.net/~njian/8215100/webrev.0/
> > Bug: https://bugs.openjdk.java.net/browse/JDK-8215100
> > (also https://bugs.openjdk.java.net/browse/JDK-8209402)
> >
> > The calendar test failure reported in JDK-8209402 happens because it
> > can't find a particular key in a TreeMap, but if you dump out the
> > contents of that TreeMap, the key is present but the tree is not
> > sorted correctly.
> >
> > It's caused by a bug in the compareTo intrinsic that's triggered when
> > both arguments are four characters long but one is Latin1 encoded and
> > one is Unicode encoded. In that case the "Compare longwords" loop
> > header (isLU or else branches) will branch to the TAIL_CHECK label
> > with tmp1 (isLU) or tmp2 (isUL) uninitialized. The instructions after
> > TAIL_CHECK assume tmp1 and tmp2 contain the final 64-bit words of each
> > string, and use these to compute the difference between the final
> > characters. In the LU/UL case this result is garbage.
> >
> > Fix by handling four-character mixed U/L strings in the SHORT_STRING
> > loop (currently this is done for length < 4). Also extended the
> > existing string intrinsics test to catch this.
> 
> Great catch! I think I'd slightly prefer this
> 
> --- a/src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp
> +++ b/src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp
> @@ -4896,7 +4896,7 @@ void MacroAssembler::string_compare(Regi
> 
>    // A very short string
>    cmpw(cnt2, minCharsInWord);
> -  br(Assembler::LT, SHORT_STRING);
> +  br(str1_isL == str2_isL ? Assembler::LT : Assembler::LE, SHORT_STRING);
> 
>    // Compare longwords
>    // load first parts of strings and finish initialization while loading
> 
> to be
> 
> --- a/src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp
> +++ b/src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp
> @@ -4896,7 +4896,7 @@ void MacroAssembler::string_compare(Regi
> 
>    // A very short string
>    cmpw(cnt2, minCharsInWord);
> -  br(Assembler::LT, SHORT_STRING);
> +  br(Assembler::LE, SHORT_STRING);
> 
>    // Compare longwords
>    // load first parts of strings and finish initialization while loading
> 
> ... because the code is already difficult to understand, which is what caused the
> bug
> in the first place. But if you strongly prefer the former I'll accept it.
> --
> Andrew Haley
> Java Platform Lead Engineer
> Red Hat UK Ltd. <https://www.redhat.com>
> EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671