Request for reviews (M): 6709742: find_base_for_derived's use of Ideal NULL is unsafe causing crashes during register allocation
Thomas.Rodriguez at Sun.COM
Mon Apr 20 15:33:38 PDT 2009
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
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.
On Apr 20, 2009, at 2:50 PM, Vladimir Kozlov wrote:
> Fixed 6709742: find_base_for_derived's use of Ideal NULL is unsafe
> causing crashes during register allocation
> 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.
> 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
More information about the hotspot-compiler-dev