From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 51411 invoked by alias); 17 Mar 2016 19:00:43 -0000 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 Received: (qmail 51395 invoked by uid 89); 17 Mar 2016 19:00:43 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=Hx-languages-length:5722, claims, claim, talk 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 (AES256-GCM-SHA384 encrypted) ESMTPS; Thu, 17 Mar 2016 19:00:40 +0000 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (Postfix) with ESMTPS id 1F519711C5; Thu, 17 Mar 2016 19:00:39 +0000 (UTC) Received: from [10.3.113.138] (ovpn-113-138.phx2.redhat.com [10.3.113.138]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u2HJ0bw1025241; Thu, 17 Mar 2016 15:00:38 -0400 Subject: Re: C++ PATCH to fix missing warning (PR c++/70194) To: Martin Sebor , Patrick Palka , Jeff Law References: <20160315104120.GC10006@redhat.com> <20160315105618.GT3017@tucnak.redhat.com> <20160315120918.GD10006@redhat.com> <56E86580.70800@redhat.com> <20160316144552.GG10006@redhat.com> <56E9FDBB.5070903@gmail.com> <56EADB0E.3010304@redhat.com> <56EAFCC6.9010905@gmail.com> Cc: Marek Polacek , Jakub Jelinek , GCC Patches From: Jason Merrill Message-ID: <56EAFED5.8090305@redhat.com> Date: Thu, 17 Mar 2016 19:17:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.6.0 MIME-Version: 1.0 In-Reply-To: <56EAFCC6.9010905@gmail.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit X-SW-Source: 2016-03/txt/msg01042.txt.bz2 On 03/17/2016 02:51 PM, Martin Sebor wrote: > On 03/17/2016 10:48 AM, Patrick Palka wrote: >> On Thu, Mar 17, 2016 at 12:27 PM, Jeff Law wrote: >>> On 03/16/2016 06:43 PM, Martin Sebor wrote: >>>>> >>>>> @@ -3974,6 +3974,38 @@ build_vec_cmp (tree_code code, tree type, >>>>> return build3 (VEC_COND_EXPR, type, cmp, minus_one_vec, >>>>> zero_vec); >>>>> } >>>>> >>>>> +/* Possibly warn about an address never being NULL. */ >>>>> + >>>>> +static void >>>>> +warn_for_null_address (location_t location, tree op, tsubst_flags_t >>>>> complain) >>>>> +{ >>>> >>>> ... >>>>> >>>>> + if (TREE_CODE (cop) == ADDR_EXPR >>>>> + && decl_with_nonnull_addr_p (TREE_OPERAND (cop, 0)) >>>>> + && !TREE_NO_WARNING (cop)) >>>>> + warning_at (location, OPT_Waddress, "the address of %qD will >>>>> never " >>>>> + "be NULL", TREE_OPERAND (cop, 0)); >>>>> + >>>>> + if (CONVERT_EXPR_P (op) >>>>> + && TREE_CODE (TREE_TYPE (TREE_OPERAND (op, 0))) == >>>>> REFERENCE_TYPE) >>>>> + { >>>>> + tree inner_op = op; >>>>> + STRIP_NOPS (inner_op); >>>>> + >>>>> + if (DECL_P (inner_op)) >>>>> + warning_at (location, OPT_Waddress, >>>>> + "the compiler can assume that the address of " >>>>> + "%qD will never be NULL", inner_op); >>>> >>>> >>>> Since I noted the subtle differences between the phrasing of >>>> the various -Waddress warnings in the bug, I have to ask: what is >>>> the significance of the difference between the two warnings here? >>>> >>>> Would it not be appropriate to issue the first warning in the latter >>>> case? Or perhaps even use the same text as is already used elsewhere: >>>> "the address of %qD will always evaluate as ‘true’" (since it may not >>>> be the macro NULL that's mentioned in the expression). >>> >>> They were added at different times AFAICT. The former is fairly old >>> (Douglas Gregor, 2008) at this point. The latter was added by >>> Patrick Palka >>> for 65168 about a year ago. >>> >>> You could directly ask Patrick about motivations for a different >>> message. >> >> There is no plausible way for the address of a non-reference variable >> to be NULL even in code with UB (aside from __attribute__ ((weak)) in >> which case the warning is suppressed). But the address of a reference >> can easily seem to be NULL if one performs UB and assigns to it *(int >> *)NULL or something like that. I think that was my motivation, anyway >> :) > > Thanks (everyone) for the explanation. > > I actually think the warning Patrick added is the most accurate > and would be appropriate in all cases. > > I suppose what bothers me besides the mention of NULL even when > there is no NULL in the code, is that a) the text of the warnings > is misleading (contradictory) in some interesting cases, and b) > I can't think of a way in which the difference between the three > phrasings of the diagnostic could be useful to a user. All three > imply the same thing: compilers can and GCC is some cases does > assume that the address of an ordinary (non weak) function, object, > or reference is not null. > > To see (a), consider the invalid test program below, in which > GCC makes this assumption about the address of i even though > the warning doesn't mention it (but it makes a claim that's > contrary to the actual address), yet doesn't make the same > assumption about the address of the reference even though > the diagnostic says it can. > > While I would find the warning less misleading if it simply said > in all three cases: "the address of 'x' will always evaluate as > ‘true’" I think it would be even more accurate if it said > "the address of 'x' may be assumed to evaluate to ‘true’" That > avoids making claims about whether or not it actually is null, > doesn't talk about the NULL macro when one isn't used in the > code, and by saying "may assume" it allows for both making > the assumption as well as not making one. That sounds good except that talking about 'true' is wrong when there is an explicit comparison to a null pointer constant. I'd be fine with changing "NULL" to "null" or similar. > I'm happy to submit a patch to make this change in stage 1 if > no one objects to it. > > Martin > > $ cat x.c && /home/msebor/build/gcc-trunk-svn/gcc/xgcc > -B/home/msebor/build/gcc-trunk-svn/gcc -c -xc++ x.c && > /home/msebor/build/gcc-trunk-svn/gcc/xgcc > -B/home/msebor/build/gcc-trunk-svn/gcc -DMAIN -Wall -Wextra -Wpedantic > x.o -xc++ x.c && ./a.out > #if MAIN > > extern int i; > extern int &r; > > extern void f (); > > int main () > { > f (); > > #define TEST(x) __builtin_printf ("%s is %s\n", #x, (x) ? "true" : "false") > > TEST (&i != 0); > TEST (&r != 0); > TEST (&i); > } > > #else > extern __attribute__ ((weak)) int i; > int &r = i; > > void f () > { > __builtin_printf ("&i = %p\n&r = %p\n", &i, &r); > } > > #endif > x.c: In function ‘int main()’: > x.c:14:17: warning: the address of ‘i’ will never be NULL [-Waddress] > TEST (&i != 0); > ^ > x.c:12:54: note: in definition of macro ‘TEST’ > #define TEST(x) __builtin_printf ("%s is %s\n", #x, (x) ? "true" : > "false") > ^ > x.c:15:14: warning: the compiler can assume that the address of ‘r’ will > never be NULL [-Waddress] > TEST (&r != 0); > ~~~^~~~ > x.c:12:54: note: in definition of macro ‘TEST’ > #define TEST(x) __builtin_printf ("%s is %s\n", #x, (x) ? "true" : > "false") > ^ > x.c:12:68: warning: the address of ‘i’ will always evaluate as ‘true’ > [-Waddres] > #define TEST(x) __builtin_printf ("%s is %s\n", #x, (x) ? "true" : > "false") > ^ > x.c:16:5: note: in expansion of macro ‘TEST’ > TEST (&i); > ^~~~ > &i = (nil) > &r = (nil) > &i != 0 is true > &r != 0 is false > &i is true >