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

[Roman Kennke]

Hi Andrew,

> On 13/11/2018 12:17, Roman Kennke wrote:
>> Apparently there is another quirk to this whole issue: It looks like
>> offset=-1 is used in a few places (at least in C1), presumably as
>> placeholder for offsets to be patched (to be honest, I am not totally
>> sure). This would result in needs_explicit_null_check=false with
>> Shenandoah because we cover [-8..page_size), but this results in weird
>> problems later. It is expected that this placeholder results in
>> needs_explicit_null_check=true for offset=-1.
> 
> Hmm, can you be more specific about where the -1 value comes from.
> That's really the crux of the matter. Giving -1 a free pass is not going
> to work if the offset that is patched in later turns out to be greater
> than os page size.

I think it's used for to-be-patched offsets in a few places in C1.

The point is to *not* (accidentally) give it a free pass, but instead
force explicit null-checking on it.

Roman

>> A possible fix is to explicitly check for this:
>> diff --git a/src/hotspot/share/asm/assembler.cpp
>> b/src/hotspot/share/asm/assembler.cpp
>> --- a/src/hotspot/share/asm/assembler.cpp
>> +++ b/src/hotspot/share/asm/assembler.cpp
>> @@ -334,6 +334,7 @@
>>
>>  bool MacroAssembler::needs_explicit_null_check(intptr_t offset) {
>>    // Check if offset is outside of [-cell_header_size, os::vm_page_size)
>> +  if (offset == -1) return true;
>>    return offset < -Universe::heap()->cell_header_size() ||
>>           offset >= os::vm_page_size();
>>  }
>>
>> Thoughts? Any better ideas?
> Well, if -1 is always an innocuous offset value then I think this looks
> fine work but we may be in the realm of counter-factual conclusions here
> so ... see above.
> 
> 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
>