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

[Andrew Dinn]

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