Avoid duplicate -Wnonnull warnings (PR c++/28656)

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?

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.


	Jakub

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

On Fri, Jul 20, 2012 at 10:37:32AM +0200, Richard Guenther wrote:
> On Thu, Jul 19, 2012 at 6:53 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> Hum.  How hard would it be to merge the attributes?

IMHO hard and ugly.  The thing is that you probably can do some hacks
easily in handle_nonnull_attribute, so that multiple nonnull attributes
on the same prototype get merged together (at the end of function when
returning without *no_add_attrs = true; before it, do
a = lookup_attribute ("nonnull", TYPE_ATTRIBUTES (type));
if (a != NULL)
  {
    merge stuff into a;
    *no_add_attrs = true;
  }
), but that handles just one of the cases, where multiple nonnull attributes
appear on the same prototype.  But you can (and with builtins.def usually
do) have also
void foo (void *, void *, void *) __attribute__((nonnull (1)));
void foo (void *, void *, void *) __attribute__((nonnull (2)));
and for that case no attribute hook is called, so either merge_attributes
would need to special case this attribute (which would be a layering
violation, as nonnull is just C/C++ attribute), or each FE would need in its
merge_decls and similar call lookup_attribute ("nonnull", TYPE_ATTRIBUTES (...));
twice and do the merging manually.  As there are just two users of the
nonnull attribute, handling all of them there looked much shorter and easier
to me.

> > 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.

	Jakub

On Fri, Jul 20, 2012 at 11:04 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Fri, Jul 20, 2012 at 10:37:32AM +0200, Richard Guenther wrote:
>> On Thu, Jul 19, 2012 at 6:53 PM, Jakub Jelinek <jakub@redhat.com> wrote:
>> Hum.  How hard would it be to merge the attributes?
>
> IMHO hard and ugly.  The thing is that you probably can do some hacks
> easily in handle_nonnull_attribute, so that multiple nonnull attributes
> on the same prototype get merged together (at the end of function when
> returning without *no_add_attrs = true; before it, do
> a = lookup_attribute ("nonnull", TYPE_ATTRIBUTES (type));
> if (a != NULL)
>   {
>     merge stuff into a;
>     *no_add_attrs = true;
>   }
> ), but that handles just one of the cases, where multiple nonnull attributes
> appear on the same prototype.  But you can (and with builtins.def usually
> do) have also
> void foo (void *, void *, void *) __attribute__((nonnull (1)));
> void foo (void *, void *, void *) __attribute__((nonnull (2)));
> and for that case no attribute hook is called, so either merge_attributes
> would need to special case this attribute (which would be a layering
> violation, as nonnull is just C/C++ attribute), or each FE would need in its
> merge_decls and similar call lookup_attribute ("nonnull", TYPE_ATTRIBUTES (...));
> twice and do the merging manually.  As there are just two users of the
> nonnull attribute, handling all of them there looked much shorter and easier
> to me.

In the end it asks for a rewrite of the attribute representation and
handling code ...
(a unified attributes.def file and a non-string-based
non-tree-list-based storage,
etc. ...).

Your patch is ok meanwhile.

Thanks,
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.
>
>         Jakub