avoid -Wnonnull for lambda stubs (PR c++/95984)

Message ID 2349884b-390a-730a-8da9-a3b06ffa695c@gmail.com
State New
Headers show
Series
  • avoid -Wnonnull for lambda stubs (PR c++/95984)
Related show

Commit Message

GT via Gcc-patches July 1, 2020, 7:31 p.m.
The attached patch avoids null pointer checking for the first
argument to calls to the member operator() of lambda objects
emitted by the C++ front end.  This avoids both the spurious
-Wnonnull warnings for such calls as well as the ICE reported
in the bug.

In addition, the patch also avoids function argument checking
for any calls when the tf_warning bit isn't set.  This isn't
strictly necessary to avoid the ICE but it seems like a good
precaution against something similar occurring in other checks
in the future(*).

Finally, while testing the fix I noticed that the common code
doesn't recognize nullptr as a poiner when processing templates.
I've extended the handling to let it handle it as well.

Martin

[*] It seems to me that a more robust solution to prevent
the diagnostic subsystem from being re-entered as a result
of callbacks into the front end would be to have the pretty
printer disable all warnings prior to the bcallbacks and
re-enable them afterwards.  That would require an efficient
and reliable way of controlling all warnings (as well as
querying their state), which I think would be a useful
feature to have in any case.  For one thing, it would make
handling warnings and responding to #pragma GCC diagnostics
more robust.

Comments

GT via Gcc-patches July 1, 2020, 9:25 p.m. | #1
On 7/1/20 3:31 PM, Martin Sebor wrote:
> The attached patch avoids null pointer checking for the first

> argument to calls to the member operator() of lambda objects

> emitted by the C++ front end.  This avoids both the spurious

> -Wnonnull warnings for such calls as well as the ICE reported

> in the bug.

> 

> In addition, the patch also avoids function argument checking

> for any calls when the tf_warning bit isn't set.  This isn't

> strictly necessary to avoid the ICE but it seems like a good

> precaution against something similar occurring in other checks

> in the future(*).

> 

> Finally, while testing the fix I noticed that the common code

> doesn't recognize nullptr as a poiner when processing templates.

> I've extended the handling to let it handle it as well.


Any possible value of nullptr_t is null, so I think ignoring it is 
appropriate.

> [*] It seems to me that a more robust solution to prevent

> the diagnostic subsystem from being re-entered as a result

> of callbacks into the front end would be to have the pretty

> printer disable all warnings prior to the bcallbacks and

> re-enable them afterwards.  That would require an efficient

> and reliable way of controlling all warnings (as well as

> querying their state), which I think would be a useful

> feature to have in any case.  For one thing, it would make

> handling warnings and responding to #pragma GCC diagnostics

> more robust.


> +	      const char *arg0str = IDENTIFIER_POINTER (arg0name);

> +	      closure = !strcmp (arg0str, "__closure");


Let's use id_equal here.

Jason
GT via Gcc-patches July 2, 2020, 3:21 p.m. | #2
On 7/1/20 3:25 PM, Jason Merrill wrote:
> On 7/1/20 3:31 PM, Martin Sebor wrote:

>> The attached patch avoids null pointer checking for the first

>> argument to calls to the member operator() of lambda objects

>> emitted by the C++ front end.  This avoids both the spurious

>> -Wnonnull warnings for such calls as well as the ICE reported

>> in the bug.

>>

>> In addition, the patch also avoids function argument checking

>> for any calls when the tf_warning bit isn't set.  This isn't

>> strictly necessary to avoid the ICE but it seems like a good

>> precaution against something similar occurring in other checks

>> in the future(*).

>>

>> Finally, while testing the fix I noticed that the common code

>> doesn't recognize nullptr as a poiner when processing templates.

>> I've extended the handling to let it handle it as well.

> 

> Any possible value of nullptr_t is null, so I think ignoring it is 

> appropriate.


