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

[Andrew Dinn]

On 06/11/18 17:20, Erik Österlund wrote:

> On 2018-11-06 17:46, Andrew Dinn wrote:
   . . .
>> 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.

Hmm, that doesn't sound quite kosher. If decode_not_null is being called
in places where we may potentially load a null value (or rather a zero
store value stored in a field) then I would have said that we really
ought to crash and then fix the code to use a decode that tests for
null. If this is recommended a way to optimize loads of potentially null
object fields then it doesn't sound like a reliably performant way to do it.

Could you point me at some place in the compiler where narrow oop loads
are done using a decode_not_null on the assumption that a load of a null
value that gets translated to a low heap_base address will be caught and
corrected by a check initiated from the signal handler?

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

I'd still like to hear the history from whoever wrote this code.


>> 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.
Absolutely. Luck is not enough.

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