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

[Daniel D. Daugherty]

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

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.

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!

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