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

[Roman Kennke]

Alright! thanks Erik! So here I'd like to propose your version plus my
little fixes for review:

http://cr.openjdk.java.net/~rkennke/JDK-8213199/webrev.02/

It passes hotspot/jtreg:tier1 locally on x86 and aarch64 and works with
Shenandoah too (and passes the Shenandoah testsuites)

Good?

Roman


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