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

[Andrew Dinn]

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.

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.

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

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