RFR 8242263: Diagnose synchronization on primitive wrappers

[Doerr, Martin]

Hi again,

I just noticed that the flags are 32 bit:
Class AccessFlags contains jint _flags.

So using 64 bit loads is incorrect (movptr on x64, PPC64 and s390 versions need to get fixed, too). May work on little Endian, but should get fixed.
In addition to that, x86 can combine load + and in one instruction (e.g. one of the test instructions).

Best regards,
Martin


> -----Original Message-----
> From: hotspot-runtime-dev <hotspot-runtime-dev-retn at openjdk.java.net>
> On Behalf Of Doerr, Martin
> Sent: Dienstag, 4. August 2020 13:20
> To: Patricio Chilano <patricio.chilano.mateo at oracle.com>; David Holmes
> <david.holmes at oracle.com>; hotspot-runtime-dev at openjdk.java.net;
> hotspot-jfr-dev at openjdk.java.net
> Subject: [CAUTION] RE: RFR 8242263: Diagnose synchronization on primitive
> wrappers
> 
> Hi Patricio,
> 
> thanks for taking care of PPC64, too.
> Unfortunately, your version is not correct. "andi" instructions don't set CCR0.
> 
> I'd implement it as follows (including s390). Not yet tested.
> 
> Best regards,
> Martin
> 
> 
> diff -r 77852e129401 src/hotspot/cpu/ppc/c1_MacroAssembler_ppc.cpp
> --- a/src/hotspot/cpu/ppc/c1_MacroAssembler_ppc.cpp     Tue Aug 04
> 10:03:57 2020 +0200
> +++ b/src/hotspot/cpu/ppc/c1_MacroAssembler_ppc.cpp     Tue Aug 04
> 13:10:51 2020 +0200
> @@ -108,7 +108,7 @@
>    if (DiagnoseSyncOnPrimitiveWrappers != 0) {
>      load_klass(Rscratch, Roop);
>      ld(Rscratch, in_bytes(Klass::access_flags_offset()), Rscratch);
> -    andi(Rscratch, Rscratch, JVM_ACC_IS_BOX_CLASS);
> +    testbitdi(CCR0, R0, Rscratch,
> exact_log2((intptr_t)JVM_ACC_IS_BOX_CLASS));
>      bne(CCR0, slow_case);
>    }
> 
> diff -r 77852e129401 src/hotspot/cpu/ppc/interp_masm_ppc_64.cpp
> --- a/src/hotspot/cpu/ppc/interp_masm_ppc_64.cpp        Tue Aug 04 10:03:57
> 2020 +0200
> +++ b/src/hotspot/cpu/ppc/interp_masm_ppc_64.cpp        Tue Aug 04
> 13:10:51 2020 +0200
> @@ -913,7 +913,7 @@
>      if (DiagnoseSyncOnPrimitiveWrappers != 0) {
>        load_klass(tmp, object);
>        ld(tmp, in_bytes(Klass::access_flags_offset()), tmp);
> -      andi(tmp, tmp, JVM_ACC_IS_BOX_CLASS);
> +      testbitdi(CCR0, R0, tmp,
> exact_log2((intptr_t)JVM_ACC_IS_BOX_CLASS));
>        bne(CCR0, slow_case);
>      }
> 
> diff -r 77852e129401 src/hotspot/cpu/ppc/macroAssembler_ppc.cpp
> --- a/src/hotspot/cpu/ppc/macroAssembler_ppc.cpp        Tue Aug 04 10:03:57
> 2020 +0200
> +++ b/src/hotspot/cpu/ppc/macroAssembler_ppc.cpp        Tue Aug 04
> 13:10:51 2020 +0200
> @@ -2839,7 +2839,7 @@
>    if (DiagnoseSyncOnPrimitiveWrappers != 0) {
>      load_klass(temp, oop);
>      ld(temp, in_bytes(Klass::access_flags_offset()), temp);
> -    andi(temp, temp, JVM_ACC_IS_BOX_CLASS);
> +    testbitdi(CCR0, R0, temp,
> exact_log2((intptr_t)JVM_ACC_IS_BOX_CLASS));
>      bne(CCR0, cont);
>    }
> 
> diff -r 77852e129401 src/hotspot/cpu/s390/c1_MacroAssembler_s390.cpp
> --- a/src/hotspot/cpu/s390/c1_MacroAssembler_s390.cpp   Tue Aug 04
> 10:03:57 2020 +0200
> +++ b/src/hotspot/cpu/s390/c1_MacroAssembler_s390.cpp   Tue Aug 04
> 13:10:51 2020 +0200
> @@ -91,6 +91,13 @@
>    // Save object being locked into the BasicObjectLock...
>    z_stg(obj, Address(disp_hdr, BasicObjectLock::obj_offset_in_bytes()));
> 
> +  if (DiagnoseSyncOnPrimitiveWrappers != 0) {
> +    load_klass(Z_R1_scratch, obj);
> +    testbit(Address(Z_R1_scratch, in_bytes(Klass::access_flags_offset())),
> +            exact_log2((intptr_t)JVM_ACC_IS_BOX_CLASS));
> +    z_btrue(slow_case);
> +  }
> +
>    if (UseBiasedLocking) {
>      biased_locking_enter(obj, hdr, Z_R1_scratch, Z_R0_scratch, done,
> &slow_case);
>    }
> diff -r 77852e129401 src/hotspot/cpu/s390/interp_masm_s390.cpp
> --- a/src/hotspot/cpu/s390/interp_masm_s390.cpp Tue Aug 04 10:03:57 2020
> +0200
> +++ b/src/hotspot/cpu/s390/interp_masm_s390.cpp Tue Aug 04 13:10:51
> 2020 +0200
> @@ -1000,6 +1000,13 @@
>    // Load markWord from object into displaced_header.
>    z_lg(displaced_header, oopDesc::mark_offset_in_bytes(), object);
> 
> +  if (DiagnoseSyncOnPrimitiveWrappers != 0) {
> +    load_klass(Z_R1_scratch, object);
> +    testbit(Address(Z_R1_scratch, in_bytes(Klass::access_flags_offset())),
> +            exact_log2((intptr_t)JVM_ACC_IS_BOX_CLASS));
> +    z_btrue(slow_case);
> +  }
> +
>    if (UseBiasedLocking) {
>      biased_locking_enter(object, displaced_header, Z_R1, Z_R0, done,
> &slow_case);
>    }
> diff -r 77852e129401 src/hotspot/cpu/s390/macroAssembler_s390.cpp
> --- a/src/hotspot/cpu/s390/macroAssembler_s390.cpp      Tue Aug 04
> 10:03:57 2020 +0200
> +++ b/src/hotspot/cpu/s390/macroAssembler_s390.cpp      Tue Aug 04
> 13:10:51 2020 +0200
> @@ -3358,6 +3358,13 @@
>    // Load markWord from oop into mark.
>    z_lg(displacedHeader, 0, oop);
> 
> +  if (DiagnoseSyncOnPrimitiveWrappers != 0) {
> +    load_klass(Z_R1_scratch, oop);
> +    testbit(Address(Z_R1_scratch, in_bytes(Klass::access_flags_offset())),
> +            exact_log2((intptr_t)JVM_ACC_IS_BOX_CLASS));
> +    z_btrue(done);
> +  }
> +
>    if (try_bias) {
>      biased_locking_enter(oop, displacedHeader, temp, Z_R0, done);
>    }
> d056149 at lu0482:/priv/d056149/jdk$ hg diff
> diff -r 77852e129401 src/hotspot/cpu/ppc/c1_MacroAssembler_ppc.cpp
> --- a/src/hotspot/cpu/ppc/c1_MacroAssembler_ppc.cpp     Tue Aug 04
> 10:03:57 2020 +0200
> +++ b/src/hotspot/cpu/ppc/c1_MacroAssembler_ppc.cpp     Tue Aug 04
> 13:17:06 2020 +0200
> @@ -108,7 +108,7 @@
>    if (DiagnoseSyncOnPrimitiveWrappers != 0) {
>      load_klass(Rscratch, Roop);
>      ld(Rscratch, in_bytes(Klass::access_flags_offset()), Rscratch);
> -    andi(Rscratch, Rscratch, JVM_ACC_IS_BOX_CLASS);
> +    testbitdi(CCR0, R0, Rscratch,
> exact_log2((intptr_t)JVM_ACC_IS_BOX_CLASS));
>      bne(CCR0, slow_case);
>    }
> 
> diff -r 77852e129401 src/hotspot/cpu/ppc/interp_masm_ppc_64.cpp
> --- a/src/hotspot/cpu/ppc/interp_masm_ppc_64.cpp        Tue Aug 04 10:03:57
> 2020 +0200
> +++ b/src/hotspot/cpu/ppc/interp_masm_ppc_64.cpp        Tue Aug 04
> 13:17:06 2020 +0200
> @@ -913,7 +913,7 @@
>      if (DiagnoseSyncOnPrimitiveWrappers != 0) {
>        load_klass(tmp, object);
>        ld(tmp, in_bytes(Klass::access_flags_offset()), tmp);
> -      andi(tmp, tmp, JVM_ACC_IS_BOX_CLASS);
> +      testbitdi(CCR0, R0, tmp,
> exact_log2((intptr_t)JVM_ACC_IS_BOX_CLASS));
>        bne(CCR0, slow_case);
>      }
> 
> diff -r 77852e129401 src/hotspot/cpu/ppc/macroAssembler_ppc.cpp
> --- a/src/hotspot/cpu/ppc/macroAssembler_ppc.cpp        Tue Aug 04 10:03:57
> 2020 +0200
> +++ b/src/hotspot/cpu/ppc/macroAssembler_ppc.cpp        Tue Aug 04
> 13:17:06 2020 +0200
> @@ -2839,7 +2839,7 @@
>    if (DiagnoseSyncOnPrimitiveWrappers != 0) {
>      load_klass(temp, oop);
>      ld(temp, in_bytes(Klass::access_flags_offset()), temp);
> -    andi(temp, temp, JVM_ACC_IS_BOX_CLASS);
> +    testbitdi(CCR0, R0, temp,
> exact_log2((intptr_t)JVM_ACC_IS_BOX_CLASS));
>      bne(CCR0, cont);
>    }
> 
> diff -r 77852e129401 src/hotspot/cpu/s390/c1_MacroAssembler_s390.cpp
> --- a/src/hotspot/cpu/s390/c1_MacroAssembler_s390.cpp   Tue Aug 04
> 10:03:57 2020 +0200
> +++ b/src/hotspot/cpu/s390/c1_MacroAssembler_s390.cpp   Tue Aug 04
> 13:17:06 2020 +0200
> @@ -91,6 +91,13 @@
>    // Save object being locked into the BasicObjectLock...
>    z_stg(obj, Address(disp_hdr, BasicObjectLock::obj_offset_in_bytes()));
> 
> +  if (DiagnoseSyncOnPrimitiveWrappers != 0) {
> +    load_klass(Z_R1_scratch, obj);
> +    testbit(Address(Z_R1_scratch, in_bytes(Klass::access_flags_offset())),
> +            exact_log2((intptr_t)JVM_ACC_IS_BOX_CLASS));
> +    z_btrue(slow_case);
> +  }
> +
>    if (UseBiasedLocking) {
>      biased_locking_enter(obj, hdr, Z_R1_scratch, Z_R0_scratch, done,
> &slow_case);
>    }
> diff -r 77852e129401 src/hotspot/cpu/s390/interp_masm_s390.cpp
> --- a/src/hotspot/cpu/s390/interp_masm_s390.cpp Tue Aug 04 10:03:57 2020
> +0200
> +++ b/src/hotspot/cpu/s390/interp_masm_s390.cpp Tue Aug 04 13:17:06
> 2020 +0200
> @@ -1000,6 +1000,13 @@
>    // Load markWord from object into displaced_header.
>    z_lg(displaced_header, oopDesc::mark_offset_in_bytes(), object);
> 
> +  if (DiagnoseSyncOnPrimitiveWrappers != 0) {
> +    load_klass(Z_R1_scratch, object);
> +    testbit(Address(Z_R1_scratch, in_bytes(Klass::access_flags_offset())),
> +            exact_log2((intptr_t)JVM_ACC_IS_BOX_CLASS));
> +    z_btrue(slow_case);
> +  }
> +
>    if (UseBiasedLocking) {
>      biased_locking_enter(object, displaced_header, Z_R1, Z_R0, done,
> &slow_case);
>    }
> diff -r 77852e129401 src/hotspot/cpu/s390/macroAssembler_s390.cpp
> --- a/src/hotspot/cpu/s390/macroAssembler_s390.cpp      Tue Aug 04
> 10:03:57 2020 +0200
> +++ b/src/hotspot/cpu/s390/macroAssembler_s390.cpp      Tue Aug 04
> 13:17:06 2020 +0200
> @@ -3358,6 +3358,13 @@
>    // Load markWord from oop into mark.
>    z_lg(displacedHeader, 0, oop);
> 
> +  if (DiagnoseSyncOnPrimitiveWrappers != 0) {
> +    load_klass(Z_R1_scratch, oop);
> +    testbit(Address(Z_R1_scratch, in_bytes(Klass::access_flags_offset())),
> +            exact_log2((intptr_t)JVM_ACC_IS_BOX_CLASS));
> +    z_btrue(done);
> +  }
> +
>    if (try_bias) {
>      biased_locking_enter(oop, displacedHeader, temp, Z_R0, done);
>    }