RFR (S): 8153751: Windows Atomic and OrderAccess are not emitting explicit compiler barriers

[Erik Osterlund]

Hi David,

Thanks for reviewing this!

> On 19 apr. 2016, at 03:54, David Holmes <david.holmes at oracle.com> wrote:
> 
> Hi Erik,
> 
> Sorry for the delay on this.
> 
>> On 8/04/2016 7:00 PM, Erik Österlund wrote:
>> Hi,
>> 
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8153751
>> Webrev: http://cr.openjdk.java.net/~eosterlund/8153751/webrev.00/
>> 
>> It turns out that on Windows, compiler barriers are not used for
>> Atomic:: code and some code in OrderAccess. It is assumed that the
>> non-volatile accesses will not be reordered across __asm statements, as
>> the current memory model should prevent. But I have not found any
>> guarantees that this is the case for __asm.
> 
> Agreed - not that I would expect it to be clearly documented even if true. There is no reason to assume that any arbitrary _asm statement automatically acts as a compiler barrier, though some may.

Agreed.

>> The closest source I could find is the documentation for the
>> MemoryBarrier macro that uses an __asm statement to produce a hardware
>> membar. The documentation says the following: "Creates a hardware memory
>> barrier (fence) that prevents the CPU from re-ordering read and write
>> operations. It may also prevent the compiler from re-ordering read and
>> write operations."
>> 
>> Note the "MAY" prevent compiler reordering. (*facepalm*) This sounds to
>> me like __asm really does not guarantee compiler reordering across __asm
>> at all.
> 
> Yeah that sounds like it was written by someone who really didn't understand the issues. :(

Yeah any "may prevent X" seems like a suboptimal guarantee.

>> A suspected victim of this reordering is here:
>> https://bugs.openjdk.java.net/browse/JDK-8033545
> 
> I don't really see how that particular bug would be helped by additional compiler-barriers. As I understand the report it was the elision of a non-volatile load that caused the problem, not a reordering. But anyway ...

The current memory model should prevent reordering, even with non-volatile fields, across cmpxchg/fence. The issue can be seen as the compiler allowing the re-load after the CAS as being allowed to float up, across the CAS, all the way up to the original load, where it is eliminated as redundant.

Compiler barriers (should) prevent this illegal compiler reordering by invalidating all memory assumptions that causes the non-volatile field to not be reloaded.

I do agree though that writing lock-free code with non-volatile fields is just asking for trouble, which is why I have a separate webrev where I made all the GC-related fields we CAS on volatile by default. They should be volatile regardless.

> 
>> To make the use of these classes more safe, I propose to simply add
>> explicit compiler barriers in this windows code. It should not make any
>> difference, as I am only enforcing compiler reordering rules that should
>> already hold.
> 
> Okay, I agree with the proposed changes - the CompilerBarrierScope should effectively have no impact on generated code, other than prohibiting code motion.

Agreed, but...

> 
>> This small patch fixes this. The only thing to note is that the __asm
>> statements that put things in %eax and avoid returning anything.
> 
> Could you elaborate on that please ??

...there are some methods that should return a value but do not and instead rely on the return value in the asm block being in the %eax register which per the C ABI is where return values are stored. The compiler is explicitly told to not complain that there is no return statement.

And on debug builds the scope thing generates some code that would break this, unless care is taken. And fortunately it is taken so it's fine. (split those methods out to internal methods where the assumptions still hold)

> 
>> Testing: JPRT
> 
> Did you compare the generated object code before and after to see if it actually changed anything?

It did in debug mode as per note above.

Thanks a lot for reviewing! :)

/Erik

> 
> Thanks,
> David
> -----
> 
>> Thanks,
>> /Erik