In ordinary (including variadic) functions, GCC warns for all
null pointers, including nullptr.  In templates (and in auto
arguments), GCC warns for nullptr only when its converted to
a pointer type (e.g., (void*)nullptr) but not for naked nullptr.
As a user, I expect a warning for all null pointers in all these
contexts.  The distinction between the two kinds is too subtle
to appreciate by most of us (or to be useful in this context).
The test case below (I put it together to verify my patch was
working) shows the difference with lambdas:

   template <class L> void f (L f) {
     f ((void*)nullptr);   // -Wnonnull
   }

   template <class L> void g (L f) {
     f (nullptr);          // missing -Wnonnull
   }

   void h ()
   {
     f ([](auto...) __attribute__ ((nonnull)) { });
     g ([](auto...) __attribute__ ((nonnull)) { });
   }

> 

>> [*] It seems to me that a more robust solution to prevent

>> the diagnostic subsystem from being re-entered as a result

>> of callbacks into the front end would be to have the pretty

>> printer disable all warnings prior to the bcallbacks and

>> re-enable them afterwards.  That would require an efficient

>> and reliable way of controlling all warnings (as well as

>> querying their state), which I think would be a useful

>> feature to have in any case.  For one thing, it would make

>> handling warnings and responding to #pragma GCC diagnostics

>> more robust.

> 

>> +          const char *arg0str = IDENTIFIER_POINTER (arg0name);

>> +          closure = !strcmp (arg0str, "__closure");

> 

> Let's use id_equal here.


Done in the attached revision.

Martin
Exclude calls to variadic lambda stubs from -Wnonnull checking (PR c++/95984).

Resolves:
PR c++/95984 - Internal compiler error: Error reporting routines re-entered in -Wnonnull on a variadic lamnda
PR c++/96021 - missing -Wnonnull passing nullptr to a nonnull variadic lambda

gcc/c-family/ChangeLog:

	PR c++/95984
	* c-common.c (check_function_nonnull):
	(check_nonnull_arg):

gcc/cp/ChangeLog:

	PR c++/95984
	* call.c (build_over_call):

gcc/testsuite/ChangeLog:

	PR c++/95984
	* g++.dg/warn/Wnonnull6.C: New test.

diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c
index aae1ddb6b89..51ecde69f2d 100644
--- a/gcc/c-family/c-common.c
+++ b/gcc/c-family/c-common.c
@@ -5308,12 +5308,26 @@ check_function_nonnull (nonnull_arg_ctx &ctx, int nargs, tree *argarray)
   int firstarg = 0;
   if (TREE_CODE (ctx.fntype) == METHOD_TYPE)
     {
+      bool closure = false;
+      if (ctx.fndecl)
+	{
+	  /* For certain lambda expressions the C++ front end emits calls
+	     that pass a null this pointer as an argument named __closure
+	     to the member operator() of empty function.  Detect those
+	     and avoid checking them, but proceed to check the remaining
+	     arguments.  */
+	  tree arg0 = DECL_ARGUMENTS (ctx.fndecl);
+	  if (tree arg0name = DECL_NAME (arg0))
+	    closure = id_equal (arg0name, "__closure");
+	}
+
       /* In calls to C++ non-static member functions check the this
 	 pointer regardless of whether the function is declared with
 	 attribute nonnull.  */
       firstarg = 1;
-      check_function_arguments_recurse (check_nonnull_arg, &ctx, argarray[0],
-					firstarg);
+      if (!closure)
+	check_function_arguments_recurse (check_nonnull_arg, &ctx, argarray[0],
+					  firstarg);
     }
 
   tree attrs = lookup_attribute ("nonnull", TYPE_ATTRIBUTES (ctx.fntype));
@@ -5503,7 +5517,9 @@ check_nonnull_arg (void *ctx, tree param, unsigned HOST_WIDE_INT param_num)
      happen if the "nonnull" attribute was given without an operand
      list (which means to check every pointer argument).  */
 
