RFR: JDK-8220714: C2 Compilation failure when accessing off-heap memory using Unsafe

[Vladimir Ivanov]

test/hotspot/jtreg/gc/shenandoah/compiler/TestUnsafeOffheapSwap.java

Since it's not a GC bug, I'd expect to see the regression test among 
compiler tests (compiler/unsafe/ or compiler/c2/).

   28  * @requires vm.gc.Shenandoah

I prefer "-XX:+IgnoreUnrecognizedVMOptions". As you noted, the bug is 
generic, but is triggered only w/ Shenandoah enabled in this particular 
case.

Best regards,
Vladimir Ivanov

On 21/03/2019 05:00, Roman Kennke wrote:
> Hi Vladimir,
> 
> Thanks for reviewing!
> 
> Not sure what happened to the webrev. Parts of it seem to have snuck in 
> from a previous working version. Here's a good one:
> 
> http://cr.openjdk.java.net/~rkennke/JDK-8220714/webrev.01/
> 
> Thanks,
> Roman
> 
> 
>> Proposed fix looks good [1].
>>
>> (FYI the patch [2] differs significantly from the webrev.)
>>
>>> Looking on changes and it seems you can modify it a little:
>>>
>>>    // Can base be NULL? Otherwise, always on-heap access.
>>>    bool can_access_non_heap = 
>>> TypePtr::NULL_PTR->higher_equal(_gvn.type(base));
>>>    if (!can_access_non_heap) {
>>>      heap_base_oop = base;
>>>      decorators |= IN_HEAP;
>>>    } else if (type == T_OBJECT) {
>>>      return false; // off-heap oop accesses are not supported
>>>    }
>>
>> It doesn't look right.
>>
>> There're 3 cases we care about here:
>>    * on-heap accesses (non-NULL base)
>>    * off-heap accesses (NULL base)
>>    * mixed accesses (base may be NULL)
>>
>> 'can_access_non_heap' distinguishes on-heap accesses from 
>> off-heap/mixed ones.
>>
>> 'heap_base_oop != top()' distinguishes off-heap accesses from 
>> on-heap/mixed ones.
>>
>> Best regards,
>> Vladimir Ivanov
>>
>> [1]
>>
>> -  bool can_access_non_heap = 
>> TypePtr::NULL_PTR->higher_equal(_gvn.type(heap_base_oop));
>> +  bool can_access_non_heap = 
>> TypePtr::NULL_PTR->higher_equal(_gvn.type(base));
>>
>>
>> [2] 
>> http://cr.openjdk.java.net/~rkennke/JDK-8220714/webrev.00/jdk-jdk.changeset 
>>
>>
>> -  bool can_access_non_heap = 
>> TypePtr::NULL_PTR->higher_equal(_gvn.type(heap_base_oop));
>> +  bool can_access_non_heap = 
>> TypePtr::NULL_PTR->higher_equal(_gvn.type(UseNewCode ? base : 
>> heap_base_oop));
>>
>>
>>> On 3/15/19 4:39 AM, Roman Kennke wrote:
>>>> A user reported misbehaving off-heap access. It looks like a C2
>>>> compilation failure that seems to only trigger with Shenandoah:
>>>>
>>>> https://mail.openjdk.java.net/pipermail/shenandoah-dev/2019-March/009060.html 
>>>>
>>>>
>>>> Eg with G1 generates this assembly for swapping two array elements:
>>>> mov (%r8),%r9d
>>>> mov (%r10),%r11d
>>>> mov %r9d,(%r10)
>>>> mov %r11d,(%r8)
>>>>
>>>> While with Shenandoah we get this:
>>>> mov (%r9),%ecx
>>>> mov %ecx,(%r10,%r11,1)
>>>> mov %ecx,(%r9)
>>>>
>>>> I.e. the two loads seem to have been wrongly coalesced into one.
>>>>
>>>> Even though that is only triggered by Shenandoah, it seems to be a 
>>>> legit
>>>> and generic C2 problem.
>>>>
>>>> Bug:
>>>> https://bugs.openjdk.java.net/browse/JDK-8220714
>>>> Webrev:
>>>> http://cr.openjdk.java.net/~rkennke/JDK-8220714/webrev.00/
>>>>
>>>> The issue seems to be that off-heap accesses are supposed to use
>>>> MO_RELAXED mem-ordering instead of MO_UNORDERED, as implemented in
>>>> C2Access::needs_cpu_membar(). However, it seems we wrongly set this 
>>>> here
>>>> (library_call.cpp around l2410:
>>>>
>>>>    // Can base be NULL? Otherwise, always on-heap access.
>>>>    bool can_access_non_heap =
>>>> TypePtr::NULL_PTR->higher_equal(_gvn.type(heap_base_oop));
>>>>    if (!can_access_non_heap) {
>>>>      decorators |= IN_HEAP;
>>>>    }
>>>>
>>>> However, heap_base_oop is initialized to top() a few lines up, and then
>>>> never updated, at least not for the off-heap-access case. And top
>>>> doesn't match NULL_PTR afaik.
>>>>
>>>> The proposed fix uses base instead of heap_base_oop for that check, 
>>>> this
>>>> should be updated correcly through make_unsafe_addr() and
>>>> classify_unsafe_addr().
>>>>
>>>> For some reason, this bug only seems to be exposed when running
>>>> Shenandoah, and then only when actually compiling with barriers. We
>>>> could not reproduce this with any other GC. It totally eludes me why
>>>> that is so. For this reason, the testcase goes under the
>>>> gc/shenandoah/compiler directory.
>>>>
>>>> Roman
>>>>