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

[Jeff Law]

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.

Jeff

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

[Martin Sebor]

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.

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 

extern void f ();

int main ()
{
f ();

#define TEST(x) __builtin_printf ("%s is %s\n", #x, (x) ? "true" : "false")

TEST ( != 0);
TEST ( != 0);
TEST ();
}

#else
extern __attribute__ ((weak)) int i;
int  = i;

void f ()
{
__builtin_printf (" = %p\n = %p\n", , );
}

#endif
x.c: In function ‘int main()’:
x.c:14:17: warning: the address of ‘i’ will never be NULL [-Waddress]
 TEST ( != 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 ( != 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) 

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

[Jeff Law]

On 03/17/2016 10:45 AM, Jason Merrill wrote:

On 03/16/2016 08: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?


The difference is that in the second case, a reference could be bound to
a null address, but that has undefined behavior, so the compiler can
assume it won't happen.
So the first can't happen, the second could, but would be considered 
undefined behavior.


jeff

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

[Martin Sebor]

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.


Sounds good.  I will use bug 47931 - missing -Waddress warning
for comparison with NULL, to take care of the outstanding cases
where a warning still isn't issued (in either C++ or C) and also
adjust the text of the warning.

Martin

PS It seems that just adding STRIP_NOPS (op) to Marek's patch
significantly increases the number of successfully diagnosed
cases.  (The small patch I attached to 47931 covers nearly all
the remaining cases I could think of.)

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

[Jason Merrill]

On 03/16/2016 08: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?


The difference is that in the second case, a reference could be bound to 
a null address, but that has undefined behavior, so the compiler can 
assume it won't happen.


Jason

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

[Marek Polacek]

On Wed, Mar 16, 2016 at 06:43:39PM -0600, 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?
 
Quite frankly, I don't know.

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

There are more discrepancies in the front ends wrt error/warning messages.
Perhaps we should try to unify them some more, but I don't think this has
a big priority, if the message is clear enough for the users.

Marek

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

extern void f ();

int main ()
{
 f ();

#define TEST(x) __builtin_printf ("%s is %s\n", #x, (x) ? "true" : "false")

 TEST ( != 0);
 TEST ( != 0);
 TEST ();
}

#else
extern __attribute__ ((weak)) int i;
int  = i;

void f ()
{
 __builtin_printf (" = %p\n = %p\n", , );
}

#endif
x.c: In function ‘int main()’:
x.c:14:17: warning: the address of ‘i’ will never be NULL [-Waddress]
  TEST ( != 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 ( != 0);
~~~^~~~
x.c:12:54: note: in definition of macro ‘TEST’
  #define 

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

[Jason Merrill]

OK.

Jason

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

[Marek Polacek]

On Tue, Mar 15, 2016 at 03:41:52PM -0400, Jason Merrill wrote:
> Let's factor out that duplicated code into a separate function.

Sure.  It also allowed me to hoist the cheap tests for both warnings, and
while at it, I used 'location' for the first warning.

Bootstrapped/regtested on x86_64-linux, ok for trunk?

2016-03-16  Marek Polacek  

PR c++/70194
* typeck.c (warn_for_null_address): New function.
(cp_build_binary_op): Call it.

* g++.dg/warn/constexpr-70194.C: New test.

diff --git gcc/cp/typeck.c gcc/cp/typeck.c
index 20f0afc..447006c 100644
--- gcc/cp/typeck.c
+++ gcc/cp/typeck.c
@@ -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 (!warn_address
+  || (complain & tf_warning) == 0
+  || c_inhibit_evaluation_warnings != 0
+  || TREE_NO_WARNING (op))
+return;
+
+  tree cop = fold_non_dependent_expr (op);
+
+  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);
+}
+}
+
 /* Build a binary-operation expression without default conversions.
CODE is the kind of expression to build.
LOCATION is the location_t of the operator in the source code.
@@ -4520,32 +4552,7 @@ cp_build_binary_op (location_t location,
  else
result_type = type0;
 
- if (TREE_CODE (op0) == ADDR_EXPR
- && decl_with_nonnull_addr_p (TREE_OPERAND (op0, 0)))
-   {
- if ((complain & tf_warning)
- && c_inhibit_evaluation_warnings == 0
- && !TREE_NO_WARNING (op0))
-   warning (OPT_Waddress, "the address of %qD will never be NULL",
-TREE_OPERAND (op0, 0));
-   }
-
- if (CONVERT_EXPR_P (op0)
- && TREE_CODE (TREE_TYPE (TREE_OPERAND (op0, 0)))
-== REFERENCE_TYPE)
-   {
- tree inner_op0 = op0;
- STRIP_NOPS (inner_op0);
-
- if ((complain & tf_warning)
- && c_inhibit_evaluation_warnings == 0
- && !TREE_NO_WARNING (op0)
- && DECL_P (inner_op0))
-   warning_at (location, OPT_Waddress,
-   "the compiler can assume that the address of "
-   "%qD will never be NULL",
-   inner_op0);
-   }
+ warn_for_null_address (location, op0, complain);
}
   else if (((code1 == POINTER_TYPE || TYPE_PTRDATAMEM_P (type1))
&& null_ptr_cst_p (op0))
@@ -4559,32 +4566,7 @@ cp_build_binary_op (location_t location,
  else
result_type = type1;
 
- if (TREE_CODE (op1) == ADDR_EXPR 
- && decl_with_nonnull_addr_p (TREE_OPERAND (op1, 0)))
-   {
- if ((complain & tf_warning)
- && c_inhibit_evaluation_warnings == 0
- && !TREE_NO_WARNING (op1))
-   warning (OPT_Waddress, "the address of %qD will never be NULL",
-TREE_OPERAND (op1, 0));
-   }
-
- if (CONVERT_EXPR_P (op1)
- && TREE_CODE (TREE_TYPE (TREE_OPERAND (op1, 0)))
-== REFERENCE_TYPE)
-   {
- tree inner_op1 = op1;
- STRIP_NOPS (inner_op1);
-
- if ((complain & tf_warning)
- && c_inhibit_evaluation_warnings == 0
- && !TREE_NO_WARNING (op1)
- && DECL_P (inner_op1))
-   warning_at (location, OPT_Waddress,
-   "the compiler can assume that the address of "
-   "%qD will never be NULL",
-   inner_op1);
-   }
+ warn_for_null_address (location, op1, complain);
}
   else if ((code0 == POINTER_TYPE && code1 == POINTER_TYPE)
   || (TYPE_PTRDATAMEM_P (type0) && TYPE_PTRDATAMEM_P (type1)))
diff --git gcc/testsuite/g++.dg/warn/constexpr-70194.C 
gcc/testsuite/g++.dg/warn/constexpr-70194.C
index e69de29..cdc56c0 100644
--- gcc/testsuite/g++.dg/warn/constexpr-70194.C
+++ gcc/testsuite/g++.dg/warn/constexpr-70194.C
@@ -0,0 +1,12 @@
+// PR c++/70194
+// { dg-do compile { target c++11 } }
+// { dg-options 

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

[Patrick Palka]

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

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

[Martin Sebor]

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

Martin

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

[Jason Merrill]

Let's factor out that duplicated code into a separate function.

Jason

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

[Marek Polacek]

On Tue, Mar 15, 2016 at 11:56:18AM +0100, Jakub Jelinek wrote:
> From compile time perspective, I wonder if it wouldn't be better to do
> the cheap tests early, like:
>   if (warn_address
>   && (complain & tf_warning)
>   && c_inhibit_evaluation_warnings == 0
>   && !TREE_NO_WARNING (op0))
> {
>   tree cop0 = fold_non_dependent_expr (op0);
> 
>   if (TREE_CODE (cop0) == ADDR_EXPR
>   && decl_with_nonnull_addr_p (TREE_OPERAND (cop0, 0))
>   && !TREE_NO_WARNING (cop0))
> warning (OPT_waddress, "the address of %qD will never be NULL",
>  TREE_OPERAND (cop0, 0));
> }
> thus perform fold_non_dependent_expr only if it is needed.

Ok, makes sense.

> Furthermore, I wonder if it isn't preferrable to %qD the non-folded
> expression (if it is ADDR_EXPR, that is), so perhaps:
> TREE_OPERAND (TREE_CODE (op0) == ADDR_EXPR ? op0 : cop0, 0)
> ?

I tried this before but it gave the same output as with what I have now,
so I left this unchanged in this version...

Thanks.

Bootstrapped/regtested on x86_64-linux, ok for trunk?

2016-03-15  Marek Polacek  

PR c++/70194
* typeck.c (cp_build_binary_op): Call fold_non_dependent_expr before
warning about an address not being null.  Check cheap stuff first.

* g++.dg/warn/constexpr-70194.C: New test.

diff --git gcc/cp/typeck.c gcc/cp/typeck.c
index 20f0afc..5069e88 100644
--- gcc/cp/typeck.c
+++ gcc/cp/typeck.c
@@ -4520,14 +4520,18 @@ cp_build_binary_op (location_t location,
  else
result_type = type0;
 
- if (TREE_CODE (op0) == ADDR_EXPR
- && decl_with_nonnull_addr_p (TREE_OPERAND (op0, 0)))
+ if (warn_address
+ && (complain & tf_warning)
+ && c_inhibit_evaluation_warnings == 0
+ && !TREE_NO_WARNING (op0))
{
- if ((complain & tf_warning)
- && c_inhibit_evaluation_warnings == 0
- && !TREE_NO_WARNING (op0))
+ tree cop0 = fold_non_dependent_expr (op0);
+
+ if (TREE_CODE (cop0) == ADDR_EXPR
+ && decl_with_nonnull_addr_p (TREE_OPERAND (cop0, 0))
+ && !TREE_NO_WARNING (cop0))
warning (OPT_Waddress, "the address of %qD will never be NULL",
-TREE_OPERAND (op0, 0));
+TREE_OPERAND (cop0, 0));
}
 
  if (CONVERT_EXPR_P (op0)
@@ -4559,14 +4563,18 @@ cp_build_binary_op (location_t location,
  else
result_type = type1;
 
- if (TREE_CODE (op1) == ADDR_EXPR 
- && decl_with_nonnull_addr_p (TREE_OPERAND (op1, 0)))
+ if (warn_address
+ && (complain & tf_warning)
+ && c_inhibit_evaluation_warnings == 0
+ && !TREE_NO_WARNING (op1))
{
- if ((complain & tf_warning)
- && c_inhibit_evaluation_warnings == 0
- && !TREE_NO_WARNING (op1))
+ tree cop1 = fold_non_dependent_expr (op1);
+
+ if (TREE_CODE (cop1) == ADDR_EXPR
+ && decl_with_nonnull_addr_p (TREE_OPERAND (cop1, 0))
+ && !TREE_NO_WARNING (cop1))
warning (OPT_Waddress, "the address of %qD will never be NULL",
-TREE_OPERAND (op1, 0));
+TREE_OPERAND (cop1, 0));
}
 
  if (CONVERT_EXPR_P (op1)
diff --git gcc/testsuite/g++.dg/warn/constexpr-70194.C 
gcc/testsuite/g++.dg/warn/constexpr-70194.C
index e69de29..cdc56c0 100644
--- gcc/testsuite/g++.dg/warn/constexpr-70194.C
+++ gcc/testsuite/g++.dg/warn/constexpr-70194.C
@@ -0,0 +1,12 @@
+// PR c++/70194
+// { dg-do compile { target c++11 } }
+// { dg-options "-Wall" }
+
+int i;
+
+const bool b0 =  == 0; // { dg-warning "the address of .i. will never be 
NULL" }
+constexpr int *p = 
+const bool b1 = p == 0; // { dg-warning "the address of .i. will never be 
NULL" }
+const bool b2 = 0 == p; // { dg-warning "the address of .i. will never be 
NULL" }
+const bool b3 = p != 0; // { dg-warning "the address of .i. will never be 
NULL" }
+const bool b4 = 0 != p; // { dg-warning "the address of .i. will never be 
NULL" }

Marek

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

[Jakub Jelinek]

On Tue, Mar 15, 2016 at 11:41:20AM +0100, Marek Polacek wrote:
> This is to fix missing "address of %qD will never be NULL" warning that went
> away since the delayed folding merge.  The problem was that cp_build_binary_op
> was getting unfolded ops so in the constexpr case it saw "(int *) p" instead 
> of
> "" (in this particular testcase).  Fixed by calling fold_non_dependent_expr
> as is done elsewhere.
> (It doesn't seem like the "if (CONVERT_EXPR_P (op?)" blocks need to use cop?
> too.)
> 
> I did not try to address the other issues Martin has raised in the PR yet.
> 
> Bootstrapped/regtested on x86_64-linux, ok for trunk?
> 
> 2016-03-15  Marek Polacek  
> 
>   PR c++/70194
>   * typeck.c (cp_build_binary_op): Call fold_non_dependent_expr before
>   warning about an address not being null.
> 
>   * g++.dg/warn/constexpr-70194.C: New test.
> 
> diff --git gcc/cp/typeck.c gcc/cp/typeck.c
> index 20f0afc..a789c7a 100644
> --- gcc/cp/typeck.c
> +++ gcc/cp/typeck.c
> @@ -4520,14 +4520,16 @@ cp_build_binary_op (location_t location,
> else
>   result_type = type0;
>  
> -   if (TREE_CODE (op0) == ADDR_EXPR
> -   && decl_with_nonnull_addr_p (TREE_OPERAND (op0, 0)))
> +   tree cop0 = fold_non_dependent_expr (op0);
> +
> +   if (TREE_CODE (cop0) == ADDR_EXPR
> +   && decl_with_nonnull_addr_p (TREE_OPERAND (cop0, 0)))

>From compile time perspective, I wonder if it wouldn't be better to do
the cheap tests early, like:
if (warn_address
&& (complain & tf_warning)
&& c_inhibit_evaluation_warnings == 0
&& !TREE_NO_WARNING (op0))
  {
tree cop0 = fold_non_dependent_expr (op0);

if (TREE_CODE (cop0) == ADDR_EXPR
&& decl_with_nonnull_addr_p (TREE_OPERAND (cop0, 0))
&& !TREE_NO_WARNING (cop0))
  warning (OPT_waddress, "the address of %qD will never be NULL",
   TREE_OPERAND (cop0, 0));
  }

thus perform fold_non_dependent_expr only if it is needed.
Furthermore, I wonder if it isn't preferrable to %qD the non-folded
expression (if it is ADDR_EXPR, that is), so perhaps:
TREE_OPERAND (TREE_CODE (op0) == ADDR_EXPR ? op0 : cop0, 0)
?

Jakub

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

[Marek Polacek]

This is to fix missing "address of %qD will never be NULL" warning that went
away since the delayed folding merge.  The problem was that cp_build_binary_op
was getting unfolded ops so in the constexpr case it saw "(int *) p" instead of
"" (in this particular testcase).  Fixed by calling fold_non_dependent_expr
as is done elsewhere.
(It doesn't seem like the "if (CONVERT_EXPR_P (op?)" blocks need to use cop?
too.)

I did not try to address the other issues Martin has raised in the PR yet.

Bootstrapped/regtested on x86_64-linux, ok for trunk?

2016-03-15  Marek Polacek  

PR c++/70194
* typeck.c (cp_build_binary_op): Call fold_non_dependent_expr before
warning about an address not being null.

* g++.dg/warn/constexpr-70194.C: New test.

diff --git gcc/cp/typeck.c gcc/cp/typeck.c
index 20f0afc..a789c7a 100644
--- gcc/cp/typeck.c
+++ gcc/cp/typeck.c
@@ -4520,14 +4520,16 @@ cp_build_binary_op (location_t location,
  else
result_type = type0;
 
- if (TREE_CODE (op0) == ADDR_EXPR
- && decl_with_nonnull_addr_p (TREE_OPERAND (op0, 0)))
+ tree cop0 = fold_non_dependent_expr (op0);
+
+ if (TREE_CODE (cop0) == ADDR_EXPR
+ && decl_with_nonnull_addr_p (TREE_OPERAND (cop0, 0)))
{
  if ((complain & tf_warning)
  && c_inhibit_evaluation_warnings == 0
- && !TREE_NO_WARNING (op0))
+ && !TREE_NO_WARNING (cop0))
warning (OPT_Waddress, "the address of %qD will never be NULL",
-TREE_OPERAND (op0, 0));
+TREE_OPERAND (cop0, 0));
}
 
  if (CONVERT_EXPR_P (op0)
@@ -4559,14 +4561,16 @@ cp_build_binary_op (location_t location,
  else
result_type = type1;
 
- if (TREE_CODE (op1) == ADDR_EXPR 
- && decl_with_nonnull_addr_p (TREE_OPERAND (op1, 0)))
+ tree cop1 = fold_non_dependent_expr (op1);
+
+ if (TREE_CODE (cop1) == ADDR_EXPR
+ && decl_with_nonnull_addr_p (TREE_OPERAND (cop1, 0)))
{
  if ((complain & tf_warning)
  && c_inhibit_evaluation_warnings == 0
- && !TREE_NO_WARNING (op1))
+ && !TREE_NO_WARNING (cop1))
warning (OPT_Waddress, "the address of %qD will never be NULL",
-TREE_OPERAND (op1, 0));
+TREE_OPERAND (cop1, 0));
}
 
  if (CONVERT_EXPR_P (op1)
diff --git gcc/testsuite/g++.dg/warn/constexpr-70194.C 
gcc/testsuite/g++.dg/warn/constexpr-70194.C
index e69de29..cdc56c0 100644
--- gcc/testsuite/g++.dg/warn/constexpr-70194.C
+++ gcc/testsuite/g++.dg/warn/constexpr-70194.C
@@ -0,0 +1,12 @@
+// PR c++/70194
+// { dg-do compile { target c++11 } }
+// { dg-options "-Wall" }
+
+int i;
+
+const bool b0 =  == 0; // { dg-warning "the address of .i. will never be 
NULL" }
+constexpr int *p = 
+const bool b1 = p == 0; // { dg-warning "the address of .i. will never be 
NULL" }
+const bool b2 = 0 == p; // { dg-warning "the address of .i. will never be 
NULL" }
+const bool b3 = p != 0; // { dg-warning "the address of .i. will never be 
NULL" }
+const bool b4 = 0 != p; // { dg-warning "the address of .i. will never be 
NULL" }

Marek