Re: C++ PATCH for c++/84684, wrong caching when evaluating a constexpr function

[Jason Merrill <jason at redhat dot com>]

On Tue, Mar 6, 2018 at 1:13 PM, Marek Polacek <polacek@redhat.com> wrote:
> In this testcase we have a constexpr function, value_to_char_helper.  Its body
> is
>    for (size_t i = 0u; i < alphabet_t::value_size; ++i)
>       value_to_char[i] = to_char(alphabet.assign_rank(i));
>
> which is genericized to a LOOP_EXPR looking roughly like this:
>
>   while (1)
>     {
>       if (i > 3)
>         goto out;
>       value_to_char[i] = to_char (..., i, ...);
>       i++;
>     }
>
> Then this is what happens when evaluating the above:
>
> 1) cxx_eval_call_expression evaluates the first call to to_char
>    it's not in the hash table -> copy the body and the bindings and
>    then evaluate the body
>    the result is 65
>    bindings: alph -> {._value=0} (correct)
>    save all this to the table
> 2) cxx_eval_store_expression evaluates i++
>    then it replaces the value in the hash map:
>      *valp = init;
>    which is generally what we want, but that also overwrites {._value=0} from
>    above to {._value=1} which causes problem in 3)
> 3) cxx_eval_call_expression tries to evaluate the second call to to_char
>    bindings: alph -> {._value=1} (correct)
>    here if the hashes match (use [1] hunk for better testing) then
>    we find to_char call in the table.  Then we invoke
>    constexpr_call_hasher::equal() to compare the two calls: fundefs
>    match, and because 2) has overwritten bindings of the previous
>    to_char call, they match too, both are {._value=1}.
>    That means we don't evaluate this second call and just use the cached
>    result (65), and that is wrong.
>
> I think the fix is to avoid sharing the constructor in the bindings which
> is what the patch does and what we do in several places already.  I think
> Jakub's patch <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84684#c13>
> wouldn't work if the bindings had more constructors.
>
> But I'm also wondering about massage_init_elt.  It has
>   tree t = fold_non_dependent_expr (init);
>   t = maybe_constant_init (t);
> but given that fold_non_dependent_expr now calls maybe_constant_value, which
> then causes that we try to cache the calls above, this seems excessive,
> wouldn't we be better off with just calling fold_non_dependent_init as
> discussed recently?

Probably.

> I bootstrapped/regtested this patch with this hunk, too, for better testing:
>
> [1]
> @@ -1598,8 +1598,12 @@ cxx_eval_call_expression (const constexpr_ctx *ctx, tree t,
>    constexpr_call *entry = NULL;
>    if (depth_ok && !non_constant_args && ctx->strict)
>      {
> +#if 0
>        new_call.hash = iterative_hash_template_arg
>         (new_call.bindings, constexpr_fundef_hasher::hash (new_call.fundef));
> +#else
> +      new_call.hash = 0;
> +#endif
>
>        /* If we have seen this call before, we are done.  */
>        maybe_initialize_constexpr_call_table ();
>
> Bootstrapped/regtested on x86_64-linux, ok for trunk?  And likely 7.

OK.

Jason