Re: [C++ PATCH] Fix cxx_eval_loop_expr ICE (PR c++/89652, take 2)
On 3/13/19 6:20 PM, Jakub Jelinek wrote:
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 save_exprs;
new_ctx.save_exprs = _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?
This is OK. Is my hash-table.h fix also OK for trunk? It does also fix
the testcase.
Jason
Re: [C++ PATCH] Fix cxx_eval_loop_expr ICE (PR c++/89652)
On 3/13/19 10:08 PM, Jason Merrill wrote: On 3/11/19 6:21 PM, Jakub Jelinek wrote: Hi! 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. It shouldn't ICE, remove_elt_with_hash is documented to do nothing if the element is not in the table. Ah, now I see you sent a follow-up, reading that now. Jason
Re: [C++ PATCH] Fix cxx_eval_loop_expr ICE (PR c++/89652)
On 3/11/19 6:21 PM, Jakub Jelinek wrote:
Hi!
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.
It shouldn't ICE, remove_elt_with_hash is documented to do nothing if
the element is not in the table.
Does this untested patch fix the test?
Jason
diff --git a/gcc/hash-table.h b/gcc/hash-table.h
index 1fd36946a53..fb2f77fdbee 100644
--- a/gcc/hash-table.h
+++ b/gcc/hash-table.h
@@ -940,7 +940,7 @@ hash_table
::remove_elt_with_hash (const compare_type , hashval_t hash)
{
value_type *slot = find_slot_with_hash (comparable, hash, NO_INSERT);
- if (is_empty (*slot))
+ if (!slot || is_empty (*slot))
return;
Descriptor::remove (*slot);
[C++ PATCH] Fix cxx_eval_loop_expr ICE (PR c++/89652, take 2)
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 save_exprs;
new_ctx.save_exprs = _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
PR c++/89652
* constexpr.c (struct constexpr_ctx): Change save_exprs type from
hash_set to vec.
(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) : 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 *values;
/* SAVE_EXPRs that we've seen within the current LOOP_EXPR. NULL if we
aren't inside a loop. */
- hash_set *save_exprs;
+ vec *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 save_exprs;
+ auto_vec save_exprs;
ctx_with_save_exprs.save_exprs = _exprs;
ctx_with_save_exprs.call = _call;
@@ -1862,9 +1862,10 @@ cxx_eval_call_expression (const constexp
}
/* Forget the saved values of the callee's SAVE_EXPRs. */
- for (hash_set::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 save_exprs;
+ auto_vec save_exprs;
new_ctx.save_exprs = _exprs;
do
{
@@ -4234,9 +4235,11 @@ cxx_eval_loop_expr (const constexpr_ctx
}
/* Forget saved values of SAVE_EXPRs. */
- for (hash_set::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::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)
-
[C++ PATCH] Fix cxx_eval_loop_expr ICE (PR c++/89652)
Hi!
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
is a hash_set, which the cxx_eval_* calls add SAVE_EXPRs to whenever they
encounter new ones, but if we don't encounter all the previously seen
SAVE_EXPRs in every iteration (e.g. some are wrapped by IF_STMT or
similar and the condition differs between iterations), I believe we can
still ICE, trying to remove a SAVE_EXPR that has been encountered by some
earlier iteration, but not the current one.
The following patch is one possible fix, only remove from new_ctx.values
what is still in there. Bootstrapped/regtested on x86_64-linux and
i686-linux, ok for trunk? Or some variant mentioned below?
Another possibility would be to save_exprs.empty (); after this
/* Forget saved values of SAVE_EXPRs. */ loop inside of main loop (no need
to do it at the end of function when we'll just destroy it anyway), the
advantage would be that we don't really walk over SAVE_EXPRs that weren't
saved in the current iteration, disadvantage is that we need to populate the
hash_set again and again each iteration.
Yet another possibility might be to turn save_exprs into a vec, from what
I understand, we do:
case SAVE_EXPR:
/* Avoid evaluating a SAVE_EXPR more than once. */
if (tree *p = ctx->values->get (t))
r = *p;
else
{
r = cxx_eval_constant_expression (ctx, TREE_OPERAND (t, 0), false,
non_constant_p, overflow_p);
ctx->values->put (t, r);
if (ctx->save_exprs)
ctx->save_exprs->add (t);
}
and thus shouldn't be adding duplicates to save_expr and if we flush it
every iteration, we could just if (ctx->save_exprs) ctx->save_exprs->safe_push
(t);
and truncate the vector after every /* Forget saved values of SAVE_EXPRs. */
loop in the main loop's body. Thoughts on this?
2019-03-11 Jakub Jelinek
PR c++/89652
* constexpr.c (cxx_eval_loop_expr): Only remove SAVE_EXPRs that are
still in new_ctx.values hash_map.
* g++.dg/cpp1y/constexpr-89652.C: New test.
--- gcc/cp/constexpr.c.jj 2019-03-08 08:43:23.529496048 +0100
+++ gcc/cp/constexpr.c 2019-03-11 15:11:32.081334270 +0100
@@ -4236,7 +4236,8 @@ cxx_eval_loop_expr (const constexpr_ctx
/* Forget saved values of SAVE_EXPRs. */
for (hash_set::iterator iter = save_exprs.begin();
iter != save_exprs.end(); ++iter)
- new_ctx.values->remove (*iter);
+ if (new_ctx.values->get (*iter))
+ new_ctx.values->remove (*iter);
if (++count >= constexpr_loop_limit)
{
@@ -4258,7 +4259,8 @@ cxx_eval_loop_expr (const constexpr_ctx
/* Forget saved values of SAVE_EXPRs. */
for (hash_set::iterator iter = save_exprs.begin();
iter != save_exprs.end(); ++iter)
-new_ctx.values->remove (*iter);
+if (new_ctx.values->get (*iter))
+ new_ctx.values->remove (*iter);
return NULL_TREE;
}
--- gcc/testsuite/g++.dg/cpp1y/constexpr-89652.C.jj 2019-03-11
15:14:21.877561575 +0100
+++ gcc/testsuite/g++.dg/cpp1y/constexpr-89652.C2019-03-11
15:16:11.962763933 +0100
@@ -0,0 +1,36 @@
+// PR c++/89652
+// { dg-do compile { target c++14 } }
+// { dg-options "" }
+
+template constexpr auto foo (T ) { return e.foo (); }
+template constexpr auto bar (T ) { return foo (e); }
+template struct A { typedef T a[N]; };
+template struct B {
+ typedef T *b;
+ typename A::a d;
+ constexpr b foo () { return d; }
+};
+template struct C { long m; };
+struct D { long n; };
+template struct E {
+ B, 1>::b p;
+ constexpr D operator* () { return {p->m}; }
+ constexpr E operator++ (int) { auto a{*this}; ++p; return a; }
+};
+template
+constexpr bool operator!= (E a, E) { return a.p; }
+template
+constexpr auto baz (B s, B)
+{
+ B t{};
+ auto q{foo (t)};
+ using u = E;
+ auto v = u{bar (s)};
+ auto w = u{};
+ while (v != w)
+*q++ = *v++;
+ return t;
+}
+constexpr auto a = B, 5>{};
+auto b = B{};
+auto c = baz (a, b);
Jakub
