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

[Roman Kennke]

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