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

Tue Nov 6 21:37:14 UTC 2018

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?

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/20181106/85e733cc/signature.asc>