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

[Vladimir Ivanov]

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