[PATCH/RFC] How to fix PR95440

[Jason Merrill]

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