[PATCH v2] c++: fix ICE with sizeof in a template [PR112869]

[Jason Merrill]

On 12/8/23 16:15, Marek Polacek wrote:
> On Fri, Dec 08, 2023 at 12:09:18PM -0500, Jason Merrill wrote:
>> On 12/5/23 15:31, Marek Polacek wrote:
>>> Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?
>>>
>>> -- >8 --
>>> This test shows that we cannot clear *walk_subtrees in
>>> cp_fold_immediate_r when we're in_immediate_context, because that,
>>> as the comment says, affects cp_fold_r as well.  Here we had an
>>> expression with
>>>
>>>     min ((long int) VIEW_CONVERT_EXPR<long unsigned int>(bytecount), (long int) <<< Unknown tree: sizeof_expr
>>>       (int) <<< error >>> >>>)
>>>
>>> as its sub-expression, and we never evaluated that into
>>>
>>>     min ((long int) bytecount, 4)
>>>
>>> so the SIZEOF_EXPR leaked into the middle end.
>>>
>>> (There's still one *walk_subtrees = 0; in cp_fold_immediate_r, but that
>>> one should be OK.)
>>>
>>> 	PR c++/112869
>>>
>>> gcc/cp/ChangeLog:
>>>
>>> 	* cp-gimplify.cc (cp_fold_immediate_r): Don't clear *walk_subtrees
>>> 	for unevaluated operands.
>>
>> I agree that we want this change for in_immediate_context (), but I don't
>> see why we want it for TYPE_P or unevaluated_p (code) or
>> cp_unevaluated_operand?
> 
> No particular reason, just paranoia.  How's this?
> 
> Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?
> 
> -- >8 --
> This test shows that we cannot clear *walk_subtrees in
> cp_fold_immediate_r when we're in_immediate_context, because that,
> as the comment says, affects cp_fold_r as well.  Here we had an
> expression with
> 
>    min ((long int) VIEW_CONVERT_EXPR<long unsigned int>(bytecount), (long int) <<< Unknown tree: sizeof_expr
>      (int) <<< error >>> >>>)
> 
> as its sub-expression, and we never evaluated that into
> 
>    min ((long int) bytecount, 4)
> 
> so the SIZEOF_EXPR leaked into the middle end.
> 
> (There's still one *walk_subtrees = 0; in cp_fold_immediate_r, but that
> one should be OK.)
> 
> 	PR c++/112869
> 
> gcc/cp/ChangeLog:
> 
> 	* cp-gimplify.cc (cp_fold_immediate_r): Don't clear *walk_subtrees
> 	for in_immediate_context.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/template/sizeof18.C: New test.
> ---
>   gcc/cp/cp-gimplify.cc                    | 6 +++++-
>   gcc/testsuite/g++.dg/template/sizeof18.C | 8 ++++++++
>   2 files changed, 13 insertions(+), 1 deletion(-)
>   create mode 100644 gcc/testsuite/g++.dg/template/sizeof18.C
> 
> diff --git a/gcc/cp/cp-gimplify.cc b/gcc/cp/cp-gimplify.cc
> index 5abb91bbdd3..6af7c787372 100644
> --- a/gcc/cp/cp-gimplify.cc
> +++ b/gcc/cp/cp-gimplify.cc
> @@ -1179,11 +1179,15 @@ cp_fold_immediate_r (tree *stmt_p, int *walk_subtrees, void *data_)
>   
>     /* No need to look into types or unevaluated operands.
>        NB: This affects cp_fold_r as well.  */
> -  if (TYPE_P (stmt) || unevaluated_p (code) || in_immediate_context ())
> +  if (TYPE_P (stmt) || unevaluated_p (code))
>       {
>         *walk_subtrees = 0;
>         return NULL_TREE;
>       }
> +  else if (in_immediate_context ())
> +    /* Don't clear *walk_subtrees here: we still need to walk the subtrees
> +       of SIZEOF_EXPR and similar.  */
> +    return NULL_TREE;
>   
>     tree decl = NULL_TREE;
>     bool call_p = false;
> diff --git a/gcc/testsuite/g++.dg/template/sizeof18.C b/gcc/testsuite/g++.dg/template/sizeof18.C
> new file mode 100644
> index 00000000000..afba9946258
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/template/sizeof18.C
> @@ -0,0 +1,8 @@
> +// PR c++/112869
> +// { dg-do compile }
> +
> +void min(long, long);
> +template <class T> void Binaryread(int &, T, unsigned long);
> +template <> void Binaryread(int &, float, unsigned long bytecount) {
> +  min(bytecount, sizeof(int));
> +}

Hmm, actually, why does the above make a difference for this testcase?

...

It seems that in_immediate_context always returns true in 
cp_fold_function because current_binding_level->kind == 
sk_template_parms.  That seems like a problem.  Maybe for 
cp_fold_immediate_r we only want to check cp_unevaluated_operand or 
DECL_IMMEDIATE_CONTEXT (current_function_decl)?

Jason