[aarch64-port-dev ] Another reproducer for AArch64 HS
Pavel Tisnovsky
ptisnovs at redhat.com
Thu Sep 12 08:57:36 PDT 2013
Hi Andrew,
your changes have fixed this well hidden bug.
Thank you very much,
Pavel
----- Andrew Haley <aph at redhat.com> wrote:
> On 09/11/2013 04:52 PM, Pavel Tisnovsky wrote:
> > Hi Andrew,
> >
> > here's another reproducer for AArch64 HS. The reproducer itself looks complicated, but the problem is
> > in the following statement:
> >
> > boolean b = (new Double(n)).equals(new Double(-0.0));
> >
> > which should return false when n==0.0
> >
> > The code breaks (starts to return true) after the method myFormat() is JITted.
> >
> > Here you can see which values is passed to Double.doubleToLongBits() from Double.equals():
> >
> > iter#: 0 value: 0.0...
> > doubleToLongBits input 1: -9223372036854775808 (-0.0)
> > doubleToLongBits input 2: 0 (0.0)
> > false
> >
> > iter#: 1 value: 0.0...
> > doubleToLongBits input 1: -9223372036854775808 (-0.0)
> > doubleToLongBits input 2: 0 (0.0)
> > false
> >
> > (JIT just compiled the method myFormat()...)
> >
> > iter#: 8 value: 0.0...
> > doubleToLongBits input 1: 0 <- wrong, should be -0.0 ie in int representation -9223372036854775808
> > doubleToLongBits input 2: 0 (0.0)
> > true
> >
> >
> > I've tried to understand what's wrong (to teach myself AArch64 assembly and JIT internals ;)
> >
> > Double.equals() looks like:
> > 807 public boolean equals(Object obj) {
> > 808 return (obj instanceof Double)
> > 809 && (doubleToLongBits(((Double)obj).value) ==
> > 810 doubleToLongBits(value));
> > 811 }
> >
> > It's bytecode is easy to understand:
> > public boolean equals(java.lang.Object);
> > Code:
> > 0: aload_1
> > 1: instanceof #38 // class java/lang/Double
> > 4: ifeq 32
> > 7: aload_1
> > 8: checkcast #38 // class java/lang/Double
> > 11: getfield #49 // Field value:D
> > 14: invokestatic #19 // Method doubleToLongBits:(D)J
> > 17: aload_0
> > 18: getfield #49 // Field value:D
> > 21: invokestatic #19 // Method doubleToLongBits:(D)J
> > 24: lcmp
> > 25: ifne 32
> > 28: iconst_1
> > 29: goto 33
> > 32: iconst_0
> > 33: ireturn
> >
> > JITed bytecode is still IMHO ok (don't know exactly the target for those bl/call instructions):
> >
> > /jck/7/jdk/bin/java -XX:+UnlockDiagnosticVMOptions -XX:+PrintCompilation -XX:+PrintAssembly FormatTests
> >
> > 0x00007fedd9a6b00c: ldr d1, [x0,#16] ;*getfield value
> > ; - java.lang.Double::equals at 11 (line 808)
> > ; - FormatTests::myFormat at 18 (line 229)
> >
> > 0x00007fedd9a6b010: fmov d0, d1 ;*invokestatic doubleToLongBits
> > ; - java.lang.Double::equals at 14 (line 809)
> > ; - FormatTests::myFormat at 18 (line 229)
> >
> > 0x00007fedd9a6b014: bl 0x00007fedd9948060 ; OopMap{[176]=Oop [184]=Oop [192]=Oop off=184}
> > ;*invokestatic doubleToLongBits
> > ; - java.lang.Double::equals at 14 (line 809)
> > ; - FormatTests::myFormat at 18 (line 229)
> > ; {static_call}
> > 0x00007fedd9a6b018: ldr x1, [sp,#176]
> > 0x00007fedd9a6b01c: ldr d0, [x1,#16] ;*getfield value
> > ; - java.lang.Double::equals at 18 (line 809)
> > ; - FormatTests::myFormat at 18 (line 229)
> >
> > 0x00007fedd9a6b020: str x0, [sp,#200]
> > 0x00007fedd9a6b024: bl 0x00007fedd9948060 ; OopMap{[184]=Oop [192]=Oop off=200}
> > ;*invokestatic doubleToLongBits
> > ; - java.lang.Double::equals at 21 (line 810)
> > ; - FormatTests::myFormat at 18 (line 229)
> > ; {static_call}
> > 0x00007fedd9a6b028: ldr x2, [sp,#200]
> > 0x00007fedd9a6b02c: cmp x2, x0
> >
> > 0x00007fedd9a6b030: b.ne 0x00007fedd9a6b03c ;*lcmp
> > ; - java.lang.Double::equals at 24 (line 810)
> > ; - FormatTests::myFormat at 18 (line 229)
> > 0x00007fedd9a6b034: orr w2, wzr, #0x1
> > ;; 64 branch [AL] [B37]
> > 0x00007fedd9a6b038: b 0x00007fedd9a6b040 ;*goto
> > ; - java.lang.Double::equals at 29 (line 810)
> > ; - FormatTests::myFormat at 18 (line 229)
> >
> > ;; block B35 [32, 33]
> >
> > ;; 0x0
> > 0x00007fedd9a6b03c: movz w2, #0x0, lsl #16 ;*ireturn
> > ; - java.lang.Double::equals at 33 (line 810)
> > ; - FormatTests::myFormat at 18 (line 229)
> >
> > I'd like to know more how to debug/trace AArm64 code :-) is it possible (how?) to see the JITed
> > code of Double.equals() and the code for double autoboxing (I suppose there would be problem)?
>
> Double.equals() was interpreted. The killer code is the second call to Double.<init>
> on Line 235:
>
> ;; 176 move [metadata:0x0|] [c_rarg3|] [patch_normal] [bci:55]
> 0x00007fffee530160: b 0x00007fffee5311e4 ; {section_word}
> ;; 178 branch [AL] [NewInstanceStub: 0xbca441e8] [bci:55]
> 0x00007fffee530164: b 0x00007fffee5311ec ;*new ; - FormatTests::myFormat at 55 (line 235)
>
> 0x00007fffee530168: fmov d1, d0
> 0x00007fffee53016c: fmov d0, d1
> 0x00007fffee530170: mov x1, x0 ;*invokespecial <init>
> ; - FormatTests::myFormat at 60 (line 235)
>
> 0x00007fffee530174: str x0, [sp,#232]
> 0x00007fffee530178: bl 0x00007fffee147c20 ; OopMap{[192]=Oop [184]=Oop [216]=Oop [232]=Oop off=956}
> ;*invokespecial <init>
> ; - FormatTests::myFormat at 60 (line 235)
> ; {optimized virtual_call}
> ;; 190 move [metadata:0x0|] [c_rarg3|] [patch_normal] [bci:63]
> 0x00007fffee53017c: b 0x00007fffee531200 ; {section_word}
> ;; 192 branch [AL] [NewInstanceStub: 0xbca44828] [bci:63]
> 0x00007fffee530180: b 0x00007fffee531208 ;*new ; - FormatTests::myFormat at 63 (line 235)
>
> 0x00007fffee530184: fmov d0, xzr
>
> HERE IS THE BUG ^^^^^^^^^^^^^^^^^^^^^^^^^^
>
>
> 0x00007fffee530188: mov x1, x0 ;*invokespecial <init>
> ; - FormatTests::myFormat at 70 (line 235
> 0x00007fffee53018c: str x0, [sp,#248]
> 0x00007fffee530190: bl 0x00007fffee147c20 ; OopMap{[192]=Oop [184]=Oop [216]=Oop [232]=Oop [248]=Oop off=980}
> ;*invokespecial <init>
> ; - FormatTests::myFormat at 70 (line 235)
> ; {optimized virtual_call}
>
> Note that we're using XZR as the source for an argument that's -0.0. This is wrong:
> XZR is the bit pattern for all zeroes, 0.0.
>
> The real cause of the bug is here:
>
> bool Assembler::operand_valid_for_float_immediate(double imm) {
> union {
> unsigned ival;
> float val;
> };
> val = (float)imm;
> unsigned result = fp_immediate_for_encoding(ival, true);
> return encoding_for_fp_immediate(result) == imm;
> }
>
> This takes a double, tries to encode it as a floating-point immediate, then
> decodes the value and makes sure it's the same. BUT -- that test is wrong.
> encoding_for_fp_immediate(result) returns 0.0 when imm is -0.0. And the
> test then does
>
> 0.0 == -0.0
>
> which returns true.
>
> Here's the fix:
>
> bool Assembler::operand_valid_for_float_immediate(double imm) {
> // We try to encode the immediate value then we convert the encoded
> // back and make sure it's the exact same bit pattern
>
> unsigned result = fp_immediate_for_encoding(floatToIntBits(imm), true);
> return doubleToLongBits(imm)
> == doubleToLongBits(encoding_for_fp_immediate(result));
> }
>
> Andrew.
>
>
>
> # HG changeset patch
> # User aph
> # Date 1378997906 -3600
> # Node ID 5b2374dbfc861f06fba6f60be6e0e4555710eb38
> # Parent 87e7a0585407aecaa36dda94bf71559d1f62afbc
> Fix operand_valid_for_float_immediate so that it returns false for negative zero.
>
> diff -r 87e7a0585407 -r 5b2374dbfc86 src/cpu/aarch64/vm/assembler_aarch64.cpp
> --- a/src/cpu/aarch64/vm/assembler_aarch64.cpp Thu Sep 12 14:37:30 2013 +0100
> +++ b/src/cpu/aarch64/vm/assembler_aarch64.cpp Thu Sep 12 15:58:26 2013 +0100
> @@ -1480,14 +1480,33 @@
> return encode_logical_immediate(is32, imm) != 0xffffffff;
> }
>
> +static jlong doubleToLongBits(jdouble d) {
> + union {
> + jdouble double_value;
> + jlong double_bits;
> + };
> +
> + double_value = d;
> + return double_bits;
> +}
> +
> +static jint floatToIntBits(jfloat f) {
> + union {
> + jfloat float_value;
> + jint float_bits;
> + };
> +
> + float_value = f;
> + return float_bits;
> +}
> +
> bool Assembler::operand_valid_for_float_immediate(double imm) {
> - union {
> - unsigned ival;
> - float val;
> - };
> - val = (float)imm;
> - unsigned result = fp_immediate_for_encoding(ival, true);
> - return encoding_for_fp_immediate(result) == imm;
> + // We try to encode the immediate value then we convert the encoded
> + // back and make sure it's the exact same bit pattern
> +
> + unsigned result = fp_immediate_for_encoding(floatToIntBits(imm), true);
> + return doubleToLongBits(imm)
> + == doubleToLongBits(encoding_for_fp_immediate(result));
> }
>
> int AbstractAssembler::code_fill_byte() {
More information about the aarch64-port-dev
mailing list