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

[Vladimir Kozlov]

Thanks, Tom

Tom Rodriguez wrote:
> This basically looks good to me.  I don't really like the ConP0 naming.  
> I might prefer mach_null and ideal_null.

OK.

> 
> You call new_lrg for every use of base, which seems wrong.  It should 
> only be done once shouldn't it and it shouldn't be done at all if 
> there's already an LRG for it, right?

I don't think it is matter since the live ranges will be recomputed
since new lrg is added:

   if( (n2lidx(base) >= _maxlrg ||// (Brand new base (hence not live) or
     must_recompute_live = true;

I think, it is more robust to do it this way.
But I will change the code as you suggested and see what happens.

> 
> I'm a little confused by the comment that we reuse a NULL only if it's 
> shared.  Constants don't really care about the shared flag so it seems 
> like you could make that code simpler.  I think it should find a 
> existing ideal one if it can and create it's own if it can't and then 
> just match it explicitly.  Every other use should just use the same mach 
> node if it needs it and that shouldn't effect whether other nodes could 
> swallow it.

The difference is the shared mach NULL has control set to _root
and not shared node don't have it so it could be placed into
use's block so we can't use it for derived base. And we don't know if
we will need it for derived base so we can't always set NULL's control
to _root since it will affect GCM and RA.

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