Re: Re: [PATCH] Emit -Waddress warnings for comparing address of reference against NULL

[Patrick Palka <patrick@parcs.ath.cx>]

On Thu, Apr 23, 2015 at 11:34 AM, Manuel López-Ibáñez
<lopezibanez@gmail.com> wrote:
> On 04/23/2015 05:12 PM, Jason Merrill wrote:
>>
>> On 04/20/2015 10:36 PM, Patrick Palka wrote:
>> Implementation is pretty straightforward.  The only catch is that the
>> middle-end doesn't actually assume that REFERENCE_TYPEs are non-NULL so
>> code like
>>
>>     int &a = *(int *)0;
>>     if (&a != 0)
>>
>> will warn that &a will never be NULL yet the middle-end will fold the
>> conditional to false instead of true anyway.  But I guess that's not a
>> big deal.
>
>
> Is this actually correct? Is it because of undefined behavior?

I would think that the assignment is UB due to the null-pointer
dereference but if it's not then we may have to fold the comparison to
true always.  According to section 8.3.2 of the C++11 standard:

A reference shall be initialized to refer to a valid object or
function. [ Note: in particular, a null reference cannot exist in a
well-defined program, because the only way to create such a reference
would be to bind it to the “object” obtained by dereferencing a null
pointer, which causes undefined behavior. As described in 9.6, a
reference cannot be bound directly to a bit-field. — end note ]

So it looks like it's not incorrect to fold the comparison to false.

>
> It seems also weird we do not warn directly for '*(int *)0' in the C/C++ FE.

That wouldn't be too hard to add probably.  I'll take a look at this.

>
>
>>> +        if (decl_with_nonnull_addr_p (inner))
>>
>>
>> Using decl_with_nonnull_addr_p doesn't make sense for reference variables,
>> since we're using their pointer value rather than their address.
>
>
> Is an extra check needed at all (can &reference ever be false)?

Consider it removed. Somehow I didn't catch the redundancy of this check..

>
>>
>>> +          warning_at (location,
>>> +              OPT_Waddress,
>>> +              "the address of reference %qD may be assumed to "
>>> +              "always evaluate to %<true%>",
>>> +              inner);
>>
>>
>> Perhaps "the compiler can assume that the address of reference %qD will
>> always
>> evaluate to %<true%>"?
>
>
> The discussion (and perhaps the patch) at
> https://gcc.gnu.org/bugzilla/PR65168 may be relevant.
>
> Jonathan suggests to match what we say for:
>
> /home/manuel/test.c:3:21: warning: the address of ‘i’ will always evaluate
> as ‘true’ [-Waddress]
>    int i; bool b = !&i;
>                      ^
>
> I think this case requires a slightly different text because the address may
> not evaluate to 'true' and also because it is not actually the address of
> the reference but the object bounded to the reference. Clang says:

OK.

>
> warning: reference cannot be bound to dereferenced null pointer in
> well-defined C++ code; pointer may be assumed to always convert to true
> [-Wundefined-bool-conversion]
>
> which is in my opinion even less clear.
>
> The testcases:
>
> int fii(int *p) {
>   int &r=*p;
>   return !&r;
> }
>
> int fii(int p) {
>   int &r=p;
>   return !&r;
> }
>
> should also generate the same warning in my opinion.

Good idea.

Thanks guys for the feedback.  I'll post a new version of this patch
in about 24 hours or so.  So many issues for such a small patch!