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