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

[Jakub Jelinek <jakub at redhat dot com>]

On Tue, Feb 16, 2016 at 11:12:27AM -0700, Jeff Law wrote:
> >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!]
> Seems like a waste to burn a flag bit for something like this.
> 
> We're already walking all the statements in this code so it's really just
> the cost of testing if the existing statement is "interesting" for this
> warning.  I guess you're suggesting jakub walk the arguments and if one is a
> marked as non-null, then walk the immediate uses.  Yea, that's probably
> faster than doing the test for every statement.

I'm already bootstrapping/regtesting following variant.

> Swapping the nonnull_arg_p and the test for whether or not op1 is 0 would be
> a slight help as well, particularly if there was machine generated code with
> oodles of arguments.
> 
> I'm not sure why we're testing OFFSET_TYPE against -1.

That is because NULL pointers to member are represented as -1 in the middle
end.

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.
	* Makefile.in (OBJS): Add gimple-ssa-nonnull-compare.o.
	* tree-pass.h (make_pass_warn_nonnull_compare): Declare.
	* passes.def (pass_warn_nonnull_compare): Add.
	* gimple-ssa-nonnull-compare.c: New file.
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-02-16 16:14:40.787071893 +0100
+++ gcc/common.opt	2016-02-16 17:34:12.850648209 +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-16 16:14:41.692059415 +0100
+++ gcc/doc/invoke.texi	2016-02-16 17:26:55.346587296 +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/Makefile.in.jj	2016-02-11 20:28:54.000000000 +0100
+++ gcc/Makefile.in	2016-02-16 17:32:26.919086223 +0100
@@ -1278,6 +1278,7 @@ OBJS = \
 	gimple-pretty-print.o \
 	gimple-ssa-backprop.o \
 	gimple-ssa-isolate-paths.o \
+	gimple-ssa-nonnull-compare.o \
 	gimple-ssa-split-paths.o \
 	gimple-ssa-strength-reduction.o \
 	gimple-streamer-in.o \
--- gcc/tree-pass.h.jj	2016-01-19 13:31:07.000000000 +0100
+++ gcc/tree-pass.h	2016-02-16 17:28:39.155178102 +0100
@@ -472,6 +472,7 @@ extern gimple_opt_pass *make_pass_oacc_k
 extern simple_ipa_opt_pass *make_pass_ipa_oacc (gcc::context *ctxt);
 extern simple_ipa_opt_pass *make_pass_ipa_oacc_kernels (gcc::context *ctxt);
 extern gimple_opt_pass *make_pass_gen_hsail (gcc::context *ctxt);
+extern gimple_opt_pass *make_pass_warn_nonnull_compare (gcc::context *ctxt);
 
 /* IPA Passes */
 extern simple_ipa_opt_pass *make_pass_ipa_lower_emutls (gcc::context *ctxt);
--- gcc/passes.def.jj	2016-02-10 16:01:57.000000000 +0100
+++ gcc/passes.def	2016-02-16 17:27:46.515892676 +0100
@@ -56,6 +56,7 @@ along with GCC; see the file COPYING3.
       NEXT_PASS (pass_fixup_cfg);
       NEXT_PASS (pass_init_datastructures);
       NEXT_PASS (pass_build_ssa);
+      NEXT_PASS (pass_warn_nonnull_compare);
       NEXT_PASS (pass_ubsan);
       NEXT_PASS (pass_early_warn_uninitialized);
       NEXT_PASS (pass_nothrow);
