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

[Tom Rodriguez]

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

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

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?

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.

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