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

[Erik Österlund]

Hi Roman,

The approach looks great! :p

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;

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