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

[Erik Österlund]

Hi Andrew,

New webrev:
http://cr.openjdk.java.net/~eosterlund/8213199/webrev.01/

On 2018-11-06 16:13, Andrew Dinn wrote:
> Hi Erik,
>
> On 06/11/18 14:33, Erik Österlund wrote:
>
>> For offsets, we are interested in the following range:
>> [-cell_header_size, os::vm_page_size)
>>
>> For addresses, we are interested in the following ranges:
>> 1) with heap base: [heap_base - cell_header_size, heap_base +
>> os::vm_page_size)
>> 2) without heap base: [-cell_header_size, os::vm_page_size)
> I don't think that is correct. Even when we are using a heap_base null
> oops are still represented by address 0. So, the address range of
> interest in case 1 is [-cell_header_size, os::vm_page_size).
>
> That's the value used when a 'null' value is instantiated in memory. It
> is also the value tested for by encode_heap_oop when it decides to write
> 0 into an object field to represent a null oop and is also the value
> returned by decode_heap_oop when it detects 0 in an object field.
>
> 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.

> So, anyway, this means your code for uses_implicit_null_check in
> assembler.cpp is borked.

Yup. Good catch.

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

>> Thoughts?
> I am agnostic as to whether this is done using one or two functions.
> However, besides the above issue I believe there is a small error in
> your rewrite of os_linux_aarch64.cpp. At line 259 you correct si_addr to
> or back in the top byte if bit 55 is non-zero.
>
> 359     address addr = (address) info->si_addr;
> 360
> 361     // Make sure the high order byte is sign extended, as it may be
> masked away by the hardware.
> 362     if ((uintptr_t(addr) & (uintptr_t(1) << 55)) != 0) {
> 363       addr = address(uintptr_t(addr) | (uintptr_t(0xFF) << 56));
> 364     }
> 365
>
> However, at line 463 you fail to pass the corrected address to
> uses_implicit_null_check
>
> 462       } else if (sig == SIGSEGV &&
> 463
> MacroAssembler::uses_implicit_null_check(info->si_addr)) {

Fixed. Might need some help testing this on AArch64 though if we decide 
to go with it.

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.

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