Re: [PATCH] c++: thinko in extract_local_specs [PR108998]

[Jason Merrill <jason@redhat.com>]

On 3/3/23 09:58, Patrick Palka wrote:
> In order to fix PR100295, r13-4730-g18499b9f848707 attempted to make
> extract_local_specs walk the given pattern twice, ignoring unevaluated
> operands the first time around so that we prefer to process a local
> specialization in an evaluated context if it appears in one (we process
> a local specialization once even if it appears multiple times in the
> pattern).
> 
> But there's a thinko in the patch, namely that we don't actually walk
> the pattern twice, because we reuse the visited set for the second walk
> (to avoid processing a local specialization twice), and the root node
> (and any nodes leading up to an unevaluated operand) is considered
> visited already.  So the patch effectively made extract_local_specs
> ignore unevaluated operands altogether, which this testcase demonstrates
> isn't quite safe (extract_local_specs never sees 'aa' and we don't save
> its local specialization, so we later try to specialize 'aa' on the spot
> with the args {{int},{42}} which causes us to nonsensically substitute
> its auto with 42.)
> 
> This patch fixes this by walking only the trees we skipped over during
> the first walk the second time around.
> 
> Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK for
> trunk/12?

OK.

> 	PR c++/108998
> 
> gcc/cp/ChangeLog:
> 
> 	* pt.cc (el_data::skipped_trees): New data member.
> 	(extract_locals_r): Push to skipped_trees any unevaluated
> 	contexts that we skipped over.
> 	(extract_local_specs): During the second walk, consider only
> 	the trees in skipped_trees.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/cpp2a/lambda-generic11.C: New test.
> ---
>   gcc/cp/pt.cc                                  | 10 +++++++++-
>   gcc/testsuite/g++.dg/cpp2a/lambda-generic11.C | 13 +++++++++++++
>   2 files changed, 22 insertions(+), 1 deletion(-)
>   create mode 100644 gcc/testsuite/g++.dg/cpp2a/lambda-generic11.C
> 
> diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc
> index ba1b3027513..85136df1730 100644
> --- a/gcc/cp/pt.cc
> +++ b/gcc/cp/pt.cc
> @@ -13052,6 +13052,8 @@ public:
>     tsubst_flags_t complain;
>     /* True iff we don't want to walk into unevaluated contexts.  */
>     bool skip_unevaluated_operands = false;
> +  /* The unevaluated contexts that we avoided walking.  */
> +  auto_vec<tree> skipped_trees;
>   
>     el_data (tsubst_flags_t c)
>       : extra (NULL_TREE), complain (c) {}
> @@ -13066,6 +13068,7 @@ extract_locals_r (tree *tp, int *walk_subtrees, void *data_)
>     if (data.skip_unevaluated_operands
>         && unevaluated_p (TREE_CODE (*tp)))
>       {
> +      data.skipped_trees.safe_push (*tp);
>         *walk_subtrees = 0;
>         return NULL_TREE;
>       }
> @@ -13168,8 +13171,13 @@ extract_local_specs (tree pattern, tsubst_flags_t complain)
>        context).  */
>     data.skip_unevaluated_operands = true;
>     cp_walk_tree (&pattern, extract_locals_r, &data, &data.visited);
> +  /* Now walk the unevaluated contexts we skipped the first time around.  */
>     data.skip_unevaluated_operands = false;
> -  cp_walk_tree (&pattern, extract_locals_r, &data, &data.visited);
> +  for (tree t : data.skipped_trees)
> +    {
> +      data.visited.remove (t);
> +      cp_walk_tree (&t, extract_locals_r, &data, &data.visited);
> +    }
>     return data.extra;
>   }
>   
> diff --git a/gcc/testsuite/g++.dg/cpp2a/lambda-generic11.C b/gcc/testsuite/g++.dg/cpp2a/lambda-generic11.C
> new file mode 100644
> index 00000000000..418650699e3
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp2a/lambda-generic11.C
> @@ -0,0 +1,13 @@
> +// PR c++/108999
> +// { dg-do compile { target c++20 } }
> +
> +template<typename T>
> +void ice(T a) {
> +  auto aa = a;
> +  auto lambda = []<int I>() {
> +    if constexpr (sizeof(aa) + I != 42) {}
> +  };
> +  lambda.template operator()<0>();
> +}
> +
> +template void ice(int);