missing COND_EXPR case in walk_stmt_load_store_addr_ops

All,

walk_stmt_load_store_ops does not handle the case of COND_EXPR on the
RHS of a GIMPLE assign statement, i.e., in 'selection assignments' of
the form

  v = [cond_expr] a RELOP b : x ? y;

This may result in an ICE at SSA verification time if/when the
ADDR_EXPRs inside the selection expression are not taken into account.
 There is a corner case in libiberty (plus-demangle-template() in
cplus-dem.c) where the only occurrence of an ADDR_TAKEN on variable
'pattern' ends up in a selection expression.  A call to
'execute_update_addresses_taken()' will miss it, resulting in an
"address taken, but ADDRESSABLE bit not set" error at SSA verification
time.  This normally goes unnoticed because there are no calls to
'execute_update_addresses_taken()' after SRA and the COND_EXPRs appear
on the RHS much later, during LIM.  However, the bug will be triggered
if/when execute_fold_all_builtins  (or a custom pass) alters data
addressing and sets TODO_update_address_taken.  In my case it was
uncovered by a custom pass.

The root cause is in the logic of 'walk_stmt_load_store_ops' which
lacks the COND_EXPR case for simple assigns.  A proposed patch is
below, bootstrapped and tested (C & C++) on x86_64-unknown-linux-gnu.

Comments welcome!

    Zbigniew

On Mon, Aug 16, 2010 at 1:57 PM, Zbigniew Chamski
<zbigniew.chamski@gmail.com> wrote:
> All,
>
> walk_stmt_load_store_ops does not handle the case of COND_EXPR on the
> RHS of a GIMPLE assign statement, i.e., in 'selection assignments' of
> the form
>
>  v = [cond_expr] a RELOP b : x ? y;
>
> This may result in an ICE at SSA verification time if/when the
> ADDR_EXPRs inside the selection expression are not taken into account.
>  There is a corner case in libiberty (plus-demangle-template() in
> cplus-dem.c) where the only occurrence of an ADDR_TAKEN on variable
> 'pattern' ends up in a selection expression.  A call to
> 'execute_update_addresses_taken()' will miss it, resulting in an
> "address taken, but ADDRESSABLE bit not set" error at SSA verification
> time.  This normally goes unnoticed because there are no calls to
> 'execute_update_addresses_taken()' after SRA and the COND_EXPRs appear
> on the RHS much later, during LIM.  However, the bug will be triggered
> if/when execute_fold_all_builtins  (or a custom pass) alters data
> addressing and sets TODO_update_address_taken.  In my case it was
> uncovered by a custom pass.
>
> The root cause is in the logic of 'walk_stmt_load_store_ops' which
> lacks the COND_EXPR case for simple assigns.  A proposed patch is
> below, bootstrapped and tested (C & C++) on x86_64-unknown-linux-gnu.
>
> Comments welcome!

The patch is ok for trunk and the 4.5 branch (do you have a testcase?).

Thanks,
Richard.

>    Zbigniew
>
> Index: gcc/gimple.c
> ===================================================================
> --- gcc/gimple.c        (revision 163224)
> +++ gcc/gimple.c        (working copy)
> @@ -4746,6 +4746,45 @@
>   return NULL_TREE;
>  }
>
> +
> +/* For an assignment STMT which contains a selection expression on the RHS,
> +   i.e.,
> +
> +     lhs = [cond_expr] a RELOP b ? x : y;
> +
> +   search the entire selection expression for occurrences of ADDR_EXPR, and
> +   if found, apply VISIT_ADDR callback on STMT, the corresponding ADDR_EXPR
> +   operand and DATA.  Return TRUE if any of the callback invocations
> +   returned TRUE.  */
> +
> +static inline bool
> +stmt_visit_select_addr_ops (gimple stmt, tree select_expr, void *data,
> +                           bool (*visit_addr)(gimple, tree, void *))
> +{
> +  bool ret = false;
> +  tree condop1, condop2;
> +  tree trueval, falseval;
> +
> +  condop1 = TREE_OPERAND (TREE_OPERAND (select_expr, 0), 0);
> +  if (TREE_CODE (condop1) == ADDR_EXPR)
> +    ret |= visit_addr (stmt, condop1, data);
> +
> +  condop2 = TREE_OPERAND (TREE_OPERAND (select_expr, 1), 0);
> +  if (TREE_CODE (condop2) == ADDR_EXPR)
> +    ret |= visit_addr (stmt, condop2, data);
> +
> +  trueval = TREE_OPERAND (select_expr, 1);
> +  if (TREE_CODE (trueval) == ADDR_EXPR)
> +    ret |= visit_addr (stmt, trueval, data);
> +
> +  falseval = TREE_OPERAND (select_expr, 2);
> +  if (TREE_CODE (falseval) == ADDR_EXPR)
> +    ret |= visit_addr (stmt, falseval, data);
> +
> +  return ret;
> +}
> +
> +
>  /* For the statement STMT call the callbacks VISIT_LOAD, VISIT_STORE and
>    VISIT_ADDR if non-NULL on loads, store and address-taken operands
>    passing the STMT, the base of the operand and DATA to it.  The base
> @@ -4761,6 +4800,7 @@
>  {
>   bool ret = false;
>   unsigned i;
> +
>   if (gimple_assign_single_p (stmt))
>     {
>       tree lhs, rhs;
> @@ -4785,6 +4825,9 @@
>                   && TREE_CODE (OBJ_TYPE_REF_OBJECT (rhs)) == ADDR_EXPR)
>            ret |= visit_addr (stmt, TREE_OPERAND (OBJ_TYPE_REF_OBJECT (rhs),
>                                                   0), data);
> +         else if (TREE_CODE (rhs) == COND_EXPR)
> +           ret |= stmt_visit_select_addr_ops (stmt, rhs, data, visit_addr);
> +
>           lhs = gimple_assign_lhs (stmt);
>          if (TREE_CODE (lhs) == TARGET_MEM_REF
>               && TMR_BASE (lhs) != NULL_TREE
>