[aarch64-port-dev ] [NEW BUG]Missing LoadStore barrier in interpreter?

[Patrick Zhang]

I am interested in this topic, here are two questions.

1. Is the right process that the patch firstly gets applied on jdk/jdk/../aarch64, then backported to jdk8u? BTW, I wonder if there is any "failed" test, and right now could get "pass" with the patch?
http://hg.openjdk.java.net/jdk/jdk/file/00baec44c94e/src/hotspot/cpu/aarch64/templateTable_aarch64.cpp#l2723

2. Compared the original patch (back to Jun 2013) on aarch64-port/jdk8u with x86's implementation, at the end of the putfield_or_static function, was the StoreStore intentionally dropped?
http://hg.openjdk.java.net/aarch64-port/jdk8u/hotspot/rev/ed26923dcc83#l3.118 
http://hg.openjdk.java.net/jdk/jdk/file/00baec44c94e/src/hotspot/cpu/x86/templateTable_x86.cpp#l3134 
-  // TODO : restore this volatile barrier call
-  //
-  // Check for volatile store
-  // __ andw(r3, r3, r3);
-  // __ br(Assembler::EQ, notVolatile);
-  // volatile_barrier(Assembler::Membar_mask_bits(Assembler::StoreLoad |
-  //                                             Assembler::StoreStore));
-  // __ bind(notVolatile);
+  {
+    Label notVolatile;
+    __ tbz(r3, ConstantPoolCacheEntry::is_volatile_shift, notVolatile);
+    __ membar(MacroAssembler::Membar_mask_bits(MacroAssembler::StoreLoad));
+    __ bind(notVolatile);
+  }

Regards
Patrick

-----Original Message-----
From: aarch64-port-dev <aarch64-port-dev-bounces at openjdk.java.net> On Behalf Of Andrew Haley
Sent: Thursday, January 31, 2019 6:39 PM
To: Lun Liu <lunliu93 at gmail.com>; aarch64-port-dev at openjdk.java.net
Subject: Re: [aarch64-port-dev ] [NEW BUG]Missing LoadStore barrier in interpreter?

On 1/30/19 8:36 PM, Lun Liu wrote:

> I noticed in the template interpreter only a StoreStore barrier is 
> used before a volatile write. According to JMM cookbook I think a 
> LoadStore barrier is also needed to prevent reordering between a 
> normal read and a volatile write.

Yes, you're right. When I translated the code from x86, I mistakenly did
this:


@@ -2382,13 +2382,16 @@
   jvmti_post_field_mod(rcpool, index, is_static);
   load_field_cp_cache_entry(obj, cache, index, off, flags, is_static);

-  // [jk] not needed currently
-  // volatile_barrier(Assembler::Membar_mask_bits(Assembler::LoadStore |
-  //                                              Assembler::StoreStore));
-
-  Label notVolatile, Done;
+  Label Done;
   __ mov(r5, flags);

+  {
+    Label notVolatile;
+    __ tbz(r5, ConstantPoolCacheEntry::is_volatile_shift, notVolatile);
+    __ membar(MacroAssembler::Membar_mask_bits(MacroAssembler::StoreStore));
+    __ bind(notVolatile);
+  }
+
   // field address
   const Address field(obj, off);


As you can see, the x86 comment is correct, but I didn't follow it.

> In the template interpreter it seems like LoadLoad | LoadStore 
> barriers are inserted after both normal read and volatile read so it 
> should be fine but I think this might cause a problem if the compiler 
> generates a normal read using dmb barriers and the interpreter is 
> executing the volatile write.

I'm not sure. The LoadStore should prevent a load (any kind) from being reordered with the volatile store. This only matters if another thread stores to the memory in the load, and that store in the other thread is sequenced after the volatile store in this thread.

> Could this be a potential bug?

We've never seen any test failures, but that doesn't prove anything. It is a bug, so we should fix it. Thank you.

--
Andrew Haley
Java Platform Lead Engineer
Red Hat UK Ltd. <https://www.redhat.com>
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671