not a type buglet in c/c-array-notation

Another potential buglet  I stumbled across whilst testing the tree-type 
work:
in c/c-array-notation.c::fix_builtin_array_notation_fn()
<...>
       if (list_size > 1)
         {
           new_yes_ind = build_modify_expr
             (location, *new_var, TREE_TYPE (*new_var), NOP_EXPR,
              location, an_loop_info[0].var, TREE_TYPE 
(an_loop_info[0].var));
           new_yes_expr = build_modify_expr
             (location, array_ind_value, TREE_TYPE (array_ind_value),
              NOP_EXPR,
              location, func_parm, TREE_TYPE ((*array_operand)[0]));
         }
       else
         {
           new_yes_ind = build_modify_expr
             (location, *new_var, TREE_TYPE (*new_var), NOP_EXPR,
              location, TREE_OPERAND (array_op0, 1),
              TREE_TYPE (TREE_OPERAND (array_op0, 1)));
           new_yes_expr = build_modify_expr
             (location, array_ind_value, TREE_TYPE (array_ind_value),
              NOP_EXPR,
              location, func_parm, TREE_OPERAND (array_op0, 1));       
<<<--------
         }

'build_modify_expr' expects a type in that final parameter position. It 
triggered as showing a non-type being passed into a type parameter.

I think the last operand on the last line ought to be wrapped in 
TREE_TYPE() just like it is in the first expression of the else.. Either 
that, or someone who understands the code needs to figure out whats 
really wanted there.   There is a second occurrence later in the same 
routine.   Patch number 1 makes this change and bootstraps/passes 
regression.

If someone that understands the code wants to have a look, The test can 
be triggered in the cilk testsuite by adding an assert to 
build_modify_expr  (thats patch number 2 for convenience)  and running 
testcase :
make RUNTESTFLAGS=cilk-plus.exp=sec_reduce_ind_same_value.c check-gcc

  I still have a couple from last fall that were discussed..I'm queuing 
these up for once stage 1 opens and we can discuss them again.

Andrew
Index: c/c-typeck.c
===================================================================
*** c/c-typeck.c	(revision 234568)
--- c/c-typeck.c	(working copy)
*************** build_modify_expr (location_t location,
*** 5542,5547 ****
--- 5542,5550 ----
    if (TREE_CODE (lhs) == ERROR_MARK || TREE_CODE (rhs) == ERROR_MARK)
      return error_mark_node;
  
+   gcc_assert (!rhs_origtype || TREE_CODE (rhs_origtype) == ERROR_MARK
+ 	      || TYPE_P (rhs_origtype));
+ 
    /* Ensure an error for assigning a non-lvalue array to an array in
       C90.  */
    if (TREE_CODE (lhstype) == ARRAY_TYPE)

On 03/31/2016 07:00 AM, Andrew MacLeod wrote:
> Another potential buglet  I stumbled across whilst testing the tree-type
> work:
> in c/c-array-notation.c::fix_builtin_array_notation_fn()
> <...>
>       if (list_size > 1)
>         {
>           new_yes_ind = build_modify_expr
>             (location, *new_var, TREE_TYPE (*new_var), NOP_EXPR,
>              location, an_loop_info[0].var, TREE_TYPE
> (an_loop_info[0].var));
>           new_yes_expr = build_modify_expr
>             (location, array_ind_value, TREE_TYPE (array_ind_value),
>              NOP_EXPR,
>              location, func_parm, TREE_TYPE ((*array_operand)[0]));
>         }
>       else
>         {
>           new_yes_ind = build_modify_expr
>             (location, *new_var, TREE_TYPE (*new_var), NOP_EXPR,
>              location, TREE_OPERAND (array_op0, 1),
>              TREE_TYPE (TREE_OPERAND (array_op0, 1)));
>           new_yes_expr = build_modify_expr
>             (location, array_ind_value, TREE_TYPE (array_ind_value),
>              NOP_EXPR,
>              location, func_parm, TREE_OPERAND (array_op0, 1));
> <<<--------
>         }
>
> 'build_modify_expr' expects a type in that final parameter position. It
> triggered as showing a non-type being passed into a type parameter.
>
> I think the last operand on the last line ought to be wrapped in
> TREE_TYPE() just like it is in the first expression of the else.. Either
> that, or someone who understands the code needs to figure out whats
> really wanted there.   There is a second occurrence later in the same
> routine.   Patch number 1 makes this change and bootstraps/passes
> regression.
>
> If someone that understands the code wants to have a look, The test can
> be triggered in the cilk testsuite by adding an assert to
> build_modify_expr  (thats patch number 2 for convenience)  and running
> testcase :
> make RUNTESTFLAGS=cilk-plus.exp=sec_reduce_ind_same_value.c check-gcc
>
>  I still have a couple from last fall that were discussed..I'm queuing
> these up for once stage 1 opens and we can discuss them again.
Looking through build_modify_expr and how it uses that last operand, 
it's unlikely that the goof in fix_builtin_array_notation_fn would cause 
problems.  In fact, I'm pretty sure it'll never be used when called from 
either of those sites.  A combination of factors come into play and we 
certainly can't guarantee those factors won't change over time, so we 
ought to go ahead and fix.

I'm going to cons up a ChangeLog and commit your patch.

jeff