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

[Vladimir Ivanov]

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