Re: [PATCH] Avoid duplicate -Wnonnull warnings (PR c++/28656)

[Richard Guenther <richard dot guenther at gmail dot com>]

On Thu, Jul 19, 2012 at 6:53 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> Hi!
>
> On the following testcase we emit various (correct) -Wnonnull warnings
> more than once, sometimes many times.  The problem on the reported memcpy
> testcase is that glibc uses __attribute__((nonnull (1, 2))) and gcc
> uses __attribute__((nonnull)) on the memset builtin and we end up with both of
> the attributes (as they have different parameters and thus aren't merged).
> The check_function_nonnull then for each nonnull attribute went through all
> the arguments and warned if the argument matched the current nonnull
> attribute (so, for the combination of glibc and gcc provided nonnull
> argument 1 and argument 2 each matched twice, once the list variant, once
> the non-argument variant).  The following patch fixes it by first looking
> if there is any nonnull attribute without argument, then it warns about all
> pointer arguments, or otherwise for each argument walks the list of
> nonnull attributes and if any of them matches, warns.
>
> tree-vrp.c apparently handled just the first nonnull attribute and not more
> than one of them.  That seems to be all users of that attribute in GCC.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

Hum.  How hard would it be to merge the attributes?

Richard.

> 2012-07-19  Jakub Jelinek  <jakub@redhat.com>
>
>         PR c++/28656
>         * tree-vrp.c (nonnull_arg_p): Handle all nonnull attributes instead
>         of just the first one.
>
>         * c-common.c (check_function_nonnull): Handle multiple nonnull
>         attributes properly.
>
>         * c-c++-common/pr28656.c: New test.
>
> --- gcc/tree-vrp.c.jj   2012-07-16 14:38:13.000000000 +0200
> +++ gcc/tree-vrp.c      2012-07-19 14:24:27.277354132 +0200
> @@ -353,32 +353,35 @@ nonnull_arg_p (const_tree arg)
>      return true;
>
>    fntype = TREE_TYPE (current_function_decl);
> -  attrs = lookup_attribute ("nonnull", TYPE_ATTRIBUTES (fntype));
> -
> -  /* If "nonnull" wasn't specified, we know nothing about the argument.  */
> -  if (attrs == NULL_TREE)
> -    return false;
> -
> -  /* If "nonnull" applies to all the arguments, then ARG is non-null.  */
> -  if (TREE_VALUE (attrs) == NULL_TREE)
> -    return true;
> -
> -  /* Get the position number for ARG in the function signature.  */
> -  for (arg_num = 1, t = DECL_ARGUMENTS (current_function_decl);
> -       t;
> -       t = DECL_CHAIN (t), arg_num++)
> +  for (attrs = TYPE_ATTRIBUTES (fntype); attrs; attrs = TREE_CHAIN (attrs))
>      {
> -      if (t == arg)
> -       break;
> -    }
> +      attrs = lookup_attribute ("nonnull", attrs);
>
> -  gcc_assert (t == arg);
> +      /* If "nonnull" wasn't specified, we know nothing about the argument.  */
> +      if (attrs == NULL_TREE)
> +       return false;
>
> -  /* Now see if ARG_NUM is mentioned in the nonnull list.  */
> -  for (t = TREE_VALUE (attrs); t; t = TREE_CHAIN (t))
> -    {
> -      if (compare_tree_int (TREE_VALUE (t), arg_num) == 0)
> +      /* If "nonnull" applies to all the arguments, then ARG is non-null.  */
> +      if (TREE_VALUE (attrs) == NULL_TREE)
>         return true;
> +
> +      /* Get the position number for ARG in the function signature.  */
> +      for (arg_num = 1, t = DECL_ARGUMENTS (current_function_decl);
> +          t;
> +          t = DECL_CHAIN (t), arg_num++)
> +       {
> +         if (t == arg)
> +           break;
> +       }
> +
> +      gcc_assert (t == arg);
> +
> +      /* Now see if ARG_NUM is mentioned in the nonnull list.  */
> +      for (t = TREE_VALUE (attrs); t; t = TREE_CHAIN (t))
> +       {
> +         if (compare_tree_int (TREE_VALUE (t), arg_num) == 0)
> +           return true;
> +       }
>      }
>
>    return false;
> --- gcc/c-family/c-common.c.jj  2012-07-18 12:02:11.000000000 +0200
> +++ gcc/c-family/c-common.c     2012-07-19 14:32:05.915905501 +0200
> @@ -8194,26 +8194,42 @@ handle_nonnull_attribute (tree *node, tr
>  static void
>  check_function_nonnull (tree attrs, int nargs, tree *argarray)
>  {
> -  tree a, args;
> +  tree a;
>    int i;
>
> -  for (a = attrs; a; a = TREE_CHAIN (a))
> +  attrs = lookup_attribute ("nonnull", attrs);
> +  if (attrs == NULL_TREE)
> +    return;
> +
> +  a = attrs;
> +  /* See if any of the nonnull attributes has no arguments.  If so,
> +     then every pointer argument is checked (in which case the check
> +     for pointer type is done in check_nonnull_arg).  */
> +  if (TREE_VALUE (a) != NULL_TREE)
> +    do
> +      a = lookup_attribute ("nonnull", TREE_CHAIN (a));
> +    while (a != NULL_TREE && TREE_VALUE (a) != NULL_TREE);
> +
> +  if (a != NULL_TREE)
> +    for (i = 0; i < nargs; i++)
> +      check_function_arguments_recurse (check_nonnull_arg, NULL, argarray[i],
> +                                       i + 1);
> +  else
>      {
> -      if (is_attribute_p ("nonnull", TREE_PURPOSE (a)))
> +      /* Walk the argument list.  If we encounter an argument number we
> +        should check for non-null, do it.  */
> +      for (i = 0; i < nargs; i++)
>         {
> -         args = TREE_VALUE (a);
> -
> -         /* Walk the argument list.  If we encounter an argument number we
> -            should check for non-null, do it.  If the attribute has no args,
> -            then every pointer argument is checked (in which case the check
> -            for pointer type is done in check_nonnull_arg).  */
> -         for (i = 0; i < nargs; i++)
> +         for (a = attrs; ; a = TREE_CHAIN (a))
>             {
> -             if (!args || nonnull_check_p (args, i + 1))
> -               check_function_arguments_recurse (check_nonnull_arg, NULL,
> -                                                 argarray[i],
> -                                                 i + 1);
> +             a = lookup_attribute ("nonnull", a);
> +             if (a == NULL_TREE || nonnull_check_p (TREE_VALUE (a), i + 1))
> +               break;
>             }
> +
> +         if (a != NULL_TREE)
> +           check_function_arguments_recurse (check_nonnull_arg, NULL,
> +                                             argarray[i], i + 1);
>         }
>      }
>  }
> --- gcc/testsuite/c-c++-common/pr28656.c.jj     2012-07-19 15:05:56.790975802 +0200
> +++ gcc/testsuite/c-c++-common/pr28656.c        2012-07-19 15:19:55.098486448 +0200
> @@ -0,0 +1,29 @@
> +/* PR c++/28656 */
> +/* { dg-do compile } */
> +/* { dg-options "-Wnonnull" } */
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +extern void *memcpy (void *__restrict, const void *__restrict, __SIZE_TYPE__)
> +  __attribute__((nonnull (1), nonnull (2), nonnull (1, 2), nonnull));
> +#ifdef __cplusplus
> +}
> +#endif
> +
> +extern void bar (void *p1, void *p2, void *p3, void *p4, void *p5)
> +  __attribute__((nonnull (1), nonnull (1, 3), nonnull (3, 5), nonnull (4)));
> +
> +void
> +foo (void)
> +{
> +  memcpy (0, 0, 0);
> +  bar (0, 0, 0, 0, 0);
> +}
> +
> +/* { dg-warning "null argument where non-null required\[^\n\r\]*argument 1" "" { target *-*-* } 20 } */
> +/* { dg-warning "null argument where non-null required\[^\n\r\]*argument 2" "" { target *-*-* } 20 } */
> +/* { dg-warning "null argument where non-null required\[^\n\r\]*argument 1" "" { target *-*-* } 21 } */
> +/* { dg-warning "null argument where non-null required\[^\n\r\]*argument 3" "" { target *-*-* } 21 } */
> +/* { dg-warning "null argument where non-null required\[^\n\r\]*argument 4" "" { target *-*-* } 21 } */
> +/* { dg-warning "null argument where non-null required\[^\n\r\]*argument 5" "" { target *-*-* } 21 } */
>
>         Jakub