RFR: 8213795: Force explicit null check on patching placeholder offset

[Vladimir Kozlov]

On 11/13/18 1:57 PM, Roman Kennke wrote:
>>> That's actually a not-completely-trivial exercise because there are a
>>> bunch of places and they don't actually say 'this is a placeholder for
>>> patching offsets' they just say '-1'. I'll try to sort it out, and while
>>> doing it maybe makes sense to replace -1 by a defined constant that can
>>> be grepped for. Right?
>>
>> Yes, definitely right.
> 
> Hmm, except that this is *exceptionally* hard. -1 are all over the
> place, and it's hard to tell if a -1 somehow propagates through to
> needs_explicit_null_check(..) or not, or if it's a totally unrelated -1.
> For example, I just found a -1 in MacroAssembler::null_check(offset) in
> macroAssembler_x86.hpp line 97. If you look around you'll also find one
> in, e.g., GraphBuilder::access_field() in c1_GraphBuilder.cpp around
> line 1664, which looks like it supports my original idea about patching
> offset. But it doesn't seem to be only that. I think it is implicitely
> assumed all over the place that an offset of -1 must result in an
> explicit null check (which it naturally does when checking for interval
> [0..page_size) ).
> 
> I guess we're lucky in Shenandoah that we have the fixed offset of -8
> for the forwarding pointer ;-)

:)

> 
> I suggest we go on with the patch as proposed with the comment that
> explains it, unless you have a better idea?

Originally I just want more detailed comment, nothing more. Current comment is too general.
I asked Graal's engineers and they said Graal does not use -1 offset. This is really C1 only "feature".
The comment should reflect that.

Thanks,
Vladimir

> 
> Roman
> 
>> Vladimir
>>
>>>
>>> Roman
>>>
>>>> Hi Roman,
>>>>
>>>> Would be nice if the comment points to C1 code which use -1 offset. In a
>>>> future, when we forgot the context, I don't want to look through all
>>>> Hotspot code to find where this -1 is coming from.
>>>>
>>>> Thanks,
>>>> Vladimir
>>>>
>>>> On 11/13/18 7:16 AM, Roman Kennke wrote:
>>>>> With current logic after JDK-8213199, we allow implicit null-checks in
>>>>> the offset range [-cell_header_size;vm_page_size). When using
>>>>> Shenandoah, cell_header_size is -8, so we allow [-8;vm_page_size).
>>>>> Unfortunately, this disables explicit null-checks on -1 which is
>>>>> used as
>>>>> placeholder for offsets to be patched in C1. This results in weird
>>>>> asserts later and may actually crash if offset is outside of legal
>>>>> range
>>>>> for implicit null-checks. We need to force explicit null checks on -1.
>>>>>
>>>>> Bug:
>>>>> https://bugs.openjdk.java.net/browse/JDK-8213795
>>>>> Webrev:
>>>>> http://cr.openjdk.java.net/~rkennke/JDK-8213795/webrev.00/
>>>>>
>>>>> Testing: tier1 and Shenandoah testing
>>>>>
>>>>> Can I please get a review?
>>>>>
>>>>> Roman
>>>>>
>>>
>