[9] RFR (XXS): 8062258: compiler/debug/TraceIterativeGVN.java segfaults in trace_PhaseIterGVN

[Vladimir Kozlov]

I am fine with this version. Reviewed.

Thanks,
Vladimir

On 11/12/14 3:46 AM, Vladimir Ivanov wrote:
> Vladimir,
>
> What do you think about the following version:
>    http://cr.openjdk.java.net/~vlivanov/8062258/webrev.02
>
> I decided to revert changes in dumping logic. IMO possible duplication
> of output is a lesser problem than a complication in dumping logic.
>
> Best regards,
> Vladimir Ivanov
>
> On 10/31/14, 2:28 AM, Vladimir Kozlov wrote:
>> Unfortunately not only MemNode has Type::MEMORY type. PhiNode,
>> SCMemProjNode and others. It looks like you need to fix all places.
>> Change in memnode.cpp is fine but in node.cpp you need:
>>
>>      } else if (t == Type::MEMORY && (!is_Mem() || is_LoadStore())) {
>>        st->print("  Memory:");
>>        MemNode::dump_adr_type(this, adr_type(), st);
>>
>> Make sure that other Type::MEMORY nodes don't crash in adr_type().
>> Don't do check under NOT_PRODUCT as in your first fix - just return NULL
>> in all cases.
>>
>> Thanks,
>> Vladimir
>>
>> On 10/30/14 1:48 PM, Vladimir Ivanov wrote:
>>> Vladimir, nice observation!
>>>
>>> http://cr.openjdk.java.net/~vlivanov/8062258/webrev.01/
>>>
>>> Best regards,
>>> Vladimir Ivanov
>>>
>>> On 10/30/14, 11:25 PM, Vladimir Kozlov wrote:
>>>> Hmm, next code in PhaseIterGVN::optimize() worries me. We dump dead
>>>> nodes before removing them from graph:
>>>>
>>>>      DEBUG_ONLY(trace_PhaseIterGVN_verbose(n, num_processed++);)
>>>>      if (n->outcnt() != 0) {
>>>>        NOT_PRODUCT(const Type* oldtype = type_or_null(n));
>>>>        // Do the transformation
>>>>        Node* nn = transform_old(n);
>>>>        NOT_PRODUCT(trace_PhaseIterGVN(n, nn, oldtype);)
>>>>      } else if (!n->is_top()) {
>>>>        remove_dead_node(n);
>>>>      }
>>>>
>>>> I am changing my suggestion: fix  Node::dump() which calls  adr_type()
>>>> without check. It should not call MemNode::dump_adr_type() because
>>>> MemNode::dump_spec(), which is called before, already does that and it
>>>> has NULL check! If you want, you can add "  Memory:" output in
>>>> MemNode::dump_spec() to be similar to Node::dump() output.
>>>>
>>>> Thanks,
>>>> Vladimir
>>>>
>>>> On 10/30/14 10:26 AM, Vladimir Ivanov wrote:
>>>>> Vladimir, do you suggest to simply skip printing info about dead nodes
>>>>> w/ -XX:+TraceIterativeGVN?
>>>>>
>>>>> Best regards,
>>>>> Vladimir Ivanov
>>>>>
>>>>> On 10/29/14, 7:00 PM, Vladimir Kozlov wrote:
>>>>>> I would suggest to dead node check in trace_PhaseIterGVN() instead of
>>>>>> in  MemNode::adr_type().
>>>>>> So the fix for your JDK-8061995 should fix this problem too.
>>>>>>
>>>>>> Thanks,
>>>>>> Vladimir
>>>>>>
>>>>>> On 10/29/14 4:57 AM, Vladimir Ivanov wrote:
>>>>>>> http://cr.openjdk.java.net/~vlivanov/8062258/webrev.00/
>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8062258
>>>>>>>
>>>>>>> Trivial fix to avoid NULL dereference w/ -XX:+TraceIterativeGVN
>>>>>>> while
>>>>>>> querying adr_type() on dead MemNode.
>>>>>>>
>>>>>>> Testing: failing test.
>>>>>>>
>>>>>>> Best regards,
>>>>>>> Vladimir Ivanov