-  if (TREE_CODE (TREE_TYPE (param)) != POINTER_TYPE)
+  tree paramtype = TREE_TYPE (param);
+  if (TREE_CODE (paramtype) != POINTER_TYPE
+      && TREE_CODE (paramtype) != NULLPTR_TYPE)
     return;
 
   /* Diagnose the simple cases of null arguments.  */
diff --git a/gcc/cp/call.c b/gcc/cp/call.c
index d8923be1d68..5341a572980 100644
--- a/gcc/cp/call.c
+++ b/gcc/cp/call.c
@@ -8842,15 +8842,16 @@ build_over_call (struct z_candidate *cand, int flags, tsubst_flags_t complain)
   gcc_assert (j <= nargs);
   nargs = j;
 
-  /* Avoid to do argument-transformation, if warnings for format, and for
-     nonnull are disabled.  Just in case that at least one of them is active
+  /* Avoid performing argument transformation if warnings are disabled.
+     When tf_warning is set and at least one of the warnings is active
      the check_function_arguments function might warn about something.  */
 
   bool warned_p = false;
-  if (warn_nonnull
-      || warn_format
-      || warn_suggest_attribute_format
-      || warn_restrict)
+  if ((complain & tf_warning)
+      && (warn_nonnull
+	  || warn_format
+	  || warn_suggest_attribute_format
+	  || warn_restrict))
     {
       tree *fargs = (!nargs ? argarray
 			    : (tree *) alloca (nargs * sizeof (tree)));
diff --git a/gcc/testsuite/g++.dg/warn/Wnonnull6.C b/gcc/testsuite/g++.dg/warn/Wnonnull6.C
new file mode 100644
index 00000000000..dae6dd2d912
--- /dev/null
+++ b/gcc/testsuite/g++.dg/warn/Wnonnull6.C
@@ -0,0 +1,37 @@
+/* PR c++/95984 - Internal compiler error: Error reporting routines re-entered
+   in -Wnonnull on a variadic lamnda
+   PR c++/missing -Wnonnull passing nullptr to a nonnull variadic lambda
+   { dg-do compile { target c++11 } }
+   { dg-options "-Wall" } */
+
+typedef int F (int);
+
+F* pr95984 ()
+{
+  // This also triggered the ICE.
+  return [](auto...) { return 0; };     // { dg-bogus "\\\[-Wnonnull" }
+}
+
+
+__attribute__ ((nonnull)) void f (int, ...);
+void ff ()
+{
+  f (1, nullptr);                       // { dg-warning "\\\[-Wnonnull" }
+}
+
+template <class T> void g (T t)
+{
+  t (1, nullptr);                       // { dg-warning "\\\[-Wnonnull" }
+}
+
+void gg (void)
+{
+  g ([](int, auto...) __attribute__ ((nonnull)) { });
+}
+
+template <class T> __attribute__ ((nonnull)) void h (T);
+
+void hh ()
+{
+  h (nullptr);                          //  { dg-warning "\\\[-Wnonnull" }
+}
GT via Gcc-patches July 3, 2020, 8:56 p.m. | #3
On 7/2/20 11:21 AM, Martin Sebor wrote:
> On 7/1/20 3:25 PM, Jason Merrill wrote:

>> On 7/1/20 3:31 PM, Martin Sebor wrote:

>>> The attached patch avoids null pointer checking for the first

>>> argument to calls to the member operator() of lambda objects

>>> emitted by the C++ front end.  This avoids both the spurious

>>> -Wnonnull warnings for such calls as well as the ICE reported

>>> in the bug.

>>>

>>> In addition, the patch also avoids function argument checking

>>> for any calls when the tf_warning bit isn't set.  This isn't

>>> strictly necessary to avoid the ICE but it seems like a good

>>> precaution against something similar occurring in other checks

>>> in the future(*).

>>>

>>> Finally, while testing the fix I noticed that the common code

>>> doesn't recognize nullptr as a poiner when processing templates.

>>> I've extended the handling to let it handle it as well.

>>

>> Any possible value of nullptr_t is null, so I think ignoring it is 

>> appropriate.

> 

> In ordinary (including variadic) functions, GCC warns for all

> null pointers, including nullptr.  In templates (and in auto

> arguments), GCC warns for nullptr only when its converted to

> a pointer type (e.g., (void*)nullptr) but not for naked nullptr.

> As a user, I expect a warning for all null pointers in all these

> contexts.  The distinction between the two kinds is too subtle

> to appreciate by most of us (or to be useful in this context).

> The test case below (I put it together to verify my patch was

> working) shows the difference with lambdas:

> 

>    template <class L> void f (L f) {

>      f ((void*)nullptr);   // -Wnonnull

>    }

> 

>    template <class L> void g (L f) {

>      f (nullptr);          // missing -Wnonnull

>    }

> 

>    void h ()

>    {

>      f ([](auto...) __attribute__ ((nonnull)) { });

>      g ([](auto...) __attribute__ ((nonnull)) { });

>    }

> 

>>

>>> [*] It seems to me that a more robust solution to prevent

>>> the diagnostic subsystem from being re-entered as a result

>>> of callbacks into the front end would be to have the pretty

>>> printer disable all warnings prior to the bcallbacks and

>>> re-enable them afterwards.  That would require an efficient

>>> and reliable way of controlling all warnings (as well as

>>> querying their state), which I think would be a useful

>>> feature to have in any case.  For one thing, it would make

>>> handling warnings and responding to #pragma GCC diagnostics

>>> more robust.

>>

>>> +          const char *arg0str = IDENTIFIER_POINTER (arg0name);

>>> +          closure = !strcmp (arg0str, "__closure");

>>

>> Let's use id_equal here.

> 

> Done in the attached revision.


> gcc/c-family/ChangeLog:

> 

> 	PR c++/95984

> 	* c-common.c (check_function_nonnull):

> 	(check_nonnull_arg):

> 

> gcc/cp/ChangeLog:

> 

> 	PR c++/95984

> 	* call.c (build_over_call):


You're missing actual descriptions of your changes.  OK with that fixed.

Jason

Patch

Exclude calls to variadic lambda stubs from -Wnonnull checking (PR c++/95984).

Resolves:
PR c++/95984 - Internal compiler error: Error reporting routines re-entered in -Wnonnull on a variadic lamnda
PR c++/96021 - missing -Wnonnull passing nullptr to a nonnull variadic lambda

gcc/c-family/ChangeLog:

	PR c++/95984
	* c-common.c (check_function_nonnull):
	(check_nonnull_arg):

gcc/cp/ChangeLog:

	PR c++/95984
	* call.c (build_over_call):

gcc/testsuite/ChangeLog:

	PR c++/95984
	* g++.dg/warn/Wnonnull6.C: New test.

diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c
index aae1ddb6b89..695e6d27262 100644
--- a/gcc/c-family/c-common.c
+++ b/gcc/c-family/c-common.c
@@ -5308,12 +5308,29 @@  check_function_nonnull (nonnull_arg_ctx &ctx, int nargs, tree *argarray)
   int firstarg = 0;
   if (TREE_CODE (ctx.fntype) == METHOD_TYPE)
     {
+      bool closure = false;
+      if (ctx.fndecl)
+	{
+	  /* For certain lambda expressions the C++ front end emits calls
+	     that pass a null this pointer as an argument named __closure
+	     to the member operator() of empty function.  Detect those
+	     and avoid checking them, but proceed to check the remaining
+	     arguments.  */
+	  tree arg0 = DECL_ARGUMENTS (ctx.fndecl);
+	  if (tree arg0name = DECL_NAME (arg0))
+	    {
+	      const char *arg0str = IDENTIFIER_POINTER (arg0name);
+	      closure = !strcmp (arg0str, "__closure");
+	    }
+	}
+
       /* In calls to C++ non-static member functions check the this
 	 pointer regardless of whether the function is declared with
 	 attribute nonnull.  */
       firstarg = 1;
-      check_function_arguments_recurse (check_nonnull_arg, &ctx, argarray[0],
-					firstarg);
+      if (!closure)
+	check_function_arguments_recurse (check_nonnull_arg, &ctx, argarray[0],
+					  firstarg);
     }
 
   tree attrs = lookup_attribute ("nonnull", TYPE_ATTRIBUTES (ctx.fntype));
@@ -5503,7 +5520,9 @@  check_nonnull_arg (void *ctx, tree param, unsigned HOST_WIDE_INT param_num)
      happen if the "nonnull" attribute was given without an operand
      list (which means to check every pointer argument).  */
 
-  if (TREE_CODE (TREE_TYPE (param)) != POINTER_TYPE)
+  tree paramtype = TREE_TYPE (param);
+  if (TREE_CODE (paramtype) != POINTER_TYPE
+      && TREE_CODE (paramtype) != NULLPTR_TYPE)
     return;
 
   /* Diagnose the simple cases of null arguments.  */
diff --git a/gcc/cp/call.c b/gcc/cp/call.c
index d8923be1d68..5341a572980 100644
--- a/gcc/cp/call.c
+++ b/gcc/cp/call.c
@@ -8842,15 +8842,16 @@  build_over_call (struct z_candidate *cand, int flags, tsubst_flags_t complain)
   gcc_assert (j <= nargs);
   nargs = j;
 
-  /* Avoid to do argument-transformation, if warnings for format, and for
-     nonnull are disabled.  Just in case that at least one of them is active
+  /* Avoid performing argument transformation if warnings are disabled.
+     When tf_warning is set and at least one of the warnings is active
      the check_function_arguments function might warn about something.  */
 
   bool warned_p = false;
-  if (warn_nonnull
-      || warn_format
-      || warn_suggest_attribute_format
-      || warn_restrict)
+  if ((complain & tf_warning)
+      && (warn_nonnull
+	  || warn_format
+	  || warn_suggest_attribute_format
+	  || warn_restrict))
     {
       tree *fargs = (!nargs ? argarray
 			    : (tree *) alloca (nargs * sizeof (tree)));
diff --git a/gcc/testsuite/g++.dg/warn/Wnonnull6.C b/gcc/testsuite/g++.dg/warn/Wnonnull6.C
new file mode 100644
index 00000000000..dae6dd2d912
--- /dev/null
+++ b/gcc/testsuite/g++.dg/warn/Wnonnull6.C
@@ -0,0 +1,37 @@ 
+/* PR c++/95984 - Internal compiler error: Error reporting routines re-entered
+   in -Wnonnull on a variadic lamnda
+   PR c++/96021 - missing -Wnonnull passing nullptr to a nonnull variadic lambda
+   { dg-do compile { target c++11 } }
+   { dg-options "-Wall" } */
+
+typedef int F (int);
+
+F* pr95984 ()
+{
+  // This also triggered the ICE.
+  return [](auto...) { return 0; };     // { dg-bogus "\\\[-Wnonnull" }
+}
+
+
+__attribute__ ((nonnull)) void f (int, ...);
+void ff ()
+{
+  f (1, nullptr);                       // { dg-warning "\\\[-Wnonnull" }
+}
+
+template <class T> void g (T t)
+{
+  t (1, nullptr);                       // { dg-warning "\\\[-Wnonnull" }
+}
+
+void gg (void)
+{
+  g ([](int, auto...) __attribute__ ((nonnull)) { });
+}
+
+template <class T> __attribute__ ((nonnull)) void h (T);
+
+void hh ()
+{
+  h (nullptr);                          //  { dg-warning "\\\[-Wnonnull" }
+}