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