RFR: JDK-8061964: Insufficient compiler barriers for GCC in OrderAccess functions

[Mikael Gerdin]

Dan,

On 2014-11-03 20:00, Daniel D. Daugherty wrote:
>  > http://cr.openjdk.java.net/~mgerdin/8061964/webrev.0/
>
> src/os_cpu/linux_x86/vm/orderAccess_linux_x86.inline.hpp
>      No comments.
>
> Thanks for being persistent and chasing this crazy bug to ground.
>
> I have one administrative request. Can you please add a complete
> set of "gdb session notes" to the bug report where you show the
> good code in JDK7-B108 and the bad code in JDK7-B109? Please do
> the same for your baseline bits and your fixed bits.

Will do.

>
> Every time we consider a GCC upgrade, I would like it to be easy
> for someone to verify that this code sequence is not messed up.
> It should also make it easy for other folks to adapt your sequence
> to investigate their own possibly broken code sequences...
>
> As for testing, I think our usual battery of JPRT, PIT, and promotion
> testing will have to do.

Yep, it may be a good idea to keep in mind that if "impossible" 
conditions seem to have occurred in a core file then it may be a good 
idea to inspect the generated assembly instruction sequences to make 
sure that no incorrect reordering has occurred.

>
> Personally, once your fix is in I'm planning to drop the bits onto
> my Linux DevOps machine and see if the following still repos:
>
>     8047212 runtime/ParallelClassLoading/bootstrap/random/inner-complex
>             assert(ObjectSynchronizer::verify_objmon_isinpool(inf)) failed:
>             monitor is invalid
>
> Again, thanks for fixing this!

Thanks for reviewing, Dan.

/Mikael

>
> Dan
>
>
> On 11/3/14 6:01 AM, Mikael Gerdin wrote:
>> Hi all,
>>
>> Please review this attempt at fixing the OrderAccess functions on
>> Linux x86 with GCC.
>>
>> While working on another bug I recently discovered that g++ was
>> reordering stores across a call to OrderAccess::storestore on Linux x86.
>>
>> The G1 code attempts to do an ordered publishing of two values:
>> _saved_mark_word = _top;
>> OrderAccess::storestore();
>> _gc_time_stamp = curr_gc_time_stamp;
>>
>> The types involved are
>> HeapWord* _top, _saved_mark_word;
>> volatile unsigned _gc_time_stamp;
>>
>> The incorrect behavior seems to have started when JDK-6973570 was
>> fixed in JDK 7.
>> Below, _top is at offset 0x58, _saved_mark_word at 0x18 and
>> _gc_time_stamp at 0x138, %rbx is "this".
>>
>> /net/jre/onestop/jdk/1.7.0/promoted/all//b108/binaries/linux-x64/jre/lib/amd64/server/libjvm.so:
>>
>>   3d9f4d:       39 d0                   cmp    %edx,%eax
>>   3d9f4f:       73 1c                   jae    3d9f6d
>> <G1OffsetTableContigSpace::set_saved_mark()+0x3d>
>>   3d9f51:       48 8b 43 58             mov    0x58(%rbx),%rax
>>   3d9f55:       48 89 43 18             mov    %rax,0x18(%rbx)
>>   3d9f59:       48 8b 05 40 f9 70 00    mov 0x70f940(%rip),%rax    #
>> ae98a0 <_DYNAMIC+0x12f8>
>>   3d9f60:       48 c7 00 00 00 00 00    movq   $0x0,(%rax)
>>   3d9f67:       89 93 38 01 00 00       mov    %edx,0x138(%rbx)
>>
>> /net/jre/onestop/jdk/1.7.0/promoted/all//b109/binaries/linux-x64/jre/lib/amd64/server/libjvm.so
>>
>>   3da05d:       39 d0                   cmp    %edx,%eax
>>   3da05f:       73 15                   jae    3da076
>> <G1OffsetTableContigSpace::set_saved_mark()+0x36>
>>   3da061:       48 8b 43 58             mov    0x58(%rbx),%rax
>>   3da065:       c7 45 f4 00 00 00 00    movl   $0x0,-0xc(%rbp)
>>   3da06c:       89 93 38 01 00 00       mov    %edx,0x138(%rbx)
>>   3da072:       48 89 43 18             mov    %rax,0x18(%rbx)
>>
>> In b109 the store of %rax to 0x18(%rbx) has been ordered after the
>> store of %edx to 0x138(%rbx) in the same build as JDK-6973570 was
>> integrated.
>>
>> My suggestion to fix this is to extend all the OrderAccess::release*
>> variants on x86 with a:
>> __asm__ volatile ("" : : : "memory");
>> to attempt to prevent GCC from reordering any memory accesses across
>> those function calls.
>>
>> I've verified that this solves the issue in the assembly with our
>> current JDK 9 build platform compilers.
>> I've also verified that this particular piece of code is compiled
>> correctly on our other x86 platforms: Solaris, Windows and OS X.
>>
>> Webrev:
>> http://cr.openjdk.java.net/~mgerdin/8061964/webrev.0/
>> Bug:
>> https://bugs.openjdk.java.net/browse/JDK-8061964
>> Testing:
>> JPRT, inspecting generated assembly for the function
>> G1OffsetTableContigSpace::record_top_and_timestamp (as the method is
>> currently named).
>> Suggestions of further testing is greatly appreciated.
>>
>> Thanks
>> Mikael
>