On Tue, Mar 6, 2018 at 1:13 PM, Marek Polacek 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
> 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
