C++ PATCH to fix missing warning (PR c++/70194)

[Jason Merrill]

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 <law@redhat.com> 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
>