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

[Andrew Haley]

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