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

[Roman Kennke]

Hi Erik,

> The approach looks great! :p

Ha! :-)

> Looks like "you" forgot to call the new function in os_solaris_sparc.cpp
> though, he he. With that fixed, we pass tier1-3 on our platforms.
> Oh and the cast in the following line is unnecessary: intptr_t start =
> (intptr_t)-cell_header_size;

Both fixed here (solaris/sparc stuff blindly though):
Incremental:
http://cr.openjdk.java.net/~rkennke/JDK-8213199/webrev.03.diff
Full:
http://cr.openjdk.java.net/~rkennke/JDK-8213199/webrev.03/

Good now?

Thanks for reviewing and helping!

Roman

> Thanks,
> /Erik
> 
> On 2018-11-07 09:52, Roman Kennke wrote:
>> 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
>>>>>>
>