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

[Tom Rodriguez]

On Jan 5, 2012, at 12:18 PM, Vladimir Kozlov wrote:

> Tom Rodriguez wrote:
>> 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");
> 
> So you want to put check only for original old node and allow input to be Phi node for any original old node? It is different from existing code. My assert check allows this (Phis) only if original old node was load from constant table node so it is more strict.

I was having a little trouble mapping the new assert onto the old logic since it seems more strict in other ways, since it requires  req == 3 for the handling of temps and I don't see any reason for that restriction.  I think reversing the sense of only_one_input would make the assert read more easily.  Also if you swap the assert and is_Con() lines then the !only_one_input would subsumes is_MachTemp checks.

+     if (old->is_Con() && (old->req() > 1)) {
+       // Load from constant table node may have Phi node as input.
+       multiple_inputs = true;
+     }
+     assert(multiple_inputs || old->req() < 3, "expecting only one data input");

You could assert in the is_Con case that the inputs are only Phi, MachTemp and MachConstantBase.

tom

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