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

[Paul Hohensee]

Note that the current model doesn't prevent one from using orderAccess for
all accesses to orderable data, so one can use that model if desired.

On Fri, May 26, 2017 at 6:47 AM, Paul Hohensee <paul.hohensee at gmail.com>
wrote:

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