Request for reviews (M): 6709742: find_base_for_derived's use of Ideal NULL is unsafe causing crashes during register allocation

[Tom Rodriguez]

>>>
> I did next change and it passed CTW:
>
>      _cfg._bbs.map( base->_idx, startb );
> +     assert (n2lidx(base) == 0, "should not have LRG yet");
>    }
> +   if (n2lidx(base) == 0) {
> +     new_lrg(base, maxlrg++);
> +   }
> -   new_lrg(base, maxlrg++);

ok.

> So I am thinking may be instead of using generated mach_null in
> find_base_for_derived() we should look for live in values which is  
> NULL
> for this block and use it. And if there is no live in then use
> mach_null's clone and place it in this block with local control.
> It will prevent having a long live range for such NULL.

The register allocator will preferentially split the rematerializable  
NULL live range instead of spilling other things so I'm not sure it's  
worth trying to do this.  Also if we've properly commoned the machnode  
for NULL as we normally do, then at this point in register allocation  
any live in copy should be the same node as the original one shouldn't  
it?  By the way, the code below would need to handle the possibility  
that _def == NodeSentinel (-1) which happens for multidef live ranges.

Any attempt to be more clever about this should be driven by whether  
the code looks any different or not.    Are you seeing much of a  
difference?

tom


>    Node *base = NULL;
>    while ((neighbor = elements.next()) != 0) {
>      Node *mach_null = lrgs(neighbor)._def;
>      if (mach_null->is_Mach() &&
>          mach_null->as_Mach()->ideal_Opcode() == Op_ConP &&
>          mach_null->bottom_type() == TypePtr::NULL_PTR) {
>        base == mach_null;
>        break;
>      }
>    }
>    if (base == NULL) { // No livein NULL
>      base = _matcher.mach_null()->clone();
>      base->init_req(0, _cfg._root);
>      new_lrg(base, maxlrg++);
>      b->_nodes.insert(b->find_node(derived), base);
>      _cfg._bbs.map(base->_idx, b);
>    }
>    assert(base != NULL, "sanity");
>
> Vladimir
>
>> tom
>>>> Have you compared the generated code for this change?  We've  
>>>> potentially got a bunch of new values floating around and we want  
>>>> to make sure it doesn't really change the code we generate.  I  
>>>> played with a variant of this fix and saw a lot of new NULL  
>>>> values floating around.  Might we need to clone these derived  
>>>> mach nulls to uncommon traps?
>>>
>>> No, I did not look on generated code. I will look.
>>>
>>>> I have a vague memory of some old bug related NULL+con derived  
>>>> oops appearing in oopmaps and thought there was some special  
>>>> handling of the ideal ConP so that we didn't bother putting these  
>>>> in oopmaps.  I can't find the code I'm thinking of anymore but  
>>>> maybe Chuck has some thoughts on this?
>>>> Once this is fixed we can restore the logic for undoing Phi of  
>>>> AddP, though maybe we should consider revisiting when we do this  
>>>> transformation.  I think we did a lot of ping ponging with the  
>>>> old code and maybe we should clean it up at the end of the  
>>>> compile instead.
>>>
>>> Yes, I have bug for it: 6747632.
>>>
>>> Thanks,
>>> Vladimir
>>>
>>>> tom
>>>> On Apr 20, 2009, at 2:50 PM, Vladimir Kozlov wrote:
>>>>>
>>>>> http://cr.openjdk.java.net/~kvn/6709742/webrev.00
>>>>>
>>>>> Fixed 6709742: find_base_for_derived's use of Ideal NULL is  
>>>>> unsafe causing crashes during register allocation
>>>>>
>>>>> Problem:
>>>>> PhaseChaitin::stretch_base_pointer_live_ranges() stretches
>>>>> the base pointers for live ranges and in some cases may
>>>>> have to construct a NULL base in find_base_for_derived.
>>>>> It constructs an Ideal NULL instead of a mach one and
>>>>> if the Ideal NULL is ever used in Phi with real machine
>>>>> values we will die during register allocation.
>>>>>
>>>>> Solution:
>>>>> Create a mach node corresponding to ideal node ConP #NULL
>>>>> specifically for derived pointers.
>>>>> Use an existing mach node (matched for ConP #NULL) only
>>>>> if it is shared to avoid false sharing if the mach node
>>>>> for derived pointers is not used.
>>>>>
>>>>> Add the assert to catch the bug case.
>>>>> Add asserts to verify that narrow pointers can't be derived.
>>>>>
>>>>> Reviewed by:
>>>>>
>>>>> Fix verified (y/n): y
>>>>>
>>>>> Other testing:
>>>>> JPRT, CTW (32- and 64-bit), JPRT and CTW with compressed oops