[aarch64-port-dev ] Problem in MacroAssembler::needs_explicit_null_check

[Roman Kennke]

Hi Andrew,

Thanks for the analysis and proposal.

I find the bitwise magic a bit too magical:bool
MacroAssembler::needs_explicit_null_check(intptr_t offset) {
>   // Exception handler checks the nmethod's implicit null checks table
>   // only when this method returns false.
> #ifdef AARCH64
>   // AArch64 addresses should only have 0x0000 or 0xffff
>   // in the top 16 bits but in a fault address the latter
>   // may be reset to 0x00ff. Non-zero bits are only legitimate
>   // when Shenandoah loads a Brooks pointer via a null oop
>   // i.e. original address should be - BrooksPointer::byte_offset().
>   long loffset = (long)offset;
>   long hi = loffset >> 48;
>   int adj = BrooksPointer::byte_offset();
>   if (hi != 0) {
>     if (hi == 0x00ffL || hi == 0xffffL) {
>       // the top 8 bits may have been cleared
>       // if so make sure they are set again
>       loffset = (loffset << 8) >> 8);
Why cast to long? Isn't intptr_t basically the same, but more
explicitely pointer-sized?
How does shifting left then right 'make sure they are set again' ? Do we
want them all set? Then simply loffset |= 0xffff << 48; should do it, no?
Also, it seems we want to enclose this in if (UseShenandoahGC) ?

>       // if this is not 0 - Brooks pointer offset
>       // we need an explicit null check
>       return ((loffset + adj) != 0);
>     }
>   }
And if not, then we fall through. Is this the intention?

> #endif
> #ifdef _LP64

Does AArch64 define _LP64 ?
>   if (UseCompressedOops && Universe::narrow_oop_base() != NULL) {
>     assert (Universe::heap() != NULL, "java heap should be initialized");
>     // The first page after heap_base is unmapped and
>     // the 'offset' is equal to [heap_base + offset] for
>     // narrow oop implicit null checks.
>     uintptr_t base = (uintptr_t)Universe::narrow_oop_base();
>     int adj = MIN2(0, UseShenandoahGC ? BrooksPointer::byte_offset() : 0);
>     if (((uintptr_t)(offset - adj)) >= base) {
>       // Normalize offset for the next check.
>       offset = (intptr_t)(pointer_delta((void*)offset, (void*)base, 1));
>     }
>   }
> #endif
>
> #ifndef AARCH64
>   if (UseShenandoahGC) {
>     if (offset == BrooksPointer::byte_offset()) {
>       return false;
>     }
>   }
> #endif
>   return offset < 0 || os::vm_page_size() <= offset;
> }
>
> Comments and or corrections welcome.
>
> regards,
>
>
> Andrew Dinn
> -----------
>
I don't much like littering shared code with #ifdef SOMEARCH... Maybe
this stuff should be moved to src/cpu somewhere?

Roman