This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

C/C++ PATCH to implement -Wpointer-compare warning (PR c++/64767)


Spurred by the recent <http://www.viva64.com/en/b/0425/> findings, I decided to
implement a warning that warns when a pointer is compared with a zero character
literal (constant), because this isn't likely to be intended.  So e.g.

  ptr == L'\0'

is probably wrong and should've been written as

  ptr[0] == L'\0'

Jonathan pointed out that this would actually be invalid C++11 since pointer
conversions are only allowed for integer literals, not char literals.
There's -Wzero-as-null-pointer-constant to catch other cases such as

  void *o = '\0';

This patch deals strictly with ==/!=.

Now, I'm not exactly sure how to name this, -Wpointer-compare seems pretty
general.  So maybe as a followup I could use this option for C's
"comparison between pointer and integer" (that's a pedwarn)?

Bootstrapped/regtested on x86_64-linux, ok for trunk?

2016-09-10  Marek Polacek  <polacek@redhat.com>

	PR c++/64767
	* c.opt (Wpointer-compare): New option.

	* c-parser.c (c_parser_postfix_expression): Mark zero character
	constants by setting original_type in c_expr.
	* c-typeck.c (parser_build_binary_op): Warn when a pointer is compared
	with a zero character constant.

	* cp-tree.h (class cp_expr): Add char_literal_p member.  Update the
	constructors and the copy constructor.
	* parser.c (cp_parser_primary_expression): Mark zero character
	literals.
	(cp_parser_binary_expression): Warn when a pointer is compared with a
	zero character literal.

	* doc/invoke.texi: Document -Wpointer-compare.

	* c-c++-common/Wpointer-compare-1.c: New test.

diff --git gcc/c-family/c.opt gcc/c-family/c.opt
index a5358ed..ac299b6 100644
--- gcc/c-family/c.opt
+++ gcc/c-family/c.opt
@@ -776,6 +776,10 @@ Wpointer-sign
 C ObjC Var(warn_pointer_sign) Warning LangEnabledBy(C ObjC,Wall || Wpedantic)
 Warn when a pointer differs in signedness in an assignment.
 
+Wpointer-compare
+C ObjC C++ ObjC++ Var(warn_pointer_compare) Init(1) Warning
+Warn when a pointer is compared with a zero character constant.
+
 Wpointer-to-int-cast
 C ObjC Var(warn_pointer_to_int_cast) Init(1) Warning
 Warn when a pointer is cast to an integer of a different size.
diff --git gcc/c/c-parser.c gcc/c/c-parser.c
index 0aba51c..46ca08a 100644
--- gcc/c/c-parser.c
+++ gcc/c/c-parser.c
@@ -7520,6 +7520,9 @@ c_parser_postfix_expression (c_parser *parser)
     case CPP_CHAR32:
     case CPP_WCHAR:
       expr.value = c_parser_peek_token (parser)->value;
+      /* For the purpose of warning when a pointer is compared with
+	 a zero character constant.  */
+      expr.original_type = char_type_node;
       set_c_expr_source_range (&expr, tok_range);
       c_parser_consume_token (parser);
       break;
diff --git gcc/c/c-typeck.c gcc/c/c-typeck.c
index d56c3d6..6426ae6 100644
--- gcc/c/c-typeck.c
+++ gcc/c/c-typeck.c
@@ -3709,6 +3709,21 @@ parser_build_binary_op (location_t location, enum tree_code code,
 	      && !integer_zerop (tree_strip_nop_conversions (arg1.value))))
 	warning_at (location, OPT_Waddress,
 		    "comparison with string literal results in unspecified behavior");
