This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
C/C++ PATCH to implement -Wpointer-compare warning (PR c++/64767)
- From: Marek Polacek <polacek at redhat dot com>
- To: GCC Patches <gcc-patches at gcc dot gnu dot org>, Jason Merrill <jason at redhat dot com>, Joseph Myers <joseph at codesourcery dot com>
- Date: Sat, 10 Sep 2016 16:58:43 +0200
- Subject: C/C++ PATCH to implement -Wpointer-compare warning (PR c++/64767)
- Authentication-results: sourceware.org; auth=none
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