Request for reviews (S): 7110824: ctw/jarfiles/GUI3rdParty_jar/ob_mask_DateField crashes VM

[Tom Rodriguez]

On Jan 5, 2012, at 11:36 AM, Vladimir Kozlov wrote:

> How about this?:
> 
> http://cr.openjdk.java.net/~kvn/7110824/webrev
> 
> Normal load constant nodes have only one (root) input so I look for 2 inputs loads and allow multiple inputs only in that case. Should I use guarantee instead of assert?

Could you do the same thing with the flag but only have it true for the initial call and false for the recursive ones and then the function would start with something like:

assert(!first_yank || old->is_Con() || old->req() < 3, "unexpected node");

I'm not sure that assert is complete but I think it captures the cases we're expecting.  I'm just not sure it adds value.

tom

> 
> Thanks,
> Vladimir
> 
> Tom Rodriguez wrote:
>> On Jan 4, 2012, at 7:54 PM, Vladimir Kozlov wrote:
>>> http://cr.openjdk.java.net/~kvn/7110824/webrev
>>> 
>>> 7110824: ctw/jarfiles/GUI3rdParty_jar/ob_mask_DateField crashes VM
>>> 
>>> PhaseChaitin::yank_if_dead() can't handle nodes which load from constant table because MachConstantBase node could be spilled and Phi nodes are generated for it. In failing case loadConP_load node has Phi node as input and it is yanked since there is an other copy of the same constant load.
>>> 
>>> Change yank_if_dead() to recursive method to remove all dead inputs.
>> The change seems fine though the limited logic would protect us from bad graph shapes and I'm a little concerned that it will now just blindly trim anything away that it sees without complaining.  We'd probably die at a later point in that case but it will be a bit more mysterious.  An assert could check that we only start yanking certain node type but I don't know how long that list would be.  Would that be overkill?
>> tom
>>> Tested with full CTW and failing test.
>>> 
>>> Thanks,
>>> Vladimir