RFR (S): 8024067: Missing replace_in_map() calls following null checks

[Vladimir Kozlov]

Yes, it is good.

Thanks,
Vladimir

On 9/4/13 2:52 AM, Roland Westrelin wrote:
> Thanks for taking the time to discuss this.
>
> Is this better:
> http://cr.openjdk.java.net/~roland/8024067/webrev.01/
> ?
>
> Roland.
>
> On Sep 4, 2013, at 12:15 AM, Vladimir Kozlov <vladimir.kozlov at oracle.com> wrote:
>
>> Can you pass safe_for_replace into null_check_oop()? I want replace_in_map in one place.
>>
>> Vladimir
>>
>> On 9/3/13 2:49 PM, Roland Westrelin wrote:
>>>> Then why not do that in null_check_oop()? And why we need safe_for_replace?
>>>
>>> For instance, LibraryCallKit::inline_native_Class_query() for vmIntrinsics::_isInstance builds a region with 2 control paths. gen_instanceof() is used on one of the paths. gen_instanceof() calls null_check_oop(). If null_check_oop() does the replace_in_map then we can end up with a reference in the map after the isInstance that is only valid on one of the control paths. In general, null_check_oop() is called directly or indirectly from library_call.cpp where we build our own control flow and null_check_oop() can be only valid on one control path and so a replace_in_map cannot be safely performed.
>>>
>>> Roland.
>>>
>>>> PS: Roland, please, include previous mails text (with webrev link) in your responses.
>>>>
>>>> On 9/3/13 1:48 PM, Roland Westrelin wrote:
>>>>>
>>>>>> Why you need this when null_check_common() does this already?
>>>>>
>>>>> GraphKit::null_check_oop() sets (*null_control) = top()
>>>>> then GraphKit::null_check_common() does:
>>>>> if (null_control != NULL) {
>>>>> (*null_control) = null_true;
>>>>> so (*null_control) is no longer top and:
>>>>>
>>>>>     if (null_control == NULL || (*null_control) == top())
>>>>>        replace_in_map(value, cast);
>>>>>
>>>>> doesn't help. Back in GraphKit::null_check_oop():
>>>>>
>>>>>    if (never_see_null && (*null_control) != top()) {
>>>>>
>>>>> is where the uncommon trap is added and (*null_control) = top() is set back to top and nothing does the replace_in_map.
>>>>>
>>>>> Roland.
>>>>>
>>>
>