RFR: Quick-fix C1 assert with cherry-picked "8213199: GC abstraction for Assembler::needs_explicit_null_check()"

[Aleksey Shipilev]

On 11/13/2018 01:22 PM, Roman Kennke wrote:
> Yesterday I cherry-picked "8213199: GC abstraction for
> Assembler::needs_explicit_null_check()". However, there is a problem
> with this patch: for code that needs patching, it uses offset=-1 and it
> is expected to return true for needs_explict_null_check(). However,
> since we're checking the *range* [-8..page_size) in Shenandoah, this
> would result in needs_explicit_null_check=false and later on fail in
> funny ways like the assert that we have seen in specjvm.
> 
> A quick-fix is:
> 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();
>  }
> 
> 
> I am also discussing it upstream and come up with a proper fix, until
> then we should push this. OK?

Yeah, but please put the comment why this check is there, e.g.:

 // Patching code expects to return true for "special" offsets like -1.

-Aleksey