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

[Vladimir Kozlov]

Tom Rodriguez wrote:
> 
>> 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?

jvm98 has several derived_oop cases where mach_null is used.
But generated code stays the same with my fix and without it.
I tried the new change below and it also did not change the generated code.
I also did not reuse preexisting loadconp0 (which we can't control where
it is placed) and use only newly generated mach_null.
It simplified the changes. I think, I will go with it.

Vladimir

> 
> 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
>