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

[Paul Hohensee]

What David said, and a little history.

orderAccess was originally written (by me, though not as well as Erik's
rewrite) in order to support ia64, which also has a very weakly ordered
memory system. The idea is that there are two sources of potential
reordering, the first by the C++ compilers and the second by the hardware.
Using the volatile specifier consistently blocks the C++ compilers from
reordering, and the orderAccess methods block the hardware from reordering.
The idea was to minimize the number of required hardware barriers (which
can be quite expensive), so the model allows for code that need only
prevent compiler reordering. Another way to put it is that it allows for
the minimal use of hardware barriers.

An alternative would be to use only orderAccess methods to access data that
require ordering. The reason that works is because the formal parameter
types on the orderAccess methods' pointer formals are marked volatile, thus
preventing the C++ compilers from, say, inlining orderAccess methods and
reordering accesses derived from them.

I'm not a ppc memory ordering expert, but from the discussion it seems to
me that there are two bugs, one fixed by amending the ppc implementation of
release_store_ptr and the other by marking _f1 volatile.

Thanks,

Paul

On Fri, May 26, 2017 at 5:35 AM, Andrew Haley <aph at redhat.com> wrote:

> On 26/05/17 10:20, David Holmes wrote:
> > Hi Andrew,
> >
> > On 26/05/2017 6:26 PM, Andrew Haley wrote:
> >> On 26/05/17 03:20, David Holmes wrote:
> >>> 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.
> >>
> >> That's not much more than paranoia, IMO.  If the barriers are strong
> >> enough it'll be fine.  The problem was, I suppose, with old compilers
> >> which didn't handle memory barriers properly, but we should be moving
> >> towards standard ways of doing these things.  Standard atomics have
> >> been available since C++11 (I think) and GCC has had support since long
> >> before then.
> >
> > The issue isn't just the barriers that might be involved inside
> > orderAccess methods. If these variables are being used in racy
> > lock-free code then they should be marked volatile to ensure other
> > compiler optimizations don't interfere. Perhaps that is paranoia,
> > but I'd rather a little harmless paranoia than try to debug what
> > might otherwise go wrong.
>
> I'm always leery of this kind of reasoning because the hardware I most
> care about has a very weakly-ordered memory system and will reorder
> everything in the absence of synchronization.  If it is actually
> necessary to use volatile on a TSO machine to get multi-thread
> ordering then it is almost certainly incorrect code, because volatile
> is not sufficient to do what is needed on non-TSO hardware.
>
> So, if you "fix" code on a TSO machine by using volatile, you are
> making work for me because I'll have to debug it on a non-TSO machine.
> Fix it in a portable way by using the correct primitives and it's
> correct everywhere, it's easier to reason about, and you lost nothing.
>
> --
> Andrew Haley
> Java Platform Lead Engineer
> Red Hat UK Ltd. <https://www.redhat.com>
> EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671
>