On Tue, 16 Feb 2016, Jakub Jelinek wrote:
> Hi!
>
> As has been reported today on gcc@, the new -Wnonnull warning addition
> warns even about nonnull parameters that were changed (or could be changed)
> in the function. IMHO the nonnull attribute only talks about the value of
> the parameter upon entry to the function, if you assign something else
> to the argument afterwards and test that for NULL or non-NULL, it is like
> any other variable.
> But, we don't have the infrastructure to detect this in the FE, so this
> patch moves the warning soon after we go into SSA form.
> As -Wnonnull is a C/C++/ObjC/ObjC++ only warning, I've added a new
> -Wnonnull-compare switch for it too (and enable it for C/C++/ObjC/ObjC++
> from -Wall).
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
Not sure if it matters but wouldn't walking over function parameters
and their default def SSA names immediate uses be cheaper than
matching all conditions? Esp. as nonnull_arg_p walks over all
DECL_ARGUMENTS (up to the searched index) over and over again?
[we should simply set a flag on the PARM_DECL!]
Richard.
> 2016-02-16 Jakub Jelinek <ja...@redhat.com>
>
> PR c/69835
> * common.opt (Wnonnull-compare): New warning.
> * doc/invoke.texi (-Wnonnull): Remove text about comparison
> of arguments against NULL.
> (-Wnonnull-compare): Document.
> * tree-ssa-uninit.c (warn_uninitialized_vars): Add warn_nonnull_cmp
> argument, if true, warn about SSA_NAME (D) of nonnull_arg_p
> comparisons against NULL pointers.
> (pass_late_warn_uninitialized::execute): Adjust caller.
> (execute_early_warn_uninitialized): Likewise.
> (gate_early_warn_uninitialized): New function.
> (pass_early_warn_uninitialized::gate): Call it instead of
> gate_warn_uninitialized.
> c-family/
> * c.opt (Wnonnull-compare): Enable for -Wall.
> c/
> * c-typeck.c (build_binary_op): Revert 2015-09-09 change.
> cp/
> * typeck.c (cp_build_binary_op): Revert 2015-09-09 change.
> testsuite/
> * c-c++-common/nonnull-1.c: Use -Wnonnull-compare instead of
> -Wnonnull in dg-options.
> * c-c++-common/nonnull-2.c: New test.
>
> --- gcc/common.opt.jj 2016-01-27 19:47:35.000000000 +0100
> +++ gcc/common.opt 2016-02-16 11:52:42.641623690 +0100
> @@ -616,6 +616,10 @@ Wlarger-than=
> Common RejectNegative Joined UInteger Warning
> -Wlarger-than=<number> Warn if an object is larger than <number> bytes.
>
> +Wnonnull-compare
> +Var(warn_nonnull_compare) Warning
> +Warn if comparing pointer parameter with nonnull attribute with NULL.
> +
> Wnull-dereference
> Common Var(warn_null_dereference) Warning
> Warn if dereferencing a NULL pointer may lead to erroneous or undefined
> behavior.
> --- gcc/doc/invoke.texi.jj 2016-02-08 18:39:16.000000000 +0100
> +++ gcc/doc/invoke.texi 2016-02-16 12:31:42.037232875 +0100
> @@ -276,7 +276,8 @@ Objective-C and Objective-C++ Dialects}.
> -Wmain -Wmaybe-uninitialized -Wmemset-transposed-args @gol
> -Wmisleading-indentation -Wmissing-braces @gol
> -Wmissing-field-initializers -Wmissing-include-dirs @gol
> --Wno-multichar -Wnonnull
> -Wnormalized=@r{[}none@r{|}id@r{|}nfc@r{|}nfkc@r{]} @gol
> +-Wno-multichar -Wnonnull -Wnonnull-compare @gol
> +-Wnormalized=@r{[}none@r{|}id@r{|}nfc@r{|}nfkc@r{]} @gol
> -Wnull-dereference -Wodr -Wno-overflow -Wopenmp-simd @gol
> -Woverride-init-side-effects -Woverlength-strings @gol
> -Wpacked -Wpacked-bitfield-compat -Wpadded @gol
> @@ -3537,6 +3538,7 @@ Options} and @ref{Objective-C and Object
> -Wmissing-braces @r{(only for C/ObjC)} @gol
> -Wnarrowing @r{(only for C++)} @gol
> -Wnonnull @gol
> +-Wnonnull-compare @gol
> -Wopenmp-simd @gol
> -Wparentheses @gol
> -Wpointer-sign @gol
> @@ -3795,12 +3797,18 @@ formats that may yield only a two-digit
> Warn about passing a null pointer for arguments marked as
> requiring a non-null value by the @code{nonnull} function attribute.
>
> -Also warns when comparing an argument marked with the @code{nonnull}
> -function attribute against null inside the function.
> -
> @option{-Wnonnull} is included in @option{-Wall} and @option{-Wformat}. It
> can be disabled with the @option{-Wno-nonnull} option.
>
> +@item -Wnonnull-compare
> +@opindex Wnonnull-compare
> +@opindex Wno-nonnull-compare
> +Warn when comparing an argument marked with the @code{nonnull}
> +function attribute against null inside the function.
> +
> +@option{-Wnonnull-compare} is included in @option{-Wall}. It
> +can be disabled with the @option{-Wno-nonnull-compare} option.
> +
> @item -Wnull-dereference
> @opindex Wnull-dereference
> @opindex Wno-null-dereference
> --- gcc/tree-ssa-uninit.c.jj 2016-01-13 13:28:41.000000000 +0100
> +++ gcc/tree-ssa-uninit.c 2016-02-16 12:50:30.390619823 +0100
> @@ -171,7 +171,8 @@ warn_uninit (enum opt_code wc, tree t, t
> }
>
> static unsigned int
> -warn_uninitialized_vars (bool warn_possibly_uninitialized)
> +warn_uninitialized_vars (bool warn_possibly_uninitialized,
> + bool warn_nonnull_cmp)
> {
> gimple_stmt_iterator gsi;
> basic_block bb;
> @@ -190,6 +191,60 @@ warn_uninitialized_vars (bool warn_possi
> if (is_gimple_debug (stmt))
> continue;
>
> + if (warn_nonnull_cmp)
> + {
> + tree op0 = NULL_TREE, op1 = NULL_TREE;
> + location_t loc = gimple_location (stmt);
> + if (gimple_code (stmt) == GIMPLE_COND)
For new if (gimple_code () ...) code I'd prefer dyn_cast and gcond *
and gassign *, that should be marginally faster, esp. with checking.
> + switch (gimple_cond_code (stmt))
> + {
> + case EQ_EXPR:
> + case NE_EXPR:
> + op0 = gimple_cond_lhs (stmt);
> + op1 = gimple_cond_rhs (stmt);
> + break;
> + default:
> + break;
> + }
> + else if (is_gimple_assign (stmt))
> + switch (gimple_assign_rhs_code (stmt))
> + {
> + case EQ_EXPR:
> + case NE_EXPR:
> + op0 = gimple_assign_rhs1 (stmt);
> + op1 = gimple_assign_rhs2 (stmt);
> + break;
> + case COND_EXPR:
> + switch (TREE_CODE (gimple_assign_rhs1 (stmt)))
> + {
> + case EQ_EXPR:
> + case NE_EXPR:
> + op1 = gimple_assign_rhs1 (stmt);
> + loc = EXPR_LOC_OR_LOC (op1, loc);
> + op0 = TREE_OPERAND (op1, 0);
> + op1 = TREE_OPERAND (op1, 1);
> + break;
> + default:
> + break;
> + }
> + break;
> + default:
> + break;
> + }
> + if (op0
> + && TREE_CODE (op0) == SSA_NAME
> + && SSA_NAME_IS_DEFAULT_DEF (op0)
> + && TREE_CODE (SSA_NAME_VAR (op0)) == PARM_DECL
> + && (POINTER_TYPE_P (TREE_TYPE (op0))
> + || TREE_CODE (TREE_TYPE (op0)) == OFFSET_TYPE)
> + && nonnull_arg_p (SSA_NAME_VAR (op0))
> + && (POINTER_TYPE_P (TREE_TYPE (op0))
> + ? integer_zerop (op1) : integer_minus_onep (op1)))
Why do we have OFFSET_TYPE and why do we say "nonnull" for -1 offset?
Other than the walking issue the patch looks reasonable.
Thanks,
Richard.
> + warning_at (gimple_location (stmt), OPT_Wnonnull_compare,
> + "nonnull argument %qD compared to NULL",
> + SSA_NAME_VAR (op0));
> + }
> +
> /* We only do data flow with SSA_NAMEs, so that's all we
> can warn about. */
> FOR_EACH_SSA_USE_OPERAND (use_p, stmt, op_iter, SSA_OP_USE)
> @@ -2416,7 +2471,7 @@ pass_late_warn_uninitialized::execute (f
> /* Re-do the plain uninitialized variable check, as optimization may have
> straightened control flow. Do this first so that we don't accidentally
> get a "may be" warning when we'd have seen an "is" warning later. */
> - warn_uninitialized_vars (/*warn_possibly_uninitialized=*/1);
> + warn_uninitialized_vars (/*warn_possibly_uninitialized=*/true, false);
>
> timevar_push (TV_TREE_UNINIT);
>
> @@ -2478,6 +2533,14 @@ make_pass_late_warn_uninitialized (gcc::
> }
>
>
> +static bool
> +gate_early_warn_uninitialized (void)
> +{
> + return (warn_uninitialized
> + || warn_maybe_uninitialized
> + || warn_nonnull_compare);
> +}
> +
> static unsigned int
> execute_early_warn_uninitialized (void)
> {
> @@ -2488,7 +2551,8 @@ execute_early_warn_uninitialized (void)
> optimization we need to warn here about "may be uninitialized". */
> calculate_dominance_info (CDI_POST_DOMINATORS);
>
> - warn_uninitialized_vars (/*warn_possibly_uninitialized=*/!optimize);
> + warn_uninitialized_vars (/*warn_possibly_uninitialized=*/!optimize,
> + warn_nonnull_compare);
>
> /* Post-dominator information can not be reliably updated. Free it
> after the use. */
> @@ -2521,7 +2585,7 @@ public:
> {}
>
> /* opt_pass methods: */
> - virtual bool gate (function *) { return gate_warn_uninitialized (); }
> + virtual bool gate (function *) { return gate_early_warn_uninitialized (); }
> virtual unsigned int execute (function *)
> {
> return execute_early_warn_uninitialized ();
> --- gcc/c-family/c.opt.jj 2016-02-08 18:39:17.000000000 +0100
> +++ gcc/c-family/c.opt 2016-02-16 12:30:00.299641823 +0100
> @@ -677,6 +677,10 @@ Wnonnull
> C ObjC C++ ObjC++ LangEnabledBy(C ObjC C++ ObjC++,Wall)
> ;
>
> +Wnonnull-compare
> +C ObjC C++ ObjC++ LangEnabledBy(C ObjC C++ ObjC++,Wall)
> +;
> +
> Wnormalized
> C ObjC C++ ObjC++ Warning Alias(Wnormalized=,nfc,none)
> ;
> --- gcc/c/c-typeck.c.jj 2016-02-12 10:23:50.000000000 +0100
> +++ gcc/c/c-typeck.c 2016-02-16 11:40:08.474048701 +0100
> @@ -11086,11 +11086,6 @@ build_binary_op (location_t location, en
> short_compare = 1;
> else if (code0 == POINTER_TYPE && null_pointer_constant_p (orig_op1))
> {
> - if (warn_nonnull
> - && TREE_CODE (op0) == PARM_DECL && nonnull_arg_p (op0))
> - warning_at (location, OPT_Wnonnull,
> - "nonnull argument %qD compared to NULL", op0);
> -
> if (TREE_CODE (op0) == ADDR_EXPR
> && decl_with_nonnull_addr_p (TREE_OPERAND (op0, 0)))
> {
> @@ -11111,11 +11106,6 @@ build_binary_op (location_t location, en
> }
> else if (code1 == POINTER_TYPE && null_pointer_constant_p (orig_op0))
> {
> - if (warn_nonnull
> - && TREE_CODE (op1) == PARM_DECL && nonnull_arg_p (op1))
> - warning_at (location, OPT_Wnonnull,
> - "nonnull argument %qD compared to NULL", op1);
> -
> if (TREE_CODE (op1) == ADDR_EXPR
> && decl_with_nonnull_addr_p (TREE_OPERAND (op1, 0)))
> {
> --- gcc/cp/typeck.c.jj 2016-02-12 10:23:50.000000000 +0100
> +++ gcc/cp/typeck.c 2016-02-16 11:40:37.783643278 +0100
> @@ -4514,11 +4514,6 @@ cp_build_binary_op (location_t location,
> || (code0 == POINTER_TYPE
> && TYPE_PTR_P (type1) && integer_zerop (op1)))
> {
> - if (warn_nonnull
> - && TREE_CODE (op0) == PARM_DECL && nonnull_arg_p (op0))
> - warning_at (location, OPT_Wnonnull,
> - "nonnull argument %qD compared to NULL", op0);
> -
> if (TYPE_PTR_P (type1))
> result_type = composite_pointer_type (type0, type1, op0, op1,
> CPO_COMPARISON, complain);
> @@ -4558,11 +4553,6 @@ cp_build_binary_op (location_t location,
> || (code1 == POINTER_TYPE
> && TYPE_PTR_P (type0) && integer_zerop (op0)))
> {
> - if (warn_nonnull
> - && TREE_CODE (op1) == PARM_DECL && nonnull_arg_p (op1))
> - warning_at (location, OPT_Wnonnull,
> - "nonnull argument %qD compared to NULL", op1);
> -
> if (TYPE_PTR_P (type0))
> result_type = composite_pointer_type (type0, type1, op0, op1,
> CPO_COMPARISON, complain);
> --- gcc/testsuite/c-c++-common/nonnull-1.c.jj 2015-10-11 19:11:06.000000000
> +0200
> +++ gcc/testsuite/c-c++-common/nonnull-1.c 2016-02-16 12:38:53.917256278
> +0100
> @@ -1,7 +1,7 @@
> /* Test for the bad usage of "nonnull" function attribute parms. */
> /* */
> /* { dg-do compile } */
> -/* { dg-options "-Wnonnull" } */
> +/* { dg-options "-Wnonnull-compare" } */
>
> #include <stddef.h>
> #include <stdlib.h>
> --- gcc/testsuite/c-c++-common/nonnull-2.c.jj 2016-02-16 12:52:17.149142596
> +0100
> +++ gcc/testsuite/c-c++-common/nonnull-2.c 2016-02-16 12:56:29.904645198
> +0100
> @@ -0,0 +1,26 @@
> +/* Test for the bad usage of "nonnull" function attribute parms. */
> +/* { dg-do compile } */
> +/* { dg-options "-Wnonnull-compare" } */
> +
> +void bar (char **);
> +
> +__attribute__((nonnull (1, 3))) int
> +foo (char *cp1, char *cp2, char *cp3, char *cp4)
> +{
> + if (cp1 == (char *) 0) /* { dg-warning "nonnull argument" "cp1 compared to
> NULL" } */
> + return 1;
> +
> + cp1 = cp2;
> + if (cp1 == (char *) 0) /* { dg-bogus "nonnull argument" } */
> + return 2;
> +
> + if (!cp4) /* { dg-bogus "nonnull argument" } */
> + return 3;
> +
> + char **p = &cp3;
> + bar (p);
> + if (cp3 == (char *) 0) /* { dg-bogus "nonnull argument" } */
> + return 4;
> +
> + return 5;
> +}
>
> Jakub
>
>
--
Richard Biener <rguent...@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB
21284 (AG Nuernberg)
