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

Wed Nov 7 15:17:07 UTC 2018

Hi Roman,

The si_addr is always void*, so the cast to void* is redundant. 
Otherwise, looks good.
Don't need to see another webrev for that.

Thanks,
/Erik

On 2018-11-07 15:51, Roman Kennke wrote:
> 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
>>>>>>>