[PATCH] avoid -Wnonnull on synthesized condition in static_cast (PR 96003)

[PATCH] avoid -Wnonnull on synthesized condition in static_cast (PR 96003)

[Martin Sebor]

[-- Attachment #1: Type: text/plain, Size: 1749 bytes --]

The recent enhancement to treat the implicit this pointer argument
as nonnull in member functions triggers spurious -Wnonnull for
the synthesized conditional expression the C++ front end replaces
the pointer with in some static_cast expressions.  The front end
already sets the no-warning bit for the test but not for the whole
conditional expression, so the attached fix extends the same solution
to it.

The consequence of this fix is that user-written code like this:

   static_cast<T*>(p ? p : 0)->f ();
or
   static_cast<T*>(p ? p : nullptr)->f ();

don't trigger the warning because they are both transformed into
the same expression as:

   static_cast<T*>(p)->f ();

What still does trigger it is this:

   static_cast<T*>(p ? p : (T*)0)->f ();

because here it's the inner COND_EXPR's no-warning bit that's set
(the outer one is clear), whereas in the former expressions it's
the other way around.  It would be nice if this worked consistently
but I didn't see an easy way to do that and more than a quick fix
seems outside the scope for this bug.

Another case reported by someone else in the same bug involves
a dynamic_cast.  A simplified test case goes something like this:

   if (dynamic_cast<T*>(p))
     dynamic_cast<T*>(p)->f ();

The root cause is the same: the front end emitting the COND_EXPR

   ((p != 0) ? ((T*)__dynamic_cast(p, (& _ZTI1B), (& _ZTI1C), 0)) : 0)

I decided not to suppress the warning in this case because doing
so would also suppress it in unconditional calls with the result
of the cast:

   dynamic_cast<T*>(p)->f ();

and that doesn't seem helpful.  Instead, I'd suggest to make
the second cast in the if statement to reference to T&:

   if (dynamic_cast<T*>(p))
     dynamic_cast<T&>(*p).f ();

Martin

[-- Attachment #2: gcc-96003.diff --]
[-- Type: text/x-patch, Size: 2349 bytes --]

PR c++/96003 spurious -Wnonnull calling a member on the result of static_cast

gcc/c-family/ChangeLog:

	PR c++/96003
	* c-common.c (check_function_arguments_recurse): Return early when
	no-warning bit is set.

gcc/cp/ChangeLog:

	PR c++/96003
	* class.c (build_base_path): Set no-warning bit on the synthesized
	conditional expression in static_cast.

gcc/testsuite/ChangeLog:

	PR c++/96003
	* g++.dg/warn/Wnonnull7.C: New test.

diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c
index 51ecde69f2d..7ab9966b7c0 100644
--- a/gcc/c-family/c-common.c
+++ b/gcc/c-family/c-common.c
@@ -5821,6 +5821,11 @@ check_function_arguments_recurse (void (*callback)
 				  void *ctx, tree param,
 				  unsigned HOST_WIDE_INT param_num)
 {
+  /* Avoid warning in synthesized expressions like C++ static_cast.
+     See PR 96003.  */
+  if (TREE_NO_WARNING (param))
+    return;
+
   if (CONVERT_EXPR_P (param)
       && (TYPE_PRECISION (TREE_TYPE (param))
 	  == TYPE_PRECISION (TREE_TYPE (TREE_OPERAND (param, 0)))))
diff --git a/gcc/cp/class.c b/gcc/cp/class.c
index 14380c7a08c..b460f8aefcd 100644
--- a/gcc/cp/class.c
+++ b/gcc/cp/class.c
@@ -516,8 +516,14 @@ build_base_path (enum tree_code code,
 
  out:
   if (null_test)
-    expr = fold_build3_loc (input_location, COND_EXPR, target_type, null_test, expr,
-			    build_zero_cst (target_type));
+    {
+      expr = fold_build3_loc (input_location, COND_EXPR, target_type, null_test,
+			      expr, build_zero_cst (target_type));
+      /* Avoid warning for the whole conditional expression (in addition
+	 to NULL_TEST itself -- see above) in case the result is used in
+	 a nonnull context that the front end -Wnonnull checks.  */
+      TREE_NO_WARNING (expr) = 1;
+    }
 
   return expr;
 }
diff --git a/gcc/testsuite/g++.dg/warn/Wnonnull7.C b/gcc/testsuite/g++.dg/warn/Wnonnull7.C
new file mode 100644
index 00000000000..b7a64c83436
--- /dev/null
+++ b/gcc/testsuite/g++.dg/warn/Wnonnull7.C
@@ -0,0 +1,25 @@
+/* PR c++/96003 - spurious -Wnonnull calling a member on the result
+   of static_cast
+   { dg-do compile }
+   { dg-options "-Wall" } */
+
+struct D;
+struct B
+{
+  B* next;
+  D* Next ();
+};
+
+struct D: B
+{
+  virtual ~D ();
+};
+
+struct Iterator
+{
+  D* p;
+  void advance ()
+  {
+    p = static_cast<B*>(p)->Next ();    // { dg-bogus "\\\[-Wnonnull" }
+  }
+};

Re: [PATCH] avoid -Wnonnull on synthesized condition in static_cast (PR 96003)

[Martin Sebor]

[-- Attachment #1: Type: text/plain, Size: 2592 bytes --]

Another test case added to the bug since I posted the patch shows
that the no-warning bit set by the C++ front end is easily lost
during early folding, triggering the -Wnonull again despite
the suppression.  The attached revision tweaks the folder in just
this one case to let the suppression take effect in this situation.

In light of how pervasive this potential for stripping looks (none
of the other folders preserves the bit) the tweak feels like a band-
aid rather than a general solution.  But implementing something
better (and mainly developing tests to verify that it works in
general) would probably be quite the project.  So I leave it for
some other time.

Martin


On 7/17/20 1:00 PM, Martin Sebor wrote:
> The recent enhancement to treat the implicit this pointer argument
> as nonnull in member functions triggers spurious -Wnonnull for
> the synthesized conditional expression the C++ front end replaces
> the pointer with in some static_cast expressions.  The front end
> already sets the no-warning bit for the test but not for the whole
> conditional expression, so the attached fix extends the same solution
> to it.
> 
> The consequence of this fix is that user-written code like this:
> 
>    static_cast<T*>(p ? p : 0)->f ();
> or
>    static_cast<T*>(p ? p : nullptr)->f ();
> 
> don't trigger the warning because they are both transformed into
> the same expression as:
> 
>    static_cast<T*>(p)->f ();
> 
> What still does trigger it is this:
> 
>    static_cast<T*>(p ? p : (T*)0)->f ();
> 
> because here it's the inner COND_EXPR's no-warning bit that's set
> (the outer one is clear), whereas in the former expressions it's
> the other way around.  It would be nice if this worked consistently
> but I didn't see an easy way to do that and more than a quick fix
> seems outside the scope for this bug.
> 
> Another case reported by someone else in the same bug involves
> a dynamic_cast.  A simplified test case goes something like this:
> 
>    if (dynamic_cast<T*>(p))
>      dynamic_cast<T*>(p)->f ();
> 
> The root cause is the same: the front end emitting the COND_EXPR
> 
>    ((p != 0) ? ((T*)__dynamic_cast(p, (& _ZTI1B), (& _ZTI1C), 0)) : 0)
> 
> I decided not to suppress the warning in this case because doing
> so would also suppress it in unconditional calls with the result
> of the cast:
> 
>    dynamic_cast<T*>(p)->f ();
> 
> and that doesn't seem helpful.  Instead, I'd suggest to make
> the second cast in the if statement to reference to T&:
> 
>    if (dynamic_cast<T*>(p))
>      dynamic_cast<T&>(*p).f ();
> 
> Martin


[-- Attachment #2: gcc-96003.diff --]
[-- Type: text/x-patch, Size: 3375 bytes --]

PR c++/96003 spurious -Wnonnull calling a member on the result of static_cast

gcc/c-family/ChangeLog:

	PR c++/96003
	* c-common.c (check_function_arguments_recurse): Return early when
	no-warning bit is set.

gcc/cp/ChangeLog:

	PR c++/96003
	* class.c (build_base_path): Set no-warning bit on the synthesized
	conditional expression in static_cast.

gcc/ChangeLog:
	fold-const.c (fold_unary_loc): Set no-warning on a rebuilt COND_EXPR
	if the original had it set.

gcc/testsuite/ChangeLog:

	PR c++/96003
	* g++.dg/warn/Wnonnull7.C: New test.

diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c
index 51ecde69f2d..8a2d641f17c 100644
--- a/gcc/c-family/c-common.c
+++ b/gcc/c-family/c-common.c
@@ -5821,6 +5821,9 @@ check_function_arguments_recurse (void (*callback)
 				  void *ctx, tree param,
 				  unsigned HOST_WIDE_INT param_num)
 {
+  if (TREE_NO_WARNING (param))
+    return;
+
   if (CONVERT_EXPR_P (param)
       && (TYPE_PRECISION (TREE_TYPE (param))
 	  == TYPE_PRECISION (TREE_TYPE (TREE_OPERAND (param, 0)))))
diff --git a/gcc/cp/class.c b/gcc/cp/class.c
index a3913f4ce0b..e024e8a4a74 100644
--- a/gcc/cp/class.c
+++ b/gcc/cp/class.c
@@ -516,8 +516,14 @@ build_base_path (enum tree_code code,
 
  out:
   if (null_test)
-    expr = fold_build3_loc (input_location, COND_EXPR, target_type, null_test, expr,
-			    build_zero_cst (target_type));
+    {
+      expr = fold_build3_loc (input_location, COND_EXPR, target_type, null_test,
+			      expr, build_zero_cst (target_type));
+      /* Avoid warning for the whole conditional expression (in addition
+	 to NULL_TEST itself -- see above) in case the result is used in
+	 a nonnull context that the front end -Wnonnull checks.  */
+      TREE_NO_WARNING (expr) = 1;
+    }
 
   return expr;
 }
diff --git a/gcc/fold-const.c b/gcc/fold-const.c
index 300d959278b..67153e3f484 100644
--- a/gcc/fold-const.c
+++ b/gcc/fold-const.c
@@ -8754,6 +8754,7 @@ fold_unary_loc (location_t loc, enum tree_code code, tree type, tree op0)
 	    arg02 = fold_build1_loc (loc, code, type,
 				 fold_convert_loc (loc,
 						   TREE_TYPE (op0), arg02));
+	  bool nowarn = TREE_NO_WARNING (op0);
 	  tem = fold_build3_loc (loc, COND_EXPR, type, TREE_OPERAND (arg0, 0),
 			     arg01, arg02);
 
@@ -8788,6 +8789,8 @@ fold_unary_loc (location_t loc, enum tree_code code, tree type, tree op0)
 				      TREE_OPERAND (TREE_OPERAND (tem, 1), 0),
 				      TREE_OPERAND (TREE_OPERAND (tem, 2),
 						    0)));
+	  if (nowarn)
+	    TREE_NO_WARNING (tem) = true;
 	  return tem;
 	}
    }
diff --git a/gcc/testsuite/g++.dg/warn/Wnonnull7.C b/gcc/testsuite/g++.dg/warn/Wnonnull7.C
new file mode 100644
index 00000000000..7611c18d948
--- /dev/null
+++ b/gcc/testsuite/g++.dg/warn/Wnonnull7.C
@@ -0,0 +1,36 @@
+/* PR c++/96003 - spurious -Wnonnull calling a member on the result
+   of static_cast
+   { dg-do compile }
+   { dg-options "-Wall" } */
+
+struct D;
+struct B
+{
+  B* next;
+  D* Next ();
+};
+
+struct D: B
+{
+  virtual ~D ();
+};
+
+struct Iterator
+{
+  D* p;
+  void advance ()
+  {
+    p = static_cast<B*>(p)->Next ();    // { dg-bogus "\\\[-Wnonnull" }
+  }
+};
+
+// Test case from comment #11.
+
+struct S1 { virtual ~S1 (); };
+struct S2 { virtual ~S2 (); };
+struct S3: S1, S2 { void f (); };
+
+void f (S2 *p)
+{
+  static_cast<S3 *>(p)->f ();           // { dg-bogus "\\\[-Wnonnull" }
+}

Re: [PATCH] avoid -Wnonnull on synthesized condition in static_cast (PR 96003)

[Jason Merrill]

On 7/23/20 3:08 PM, Martin Sebor wrote:
> Another test case added to the bug since I posted the patch shows
> that the no-warning bit set by the C++ front end is easily lost
> during early folding, triggering the -Wnonull again despite
> the suppression.  The attached revision tweaks the folder in just
> this one case to let the suppression take effect in this situation.
> 
> In light of how pervasive this potential for stripping looks (none
> of the other folders preserves the bit) the tweak feels like a band-
> aid rather than a general solution.  But implementing something
> better (and mainly developing tests to verify that it works in
> general) would probably be quite the project.  So I leave it for
> some other time.

How about in check_function_arguments_recurse COND_EXPR handling, 
checking for TREE_NO_WARNING on the condition?  Then we wouldn't need to 
deal with setting and copying it on the COND_EXPR itself.

> 
> On 7/17/20 1:00 PM, Martin Sebor wrote:
>> The recent enhancement to treat the implicit this pointer argument
>> as nonnull in member functions triggers spurious -Wnonnull for
>> the synthesized conditional expression the C++ front end replaces
>> the pointer with in some static_cast expressions.  The front end
>> already sets the no-warning bit for the test but not for the whole
>> conditional expression, so the attached fix extends the same solution
>> to it.
>>
>> The consequence of this fix is that user-written code like this:
>>
>>    static_cast<T*>(p ? p : 0)->f ();
>> or
>>    static_cast<T*>(p ? p : nullptr)->f ();
>>
>> don't trigger the warning because they are both transformed into
>> the same expression as:
>>
>>    static_cast<T*>(p)->f ();
>>
>> What still does trigger it is this:
>>
>>    static_cast<T*>(p ? p : (T*)0)->f ();
>>
>> because here it's the inner COND_EXPR's no-warning bit that's set
>> (the outer one is clear), whereas in the former expressions it's
>> the other way around.  It would be nice if this worked consistently
>> but I didn't see an easy way to do that and more than a quick fix
>> seems outside the scope for this bug.
>>
>> Another case reported by someone else in the same bug involves
>> a dynamic_cast.  A simplified test case goes something like this:
>>
>>    if (dynamic_cast<T*>(p))
>>      dynamic_cast<T*>(p)->f ();
>>
>> The root cause is the same: the front end emitting the COND_EXPR
>>
>>    ((p != 0) ? ((T*)__dynamic_cast(p, (& _ZTI1B), (& _ZTI1C), 0)) : 0)
>>
>> I decided not to suppress the warning in this case because doing
>> so would also suppress it in unconditional calls with the result
>> of the cast:
>>
>>    dynamic_cast<T*>(p)->f ();
>>
>> and that doesn't seem helpful.  Instead, I'd suggest to make
>> the second cast in the if statement to reference to T&:
>>
>>    if (dynamic_cast<T*>(p))
>>      dynamic_cast<T&>(*p).f ();
>>
>> Martin

Re: [PATCH] avoid -Wnonnull on synthesized condition in static_cast (PR 96003)

[Martin Sebor]

[-- Attachment #1: Type: text/plain, Size: 3341 bytes --]

On 7/29/20 2:27 PM, Jason Merrill wrote:
> On 7/23/20 3:08 PM, Martin Sebor wrote:
>> Another test case added to the bug since I posted the patch shows
>> that the no-warning bit set by the C++ front end is easily lost
>> during early folding, triggering the -Wnonull again despite
>> the suppression.  The attached revision tweaks the folder in just
>> this one case to let the suppression take effect in this situation.
>>
>> In light of how pervasive this potential for stripping looks (none
>> of the other folders preserves the bit) the tweak feels like a band-
>> aid rather than a general solution.  But implementing something
>> better (and mainly developing tests to verify that it works in
>> general) would probably be quite the project.  So I leave it for
>> some other time.
> 
> How about in check_function_arguments_recurse COND_EXPR handling, 
> checking for TREE_NO_WARNING on the condition?  Then we wouldn't need to 
> deal with setting and copying it on the COND_EXPR itself.

The condition is already checked at the beginning of the function.

But the latest trunk doesn't seem to need the fold-const.c change
anymore to suppress the warning and the original patch is good
enough.  The updated test should catch the regression if
the COND_EXPR should again lose the no warning bit.

Martin

> 
>>
>> On 7/17/20 1:00 PM, Martin Sebor wrote:
>>> The recent enhancement to treat the implicit this pointer argument
>>> as nonnull in member functions triggers spurious -Wnonnull for
>>> the synthesized conditional expression the C++ front end replaces
>>> the pointer with in some static_cast expressions.  The front end
>>> already sets the no-warning bit for the test but not for the whole
>>> conditional expression, so the attached fix extends the same solution
>>> to it.
>>>
>>> The consequence of this fix is that user-written code like this:
>>>
>>>    static_cast<T*>(p ? p : 0)->f ();
>>> or
>>>    static_cast<T*>(p ? p : nullptr)->f ();
>>>
>>> don't trigger the warning because they are both transformed into
>>> the same expression as:
>>>
>>>    static_cast<T*>(p)->f ();
>>>
>>> What still does trigger it is this:
>>>
>>>    static_cast<T*>(p ? p : (T*)0)->f ();
>>>
>>> because here it's the inner COND_EXPR's no-warning bit that's set
>>> (the outer one is clear), whereas in the former expressions it's
>>> the other way around.  It would be nice if this worked consistently
>>> but I didn't see an easy way to do that and more than a quick fix
>>> seems outside the scope for this bug.
>>>
>>> Another case reported by someone else in the same bug involves
>>> a dynamic_cast.  A simplified test case goes something like this:
>>>
>>>    if (dynamic_cast<T*>(p))
>>>      dynamic_cast<T*>(p)->f ();
>>>
>>> The root cause is the same: the front end emitting the COND_EXPR
>>>
>>>    ((p != 0) ? ((T*)__dynamic_cast(p, (& _ZTI1B), (& _ZTI1C), 0)) : 0)
>>>
>>> I decided not to suppress the warning in this case because doing
>>> so would also suppress it in unconditional calls with the result
>>> of the cast:
>>>
>>>    dynamic_cast<T*>(p)->f ();
>>>
>>> and that doesn't seem helpful.  Instead, I'd suggest to make
>>> the second cast in the if statement to reference to T&:
>>>
>>>    if (dynamic_cast<T*>(p))
>>>      dynamic_cast<T&>(*p).f ();
>>>
>>> Martin
> 
> 


[-- Attachment #2: gcc-96003.diff --]
[-- Type: text/x-patch, Size: 2487 bytes --]

PR c++/96003 spurious -Wnonnull calling a member on the result of static_cast

gcc/c-family/ChangeLog:

	PR c++/96003
	* c-common.c (check_function_arguments_recurse): Return early when
	no-warning bit is set.

gcc/cp/ChangeLog:

	PR c++/96003
	* class.c (build_base_path): Set no-warning bit on the synthesized
	conditional expression in static_cast.

gcc/testsuite/ChangeLog:

	PR c++/96003
	* g++.dg/warn/Wnonnull7.C: New test.

diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c
index 116867a4513..72935b274ec 100644
--- a/gcc/c-family/c-common.c
+++ b/gcc/c-family/c-common.c
@@ -5821,6 +5821,9 @@ check_function_arguments_recurse (void (*callback)
 				  void *ctx, tree param,
 				  unsigned HOST_WIDE_INT param_num)
 {
+  if (TREE_NO_WARNING (param))
+    return;
+
   if (CONVERT_EXPR_P (param)
       && (TYPE_PRECISION (TREE_TYPE (param))
 	  == TYPE_PRECISION (TREE_TYPE (TREE_OPERAND (param, 0)))))
diff --git a/gcc/cp/class.c b/gcc/cp/class.c
index 7a25d8fc76c..b39bdaaa3ab 100644
--- a/gcc/cp/class.c
+++ b/gcc/cp/class.c
@@ -516,8 +516,14 @@ build_base_path (enum tree_code code,
 
  out:
   if (null_test)
-    expr = fold_build3_loc (input_location, COND_EXPR, target_type, null_test, expr,
-			    build_zero_cst (target_type));
+    {
+      expr = fold_build3_loc (input_location, COND_EXPR, target_type, null_test,
+			      expr, build_zero_cst (target_type));
+      /* Avoid warning for the whole conditional expression (in addition
+	 to NULL_TEST itself -- see above) in case the result is used in
+	 a nonnull context that the front end -Wnonnull checks.  */
+      TREE_NO_WARNING (expr) = 1;
+    }
 
   return expr;
 }
diff --git a/gcc/testsuite/g++.dg/warn/Wnonnull7.C b/gcc/testsuite/g++.dg/warn/Wnonnull7.C
new file mode 100644
index 00000000000..7611c18d948
--- /dev/null
+++ b/gcc/testsuite/g++.dg/warn/Wnonnull7.C
@@ -0,0 +1,36 @@
+/* PR c++/96003 - spurious -Wnonnull calling a member on the result
+   of static_cast
+   { dg-do compile }
+   { dg-options "-Wall" } */
+
+struct D;
+struct B
+{
+  B* next;
+  D* Next ();
+};
+
+struct D: B
+{
+  virtual ~D ();
+};
+
+struct Iterator
+{
+  D* p;
+  void advance ()
+  {
+    p = static_cast<B*>(p)->Next ();    // { dg-bogus "\\\[-Wnonnull" }
+  }
+};
+
+// Test case from comment #11.
+
+struct S1 { virtual ~S1 (); };
+struct S2 { virtual ~S2 (); };
+struct S3: S1, S2 { void f (); };
+
+void f (S2 *p)
+{
+  static_cast<S3 *>(p)->f ();           // { dg-bogus "\\\[-Wnonnull" }
+}

Re: [PATCH] avoid -Wnonnull on synthesized condition in static_cast (PR 96003)

[Jason Merrill]

On 7/30/20 12:25 PM, Martin Sebor wrote:
> On 7/29/20 2:27 PM, Jason Merrill wrote:
>> On 7/23/20 3:08 PM, Martin Sebor wrote:
>>> Another test case added to the bug since I posted the patch shows
>>> that the no-warning bit set by the C++ front end is easily lost
>>> during early folding, triggering the -Wnonull again despite
>>> the suppression.  The attached revision tweaks the folder in just
>>> this one case to let the suppression take effect in this situation.
>>>
>>> In light of how pervasive this potential for stripping looks (none
>>> of the other folders preserves the bit) the tweak feels like a band-
>>> aid rather than a general solution.  But implementing something
>>> better (and mainly developing tests to verify that it works in
>>> general) would probably be quite the project.  So I leave it for
>>> some other time.
>>
>> How about in check_function_arguments_recurse COND_EXPR handling, 
>> checking for TREE_NO_WARNING on the condition?  Then we wouldn't need 
>> to deal with setting and copying it on the COND_EXPR itself.
> 
> The condition is already checked at the beginning of the function.

I mean:

>    if (TREE_CODE (param) == COND_EXPR)
>      {
> +      if (TREE_NO_WARNING (TREE_OPERAND (param, 0)))
> +       return;
> +
>        /* Simplify to avoid warning for an impossible case.  */

But your patch is OK as is.

> But the latest trunk doesn't seem to need the fold-const.c change
> anymore to suppress the warning and the original patch is good
> enough.  The updated test should catch the regression if
> the COND_EXPR should again lose the no warning bit.
> 
> Martin
> 
>>
>>>
>>> On 7/17/20 1:00 PM, Martin Sebor wrote:
>>>> The recent enhancement to treat the implicit this pointer argument
>>>> as nonnull in member functions triggers spurious -Wnonnull for
>>>> the synthesized conditional expression the C++ front end replaces
>>>> the pointer with in some static_cast expressions.  The front end
>>>> already sets the no-warning bit for the test but not for the whole
>>>> conditional expression, so the attached fix extends the same solution
>>>> to it.
>>>>
>>>> The consequence of this fix is that user-written code like this:
>>>>
>>>>    static_cast<T*>(p ? p : 0)->f ();
>>>> or
>>>>    static_cast<T*>(p ? p : nullptr)->f ();
>>>>
>>>> don't trigger the warning because they are both transformed into
>>>> the same expression as:
>>>>
>>>>    static_cast<T*>(p)->f ();
>>>>
>>>> What still does trigger it is this:
>>>>
>>>>    static_cast<T*>(p ? p : (T*)0)->f ();
>>>>
>>>> because here it's the inner COND_EXPR's no-warning bit that's set
>>>> (the outer one is clear), whereas in the former expressions it's
>>>> the other way around.  It would be nice if this worked consistently
>>>> but I didn't see an easy way to do that and more than a quick fix
>>>> seems outside the scope for this bug.
>>>>
>>>> Another case reported by someone else in the same bug involves
>>>> a dynamic_cast.  A simplified test case goes something like this:
>>>>
>>>>    if (dynamic_cast<T*>(p))
>>>>      dynamic_cast<T*>(p)->f ();
>>>>
>>>> The root cause is the same: the front end emitting the COND_EXPR
>>>>
>>>>    ((p != 0) ? ((T*)__dynamic_cast(p, (& _ZTI1B), (& _ZTI1C), 0)) : 0)
>>>>
>>>> I decided not to suppress the warning in this case because doing
>>>> so would also suppress it in unconditional calls with the result
>>>> of the cast:
>>>>
>>>>    dynamic_cast<T*>(p)->f ();
>>>>
>>>> and that doesn't seem helpful.  Instead, I'd suggest to make
>>>> the second cast in the if statement to reference to T&:
>>>>
>>>>    if (dynamic_cast<T*>(p))
>>>>      dynamic_cast<T&>(*p).f ();
>>>>
>>>> Martin
>>
>>
> 

Re: [PATCH] avoid -Wnonnull on synthesized condition in static_cast (PR 96003)

[Martin Sebor]

On 7/30/20 1:47 PM, Jason Merrill wrote:
> On 7/30/20 12:25 PM, Martin Sebor wrote:
>> On 7/29/20 2:27 PM, Jason Merrill wrote:
>>> On 7/23/20 3:08 PM, Martin Sebor wrote:
>>>> Another test case added to the bug since I posted the patch shows
>>>> that the no-warning bit set by the C++ front end is easily lost
>>>> during early folding, triggering the -Wnonull again despite
>>>> the suppression.  The attached revision tweaks the folder in just
>>>> this one case to let the suppression take effect in this situation.
>>>>
>>>> In light of how pervasive this potential for stripping looks (none
>>>> of the other folders preserves the bit) the tweak feels like a band-
>>>> aid rather than a general solution.  But implementing something
>>>> better (and mainly developing tests to verify that it works in
>>>> general) would probably be quite the project.  So I leave it for
>>>> some other time.
>>>
>>> How about in check_function_arguments_recurse COND_EXPR handling, 
>>> checking for TREE_NO_WARNING on the condition?  Then we wouldn't need 
>>> to deal with setting and copying it on the COND_EXPR itself.
>>
>> The condition is already checked at the beginning of the function.
> 
> I mean:
> 
>>    if (TREE_CODE (param) == COND_EXPR)
>>      {
>> +      if (TREE_NO_WARNING (TREE_OPERAND (param, 0)))
>> +       return;
>> +
>>        /* Simplify to avoid warning for an impossible case.  */
> 
> But your patch is OK as is.

It only occurred to me after I replied that that's what you meant.
I had considered it but using on the no-warning bit on one expression
to decide whether to warn for another seemed a bit fragile.  I checked
in the original and final patch in r11-2457.

Martin

> 
>> But the latest trunk doesn't seem to need the fold-const.c change
>> anymore to suppress the warning and the original patch is good
>> enough.  The updated test should catch the regression if
>> the COND_EXPR should again lose the no warning bit.
>>
>> Martin
>>
>>>
>>>>
>>>> On 7/17/20 1:00 PM, Martin Sebor wrote:
>>>>> The recent enhancement to treat the implicit this pointer argument
>>>>> as nonnull in member functions triggers spurious -Wnonnull for
>>>>> the synthesized conditional expression the C++ front end replaces
>>>>> the pointer with in some static_cast expressions.  The front end
>>>>> already sets the no-warning bit for the test but not for the whole
>>>>> conditional expression, so the attached fix extends the same solution
>>>>> to it.
>>>>>
>>>>> The consequence of this fix is that user-written code like this:
>>>>>
>>>>>    static_cast<T*>(p ? p : 0)->f ();
>>>>> or
>>>>>    static_cast<T*>(p ? p : nullptr)->f ();
>>>>>
>>>>> don't trigger the warning because they are both transformed into
>>>>> the same expression as:
>>>>>
>>>>>    static_cast<T*>(p)->f ();
>>>>>
>>>>> What still does trigger it is this:
>>>>>
>>>>>    static_cast<T*>(p ? p : (T*)0)->f ();
>>>>>
>>>>> because here it's the inner COND_EXPR's no-warning bit that's set
>>>>> (the outer one is clear), whereas in the former expressions it's
>>>>> the other way around.  It would be nice if this worked consistently
>>>>> but I didn't see an easy way to do that and more than a quick fix
>>>>> seems outside the scope for this bug.
>>>>>
>>>>> Another case reported by someone else in the same bug involves
>>>>> a dynamic_cast.  A simplified test case goes something like this:
>>>>>
>>>>>    if (dynamic_cast<T*>(p))
>>>>>      dynamic_cast<T*>(p)->f ();
>>>>>
>>>>> The root cause is the same: the front end emitting the COND_EXPR
>>>>>
>>>>>    ((p != 0) ? ((T*)__dynamic_cast(p, (& _ZTI1B), (& _ZTI1C), 0)) : 0)
>>>>>
>>>>> I decided not to suppress the warning in this case because doing
>>>>> so would also suppress it in unconditional calls with the result
>>>>> of the cast:
>>>>>
>>>>>    dynamic_cast<T*>(p)->f ();
>>>>>
>>>>> and that doesn't seem helpful.  Instead, I'd suggest to make
>>>>> the second cast in the if statement to reference to T&:
>>>>>
>>>>>    if (dynamic_cast<T*>(p))
>>>>>      dynamic_cast<T&>(*p).f ();
>>>>>
>>>>> Martin
>>>
>>>
>>
> 

Re: [PATCH] avoid -Wnonnull on synthesized condition in static_cast (PR 96003)

[Jeff Law]

On Fri, 2020-07-17 at 13:00 -0600, Martin Sebor via Gcc-patches wrote:
> The recent enhancement to treat the implicit this pointer argument
> as nonnull in member functions triggers spurious -Wnonnull for
> the synthesized conditional expression the C++ front end replaces
> the pointer with in some static_cast expressions.  The front end
> already sets the no-warning bit for the test but not for the whole
> conditional expression, so the attached fix extends the same solution
> to it.
> 
> The consequence of this fix is that user-written code like this:
> 
>    static_cast<T*>(p ? p : 0)->f ();
> or
>    static_cast<T*>(p ? p : nullptr)->f ();
> 
> don't trigger the warning because they are both transformed into
> the same expression as:
> 
>    static_cast<T*>(p)->f ();
> 
> What still does trigger it is this:
> 
>    static_cast<T*>(p ? p : (T*)0)->f ();
> 
> because here it's the inner COND_EXPR's no-warning bit that's set
> (the outer one is clear), whereas in the former expressions it's
> the other way around.  It would be nice if this worked consistently
> but I didn't see an easy way to do that and more than a quick fix
> seems outside the scope for this bug.
> 
> Another case reported by someone else in the same bug involves
> a dynamic_cast.  A simplified test case goes something like this:
> 
>    if (dynamic_cast<T*>(p))
>      dynamic_cast<T*>(p)->f ();
> 
> The root cause is the same: the front end emitting the COND_EXPR
> 
>    ((p != 0) ? ((T*)__dynamic_cast(p, (& _ZTI1B), (& _ZTI1C), 0)) : 0)
> 
> I decided not to suppress the warning in this case because doing
> so would also suppress it in unconditional calls with the result
> of the cast:
> 
>    dynamic_cast<T*>(p)->f ();
> 
> and that doesn't seem helpful.  Instead, I'd suggest to make
> the second cast in the if statement to reference to T&:
> 
>    if (dynamic_cast<T*>(p))
>      dynamic_cast<T&>(*p).f ();
Hmmm, I wonder if this would fix a handful of errors I got when doing the testing
of the Ranger work for Aldy.  Let me throw the new version into the tester and
respin just the failing packages. 

Jeff
>