RFR: JDK-8213199: GC abstraction for Assembler::needs_explicit_null_check()

[Erik Österlund]

Hi Roman,

Glad you liked the idea!

On 2018-11-06 22:37, Roman Kennke wrote:
> Hi Erik,
> 
> I also like it. I needed to change a little bit to make it work with
> Shenandoah:
> 
> http://cr.openjdk.java.net/~rkennke/JDK-8213199/JDK-8213199-eosterlund.patch
> 
> I will give it a spin on aarch64 as soon as I get access to the machine.
> 
> Do you want to take the bug and finish it, or do you want me to take it
> from here?

You can take it from here. I just wanted to show a possible direction.

Thanks,
/Erik

> Roman
> 
>> Hi Andrew,
>>
>> On 2018-11-06 17:46, Andrew Dinn wrote:
>>> On 06/11/18 16:04, Erik Österlund wrote:
>>>> New webrev:
>>>> http://cr.openjdk.java.net/~eosterlund/8213199/webrev.01/
>>>
>>> Thanks for the updated version.
>>>
>>>> On 2018-11-06 16:13, Andrew Dinn wrote:
>>>>> In that case, I hear you ask, why does the old code check for values in
>>>>> the range [heap_base - cell_header_size, heap_base + os::vm_page_size)?
>>>>> Good question. I don't know the answer. I think that test is bogus with
>>>>> no Shenandoah GC present.
>>>>
>>>> Interesting. It turns out that both ranges are relevant
>>>> [-cell_header_size, os::vm_page_size) and [heap_base - cell_header_size,
>>>> heap_base + os::vm_page_size). Looks like we both missed that from the
>>>> original check.
>>>> I updated my webrev to explicitly reflect those two ranges, as two
>>>> different checks.
>>>
>>> Well, you say that but -- as I said before -- I am not sure why *both*
>>> are relevant to the non-Shenandoah case. And, given Roman's correction
>>> (doh, yes of course! the space for the Brooks pointer field had better
>>> lie in RW-mapped memory :-) I don't see why it is relevant to the
>>> Shenandoah case either.
>>
>> The reason it is needed is because there may or may not be null checks
>> in both the decode() step to get the base pointer of an access, and the
>> access itself, given some base pointer. This results in two ranges for
>> those two cases correspondingly. If the decoding uses decode_not_null,
>> with an implicit null check in the decoding itself that blindly adds the
>> heap base without checking for null, we will crash into the first page
>> of the heap which is protected when performing accesses into that. But
>> sometimes we have a decode()ed base pointer (with null check), that has
>> already been fixed up to NULL, and then perform an access at an offset
>> into that with an implicit null check. This results in a trap at the
>> first page. So in summary, both ranges are needed because the
>> implicitness of the null check may be both in the access and the
>> narrowOop decoding.
>>
>> But as I pointed out, this (seemingly incorrectly) assumes there may not
>> be any offset values in the (heap_base, heap_base + os::vm_page_size)
>> range. If you hit such an offset, we will generate incorrect code, due
>> to the lack of distinction between addresses and offsets.
>>
>>> I'd be very glad to hear from someone who knows about the history of the
>>> original function as to why the extra test for addresses above heap_base
>>> is included [hint: John Rose? David Holmes? Coleen? anyone else who
>>> might have been around when it was added?]
>>>
>>> If that check is not actually needed then getting rid of it would make
>>> the whole thing a lot simpler.
>>
>> It would indeed. But unfortunately we need it.
>>
>>>>>> Just expressing these trivial ranges separately would have been very
>>>>>> easy to understand.
>>>>> Except it's not that easy. Also, with Shenandoah there is a new
>>>>> wrinkle.
>>>>> It is possible for an object to be allocated at the first non-guarded
>>>>> address in the heap area i.e. heap_base + os::vm_page_size. I that case
>>>>> a dereference of its Brooks pointer will lie in the protected page.
>>>>
>>>> Looks like Roman just said that's not possible.
>>>
>>> Yes, my dumb!
>>>
>>>> Fixed. Might need some help testing this on AArch64 though if we decide
>>>> to go with it.
>>>
>>> I am sure Roman and Aleksey can do that but if needed I will.
>>>
>>>> Do you agree though, that the current version that merges the address
>>>> and offset case relies on offsets being smaller than the heap base for
>>>> its magic check to work, and that we actually do not have such a
>>>> guarantee in the VM? Apart from readability preferences, this seems like
>>>> it is broken in the current form, unless there are seat belts I am not
>>>> aware of. In particular, if you have an offset value that is in the
>>>> virtual address range (heap_base, heap_base + os::vm_page_size()), which
>>>> I argue can happen, then it looks like we will normalize that as if it
>>>> was an address (even though it is not), and incorrectly say that this
>>>> offset does not need explicit null checks, despite needing explicit null
>>>> checks.
>>> Well, without the answer as to whether the check against heap_base is
>>> needed I am not sure. Assuming it is then then ...
>>>
>>> Yes, it is possible, in theory, that an oop offset could turn out to
>>> have value (heap_base + D) where 0 <= D < os_page_size(). In that case
>>> the offset might wrongly be classified as *not* needing a null check
>>> even though it really does. I suspect it is unlikely in practice given
>>> how mmap hands out addresses but you never know.
>>>
>>> As for readibility, I think the two functions are probably clearer --
>>> expecially if they stay adjacent in the source code.
>>
>> Okay, sounds like we are on the same page. I agree it is unlikely that
>> this will crash the VM (as evidenced by the lack of crashes observed).
>> But I don't like relying on luck for correctness.
>>
>> Thanks,
>> /Erik
>>
>>> regards,
>>>
>>>
>>> Andrew Dinn
>>> -----------
>>> Senior Principal Software Engineer
>>> Red Hat UK Ltd
>>> Registered in England and Wales under Company Registration No. 03798903
>>> Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander
>>>
>