RFR: 8351997: AArch64: Interpreter volatile reference stores with G1 are not sequentially consistent

[Erik Österlund]

On Wed, 28 May 2025 14:05:41 GMT, Erik Österlund <eosterlund at openjdk.org> wrote:

>>> Ouch. I think this is fine without further refactoring, especially since I see it should be backported into JDK 21 as well.
>> 
>> It's many things, but IMO fine is not one of them. It would surely be better if this evil were expunged from JDK 21 as well, lest it also confuse a backporter.
>> 
>>> I was wondering why haven't we caught this in jcstress. jcstress runs in int/C1/C2 modes specifically to catch issues like these. I believe this slipped through because all of our seqcst tests, Dekker included, operate on primitives. So we never actually explore what happens with reference load/stores, and as this bug shows, there are _interesting_ interactions with GC barriers. I'll see how to amend jcstress to cover this...
>> 
>> It's interesting to see how to do that. One question for @fisk : did this problem manifest interpreter-only, or with a combination of interpreted in one thread, compiled in the other?
>
>> Ouch. I think this is fine without further refactoring, especially since I see it should be backported into JDK 21 as well.
> 
> Yeah it looks like this will need backporting all the way back to 8 - before the barrier registers were explicit at all at the use site, and before there was even an access API.
> 
>> I was wondering why haven't we caught this in jcstress. jcstress runs in int/C1/C2 modes specifically to catch issues like these. I believe this slipped through because all of our seqcst tests, Dekker included, operate on primitives. So we never actually explore what happens with reference load/stores, and as this bug shows, there are _interesting_ interactions with GC barriers. I'll see how to amend jcstress to cover this...
> 
> I was also confused why jcstress hasn't caught this - seems like basic seq cst testing would have hashed this out. Great that you found out why. :-)

> One question for @fisk : did this problem manifest interpreter-only, or with a combination of interpreted in one thread, compiled in the other?

It manifested with -Xint and with -XX:TieredStopAtLevel=1, but mysteriously not when allowing C2 compilation. I traced down the reason to be the extra dmb you added to the following conditional leading fence for volatile loads in the interpreter:


  // 8179954: We need to make sure that the code generated for
  // volatile accesses forms a sequentially-consistent set of
  // operations when combined with STLR and LDAR.  Without a leading
  // membar it's possible for a simple Dekker test to fail if loads
  // use LDR;DMB but stores use STLR.  This can happen if C2 compiles
  // the stores in one method and we interpret the loads in another.
  if (!CompilerConfig::is_c1_or_interpreter_only_no_jvmci()) {
    Label notVolatile;
    __ tbz(r3, ResolvedFieldEntry::is_volatile_shift, notVolatile);
    __ membar(MacroAssembler::AnyAny);
    __ bind(notVolatile);
  }


This extra leading fence on volatile loads when C2 is available masked the lack of trailing fence on the store side. Therefore, the Dekker duality in the test that hung worked out anyway then.

-------------

PR Comment: https://git.openjdk.org/jdk/pull/25483#issuecomment-2916521838