RFR: 8253464: ARM32 Zero: atomic_copy64 is incorrect, breaking volatile stores

[Aleksey Shipilev]

On Wed, 23 Sep 2020 15:43:18 GMT, Boris Ulasevich <bulasevich at openjdk.org> wrote:

>> There is a regression introduced by addition of ARMv7-specific block by JDK-8211387. It readily manifests as crash
>> during jcstress initialization, and investigation points at broken volatile stores. Reverting JDK-8211387 from head JDK
>> makes ARM32 start and run jcstress.  The underlying reason seems to be the half-done `atomic_copy64`: it does the load
>> with exclusive load, but then defers to the C++ store. Somewhere during handing over the value from the asm load to C++
>> store and/or C++ store itself, we garble the value. The way out is to implement the whole thing in asm.   Also see
>> `StubGenerator::generate_atomic_load_long` and `StubGenerator::generate_atomic_store_long` in `stubGenerator_arm.cpp`,
>> that do roughly the same thing and were the basis for this implementation.  Attention @theRealAph, @bulasevich.
>> 
>> Testing:
>>  - [x] ARM32 Linux zero release jcstress run
>
> src/hotspot/os_cpu/linux_zero/os_linux_zero.hpp line 79:
> 
>> 77:     asm volatile ("ldrexd %[tmp_r], [%[src]]\n"
>> 78:                   "clrex\n"
>> 79:                   "1:\n"
> 
> The change is good.
> Minor remarks: I don't see reason of tmp_r(_w) naming and I'd prefer meaningful label name.

There are five operands, I'd prefer to use symbolic names to avoid confusion.

"1:" is the local _numeric_ label, it does not show up in (and potentially conflict with) symbol table. Meaningful
names would necessarily be _symbolic_ labels.

Does that resolve your concerns?

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

PR: https://git.openjdk.java.net/jdk/pull/299