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

[Roman Kennke]

Hi Erik,

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

Right. Thanks again for reviewing and helping.

I'll push it through jdk/submit while waiting for another review. Andrew?

Roman


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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20181107/bbb07655/signature.asc>