From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 29343 invoked by alias); 5 Jan 2007 06:58:34 -0000 Received: (qmail 29335 invoked by uid 22791); 5 Jan 2007 06:58:32 -0000 X-Spam-Check-By: sourceware.org Received: from smtp-out.google.com (HELO smtp-out.google.com) (216.239.45.13) by sourceware.org (qpsmtpd/0.31) with ESMTP; Fri, 05 Jan 2007 06:58:26 +0000 Received: from zps38.corp.google.com (zps38.corp.google.com [172.25.146.38]) by smtp-out.google.com with ESMTP id l056wNfR014227; Thu, 4 Jan 2007 22:58:23 -0800 Received: from smtp.corp.google.com (spacemonkey2.corp.google.com [192.168.120.114]) by zps38.corp.google.com with ESMTP id l056wFou026607 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NOT); Thu, 4 Jan 2007 22:58:15 -0800 Received: from dhcp-172-18-118-195.corp.google.com.google.com (adsl-71-133-8-30.dsl.pltn13.pacbell.net [71.133.8.30]) (authenticated bits=0) by smtp.corp.google.com (8.13.7/8.13.7) with ESMTP id l056wEuv031923 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NOT); Thu, 4 Jan 2007 22:58:14 -0800 To: Mark Mitchell Cc: gcc-patches@gcc.gnu.org Subject: Re: PATCH RFA: C++ frontend warning: comparing function pointer to NULL References: <200701041848.l04Im9Z0015493@localhost.localdomain> <459DDEC4.6050909@codesourcery.com> From: Ian Lance Taylor Date: Fri, 05 Jan 2007 06:58:00 -0000 In-Reply-To: <459DDEC4.6050909@codesourcery.com> Message-ID: User-Agent: Gnus/5.09 (Gnus v5.9.0) Emacs/21.4 MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-IsSubscribed: yes Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org X-SW-Source: 2007-01/txt/msg00343.txt.bz2 Mark Mitchell writes: > I hate to be niggly, but it looks like you've introduced two copies of > the same test here (modulo op0 vs. op1), and it sounds like the C front > end has at least one more copy. Would you mind creating a function, > shared as widely as possible? How about this version? Since I changed the C frontend, I added C test cases too (they are actually exactly the same as the C++ test cases, except for comment syntax). Bootstrapped on i686-pc-linux-gnu, testsuite run in progress. Ian Index: gcc/c-common.c =================================================================== --- gcc/c-common.c (revision 120376) +++ gcc/c-common.c (working copy) @@ -2591,6 +2591,18 @@ pointer_int_sum (enum tree_code resultco return fold_build2 (resultcode, result_type, ptrop, intop); } +/* Return whether EXPR is a declaration whose address can never be + NULL. */ + +bool +decl_with_nonnull_addr_p (tree expr) +{ + return (DECL_P (expr) + && (TREE_CODE (expr) == PARM_DECL + || TREE_CODE (expr) == LABEL_DECL + || !DECL_WEAK (expr))); +} + /* Prepare expr to be an argument of a TRUTH_NOT_EXPR, or for an `if' or `while' statement or ?..: exp. It should already have been validated to be of suitable type; otherwise, a bad @@ -2656,23 +2668,22 @@ c_common_truthvalue_conversion (tree exp case ADDR_EXPR: { tree inner = TREE_OPERAND (expr, 0); - if (DECL_P (inner) - && (TREE_CODE (inner) == PARM_DECL - || TREE_CODE (inner) == LABEL_DECL - || !DECL_WEAK (inner))) + if (decl_with_nonnull_addr_p (inner)) { - /* Common Ada/Pascal programmer's mistake. We always warn - about this since it is so bad. */ - warning (OPT_Walways_true, "the address of %qD will always evaluate as %", + /* Common Ada/Pascal programmer's mistake. */ + warning (OPT_Walways_true, + "the address of %qD will always evaluate as %", inner); return truthvalue_true_node; } - /* If we are taking the address of an external decl, it might be - zero if it is weak, so we cannot optimize. */ - if (DECL_P (inner) - && DECL_EXTERNAL (inner)) - break; + /* If we still have a decl, it is possible for its address to + be NULL, so we cannot optimize. */ + if (DECL_P (inner)) + { + gcc_assert (DECL_WEAK (inner)); + break; + } if (TREE_SIDE_EFFECTS (inner)) return build2 (COMPOUND_EXPR, truthvalue_type_node, Index: gcc/c-typeck.c =================================================================== --- gcc/c-typeck.c (revision 120376) +++ gcc/c-typeck.c (working copy) @@ -8013,10 +8013,7 @@ build_binary_op (enum tree_code code, tr else if (code0 == POINTER_TYPE && null_pointer_constant_p (orig_op1)) { if (TREE_CODE (op0) == ADDR_EXPR - && DECL_P (TREE_OPERAND (op0, 0)) - && (TREE_CODE (TREE_OPERAND (op0, 0)) == PARM_DECL - || TREE_CODE (TREE_OPERAND (op0, 0)) == LABEL_DECL - || !DECL_WEAK (TREE_OPERAND (op0, 0)))) + && decl_with_nonnull_addr_p (TREE_OPERAND (op0, 0))) warning (OPT_Walways_true, "the address of %qD will never be NULL", TREE_OPERAND (op0, 0)); result_type = type0; @@ -8024,10 +8021,7 @@ build_binary_op (enum tree_code code, tr else if (code1 == POINTER_TYPE && null_pointer_constant_p (orig_op0)) { if (TREE_CODE (op1) == ADDR_EXPR - && DECL_P (TREE_OPERAND (op1, 0)) - && (TREE_CODE (TREE_OPERAND (op1, 0)) == PARM_DECL - || TREE_CODE (TREE_OPERAND (op1, 0)) == LABEL_DECL - || !DECL_WEAK (TREE_OPERAND (op1, 0)))) + && decl_with_nonnull_addr_p (TREE_OPERAND (op1, 0))) warning (OPT_Walways_true, "the address of %qD will never be NULL", TREE_OPERAND (op1, 0)); result_type = type1; Index: gcc/c-common.h =================================================================== --- gcc/c-common.h (revision 120376) +++ gcc/c-common.h (working copy) @@ -652,6 +652,7 @@ extern tree c_common_unsigned_type (tree extern tree c_common_signed_type (tree); extern tree c_common_signed_or_unsigned_type (int, tree); extern tree c_build_bitfield_integer_type (unsigned HOST_WIDE_INT, int); +extern bool decl_with_nonnull_addr_p (tree); extern tree c_common_truthvalue_conversion (tree); extern void c_apply_type_quals_to_decl (int, tree); extern tree c_sizeof_or_alignof_type (tree, bool, int); Index: gcc/cp/typeck.c =================================================================== --- gcc/cp/typeck.c (revision 120376) +++ gcc/cp/typeck.c (working copy) @@ -3334,10 +3334,22 @@ build_binary_op (enum tree_code code, tr "comparison"); else if ((code0 == POINTER_TYPE || TYPE_PTRMEM_P (type0)) && null_ptr_cst_p (op1)) - result_type = type0; + { + if (TREE_CODE (op0) == ADDR_EXPR + && decl_with_nonnull_addr_p (TREE_OPERAND (op0, 0))) + warning (OPT_Walways_true, "the address of %qD will never be NULL", + TREE_OPERAND (op0, 0)); + result_type = type0; + } else if ((code1 == POINTER_TYPE || TYPE_PTRMEM_P (type1)) && null_ptr_cst_p (op0)) - result_type = type1; + { + if (TREE_CODE (op1) == ADDR_EXPR + && decl_with_nonnull_addr_p (TREE_OPERAND (op1, 0))) + warning (OPT_Walways_true, "the address of %qD will never be NULL", + TREE_OPERAND (op1, 0)); + result_type = type1; + } else if (code0 == POINTER_TYPE && code1 == INTEGER_TYPE) { result_type = type0; Index: gcc/testsuite/gcc.dg/Walways-true-1.c =================================================================== --- gcc/testsuite/gcc.dg/Walways-true-1.c (revision 0) +++ gcc/testsuite/gcc.dg/Walways-true-1.c (revision 0) @@ -0,0 +1,57 @@ +/* Test -Walways-true for testing an address against NULL. + Origin: Ian Lance Taylor . */ + +/* { dg-do compile} */ +/* { dg-options "-Walways-true" } */ + +extern int foo (int); + +int i; + +void +bar (int a) +{ + lab: + if (foo) /* { dg-warning "always evaluate as" "correct warning" } */ + foo (0); + if (foo (1)) + ; + if (&i) /* { dg-warning "always evaluate as" "correct warning" } */ + foo (2); + if (i) + foo (3); + if (&a) /* { dg-warning "always evaluate as" "correct warning" } */ + foo (4); + if (a) + foo (5); + if (&&lab) /* { dg-warning "always evaluate as" "correct warning" } */ + foo (6); + if (foo == 0) /* { dg-warning "never be NULL" "correct warning" } */ + foo (7); + if (foo (1) == 0) + foo (8); + if (&i == 0) /* { dg-warning "never be NULL" "correct warning" } */ + foo (9); + if (i == 0) + foo (10); + if (&a == 0) /* { dg-warning "never be NULL" "correct warning" } */ + foo (11); + if (a == 0) + foo (12); + if (&&lab == 0) /* { dg-warning "never be NULL" "correct warning" } */ + foo (13); + if (0 == foo) /* { dg-warning "never be NULL" "correct warning" } */ + foo (14); + if (0 == foo (1)) + foo (15); + if (0 == &i) /* { dg-warning "never be NULL" "correct warning" } */ + foo (16); + if (0 == i) + foo (17); + if (0 == &a) /* { dg-warning "never be NULL" "correct warning" } */ + foo (18); + if (0 == a) + foo (19); + if (0 == &&lab) /* { dg-warning "never be NULL" "correct warning" } */ + foo (20); +} Index: gcc/testsuite/gcc.dg/Walways-true-2.c =================================================================== --- gcc/testsuite/gcc.dg/Walways-true-2.c (revision 0) +++ gcc/testsuite/gcc.dg/Walways-true-2.c (revision 0) @@ -0,0 +1,60 @@ +/* Make sure we don't assume that a weak symbol is always non-NULL. + This is just like Walways-true-1.C, except that it uses a weak + symbol. + Origin: Ian Lance Taylor . */ + +/* { dg-do compile} */ +/* { dg-options "-Walways-true" } */ +/* { dg-require-weak "" } */ + +extern int foo (int) __attribute__ ((weak)); + +int i __attribute__ ((weak)); + +void +bar (int a) +{ + lab: + if (foo) + foo (0); + if (foo (1)) + ; + if (&i) + foo (2); + if (i) + foo (3); + if (&a) /* { dg-warning "always evaluate as" "correct warning" } */ + foo (4); + if (a) + foo (5); + if (&&lab) /* { dg-warning "always evaluate as" "correct warning" } */ + foo (6); + if (foo == 0) + foo (7); + if (foo (1) == 0) + foo (8); + if (&i == 0) + foo (9); + if (i == 0) + foo (10); + if (&a == 0) /* { dg-warning "never be NULL" "correct warning" } */ + foo (11); + if (a == 0) + foo (12); + if (&&lab == 0) /* { dg-warning "never be NULL" "correct warning" } */ + foo (13); + if (0 == foo) + foo (14); + if (0 == foo (1)) + foo (15); + if (0 == &i) + foo (16); + if (0 == i) + foo (17); + if (0 == &a) /* { dg-warning "never be NULL" "correct warning" } */ + foo (18); + if (0 == a) + foo (19); + if (0 == &&lab) /* { dg-warning "never be NULL" "correct warning" } */ + foo (20); +} Index: gcc/testsuite/g++.dg/warn/Walways-true-1.C =================================================================== --- gcc/testsuite/g++.dg/warn/Walways-true-1.C (revision 0) +++ gcc/testsuite/g++.dg/warn/Walways-true-1.C (revision 0) @@ -0,0 +1,57 @@ +// Test -Walways-true for testing an address against NULL. +// Origin: Ian Lance Taylor + +// { dg-do compile} +// { dg-options "-Walways-true" } + +extern int foo (int); + +int i; + +void +bar (int a) +{ + lab: + if (foo) // { dg-warning "always evaluate as" "correct warning" } + foo (0); + if (foo (1)) + ; + if (&i) // { dg-warning "always evaluate as" "correct warning" } + foo (2); + if (i) + foo (3); + if (&a) // { dg-warning "always evaluate as" "correct warning" } + foo (4); + if (a) + foo (5); + if (&&lab) // { dg-warning "always evaluate as" "correct warning" } + foo (6); + if (foo == 0) // { dg-warning "never be NULL" "correct warning" } + foo (7); + if (foo (1) == 0) + foo (8); + if (&i == 0) // { dg-warning "never be NULL" "correct warning" } + foo (9); + if (i == 0) + foo (10); + if (&a == 0) // { dg-warning "never be NULL" "correct warning" } + foo (11); + if (a == 0) + foo (12); + if (&&lab == 0) // { dg-warning "never be NULL" "correct warning" } + foo (13); + if (0 == foo) // { dg-warning "never be NULL" "correct warning" } + foo (14); + if (0 == foo (1)) + foo (15); + if (0 == &i) // { dg-warning "never be NULL" "correct warning" } + foo (16); + if (0 == i) + foo (17); + if (0 == &a) // { dg-warning "never be NULL" "correct warning" } + foo (18); + if (0 == a) + foo (19); + if (0 == &&lab) // { dg-warning "never be NULL" "correct warning" } + foo (20); +} Index: gcc/testsuite/g++.dg/warn/Walways-true-2.C =================================================================== --- gcc/testsuite/g++.dg/warn/Walways-true-2.C (revision 0) +++ gcc/testsuite/g++.dg/warn/Walways-true-2.C (revision 0) @@ -0,0 +1,60 @@ +// Make sure we don't assume that a weak symbol is always non-NULL. +// This is just like Walways-true-1.C, except that it uses a weak +// symbol. +// Origin: Ian Lance Taylor + +// { dg-do compile} +// { dg-options "-Walways-true" } +// { dg-require-weak "" } + +extern int foo (int) __attribute__ ((weak)); + +int i __attribute__ ((weak)); + +void +bar (int a) +{ + lab: + if (foo) + foo (0); + if (foo (1)) + ; + if (&i) + foo (2); + if (i) + foo (3); + if (&a) // { dg-warning "always evaluate as" "correct warning" } + foo (4); + if (a) + foo (5); + if (&&lab) // { dg-warning "always evaluate as" "correct warning" } + foo (6); + if (foo == 0) + foo (7); + if (foo (1) == 0) + foo (8); + if (&i == 0) + foo (9); + if (i == 0) + foo (10); + if (&a == 0) // { dg-warning "never be NULL" "correct warning" } + foo (11); + if (a == 0) + foo (12); + if (&&lab == 0) // { dg-warning "never be NULL" "correct warning" } + foo (13); + if (0 == foo) + foo (14); + if (0 == foo (1)) + foo (15); + if (0 == &i) + foo (16); + if (0 == i) + foo (17); + if (0 == &a) // { dg-warning "never be NULL" "correct warning" } + foo (18); + if (0 == a) + foo (19); + if (0 == &&lab) // { dg-warning "never be NULL" "correct warning" } + foo (20); +}