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

[Roman Kennke]

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

Roman


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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: OpenPGP digital signature
URL: <https://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20190321/e32e3a96/signature.asc>