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

[Erik Österlund]

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
>