On Mon, Mar 11, 2019 at 11:21:00PM +0100, Jakub Jelinek wrote:
> The following testcase ICEs since my recent cxx_eval_loop_expr changes.
> The problem is that the Forget saved values of SAVE_EXPRs. inside of the
> loop can remove SAVE_EXPRs from new_ctx.values and if that is the last
> iteration, we can also do the loop at the end of function (which has been
> added there mainly to handle cases where the main loop breaks earlier)
> and new_ctx.values->remove ICEs because *iter is already not in the table.
>
> While I could e.g. add some bool whether there is anything to be removed
> after the loop or not which would fix the regression part of this PR,
> I believe there is a latent issue as well. The thing is, new_ctx.save_exprs
That is actually not true, the bug is fully my fault and previously it was
ok, as cxx_eval_loop_expr used to do:
do
{
hash_set<tree> save_exprs;
new_ctx.save_exprs = &save_exprs;
...
}
while (...)
and thus there was a new hash_set for each iteration, it was just me moving
it out of the loop (so that I don't have to repeat the ->remove loop on each
break or use gotos).
> Yet another possibility might be to turn save_exprs into a vec, from what
That said, I believe using a vec must be faster and the hash_set seems
overkill, as if SAVE_EXPR wasn't seen yet in the current iteration, it is
desirable to add it to the vec and if it has been added there,
ctx->value->get will be non-NULL, so we'll never add a duplicate, and by
using a vec we don't have to allocate storage for each iteration etc.
So, here is a variant patch I've bootstrapped/regtested on x86_64-linux and
i686-linux, ok for trunk?
2019-03-13 Jakub Jelinek <ja...@redhat.com>
PR c++/89652
* constexpr.c (struct constexpr_ctx): Change save_exprs type from
hash_set<tree> to vec<tree>.
(cxx_eval_call_expression): Adjust for save_exprs being a vec instead
of hash_set.
(cxx_eval_loop_expr): Likewise. Truncate the vector after each
removal of SAVE_EXPRs from values.
(cxx_eval_constant_expression) <case SAVE_EXPR>: Call safe_push
method on save_exprs instead of add.
* g++.dg/cpp1y/constexpr-89652.C: New test.
--- gcc/cp/constexpr.c.jj 2019-03-11 22:56:51.856732730 +0100
+++ gcc/cp/constexpr.c 2019-03-13 13:21:48.986687463 +0100
@@ -1024,7 +1024,7 @@ struct constexpr_ctx {
hash_map<tree,tree> *values;
/* SAVE_EXPRs that we've seen within the current LOOP_EXPR. NULL if we
aren't inside a loop. */
- hash_set<tree> *save_exprs;
+ vec<tree> *save_exprs;
/* The CONSTRUCTOR we're currently building up for an aggregate
initializer. */
tree ctor;
@@ -1831,7 +1831,7 @@ cxx_eval_call_expression (const constexp
/* Track the callee's evaluated SAVE_EXPRs so that we can forget
their values after the call. */
constexpr_ctx ctx_with_save_exprs = *ctx;
- hash_set<tree> save_exprs;
+ auto_vec<tree, 10> save_exprs;
ctx_with_save_exprs.save_exprs = &save_exprs;
ctx_with_save_exprs.call = &new_call;
@@ -1862,9 +1862,10 @@ cxx_eval_call_expression (const constexp
}
/* Forget the saved values of the callee's SAVE_EXPRs. */
- for (hash_set<tree>::iterator iter = save_exprs.begin();
- iter != save_exprs.end(); ++iter)
- ctx_with_save_exprs.values->remove (*iter);
+ unsigned int i;
+ tree save_expr;
+ FOR_EACH_VEC_ELT (save_exprs, i, save_expr)
+ ctx_with_save_exprs.values->remove (save_expr);
/* Remove the parms/result from the values map. Is it worth
bothering to do this when the map itself is only live for
@@ -4190,7 +4191,7 @@ cxx_eval_loop_expr (const constexpr_ctx
default:
gcc_unreachable ();
}
- hash_set<tree> save_exprs;
+ auto_vec<tree, 10> save_exprs;
new_ctx.save_exprs = &save_exprs;
do
{
@@ -4234,9 +4235,11 @@ cxx_eval_loop_expr (const constexpr_ctx
}
/* Forget saved values of SAVE_EXPRs. */
- for (hash_set<tree>::iterator iter = save_exprs.begin();
- iter != save_exprs.end(); ++iter)
- new_ctx.values->remove (*iter);
+ unsigned int i;
+ tree save_expr;
+ FOR_EACH_VEC_ELT (save_exprs, i, save_expr)
+ new_ctx.values->remove (save_expr);
+ save_exprs.truncate (0);
if (++count >= constexpr_loop_limit)
{
@@ -4256,9 +4259,10 @@ cxx_eval_loop_expr (const constexpr_ctx
&& !*non_constant_p);
/* Forget saved values of SAVE_EXPRs. */
- for (hash_set<tree>::iterator iter = save_exprs.begin();
- iter != save_exprs.end(); ++iter)
- new_ctx.values->remove (*iter);
+ unsigned int i;
+ tree save_expr;
+ FOR_EACH_VEC_ELT (save_exprs, i, save_expr)
+ new_ctx.values->remove (save_expr);
return NULL_TREE;
}
@@ -4616,7 +4620,7 @@ cxx_eval_constant_expression (const cons
non_constant_p, overflow_p);
ctx->values->put (t, r);
if (ctx->save_exprs)
- ctx->save_exprs->add (t);
+ ctx->save_exprs->safe_push (t);
}
break;
--- gcc/testsuite/g++.dg/cpp1y/constexpr-89652.C.jj 2019-03-13
13:22:51.532678917 +0100
+++ gcc/testsuite/g++.dg/cpp1y/constexpr-89652.C 2019-03-13
13:22:51.532678917 +0100
@@ -0,0 +1,36 @@
+// PR c++/89652
+// { dg-do compile { target c++14 } }
+// { dg-options "" }
+
+template <typename T> constexpr auto foo (T &e) { return e.foo (); }
+template <typename T> constexpr auto bar (T &e) { return foo (e); }
+template <typename T, int N> struct A { typedef T a[N]; };
+template <typename T, unsigned long N> struct B {
+ typedef T *b;
+ typename A<T, N>::a d;
+ constexpr b foo () { return d; }
+};
+template <typename> struct C { long m; };
+struct D { long n; };
+template <typename, unsigned long> struct E {
+ B<C<int>, 1>::b p;
+ constexpr D operator* () { return {p->m}; }
+ constexpr E operator++ (int) { auto a{*this}; ++p; return a; }
+};
+template <typename T, unsigned long N>
+constexpr bool operator!= (E<T, N> a, E<T, N>) { return a.p; }
+template <unsigned long N, typename T, unsigned long M>
+constexpr auto baz (B<T, M> s, B<D, N>)
+{
+ B<D, M> t{};
+ auto q{foo (t)};
+ using u = E<T, M>;
+ auto v = u{bar (s)};
+ auto w = u{};
+ while (v != w)
+ *q++ = *v++;
+ return t;
+}
+constexpr auto a = B<C<int>, 5>{};
+auto b = B<D, 0>{};
+auto c = baz (a, b);
Jakub
