Emit -Waddress warnings for comparing address of reference against NULL

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.

Does this look OK after testing?

gcc/c-family/ChangeLog:
	* c-common.c (c_common_truthvalue_conversion): Warn when
	converting an address of a reference to a truth value.

gcc/cp/ChangeLog:
	* typeck.c (cp_build_binary_op): Warn when comparing an address
	of a reference against NULL.

gcc/testsuite/ChangeLog:
	g++.dg/warn/Walways-true-3.C: New test.
---
 gcc/c-family/c-common.c                    | 12 ++++++++++++
 gcc/cp/typeck.c                            | 10 ++++++++--
 gcc/testsuite/g++.dg/warn/Walways-true-3.C | 30 ++++++++++++++++++++++++++++++
 3 files changed, 50 insertions(+), 2 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/warn/Walways-true-3.C

On 04/20/2015 10:36 PM, Patrick Palka wrote:
> +	    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.

> +	      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%>"?

> -	  if (TREE_CODE (op0) == ADDR_EXPR
> +	  if ((TREE_CODE (op0) == ADDR_EXPR
> +	       || (CONVERT_EXPR_P (op0)
> +		   && TREE_CODE (TREE_TYPE (TREE_OPERAND (op0, 0)))
> +		      == REFERENCE_TYPE))
>   	      && decl_with_nonnull_addr_p (TREE_OPERAND (op0, 0)))
>   	    {
>   	      if ((complain & tf_warning)
> @@ -4437,7 +4440,10 @@ cp_build_binary_op (location_t location,
>   	  else
>   	    result_type = type1;
>
> -	  if (TREE_CODE (op1) == ADDR_EXPR
> +	  if ((TREE_CODE (op1) == ADDR_EXPR
> +	       || (CONVERT_EXPR_P (op1)
> +		   && TREE_CODE (TREE_TYPE (TREE_OPERAND (op1, 0)))
> +		      == REFERENCE_TYPE))

Let's distinguish between address of object vs reference in these 
warnings, too.

Jason

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?

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


>> +        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)?

>
>> +          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:

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.

Cheers,

Manuel.

On 04/23/2015 11:34 AM, Manuel López-Ibáñez wrote:
> It seems also weird we do not warn directly for '*(int *)0' in the C/C++
> FE.

Agreed.

>> 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)?

No, I don't think we need any extra check here.

> The discussion (and perhaps the patch) at
> https://gcc.gnu.org/bugzilla/PR65168 may be relevant.

Yes, I thought this sounded familiar...

> 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.

I agree with the first point, but C++ (unfortunately) uses "address" to 
refer both to the actual numerical address and the result of the & 
operator.  So I think it's OK to use the word in the latter sense here.

> 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.

Agreed.

Jason

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!