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

[Vladimir Ivanov]

On 21/03/2019 14:35, Roman Kennke wrote:
>> 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/).
> 
> We specifically have gc/shenandoah/compiler to host compiler tests that
> are known to fail only with Shenandoah. I'd rather keep it there as it
> is. Maybe we shall try to come up with a more generic test that also
> fails without Shenandoah? (Note: I tried for a little bit - not very
> much -, but failed... ).
> 
>>    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.
> 
> The @requires vm.gc.Shenandoah seems to be the correct tag to filter this.
> 
> Can I push it as it is?

Ok.

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