RFR: 8181085: Race condition in method resolution may produce spurious NullPointerException

[David Holmes]

One important correction:

>   7143664: Clean up OrderAccess implementations and usage
> 
> (n.b. I believe the author is one D Holmes 

No that work was done by Erik Osterlund (before he joined Oracle). I was 
only the sponsor.

7143664: Clean up OrderAccess implementations and usage
Summary: Clarify and correct the abstract model for memory barriers 
provided by the orderAccess class. Refactor the implementations using 
template specialization to allow the bulk of the code to be shared, with 
platform specific customizations applied as needed.
Reviewed-by: acorn, dcubed, dholmes, dlong, goetz, kbarrett, sgehwolf
Contributed-by: Erik Osterlund <erik.osterlund at lnu.se>

Cheers,
David
-----

On 26/05/2017 6:11 PM, Andrew Dinn wrote:
> On 26/05/17 03:20, David Holmes wrote:
>> On 26/05/2017 6:29 AM, Paul Hohensee wrote:
>>> I don't know that you want to retract the patch. There's still a bug here
>>> imo that your patch fixes.
>>
>> I agree. This is a common error when dealing with pointer variables,
>> especially when looking at surrounding usage on non-pointer variables.
>> We need the _f1 pointer to be volatile, not the thing to which the _f1
>> pointer points (well it's possible we may need both, I haven't dived
>> that deep).
>>
>> Any variable passed to an OrderAccess, or Atomic, function should be
>> volatile to minimise the chances the C compiler will do something
>> unexpected with it.
>>
>> I don't even know what to make of the vmStructs.cpp existing code!
> 
> Hmm, well this is a conundrum then. One piece of advice from the two of
> you and another from Andrew Haley (who is 'technically' my boss --- i.e.
> he's the technical lead for my team :-).
> 
> Your view is precisely what I originally assumed was at play here i.e.
> that where successive writes to fields must be seen by other threads in
> the correct order that is to be achieved on x86 by making both fields
> volatile. This guarantees sequencing of generated store instructions by
> the compiler in accordance with source order and, hence, because x86 is
> TCO, visibility of those store instructions in that same order.
> 
> Of course, that is inadequate on weak-memory models like ppc and
> AArch64. So, to make your suggestion work properly for all architectures
> the second write (at least, if not the first) also needs to be
> implemented using a call to store_release. That will definitely ensure
> that the first write is visible before the second on all architectures.
> It has been our hope (Andrew's and mine) since we completed the AArch64
> port that all pairs of stores which require ordering do indeed employ a
> store_release (we have had to correct a few cases over the last few years).
> 
> Andrew's belief seems to be that your model is error prone and is fixed
> more correctly by introducing a memory and/or compiler barrier into the
> implementation of release_store. If instead release_store is used
> consistently whenever the second of a pair of writes needs to be
> guaranteed to be visible after the first then it will provide the
> desired outcome. This belief seems indeed to be backed up by the changes
> made to the jdk9 code base quite some while back (the ones I failed to
> notice). The relevant commit is
> 
>    7143664: Clean up OrderAccess implementations and usage
> 
> (n.b. I believe the author is one D Holmes :-)
> 
> I think Andrew's view is probably sound (and not just because he is my
> boss). Since we must use release_store everywhere we want visibility of
> writes to be ordered then also requiring both the fields involved to be
> volatile is redundant. Given what little C++ volatile declarations do
> achieve it might be wiser not to be using volatile declarations at all.
> We would certainly start finding missing store_release calls quicker ;-)
> 
> regards,
> 
> 
> Andrew Dinn
> -----------
> Senior Principal Software Engineer
> Red Hat UK Ltd
> Registered in England and Wales under Company Registration No. 03798903
> Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander
>