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

Reply via email to