+      /* Warn for ptr == '\0', it's likely that it should've been ptr[0].  */
+      if (POINTER_TYPE_P (type1)
+	   && null_pointer_constant_p (arg2.value)
+	   && arg2.original_type == char_type_node
+	   && warning_at (location, OPT_Wpointer_compare,
+			  "comparison between pointer and zero character "
+			  "constant"))
+	inform (arg1.get_start (), "did you mean to dereference the pointer?");
+      else if (POINTER_TYPE_P (type2)
+	       && null_pointer_constant_p (arg1.value)
+	       && arg1.original_type == char_type_node
+	       && warning_at (location, OPT_Wpointer_compare,
+			      "comparison between pointer and zero character "
+			      "constant"))
+	inform (arg2.get_start (), "did you mean to dereference the pointer?");
     }
   else if (TREE_CODE_CLASS (code) == tcc_comparison
 	   && (code1 == STRING_CST || code2 == STRING_CST))
diff --git gcc/cp/cp-tree.h gcc/cp/cp-tree.h
index 5bcb98b..a29905b 100644
--- gcc/cp/cp-tree.h
+++ gcc/cp/cp-tree.h
@@ -51,16 +51,21 @@ class cp_expr
 {
 public:
   cp_expr () :
-    m_value (NULL), m_loc (UNKNOWN_LOCATION) {}
+    m_value (NULL), m_loc (UNKNOWN_LOCATION), m_char_literal_p (false) {}
 
   cp_expr (tree value) :
-    m_value (value), m_loc (EXPR_LOCATION (m_value)) {}
+    m_value (value), m_loc (EXPR_LOCATION (m_value)),
+    m_char_literal_p (false) {}
 
   cp_expr (tree value, location_t loc):
-    m_value (value), m_loc (loc) {}
+    m_value (value), m_loc (loc), m_char_literal_p (false) {}
+
+  cp_expr (tree value, location_t loc, bool char_literal_p):
+    m_value (value), m_loc (loc), m_char_literal_p (char_literal_p) {}
 
   cp_expr (const cp_expr &other) :
-    m_value (other.m_value), m_loc (other.m_loc) {}
+    m_value (other.m_value), m_loc (other.m_loc),
+    m_char_literal_p (other.m_char_literal_p) {}
 
   /* Implicit conversions to tree.  */
   operator tree () const { return m_value; }
@@ -91,9 +96,12 @@ public:
     set_location (make_location (m_loc, start, finish));
   }
 
+  bool is_char_literal_p () const { return m_char_literal_p; }
+
  private:
   tree m_value;
   location_t m_loc;
+  bool m_char_literal_p;
 };
 
 inline bool
