> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63747 is likely caused by
> this patch. compare_gimple_switch does not check CASE_LOW and
> CASE_HIGH, resulting merging functions not identical.
>
> Interestingly in the first a few versions of this patch CASE_LOW/HIGH
> were checked. But last versions only checks CASE_LABEL. What was the
> intention?
It sounds like an accidental change. CASE_LOW/CASE_HIGH needs to be
compared ;)
Honza
>
> Thanks,
> Joey
>
> On Thu, Oct 23, 2014 at 5:18 AM, Jiong Wang
> <wong.kwongyuan.to...@gmail.com> wrote:
> > PR 63574 ICE building libjava (segfault) on arm-linux-gnueabihf is
> > caused by this commit.
> >
> > from the backtrace, the ICF pass is trying to compare two label tree
> > node without type info.
> >
> > while looks like "compare_operand" expect the type info always be not
> > empty before invoking "func_checker::compatible_types_p".
> >
> > I think we should add a similiar t1/t2 check at the start of
> > "func_checker::compatible_types_p", just
> > like what has been done at the head of "func_checker::compare_operand".
> >
> > And I verified if we add that check, the crash gone away.
> >
> > Regards,
> > Jiong
> >
> >
> > 2014-10-15 18:03 GMT+01:00 Martin Liška <mli...@suse.cz>:
> >> On 10/14/2014 06:04 PM, Jan Hubicka wrote:
> >>>>
> >>>> diff --git a/gcc/cgraph.h b/gcc/cgraph.h
> >>>> index fb41b01..2de98b4 100644
> >>>> --- a/gcc/cgraph.h
> >>>> +++ b/gcc/cgraph.h
> >>>> @@ -172,6 +172,12 @@ public:
> >>>> /* Dump referring in list to FILE. */
> >>>> void dump_referring (FILE *);
> >>>>
> >>>> + /* Get number of references for this node. */
> >>>> + inline unsigned get_references_count (void)
> >>>> + {
> >>>> + return ref_list.references ? ref_list.references->length () : 0;
> >>>> + }
> >>>
> >>>
> >>> Probably better called num_references() (like we have num_edge in
> >>> basic-block.h)
> >>>>
> >>>> @@ -8068,6 +8069,19 @@ it may significantly increase code size
> >>>> (see @option{--param ipcp-unit-growth=@var{value}}).
> >>>> This flag is enabled by default at @option{-O3}.
> >>>>
> >>>> +@item -fipa-icf
> >>>> +@opindex fipa-icf
> >>>> +Perform Identical Code Folding for functions and read-only variables.
> >>>> +The optimization reduces code size and may disturb unwind stacks by
> >>>> replacing
> >>>> +a function by equivalent one with a different name. The optimization
> >>>> works
> >>>> +more effectively with link time optimization enabled.
> >>>> +
> >>>> +Nevertheless the behavior is similar to Gold Linker ICF optimization,
> >>>> GCC ICF
> >>>> +works on different levels and thus the optimizations are not same -
> >>>> there are
> >>>> +equivalences that are found only by GCC and equivalences found only by
> >>>> Gold.
> >>>> +
> >>>> +This flag is enabled by default at @option{-O2}.
> >>>
> >>> ... and -Os?
> >>>>
> >>>> + case ARRAY_REF:
> >>>> + case ARRAY_RANGE_REF:
> >>>> + {
> >>>> + x1 = TREE_OPERAND (t1, 0);
> >>>> + x2 = TREE_OPERAND (t2, 0);
> >>>> + y1 = TREE_OPERAND (t1, 1);
> >>>> + y2 = TREE_OPERAND (t2, 1);
> >>>> +
> >>>> + if (!compare_operand (array_ref_low_bound (t1),
> >>>> + array_ref_low_bound (t2)))
> >>>> + return return_false_with_msg ("");
> >>>> + if (!compare_operand (array_ref_element_size (t1),
> >>>> + array_ref_element_size (t2)))
> >>>> + return return_false_with_msg ("");
> >>>> + if (!compare_operand (x1, x2))
> >>>> + return return_false_with_msg ("");
> >>>> + return compare_operand (y1, y2);
> >>>> + }
> >>>
> >>>
> >>> No need for {...} if there are no local vars.
> >>>>
> >>>> +bool
> >>>> +func_checker::compare_function_decl (tree t1, tree t2)
> >>>> +{
> >>>> + bool ret = false;
> >>>> +
> >>>> + if (t1 == t2)
> >>>> + return true;
> >>>> +
> >>>> + symtab_node *n1 = symtab_node::get (t1);
> >>>> + symtab_node *n2 = symtab_node::get (t2);
> >>>> +
> >>>> + if (m_ignored_source_nodes != NULL && m_ignored_target_nodes != NULL)
> >>>> + {
> >>>> + ret = m_ignored_source_nodes->contains (n1)
> >>>> + && m_ignored_target_nodes->contains (n2);
> >>>> +
> >>>> + if (ret)
> >>>> + return true;
> >>>> + }
> >>>> +
> >>>> + /* If function decl is WEAKREF, we compare targets. */
> >>>> + cgraph_node *f1 = cgraph_node::get (t1);
> >>>> + cgraph_node *f2 = cgraph_node::get (t2);
> >>>> +
> >>>> + if(f1 && f2 && f1->weakref && f2->weakref)
> >>>> + ret = f1->alias_target == f2->alias_target;
> >>>> +
> >>>> + return ret;
> >>>
> >>>
> >>> Comparing aliases is bit more complicated than just handling weakrefs. I
> >>> have
> >>> patch for symtab_node::equivalent_address_p somewhre in queue. lets just
> >>> drop
> >>> the fancy stuff for the moment and compare f1&&f2 for equivalence.
> >>>>
> >>>> + ret = compare_decl (t1, t2);
> >>>
> >>>
> >>> Why functions are not compared with compare_decl while variables are?
> >>>>
> >>>> +
> >>>> + return return_with_debug (ret);
> >>>> +}
> >>>> +
> >>>> +void
> >>>> +func_checker::parse_labels (sem_bb *bb)
> >>>> +{
> >>>> + for (gimple_stmt_iterator gsi = gsi_start_bb (bb->bb); !gsi_end_p
> >>>> (gsi);
> >>>> + gsi_next (&gsi))
> >>>> + {
> >>>> + gimple stmt = gsi_stmt (gsi);
> >>>> +
> >>>> + if (gimple_code (stmt) == GIMPLE_LABEL)
> >>>> + {
> >>>> + tree t = gimple_label_label (stmt);
> >>>> + gcc_assert (TREE_CODE (t) == LABEL_DECL);
> >>>> +
> >>>> + m_label_bb_map.put (t, bb->bb->index);
> >>>> + }
> >>>> + }
> >>>> +}
> >>>> +
> >>>> +/* Basic block equivalence comparison function that returns true if
> >>>> + basic blocks BB1 and BB2 (from functions FUNC1 and FUNC2) correspond.
> >>>> +
> >>>> + In general, a collection of equivalence dictionaries is built for
> >>>> types
> >>>> + like SSA names, declarations (VAR_DECL, PARM_DECL, ..). This
> >>>> infrastructure
> >>>> + is utilized by every statement-by-stament comparison function. */
> >>>> +
> >>>> +bool
> >>>> +func_checker::compare_bb (sem_bb *bb1, sem_bb *bb2)
> >>>> +{
> >>>> + unsigned i;
> >>>> + gimple_stmt_iterator gsi1, gsi2;
> >>>> + gimple s1, s2;
> >>>> +
> >>>> + if (bb1->nondbg_stmt_count != bb2->nondbg_stmt_count
> >>>> + || bb1->edge_count != bb2->edge_count)
> >>>> + return return_false ();
> >>>> +
> >>>> + gsi1 = gsi_start_bb (bb1->bb);
> >>>> + gsi2 = gsi_start_bb (bb2->bb);
> >>>> +
> >>>> + for (i = 0; i < bb1->nondbg_stmt_count; i++)
> >>>> + {
> >>>> + if (is_gimple_debug (gsi_stmt (gsi1)))
> >>>> + gsi_next_nondebug (&gsi1);
> >>>> +
> >>>> + if (is_gimple_debug (gsi_stmt (gsi2)))
> >>>> + gsi_next_nondebug (&gsi2);
> >>>> +
> >>>> + s1 = gsi_stmt (gsi1);
> >>>> + s2 = gsi_stmt (gsi2);
> >>>> +
> >>>> + int eh1 = lookup_stmt_eh_lp_fn
> >>>> + (DECL_STRUCT_FUNCTION (m_source_func_decl), s1);
> >>>> + int eh2 = lookup_stmt_eh_lp_fn
> >>>> + (DECL_STRUCT_FUNCTION (m_target_func_decl), s2);
> >>>> +
> >>>> + if (eh1 != eh2)
> >>>> + return return_false_with_msg ("EH regions are different");
> >>>> +
> >>>> + if (gimple_code (s1) != gimple_code (s2))
> >>>> + return return_false_with_msg ("gimple codes are different");
> >>>> +
> >>>> + switch (gimple_code (s1))
> >>>> + {
> >>>> + case GIMPLE_CALL:
> >>>> + if (!compare_gimple_call (s1, s2))
> >>>> + return return_different_stmts (s1, s2, "GIMPLE_CALL");
> >>>> + break;
> >>>> + case GIMPLE_ASSIGN:
> >>>> + if (!compare_gimple_assign (s1, s2))
> >>>> + return return_different_stmts (s1, s2, "GIMPLE_ASSIGN");
> >>>> + break;
> >>>> + case GIMPLE_COND:
> >>>> + if (!compare_gimple_cond (s1, s2))
> >>>> + return return_different_stmts (s1, s2, "GIMPLE_COND");
> >>>> + break;
> >>>> + case GIMPLE_SWITCH:
> >>>> + if (!compare_gimple_switch (s1, s2))
> >>>> + return return_different_stmts (s1, s2, "GIMPLE_SWITCH");
> >>>> + break;
> >>>> + case GIMPLE_DEBUG:
> >>>> + case GIMPLE_EH_DISPATCH:
> >>>> + break;
> >>>> + case GIMPLE_RESX:
> >>>> + if (!compare_gimple_resx (s1, s2))
> >>>> + return return_different_stmts (s1, s2, "GIMPLE_RESX");
> >>>> + break;
> >>>> + case GIMPLE_LABEL:
> >>>> + if (!compare_gimple_label (s1, s2))
> >>>> + return return_different_stmts (s1, s2, "GIMPLE_LABEL");
> >>>> + break;
> >>>> + case GIMPLE_RETURN:
> >>>> + if (!compare_gimple_return (s1, s2))
> >>>> + return return_different_stmts (s1, s2, "GIMPLE_RETURN");
> >>>> + break;
> >>>> + case GIMPLE_GOTO:
> >>>> + if (!compare_gimple_goto (s1, s2))
> >>>> + return return_different_stmts (s1, s2, "GIMPLE_GOTO");
> >>>> + break;
> >>>> + case GIMPLE_ASM:
> >>>> + if (!compare_gimple_asm (s1, s2))
> >>>> + return return_different_stmts (s1, s2, "GIMPLE_ASM");
> >>>> + break;
> >>>> + case GIMPLE_PREDICT:
> >>>> + case GIMPLE_NOP:
> >>>> + return true;
> >>>> + default:
> >>>> + return return_false_with_msg ("Unknown GIMPLE code reached");
> >>>> + }
> >>>> +
> >>>> + gsi_next (&gsi1);
> >>>> + gsi_next (&gsi2);
> >>>> + }
> >>>> +
> >>>> + return true;
> >>>> +}
> >>>> +
> >>>> +/* Verifies for given GIMPLEs S1 and S2 that
> >>>> + call statements are semantically equivalent. */
> >>>> +
> >>>> +bool
> >>>> +func_checker::compare_gimple_call (gimple s1, gimple s2)
> >>>> +{
> >>>> + unsigned i;
> >>>> + tree t1, t2;
> >>>> +
> >>>> + if (gimple_call_num_args (s1) != gimple_call_num_args (s2))
> >>>> + return false;
> >>>> +
> >>>> + t1 = gimple_call_fndecl (s1);
> >>>> + t2 = gimple_call_fndecl (s2);
> >>>> +
> >>>> + /* Function pointer variables are not supported yet. */
> >>>> + if (t1 == NULL || t2 == NULL)
> >>>> + {
> >>>> + if (!compare_operand (t1, t2))
> >>>> + return return_false();
> >>>
> >>>
> >>> I think the comment above is out of date. compare_operand should do the
> >>> right
> >>> job for indirect calls.
> >>>>
> >>>> +
> >>>> + if (cn1 && cn2 && cn1->weakref && cn2->weakref
> >>>> + && cn1->alias_target == cn2->alias_target)
> >>>> + return true;
> >>>
> >>>
> >>> Lets consistently drop the weakrefs handling and add full alias handling
> >>> incrementally.
> >>>>
> >>>> +
> >>>> + /* Checking function arguments. */
> >>>
> >>> attributes
> >>>>
> >>>> + tree decl1 = DECL_ATTRIBUTES (decl);
> >>>> + tree decl2 = DECL_ATTRIBUTES (m_compared_func->decl);
> >>>
> >>>
> >>> You can still do this as part of the wap_comparison, right?
> >>>>
> >>>> +
> >>>> + m_checker = new func_checker (decl, m_compared_func->decl,
> >>>> + compare_polymorphic_p (),
> >>>> + false,
> >>>> + &refs_set,
> >>>> + &m_compared_func->refs_set);
> >>>> + while (decl1)
> >>>> + {
> >>>> + if (decl2 == NULL)
> >>>> + return return_false ();
> >>>> +
> >>>> + if (get_attribute_name (decl1) != get_attribute_name (decl2))
> >>>> + return return_false ();
> >>>> +
> >>>> + tree attr_value1 = TREE_VALUE (decl1);
> >>>> + tree attr_value2 = TREE_VALUE (decl2);
> >>>> +
> >>>> + if (attr_value1 && attr_value2)
> >>>> + {
> >>>> + bool ret = m_checker->compare_operand (TREE_VALUE
> >>>> (attr_value1),
> >>>> + TREE_VALUE
> >>>> (attr_value2));
> >>>> + if (!ret)
> >>>> + return return_false_with_msg ("attribute values are
> >>>> different");
> >>>> + }
> >>>> + else if (!attr_value1 && !attr_value2)
> >>>> + {}
> >>>> + else
> >>>> + return return_false ();
> >>>> +
> >>>> + decl1 = TREE_CHAIN (decl1);
> >>>> + decl2 = TREE_CHAIN (decl2);
> >>>> + }
> >>>> +
> >>>> + if (decl1 != decl2)
> >>>> + return return_false();
> >>>> +
> >>>> +
> >>>> + for (arg1 = DECL_ARGUMENTS (decl),
> >>>> + arg2 = DECL_ARGUMENTS (m_compared_func->decl);
> >>>> + arg1; arg1 = DECL_CHAIN (arg1), arg2 = DECL_CHAIN (arg2))
> >>>> + if (!m_checker->compare_decl (arg1, arg2))
> >>>> + return return_false ();
> >>>> +
> >>>> + /* Fill-up label dictionary. */
> >>>> + for (unsigned i = 0; i < bb_sorted.length (); ++i)
> >>>> + {
> >>>> + m_checker->parse_labels (bb_sorted[i]);
> >>>> + m_checker->parse_labels (m_compared_func->bb_sorted[i]);
> >>>> + }
> >>>> +
> >>>> + /* Checking all basic blocks. */
> >>>> + for (unsigned i = 0; i < bb_sorted.length (); ++i)
> >>>> + if(!m_checker->compare_bb (bb_sorted[i],
> >>>> m_compared_func->bb_sorted[i]))
> >>>> + return return_false();
> >>>> +
> >>>> + dump_message ("All BBs are equal\n");
> >>>> +
> >>>> + /* Basic block edges check. */
> >>>> + for (unsigned i = 0; i < bb_sorted.length (); ++i)
> >>>> + {
> >>>> + bb_dict = XNEWVEC (int, bb_sorted.length () + 2);
> >>>> + memset (bb_dict, -1, (bb_sorted.length () + 2) * sizeof (int));
> >>>> +
> >>>> + bb1 = bb_sorted[i]->bb;
> >>>> + bb2 = m_compared_func->bb_sorted[i]->bb;
> >>>> +
> >>>> + ei2 = ei_start (bb2->preds);
> >>>> +
> >>>> + for (ei1 = ei_start (bb1->preds); ei_cond (ei1, &e1); ei_next
> >>>> (&ei1))
> >>>> + {
> >>>> + ei_cond (ei2, &e2);
> >>>> +
> >>>> + if (e1->flags != e2->flags)
> >>>> + return return_false_with_msg ("flags comparison returns
> >>>> false");
> >>>> +
> >>>> + if (!bb_dict_test (bb_dict, e1->src->index, e2->src->index))
> >>>> + return return_false_with_msg ("edge comparison returns
> >>>> false");
> >>>> +
> >>>> + if (!bb_dict_test (bb_dict, e1->dest->index, e2->dest->index))
> >>>> + return return_false_with_msg ("BB comparison returns false");
> >>>> +
> >>>> + if (!m_checker->compare_edge (e1, e2))
> >>>> + return return_false_with_msg ("edge comparison returns
> >>>> false");
> >>>> +
> >>>> + ei_next (&ei2);
> >>>> + }
> >>>> + }
> >>>> +
> >>>> + /* Basic block PHI nodes comparison. */
> >>>> + for (unsigned i = 0; i < bb_sorted.length (); i++)
> >>>> + if (!compare_phi_node (bb_sorted[i]->bb,
> >>>> m_compared_func->bb_sorted[i]->bb))
> >>>> + return return_false_with_msg ("PHI node comparison returns
> >>>> false");
> >>>> +
> >>>> + return result;
> >>>> +}
> >>>
> >>>
> >>> The rest of patch seems fine. I think we went across enough of
> >>> iteraitons, the patch is OK
> >>> with changes above and lets handle rest incrementally.
> >>>
> >>> Thanks!
> >>> Honza
> >>>
> >>
> >> Hello
> >>
> >> There's final version of the patch I'm going to commit tomorrow in the
> >> morning (CEST).
> >> Thank you Honza for the review.
> >>
> >> Martin
