Re: [PATCH] Move the -Wnonnull compare against NULL warning from FEs to early uninit under -Wnonnull-compare (PR c/69835)

[Richard Biener <rguenther at suse dot de>]

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  <jakub@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 <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)