On Thu, Jul 19, 2012 at 6:53 PM, Jakub Jelinek <ja...@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 <ja...@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
