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

[Andrew Haley]

On 25/05/17 15:25, Andrew Dinn wrote:
> On 25/05/17 15:07, Andrew Haley wrote:
>> On 25/05/17 14:57, Andrew Dinn wrote:
>>> On x86 the release_store_ptr operation just reduces to an assignment of
>>> volatile field _indices. That alone doesn't stop the compiler
>>> re-ordering it before the assignment of f1. Making both fields volatile
>>> does stop them being re-ordered.
>>
>> Please bear with me.  We have to set f1 and then bytecode_1.  We do not
>> want the store to bytecode_1 to move before the store to f1.
>>
>> OrderAccess::release_store_ptr() should be strong enough to guarantee that,
>> regardless of whether f1 is volatile or not.
>> If it's not, there should be a compiler fence in release_store_ptr().
> 
> On a weak architecture like AArch64 OrderAccess::release_store_ptr()
> will be translated to an ordered write. That will ensure that order of
> generated store instructions and order of memory system visibility for
> those stores reflect source order.
> 
> On x86 OrderAccess::release_store_ptr() reduces to a simple write.
> That's because TCO means that there is no need to do anything in order
> to ensure that /memory visibility/ order respects instruction
> generation/execution order.
> 
> However, on x86 there most definitely /is/ a need to ensure that the
> compiler generates these store instructions in source order. That's why
> both fields need to be volatile. A C++ compiler may not re-order
> volatile writes.

Well, that's wrong.  The bug is in OrderAccess::release_store_ptr(),
which must not allow this reordering.  Put a proper release barrier
in there before the store, and all will be well:

    __atomic_thread_fence(__ATOMIC_RELEASE);

There's really no need to make both fields volatile.  And to do so
leaves a lurking bug for any other unsuspecting user of
release_store_ptr().

Andrew.