RFR: 8225716: G1 GC: Undefined behaviour in G1BlockOffsetTablePart::block_at_or_preceding

[Andrew Haley]

On 6/17/19 2:05 PM, Aleksey Shipilev wrote:
> On 6/17/19 2:53 PM, Andrew Haley wrote:
>> On 6/17/19 1:46 PM, Aleksey Shipilev wrote:
>>> On 6/17/19 2:37 PM, Andrew Haley wrote:
>>> Well, there are OrderAccess j(u)byte methods you can use here, along
>>> with some STATIC_ASSERTs that verify uchar and jubyte actually
>>> agree? Or even explicit fences before/after the access to get
>>> acquire/release?
>>
>> We want no fences. These are relaxed loads and stores.
> 
> Ah. Ah... What does it even mean to have the relaxed load/store of
> u_char? u_char should be access-atomic already, since sizeof(u_char)
> == 1? It does not hurt here, though. Re-reading the original review
> thread, it seems to be the cleanliness choice to use explicit
> Atomic::(load|store), and the absence of it does not readily hurt.

Yes, exactly.

There is a more subtle distinction: all data races in the C++11 memory
model are undefined behaviour, *even if the data are volatile*.
Therefore, when we get around to using C++11 in HotSpot, we want the
BOT to be an array of atomic<u_char>, not an array of volatile
u_char. Otherwise it's UB.

>>> It would be awkward to have the same bug ID having different memory
>>> semantics fixes.
>>
>> I can't see any way to do it otherwise: we have no relaxed atomics on
>> 8u that I can find. maybe they're hiding, but I don't think so.
> 
> "volatile" is fine then.

OK, thanks.

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