diff --git gcc/cp/parser.c gcc/cp/parser.c
index ca9f8b9..fc916a0 100644
--- gcc/cp/parser.c
+++ gcc/cp/parser.c
@@ -4756,6 +4756,7 @@ cp_parser_primary_expression (cp_parser *parser,
 			      cp_id_kind *idk)
 {
   cp_token *token = NULL;
+  bool char_literal_p = false;
 
   /* Assume the primary expression is not an id-expression.  */
   *idk = CP_ID_KIND_NONE;
@@ -4777,6 +4778,7 @@ cp_parser_primary_expression (cp_parser *parser,
     case CPP_CHAR32:
     case CPP_WCHAR:
     case CPP_UTF8CHAR:
+      char_literal_p = true;
     case CPP_NUMBER:
     case CPP_PREPARSED_EXPR:
       if (TREE_CODE (token->u.value) == USERDEF_LITERAL)
@@ -4832,7 +4834,7 @@ cp_parser_primary_expression (cp_parser *parser,
 	  if (!cast_p)
 	    cp_parser_non_integral_constant_expression (parser, NIC_FLOAT);
 	}
-      return cp_expr (token->u.value, token->location);
+      return cp_expr (token->u.value, token->location, char_literal_p);
 
     case CPP_CHAR_USERDEF:
     case CPP_CHAR16_USERDEF:
@@ -8924,6 +8926,31 @@ cp_parser_binary_expression (cp_parser* parser, bool cast_p,
 	warn_logical_not_parentheses (current.loc, current.tree_type,
 				      current.lhs, maybe_constant_value (rhs));
 
+      /* Warn for ptr == '\0', it's likely that it should've been ptr[0].  */
+      if (current.tree_type == EQ_EXPR || current.tree_type == NE_EXPR)
+	{
+	  if (TREE_TYPE (current.lhs.get_value ())
+	      && POINTER_TYPE_P (TREE_TYPE (current.lhs.get_value ()))
+	      && integer_zerop (rhs.get_value ())
+	      && !TREE_OVERFLOW (rhs.get_value ())
+	      && rhs.is_char_literal_p ()
+	      && warning_at (current.loc, OPT_Wpointer_compare,
+			     "comparison between pointer and zero character "
+			     "literal"))
+	    inform (current.lhs.get_location (),
+		    "did you mean to dereference the pointer?");
+	  else if (TREE_TYPE (rhs.get_value ())
+		   && POINTER_TYPE_P (TREE_TYPE (rhs.get_value ()))
+		   && integer_zerop (current.lhs.get_value ())
+		   && !TREE_OVERFLOW (current.lhs.get_value ())
+		   && current.lhs.is_char_literal_p ()
+		   && warning_at (current.loc, OPT_Wpointer_compare,
+				  "comparison between pointer and zero "
+				  "character literal"))
+	    inform (rhs.get_location (),
+		    "did you mean to dereference the pointer?");
+
+	}
       overload = NULL;
 
       location_t combined_loc = make_location (current.loc,
diff --git gcc/doc/invoke.texi gcc/doc/invoke.texi
index 20be9b7..cc19874 100644
--- gcc/doc/invoke.texi
+++ gcc/doc/invoke.texi
@@ -287,7 +287,7 @@ Objective-C and Objective-C++ Dialects}.
 -Wpacked  -Wpacked-bitfield-compat  -Wpadded @gol
 -Wparentheses -Wno-pedantic-ms-format @gol
 -Wplacement-new -Wplacement-new=@var{n} @gol
--Wpointer-arith  -Wno-pointer-to-int-cast @gol
+-Wpointer-arith  -Wpointer-compare  -Wno-pointer-to-int-cast @gol
 -Wno-pragmas -Wredundant-decls  -Wno-return-local-addr @gol
 -Wreturn-type  -Wsequence-point  -Wshadow  -Wno-shadow-ivar @gol
 -Wshift-overflow -Wshift-overflow=@var{n} @gol
@@ -5115,6 +5115,20 @@ convenience in calculations with @code{void *} pointers and pointers
 to functions.  In C++, warn also when an arithmetic operation involves
 @code{NULL}.  This warning is also enabled by @option{-Wpedantic}.
 
+@item -Wpointer-compare
+@opindex Wpointer-compare
+@opindex Wno-pointer-compare
+Warn if a pointer is compared with a zero character constant.  This usually
+means that the pointer was meant to be dereferenced.  For example:
+
+@smallexample
+const char *p = foo ();
+if (p == '\0')
+  return 42;
+@end smallexample
+
+This warning is enabled by default.
+
 @item -Wtype-limits
 @opindex Wtype-limits
 @opindex Wno-type-limits
diff --git gcc/testsuite/c-c++-common/Wpointer-compare-1.c gcc/testsuite/c-c++-common/Wpointer-compare-1.c
index e69de29..80da85a 100644
--- gcc/testsuite/c-c++-common/Wpointer-compare-1.c
+++ gcc/testsuite/c-c++-common/Wpointer-compare-1.c
@@ -0,0 +1,81 @@
+/* PR c++/64767 */
+/* { dg-do compile } */
+/* { dg-options "-Wpointer-compare" } */
+/* { dg-additional-options "-std=c++11" { target c++ } } */
+
+int
+f1 (int *p, int **q)
+{
+  int r = 0;
+
+  r += p == '\0'; /* { dg-warning "comparison between pointer and zero character (literal|constant)" } */
+  r += p == L'\0'; /* { dg-warning "comparison between pointer and zero character (literal|constant)" } */
+  r += p == u'\0'; /* { dg-warning "comparison between pointer and zero character (literal|constant)" } */
+  r += p == U'\0'; /* { dg-warning "comparison between pointer and zero character (literal|constant)" } */
+  r += p != '\0'; /* { dg-warning "comparison between pointer and zero character (literal|constant)" } */
+  r += p != L'\0'; /* { dg-warning "comparison between pointer and zero character (literal|constant)" } */
+  r += p != u'\0'; /* { dg-warning "comparison between pointer and zero character (literal|constant)" } */
+  r += p != U'\0'; /* { dg-warning "comparison between pointer and zero character (literal|constant)" } */
+
+  r += '\0' == p; /* { dg-warning "comparison between pointer and zero character (literal|constant)" } */
+  r += L'\0' == p; /* { dg-warning "comparison between pointer and zero character (literal|constant)" } */
+  r += u'\0' == p; /* { dg-warning "comparison between pointer and zero character (literal|constant)" } */
+  r += U'\0' == p; /* { dg-warning "comparison between pointer and zero character (literal|constant)" } */
+  r += '\0' != p; /* { dg-warning "comparison between pointer and zero character (literal|constant)" } */
+  r += L'\0' != p; /* { dg-warning "comparison between pointer and zero character (literal|constant)" } */
+  r += u'\0' != p; /* { dg-warning "comparison between pointer and zero character (literal|constant)" } */
+  r += U'\0' != p; /* { dg-warning "comparison between pointer and zero character (literal|constant)" } */
+
+  r += q == '\0'; /* { dg-warning "comparison between pointer and zero character (literal|constant)" } */
+  r += q == L'\0'; /* { dg-warning "comparison between pointer and zero character (literal|constant)" } */
+  r += q == u'\0'; /* { dg-warning "comparison between pointer and zero character (literal|constant)" } */
+  r += q == U'\0'; /* { dg-warning "comparison between pointer and zero character (literal|constant)" } */
+  r += q != '\0'; /* { dg-warning "comparison between pointer and zero character (literal|constant)" } */
+  r += q != L'\0'; /* { dg-warning "comparison between pointer and zero character (literal|constant)" } */
+  r += q != u'\0'; /* { dg-warning "comparison between pointer and zero character (literal|constant)" } */
+  r += q != U'\0'; /* { dg-warning "comparison between pointer and zero character (literal|constant)" } */
+
+  r += '\0' == q; /* { dg-warning "comparison between pointer and zero character (literal|constant)" } */
+  r += L'\0' == q; /* { dg-warning "comparison between pointer and zero character (literal|constant)" } */
+  r += u'\0' == q; /* { dg-warning "comparison between pointer and zero character (literal|constant)" } */
+  r += U'\0' == q; /* { dg-warning "comparison between pointer and zero character (literal|constant)" } */
+  r += '\0' != q; /* { dg-warning "comparison between pointer and zero character (literal|constant)" } */
+  r += L'\0' != q; /* { dg-warning "comparison between pointer and zero character (literal|constant)" } */
+  r += u'\0' != q; /* { dg-warning "comparison between pointer and zero character (literal|constant)" } */
+  r += U'\0' != q; /* { dg-warning "comparison between pointer and zero character (literal|constant)" } */
+
+  return r;
+}
+
+int
+f2 (int *p)
+{
+  int r = 0;
+
+  /* Keep quiet.  */
+  r += p == (void *) 0;
+  r += p != (void *) 0;
+  r += (void *) 0 == p;
+  r += (void *) 0 != p;
+
+  r += p == 0;
+  r += p != 0;
+  r += 0 == p;
+  r += 0 != p;
+
+  return r;
+}
+
+int
+f3 (int *p)
+{
+  int r = 0;
+
+  r += p == (char) 0;
+  r += p != (char) 0;
+
+  r += (char) 0 == p;
+  r += (char) 0 != p;
+
+  return r;
+}

	Marek


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]