--- gcc/gimple-ssa-nonnull-compare.c.jj	2016-02-16 16:58:10.676272364 +0100
+++ gcc/gimple-ssa-nonnull-compare.c	2016-02-16 17:46:19.788692474 +0100
@@ -0,0 +1,150 @@
+/* -Wnonnull-compare warning support.
+   Copyright (C) 2016 Free Software Foundation, Inc.
+
+This file is part of GCC.
+
+GCC is free software; you can redistribute it and/or modify
+it under the terms of the GNU General Public License as published by
+the Free Software Foundation; either version 3, or (at your option)
+any later version.
+
+GCC is distributed in the hope that it will be useful,
+but WITHOUT ANY WARRANTY; without even the implied warranty of
+MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+GNU General Public License for more details.
+
+You should have received a copy of the GNU General Public License
+along with GCC; see the file COPYING3.  If not see
+<http://www.gnu.org/licenses/>.  */
+
+#include "config.h"
+#include "system.h"
+#include "coretypes.h"
+#include "backend.h"
+#include "tree.h"
+#include "gimple.h"
+#include "tree-pass.h"
+#include "ssa.h"
+#include "diagnostic-core.h"
+#include "tree-dfa.h"
+
+/* Warn about comparison of nonnull_arg_p argument initial values
+   with NULL.  */
+
+static void
+do_warn_nonnull_compare (function *fun, tree arg)
+{
+  if (!POINTER_TYPE_P (TREE_TYPE (arg))
+      && TREE_CODE (TREE_TYPE (arg)) != OFFSET_TYPE)
+    return;
+
+  if (!nonnull_arg_p (arg))
+    return;
+
+  tree d = ssa_default_def (fun, arg);
+  if (d == NULL_TREE)
+    return;
+
+  use_operand_p use_p;
+  imm_use_iterator iter;
+
+  FOR_EACH_IMM_USE_FAST (use_p, iter, d)
+    {
+      gimple *stmt = USE_STMT (use_p);
+      tree op = NULL_TREE;
+      location_t loc = gimple_location (stmt);
+      if (gimple_code (stmt) == GIMPLE_COND)
+	switch (gimple_cond_code (stmt))
+	  {
+	  case EQ_EXPR:
+	  case NE_EXPR:
+	    if (gimple_cond_lhs (stmt) == d)
+	      op = 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:
+	    if (gimple_assign_rhs1 (stmt) == d)
+	      op = gimple_assign_rhs2 (stmt);
+	    break;
+	  case COND_EXPR:
+	    switch (TREE_CODE (gimple_assign_rhs1 (stmt)))
+	      {
+	      case EQ_EXPR:
+	      case NE_EXPR:
+		op = gimple_assign_rhs1 (stmt);
+		if (TREE_OPERAND (op, 0) != d)
+		  {
+		    op = NULL_TREE;
+		    break;
+		  }
+		loc = EXPR_LOC_OR_LOC (op, loc);
+		op = TREE_OPERAND (op, 1);
+		break;
+	      default:
+		break;
+	      }
+	    break;
+	  default:
+	    break;
+	  }
+      if (op
+	  && (POINTER_TYPE_P (TREE_TYPE (arg))
+	      ? integer_zerop (op) : integer_minus_onep (op)))
+	warning_at (loc, OPT_Wnonnull_compare,
+		    "nonnull argument %qD compared to NULL", arg);
+    }
+}
+
+namespace {
+
+const pass_data pass_data_warn_nonnull_compare =
+{
+  GIMPLE_PASS, /* type */
+  "*nonnullcmp", /* name */
+  OPTGROUP_NONE, /* optinfo_flags */
+  TV_NONE, /* tv_id */
+  PROP_ssa, /* properties_required */
+  0, /* properties_provided */
+  0, /* properties_destroyed */
+  0, /* todo_flags_start */
+  0, /* todo_flags_finish */
+};
+
+class pass_warn_nonnull_compare : public gimple_opt_pass
+{
+public:
+  pass_warn_nonnull_compare (gcc::context *ctxt)
+    : gimple_opt_pass (pass_data_warn_nonnull_compare, ctxt)
+  {}
+
+  /* opt_pass methods: */
+  virtual bool gate (function *) { return warn_nonnull_compare; }
+
+  virtual unsigned int execute (function *);
+
+}; // class pass_warn_nonnull_compare
+
+unsigned int
+pass_warn_nonnull_compare::execute (function *fun)
+{
+  if (fun->static_chain_decl)
+    do_warn_nonnull_compare (fun, fun->static_chain_decl);
+
+  for (tree arg = DECL_ARGUMENTS (cfun->decl); arg; arg = DECL_CHAIN (arg))
+    do_warn_nonnull_compare (fun, arg);
+  return 0;
+}
+
+} // anon namespace
+
+gimple_opt_pass *
+make_pass_warn_nonnull_compare (gcc::context *ctxt)
+{
+  return new pass_warn_nonnull_compare (ctxt);
+}
--- gcc/c-family/c.opt.jj	2016-02-16 16:14:40.895070404 +0100
+++ gcc/c-family/c.opt	2016-02-16 17:26:55.347587282 +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-16 16:14:40.636073975 +0100
+++ gcc/c/c-typeck.c	2016-02-16 17:26:55.350587242 +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-16 16:14:40.657073685 +0100
+++ gcc/cp/typeck.c	2016-02-16 17:26:55.352587215 +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	2016-02-16 16:14:40.310078470 +0100
+++ gcc/testsuite/c-c++-common/nonnull-1.c	2016-02-16 17:26:55.352587215 +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 17:26:55.353587201 +0100
+++ gcc/testsuite/c-c++-common/nonnull-2.c	2016-02-16 17:26:55.353587201 +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