Re: [PATCH v2] c++: ICE with template code in constexpr [PR104284]

[Jason Merrill <jason@redhat.com>]

On 3/18/22 17:55, Marek Polacek wrote:
> On Fri, Mar 11, 2022 at 06:46:42PM -0500, Jason Merrill wrote:
>> On 3/10/22 18:04, Marek Polacek wrote:
>>> Since r9-6073 cxx_eval_store_expression preevaluates the value to
>>> be stored, and that revealed a crash where a template code (here,
>>> code=IMPLICIT_CONV_EXPR) leaks into cxx_eval*.
>>>
>>> It happens because we're performing build_vec_init while processing
>>> a template
>>
>> Hmm, that seems like the bug.  Where's that call coming from?
> 
>  From build_aggr_init.  So we're handling e.g.
> 
>    template<class>
>    constexpr void g () {
>      constexpr S s2[]{{'a'}};
>    }
> 
> cp_finish_decl (decl=s2, init={{'a'}}) sees we're in processing_template_decl,
> but also that we have a constexpr var which is not dependent, nor is its
> initializer:
> 
>        else if (init
>                 && (init_const_expr_p || DECL_DECLARED_CONSTEXPR_P (decl))
>                 && !TYPE_REF_P (type)
>                 && decl_maybe_constant_var_p (decl)
>                 && !(dep_init = value_dependent_init_p (init)))
>          {
>            /* This variable seems to be a non-dependent constant, so process
>               its initializer.  If check_initializer returns non-null the
>               initialization wasn't constant after all.  */
>            tree init_code;
>            cleanups = make_tree_vector ();
>            init_code = check_initializer (decl, init, flags, &cleanups);
> 
> so we call check_initializer, where we go down this path:
> 
>    init_code = build_aggr_init_full_exprs (decl, init, flags);
> 
> build_aggr_init sees that the type of 's2' is ARRAY_TYPE, so it calls
> build_vec_init.
> 
> I now recall that we've discussed build_vec_init in a template in the
> past, for example in the context of c++/93676.  So I agree we ought to
> make an effort to avoid calling build_vec_init in a template.  Perhaps
> like this: use an INIT_EXPR.  With that, we should call build_vec_init
> if needed while instantiating.  Does that make any sense?

Hmm.  If we do that, then we get back to

>           if (TREE_CODE (init_code) == INIT_EXPR)

in check_initializer, and pull out the same init again, and set 
LOOKUP_ALREADY_DIGESTED.  But I think that's wrong, we haven't digested 
it yet.

Maybe we could avoid entering the below block of check_initializer at 
all in this situation?

>       if (((type_build_ctor_call (type) || CLASS_TYPE_P (type))
>            && !(flags & LOOKUP_ALREADY_DIGESTED)
>            && !(init && BRACE_ENCLOSED_INITIALIZER_P (init)
>                 && CP_AGGREGATE_TYPE_P (type)
>                 && (CLASS_TYPE_P (type)

Maybe by adding || processing_template_decl here?

>                     || !TYPE_NEEDS_CONSTRUCTING (type)
>                     || type_has_extended_temps (type))))
>           || (DECL_DECOMPOSITION_P (decl) && TREE_CODE (type) == ARRAY_TYPE))

Jason