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

[Andrew Haley]

On 6/13/19 6:26 PM, Kim Barrett wrote:
>> On Jun 13, 2019, at 11:21 AM, Andrew Haley <aph at redhat.com> wrote:
>>
>> The Block Offset Table is accessed racily in many places. This is UB,
>> but I suspect the authors of G1 assumed that the UB was "benign". In
>> fact it is not, and there is a narrow race condition which causes G1
>> to crash at times of heavy stress.
>>
>> The bug has only ever been observed on 86-32, but it isn't a
>> target-specific bug. It's probably not specific to G1 either.
>>
>> […]
>>
>> http://cr.openjdk.java.net/~aph/8225716/
> 
> Nice find!  Thanks for the detailed analysis.
> 
> Change looks good, as far as it goes.
> 
> However, I'm a bit bothered by the related Atomic::load/store pair
> being far apart,

In what sense are they far apart? In different files, you mean?
I'm happy to move stuff.

> and the intrusion of Atomic into the header without
> an associated #include (hence must be picking it up indirectly).

OK.

> set_offset_array_raw looks quite uninterestingly different from
> set_offset_array (whose definition is in the .inline.hpp, near the
> definition of offset_array that needed to be changed). Consider
> hoisting set_offset_array_raw into set_offset_array, killing off the
> _raw function, and updating the one remaining caller.

OK.

> And even with that, Atomic is being referenced by indirect #include in
> the .inline.hpp file. I think we (GC team, at least) are (not always
> consistently) trying to reduce such indirect dependencies.

Sure, I'll do a tidyup.

-- 
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