[PATCH/RFC] How to fix PR95440

Message ID A22FB94A-5C3D-4BD2-B133-DB32DD49B415@sandoe.co.uk
State New
Headers show
Series
  • [PATCH/RFC] How to fix PR95440
Related show

Commit Message

Iain Sandoe June 9, 2020, 9:04 a.m.
Hi C++ folks,

There are a number of places in the coroutine codegen where we need to  
build calls to promise methods.

Such methods can, in several cases, also be constexpr or static (or both)  
which leads to the PR,

Now the mechanism I use is this;

1/ lookup the method in the relevant scope

   tree method
     = lookup_member (promise, member_id,  /*protect=*/1, /*want_type=*/0, tf_warning_or_error);

- this returns a BASELINK tree..

2/ In many cases, the calls are to methods with no parms.

so, with a dummy object, I do:

tree call
   = build_new_method_call (dummy, method, NULL, NULL_TREE, LOOKUP_NORMAL, NULL, flags);

‘args’ is the 3rd parm.

3/ The header comment on build_new_method_call says:

/* Build a call to "INSTANCE.FN (ARGS)".  If FN_P is non-NULL, it will
    be set, upon return, to the function called.  ARGS may be NULL.
    This may change ARGS.  */

Which says that the 3rd arg can validly be NULL (as per my example).

