From patchwork Sat Sep 10 14:58:43 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Marek Polacek X-Patchwork-Id: 668381 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 3sWcfT0xl8z9sBr for ; Sun, 11 Sep 2016 00:59:11 +1000 (AEST) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b=o1Lb80up; dkim-atps=neutral DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:date :from:to:subject:message-id:mime-version:content-type; q=dns; s= default; b=MUqcjrR8MGhrSbdtplhT8ius2zZII5HgKpJN+fW+x6ul3xUyh7Owo BxB2pr76bM/1fW2YwmHTU1g/DbYjTm6y6ZIh0GIpaiE+luBi4oayyYSWgTJD7LSw BIhVG/y98qmtFbXW0k+vf0hfwlgOP8VWTWuAsqiMyFxAZaoNSRkmks= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:date :from:to:subject:message-id:mime-version:content-type; s= default; bh=JiW7fAeLiHQo9MdC+VtulGc84Mw=; b=o1Lb80upcQMFtNFQuW9n me70/3HD1Yl+YeDqlLp93cb0MkU27mXef3zotGquRg1c+1dX7FFOnWnyOrT+adLd DsOuvN67cFTbYydKyWAEETIb29J/ndgkgn3IRETRzUpZe3ma62N+YkyQpyLxSVsm SxNQbnq71ZKPx7pqe0jivAA= Received: (qmail 99436 invoked by alias); 10 Sep 2016 14:59:02 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Received: (qmail 99412 invoked by uid 89); 10 Sep 2016 14:59:00 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-3.2 required=5.0 tests=BAYES_00, RP_MATCHES_RCVD, SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=wpointerarith, Wpointerarith, wpointer-arith, Wpointer-arith X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Sat, 10 Sep 2016 14:58:50 +0000 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 76AB261E61; Sat, 10 Sep 2016 14:58:48 +0000 (UTC) Received: from redhat.com (ovpn-204-19.brq.redhat.com [10.40.204.19]) by int-mx14.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u8AEwiGA006486 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=NO); Sat, 10 Sep 2016 10:58:46 -0400 Date: Sat, 10 Sep 2016 16:58:43 +0200 From: Marek Polacek To: GCC Patches , Jason Merrill , Joseph Myers Subject: C/C++ PATCH to implement -Wpointer-compare warning (PR c++/64767) Message-ID: <20160910145843.GJ19950@redhat.com> MIME-Version: 1.0 Content-Disposition: inline User-Agent: Mutt/1.7.0 (2016-08-17) Spurred by the recent 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 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. Marek 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; +}