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

[Andrew Dinn]

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.

> 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