4/ however, inside build_new_method_call  ‘args' gets assigned to  
‘user_args’ which is used in a lot of places without any guard to see if  
it’s non-null.   One of those places is passing it to add_candidates (),  
which eventually in the case of a static method uses the pointer and ICEs.

  	  if (same_type_p (TREE_VALUE (parmlist),

Comments

Jason Merrill via Gcc-patches June 9, 2020, 7:43 p.m. | #1
On Tue, Jun 9, 2020 at 5:04 AM Iain Sandoe <iain@sandoe.co.uk> wrote:

> Hi C++ folks,

>

> There are a number of places in the coroutine codegen where we need to

> build calls to promise methods.

>

> Such methods can, in several cases, also be constexpr or static (or both)

> which leads to the PR,

>

> Now the mechanism I use is this;

>

> 1/ lookup the method in the relevant scope

>

>    tree method

>      = lookup_member (promise, member_id,  /*protect=*/1, /*want_type=*/0,

> tf_warning_or_error);

>

> - this returns a BASELINK tree..

>

> 2/ In many cases, the calls are to methods with no parms.

>

> so, with a dummy object, I do:

>

> tree call

>    = build_new_method_call (dummy, method, NULL, NULL_TREE, LOOKUP_NORMAL,

> NULL, flags);

>

> ‘args’ is the 3rd parm.

>

> 3/ The header comment on build_new_method_call says:

>

> /* Build a call to "INSTANCE.FN (ARGS)".  If FN_P is non-NULL, it will

>     be set, upon return, to the function called.  ARGS may be NULL.

>     This may change ARGS.  */

>

> Which says that the 3rd arg can validly be NULL (as per my example).

>

> 4/ however, inside build_new_method_call  ‘args' gets assigned to

> ‘user_args’ which is used in a lot of places without any guard to see if

> it’s non-null.   One of those places is passing it to add_candidates (),

> which eventually in the case of a static method uses the pointer and ICEs.

>

> =====

>

> So…

>

> A/  I’m doing something wrong, and my use of APIs is violating an

> expectation (in which case what’s the right API usage?)

>

> B/ It happens to be other callers to build_new_method_call() have either

> had dummy args or not produced the same set of conditions

>


It does seem that most callers use dummy args, but the function isn't
documented that way, and it seems preferable to allow null args.

(for my case of static promise methods)
>

>   * if I provide an empty dummy ‘args' to the  build_new_method_call it

> works

>   * if I fix the unguarded use of ‘args' in add_candidates (as below) it

> works,

>

> AFAICT, the intent is to avoid every caller for build_new_method_call()

> having to build a dummy argument list when that list is empty - it would

> seem irritating to have to add one for every use in coros - but I can.

>

> WDYT?

> thanks

> Iain

>

> --------

>

> the fix for call.c:

>

> diff --git a/gcc/cp/call.c b/gcc/cp/call.c

> index 3c97b9846e2..dccc1bf596f 100644

> --- a/gcc/cp/call.c

> +++ b/gcc/cp/call.c

> @@ -5862,7 +5862,7 @@ add_candidates (tree fns, tree first_arg, const

> vec<tree, va_gc> *args,

>         }

>

>         /* Don't bother reversing an operator with two identical

> parameters.  */

> -      else if (args->length () == 2 && (flags & LOOKUP_REVERSED))

> +      else if (args && args->length () == 2 && (flags & LOOKUP_REVERSED))

>


The usual pattern is to use vec_safe_length here.  Similarly, in
build_new_method_call_1 I think

!user_args->is_empty()

should be

vec_safe_is_empty (user_args)

Those changes are OK.

Jason
Iain Sandoe June 10, 2020, 8:43 p.m. | #2
Hi Jason,

Jason Merrill <jason@redhat.com> wrote:

> On Tue, Jun 9, 2020 at 5:04 AM Iain Sandoe <iain@sandoe.co.uk> wrote:

> 


>         /* Don't bother reversing an operator with two identical parameters.  */

> -      else if (args->length () == 2 && (flags & LOOKUP_REVERSED))

> +      else if (args && args->length () == 2 && (flags & LOOKUP_REVERSED))

> 

> The usual pattern is to use vec_safe_length here.  Similarly, in build_new_method_call_1 I think 

> 

> !user_args->is_empty()

> 

> should be

> 

> vec_safe_is_empty (user_args)

> 

> Those changes are OK.


Thanks,

What I applied (after regtest on x86_64-linux/darwin and powerpc64-linux) is below.

Given that this fixes an IDE-on-valid filed against 10.1, I’d like to backport it for 10.2,
is that OK?

thanks
Iain

coroutines: Make call argument handling more robust [PR95440]

build_new_method_call is supposed to be able to handle a null
arguments list pointer (when the method has no parms).  There
were a couple of places where uses of the argument list pointer
were not defended against NULL.

gcc/cp/ChangeLog:

	PR c++/95440
	* call.c (add_candidates): Use vec_safe_length() for
	testing the arguments list.
	(build_new_method_call_1): Use vec_safe_is_empty() when
	checking for an empty args list.

gcc/testsuite/ChangeLog:

	PR c++/95440
	* g++.dg/coroutines/pr95440.C: New test.
---
 gcc/cp/call.c                             |  4 +--
 gcc/testsuite/g++.dg/coroutines/pr95440.C | 39 +++++++++++++++++++++++
 2 files changed, 41 insertions(+), 2 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/coroutines/pr95440.C

diff --git a/gcc/cp/call.c b/gcc/cp/call.c
index 3c97b9846e2..b99959f76f9 100644
--- a/gcc/cp/call.c
+++ b/gcc/cp/call.c
@@ -5862,7 +5862,7 @@ add_candidates (tree fns, tree first_arg, const vec<tree, va_gc> *args,
 	}
 
       /* Don't bother reversing an operator with two identical parameters.  */
-      else if (args->length () == 2 && (flags & LOOKUP_REVERSED))
+      else if (vec_safe_length (args) == 2 && (flags & LOOKUP_REVERSED))
 	{
 	  tree parmlist = TYPE_ARG_TYPES (TREE_TYPE (fn));
 	  if (same_type_p (TREE_VALUE (parmlist),
@@ -10263,7 +10263,7 @@ build_new_method_call_1 (tree instance, tree fns, vec<tree, va_gc> **args,
 	  && !(flags & LOOKUP_ONLYCONVERTING)
 	  && cxx_dialect >= cxx20
 	  && CP_AGGREGATE_TYPE_P (basetype)
-	  && !user_args->is_empty ())
+	  && !vec_safe_is_empty (user_args))
 	{
 	  /* Create a CONSTRUCTOR from ARGS, e.g. {1, 2} from <1, 2>.  */
 	  tree list = build_tree_list_vec (user_args);
diff --git a/gcc/testsuite/g++.dg/coroutines/pr95440.C b/gcc/testsuite/g++.dg/coroutines/pr95440.C
new file mode 100644
index 00000000000..8542880d1ab
--- /dev/null
+++ b/gcc/testsuite/g++.dg/coroutines/pr95440.C
@@ -0,0 +1,39 @@
+#if __has_include(<coroutine>)
+#include <coroutine>
+#else
+#include <experimental/coroutine>
+namespace std { using namespace experimental; }
+#endif
+#if 0
+struct suspend_n {
+  const int x;
+  constexpr suspend_n (int x) : x (x) {}
+  constexpr static bool await_ready() { return false; }
+  constexpr static void await_suspend(std::coroutine_handle<>) {}
+  constexpr static void await_resume() {}
+};
+#endif
+struct task
+{
+  struct promise_type
+  {
+    auto get_return_object() const { return task{}; }
+#if 0
+//    static constexpr suspend_n initial_suspend()  { return {2}; }
+#endif
+    static constexpr std::suspend_always initial_suspend()  { return {}; }
+    static constexpr std::suspend_never final_suspend() { return {}; }
+    static constexpr void return_void() {}
+    static constexpr void unhandled_exception() {}
+  };
+};
+
+task
+test_task ()
+{
+  co_await std::suspend_always{};
+}
+
+auto t = test_task();
+
+int main() {}
-- 
2.24.1
Jason Merrill via Gcc-patches June 11, 2020, 6:27 p.m. | #3
On 6/10/20 4:43 PM, Iain Sandoe wrote:
> Hi Jason,

> 

> Jason Merrill <jason@redhat.com> wrote:

> 

>> On Tue, Jun 9, 2020 at 5:04 AM Iain Sandoe <iain@sandoe.co.uk> wrote:

>>

> 

>>          /* Don't bother reversing an operator with two identical parameters.  */

>> -      else if (args->length () == 2 && (flags & LOOKUP_REVERSED))

>> +      else if (args && args->length () == 2 && (flags & LOOKUP_REVERSED))

>>

>> The usual pattern is to use vec_safe_length here.  Similarly, in build_new_method_call_1 I think

>>

>> !user_args->is_empty()

>>

>> should be

>>

>> vec_safe_is_empty (user_args)

>>

>> Those changes are OK.

> 

> Thanks,

> 

> What I applied (after regtest on x86_64-linux/darwin and powerpc64-linux) is below.

> 

> Given that this fixes an IDE-on-valid filed against 10.1, I’d like to backport it for 10.2,

> is that OK?


Yes.

> thanks

> Iain

> 

> coroutines: Make call argument handling more robust [PR95440]

> 

> build_new_method_call is supposed to be able to handle a null

> arguments list pointer (when the method has no parms).  There

> were a couple of places where uses of the argument list pointer

> were not defended against NULL.

> 

> gcc/cp/ChangeLog:

> 

> 	PR c++/95440

> 	* call.c (add_candidates): Use vec_safe_length() for

> 	testing the arguments list.

> 	(build_new_method_call_1): Use vec_safe_is_empty() when

> 	checking for an empty args list.

> 

> gcc/testsuite/ChangeLog:

> 

> 	PR c++/95440

> 	* g++.dg/coroutines/pr95440.C: New test.

> ---

>   gcc/cp/call.c                             |  4 +--

>   gcc/testsuite/g++.dg/coroutines/pr95440.C | 39 +++++++++++++++++++++++

>   2 files changed, 41 insertions(+), 2 deletions(-)

>   create mode 100644 gcc/testsuite/g++.dg/coroutines/pr95440.C

> 

> diff --git a/gcc/cp/call.c b/gcc/cp/call.c

> index 3c97b9846e2..b99959f76f9 100644

> --- a/gcc/cp/call.c

> +++ b/gcc/cp/call.c

> @@ -5862,7 +5862,7 @@ add_candidates (tree fns, tree first_arg, const vec<tree, va_gc> *args,

>   	}

>   

>         /* Don't bother reversing an operator with two identical parameters.  */

> -      else if (args->length () == 2 && (flags & LOOKUP_REVERSED))

> +      else if (vec_safe_length (args) == 2 && (flags & LOOKUP_REVERSED))

>   	{

>   	  tree parmlist = TYPE_ARG_TYPES (TREE_TYPE (fn));

>   	  if (same_type_p (TREE_VALUE (parmlist),

> @@ -10263,7 +10263,7 @@ build_new_method_call_1 (tree instance, tree fns, vec<tree, va_gc> **args,

>   	  && !(flags & LOOKUP_ONLYCONVERTING)

>   	  && cxx_dialect >= cxx20

>   	  && CP_AGGREGATE_TYPE_P (basetype)

> -	  && !user_args->is_empty ())

> +	  && !vec_safe_is_empty (user_args))

>   	{

>   	  /* Create a CONSTRUCTOR from ARGS, e.g. {1, 2} from <1, 2>.  */

>   	  tree list = build_tree_list_vec (user_args);

> diff --git a/gcc/testsuite/g++.dg/coroutines/pr95440.C b/gcc/testsuite/g++.dg/coroutines/pr95440.C

> new file mode 100644

> index 00000000000..8542880d1ab

> --- /dev/null

> +++ b/gcc/testsuite/g++.dg/coroutines/pr95440.C

> @@ -0,0 +1,39 @@

> +#if __has_include(<coroutine>)

> +#include <coroutine>

> +#else

> +#include <experimental/coroutine>

> +namespace std { using namespace experimental; }

> +#endif

> +#if 0

> +struct suspend_n {

> +  const int x;

> +  constexpr suspend_n (int x) : x (x) {}

> +  constexpr static bool await_ready() { return false; }

> +  constexpr static void await_suspend(std::coroutine_handle<>) {}

> +  constexpr static void await_resume() {}

> +};

> +#endif

> +struct task

> +{

> +  struct promise_type

> +  {

> +    auto get_return_object() const { return task{}; }

> +#if 0

> +//    static constexpr suspend_n initial_suspend()  { return {2}; }

> +#endif

> +    static constexpr std::suspend_always initial_suspend()  { return {}; }

> +    static constexpr std::suspend_never final_suspend() { return {}; }

> +    static constexpr void return_void() {}

> +    static constexpr void unhandled_exception() {}

> +  };

> +};

> +

> +task

> +test_task ()

> +{

> +  co_await std::suspend_always{};

> +}

> +

> +auto t = test_task();

> +

> +int main() {}

>

Patch

=====

So…

A/  I’m doing something wrong, and my use of APIs is violating an  
expectation (in which case what’s the right API usage?)

B/ It happens to be other callers to build_new_method_call() have either  
had dummy args or not produced the same set of conditions

(for my case of static promise methods)

  * if I provide an empty dummy ‘args' to the  build_new_method_call it works
  * if I fix the unguarded use of ‘args' in add_candidates (as below) it works,

AFAICT, the intent is to avoid every caller for build_new_method_call()  
having to build a dummy argument list when that list is empty - it would  
seem irritating to have to add one for every use in coros - but I can.

WDYT?
thanks
Iain

--------

the fix for call.c:

diff --git a/gcc/cp/call.c b/gcc/cp/call.c
index 3c97b9846e2..dccc1bf596f 100644
--- a/gcc/cp/call.c
+++ b/gcc/cp/call.c
@@ -5862,7 +5862,7 @@  add_candidates (tree fns, tree first_arg, const  
vec<tree, va_gc> *args,
  	}
 
        /* Don't bother reversing an operator with two identical parameters.  */
-      else if (args->length () == 2 && (flags & LOOKUP_REVERSED))
+      else if (args && args->length () == 2 && (flags & LOOKUP_REVERSED))
  	{
  	  tree parmlist = TYPE_ARG_TYPES (TREE_TYPE (fn));