Re: Suspicious code in fold-const.c

[Andrew MacLeod <amacleod at redhat dot com>]

On 01/22/2016 06:03 AM, Richard Biener wrote:
> On Fri, Jan 22, 2016 at 12:06 AM, Andrew MacLeod <amacleod@redhat.com> wrote:
> > I was trying the ttype prototype for static type checking on fold-const.c to
> > see how long it would take me to convert such a large file, and it choked on
> > this snippet of code in fold_unary_loc, around lines 7690-7711:
> >
> > suspect code tagged with (*)
> >
> >           if ((CONVERT_EXPR_CODE_P (code)
> >                 || code == NON_LVALUE_EXPR)
> >                && TREE_CODE (tem) == COND_EXPR
> >                && TREE_CODE (TREE_OPERAND (tem, 1)) == code
> >                && TREE_CODE (TREE_OPERAND (tem, 2)) == code
> >    (*)          && ! VOID_TYPE_P (TREE_OPERAND (tem, 1))
> >    (*)          && ! VOID_TYPE_P (TREE_OPERAND (tem, 2))
> >                && (TREE_TYPE (TREE_OPERAND (TREE_OPERAND (tem, 1), 0))
> >                    == TREE_TYPE (TREE_OPERAND (TREE_OPERAND (tem, 2), 0)))
> >                && (! (INTEGRAL_TYPE_P (TREE_TYPE (tem))
> >                       && (INTEGRAL_TYPE_P
> >                           (TREE_TYPE (TREE_OPERAND (TREE_OPERAND (tem, 1),
> > 0))))
> >                       && TYPE_PRECISION (TREE_TYPE (tem)) <= BITS_PER_WORD)
> >                    || flag_syntax_only))
> >              tem = build1_loc (loc, code, type,
> >                                build3 (COND_EXPR,
> >                                        TREE_TYPE (TREE_OPERAND
> >                                                   (TREE_OPERAND (tem, 1),
> > 0)),
> >                                        TREE_OPERAND (tem, 0),
> >                                        TREE_OPERAND (TREE_OPERAND (tem, 1),
> > 0),
> >                                        TREE_OPERAND (TREE_OPERAND (tem, 2),
> >                                                      0)));
> >
> > and with:
> > #define VOID_TYPE_P(NODE)  (TREE_CODE (NODE) == VOID_TYPE)
> >
> > I don't think this is what was intended. it would expand into:
> >
> >                && TREE_CODE (TREE_OPERAND (tem, 1)) == code
> >                && TREE_CODE (TREE_OPERAND (tem, 2)) == code
> >                 && ! (TREE_CODE (TREE_OPERAND (tem, 1)) == VOID_TYPE)
> >                 && ! (TREE_CODE (TREE_OPERAND (tem, 2)) == VOID_TYPE)
> >
> > the latter two would be obviously true if the first 2 were true.
> >
> > My guess is this is probably suppose to be
> > && ! VOID_TYPE_P (TREE_TYPE (TREE_OPERAND (tem, 1)))
> >   && ! VOID_TYPE_P (TREE_TYPE (TREE_OPERAND (tem, 2)))
> >
> > but I'm not sure.   Any guesses whats intended here?
> Not sure, it might be to detect some of the x ? : throw () constructs
> but not sure how those would survive the previous == code check.
> Maybe a ? (void) ... : (void) ... is supposed to be detected.
>
> The type check below would catch that as well
> (huh?  a flag_syntax_only check in fold-const.c!?)
>
> I'd say change to ! VOID_TYPE_P (TREE_TYPE (TREE_OPERAND (tem, 1))
> to match what the VOID_TYPE_P check does above this block.
Thats what im going with for now anyway :-)

A second one has shown up which is less obvious.   Are we in the habit 
of using  void_type_node as expressions? I would have guessed not.   I'm 
going to guess this is another case of forgetting TREE_TYPE(x)

tree_invalid_nonnegative_warnv_p has the following snippet:

  enum tree_code code = TREE_CODE (t);
  if (TYPE_UNSIGNED (TREE_TYPE (t)))
    return true;

  switch (code)
    {
    case TARGET_EXPR:
      {
        tree temp = TARGET_EXPR_SLOT (t);
        t = TARGET_EXPR_INITIAL (t);

        /* If the initializer is non-void, then it's a normal expression
           that will be assigned to the slot.  */
(*)      if (!VOID_TYPE_P (t))
 (*)         return RECURSE (t);

        /* Otherwise, the initializer sets the slot in some way. One common
           way is an assignment statement at the end of the 
initializer.  */
        while (1)
          {
            if (TREE_CODE (t) == BIND_EXPR)
              t = expr_last (BIND_EXPR_BODY (t));
            else if (TREE_CODE (t) == TRY_FINALLY_EXPR


The RECURSE macro calls tree_expr_nonnegative_warnv_p(), which performs 
a switch() on the class of TREE_CODE, and handles cases: tcc_binary:  
tcc_comparison: tcc_unary: tcc_constant: tcc_declaration: 
tcc_reference:   so nothing for tcc_type.

then performs a switch on the TREE_CODE and handles a bunch of EXPR's.. 
falls into the default case which calls back into 
tree_invalid_nonnegative_warnv_p where it ends up doing nothing for a 
type node.

Which means this ends up also executing code to no purpose.

I suspect this should also be
if (!VOID_TYPE_P(TREE_TYPE(t))

What do you think?

Andrew