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

[Martin Sebor <msebor@gmail.com>]

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.

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