| Message ID | 2663014.h6Leadj67V@arcturus.home |
|---|---|
| State | New |
| Headers | show |
On Tue, Jul 25, 2017 at 3:59 PM, Eric Botcazou <ebotcazou@adacore.com> wrote: > The attached fix to the Ada front-end introduces a regression in the ACATS > testsuite for cb4009a. The backtrace is: > > #0 operation_could_trap_helper_p(tree_code, bool, bool, bool, bool, > tree_node*, bool*) () at /home/eric/gnat/gnat-head/src/gcc/tree-eh.c:2439 > #1 0x00000000012946d7 in stmt_could_throw_1_p (stmt=<optimized out>) > at /home/eric/gnat/gnat-head/src/gcc/tree-eh.c:2759 > #2 stmt_could_throw_p(gimple*) [clone .part.186] () > at /home/eric/gnat/gnat-head/src/gcc/tree-eh.c:2809 > #3 0x0000000001295f28 in stmt_could_throw_p (stmt=0x7ffff6c5d420) > at /home/eric/gnat/gnat-head/src/gcc/tree-eh.c:2924 > #4 maybe_clean_or_replace_eh_stmt (old_stmt=old_stmt@entry=0x7ffff6c565d8, > new_stmt=new_stmt@entry=0x7ffff6c5d420) > at /home/eric/gnat/gnat-head/src/gcc/tree-eh.c:2908 > #5 0x0000000000fd8a0b in gsi_replace(gimple_stmt_iterator*, gimple*, bool) () > at /home/eric/gnat/gnat-head/src/gcc/gimple-iterator.c:447 > #6 0x0000000000fd14af in > gimple_assign_set_rhs_with_ops(gimple_stmt_iterator*, tree_code, tree_node*, > tree_node*, tree_node*) () > at /home/eric/gnat/gnat-head/src/gcc/gimple.c:1616 > #7 0x0000000000fe7af7 in replace_stmt_with_simplification (inplace=false, > seq=0x7fffffffd818, ops=0x7fffffffd820, rcode=..., gsi=0x7fffffffd8e0) > at /home/eric/gnat/gnat-head/src/gcc/gimple-fold.c:4151 > #8 fold_stmt_1(gimple_stmt_iterator*, bool, tree_node* (*)(tree_node*)) () > at /home/eric/gnat/gnat-head/src/gcc/gimple-fold.c:4462 > #9 0x0000000000fe961a in fold_stmt (gsi=gsi@entry=0x7fffffffd8e0, > valueize=valueize@entry=0x1331170 <fwprop_ssa_val(tree)>) > at /home/eric/gnat/gnat-head/src/gcc/gimple-fold.c:4689 > > The folding is turning a TRUNC_DIV_EXPR into a COND_EXPR and the code does: > > /* If the new CODE needs more operands, allocate a new statement. */ > if (gimple_num_ops (stmt) < new_rhs_ops + 1) > { > tree lhs = gimple_assign_lhs (stmt); > gimple *new_stmt = gimple_alloc (gimple_code (stmt), new_rhs_ops + 1); > memcpy (new_stmt, stmt, gimple_size (gimple_code (stmt))); > gimple_init_singleton (new_stmt); > gsi_replace (gsi, new_stmt, true); > stmt = new_stmt; > > /* The LHS needs to be reset as this also changes the SSA name > on the LHS. */ > gimple_assign_set_lhs (stmt, lhs); > } > > i.e it asks gsi_replace to update EH info, which doesn't work since the new > statement is dummy at this point. Fixed by passing false instead of true. > > Bootstrapped/regtested on x86_64-suse-linux, applied on mainline as obvious. I think we should get rid of that update-EH-info flag, always assuming false. This is IMHO a too low-level interface to do this (and we lack a gsi_replace_without_update) Not sure if you're willing to do this kind of cleanup ;) Richard. > > 2017-07-25 Eric Botcazou <ebotcazou@adacore.com> > > * gimple.c (gimple_assign_set_rhs_with_ops): Do not ask gsi_replace > to update EH info here. > > > 2017-07-25 Javier Miranda <miranda@adacore.com> > > ada/ > * checks.adb (Apply_Divide_Checks): Ensure that operands are not > evaluated twice. > > > 2017-07-25 Eric Botcazou <ebotcazou@adacore.com> > > testsuite/ > * gnat.dg/opt66.adb: New test. > > > -- > Eric Botcazou
> I think we should get rid of that update-EH-info flag, always assuming > false. This is IMHO a too low-level interface to do this (and we lack a > gsi_replace_without_update) This looks doable (there is only one call to gsi_replace that tests the return value) but the current interface is probably the most convenient.
Index: gimple.c =================================================================== --- gimple.c (revision 250379) +++ gimple.c (working copy) @@ -1613,7 +1613,7 @@ gimple_assign_set_rhs_with_ops (gimple_stmt_iterat gimple *new_stmt = gimple_alloc (gimple_code (stmt), new_rhs_ops + 1); memcpy (new_stmt, stmt, gimple_size (gimple_code (stmt))); gimple_init_singleton (new_stmt); - gsi_replace (gsi, new_stmt, true); + gsi_replace (gsi, new_stmt, false); stmt = new_stmt; /* The LHS needs to be reset as this also changes the SSA name Index: ada/checks.adb =================================================================== --- ada/checks.adb (revision 250379) +++ ada/checks.adb (working copy) @@ -1818,6 +1818,13 @@ package body Checks is and then ((not LOK) or else (Llo = LLB)) then + -- Ensure that expressions are not evaluated twice (once + -- for their runtime checks and once for their regular + -- computation). + + Force_Evaluation (Left, Mode => Strict); + Force_Evaluation (Right, Mode => Strict); + Insert_Action (N, Make_Raise_Constraint_Error (Loc, Condition =>
The attached fix to the Ada front-end introduces a regression in the ACATS testsuite for cb4009a. The backtrace is: #0 operation_could_trap_helper_p(tree_code, bool, bool, bool, bool, tree_node*, bool*) () at /home/eric/gnat/gnat-head/src/gcc/tree-eh.c:2439 #1 0x00000000012946d7 in stmt_could_throw_1_p (stmt=<optimized out>) at /home/eric/gnat/gnat-head/src/gcc/tree-eh.c:2759 #2 stmt_could_throw_p(gimple*) [clone .part.186] () at /home/eric/gnat/gnat-head/src/gcc/tree-eh.c:2809 #3 0x0000000001295f28 in stmt_could_throw_p (stmt=0x7ffff6c5d420) at /home/eric/gnat/gnat-head/src/gcc/tree-eh.c:2924 #4 maybe_clean_or_replace_eh_stmt (old_stmt=old_stmt@entry=0x7ffff6c565d8, new_stmt=new_stmt@entry=0x7ffff6c5d420) at /home/eric/gnat/gnat-head/src/gcc/tree-eh.c:2908 #5 0x0000000000fd8a0b in gsi_replace(gimple_stmt_iterator*, gimple*, bool) () at /home/eric/gnat/gnat-head/src/gcc/gimple-iterator.c:447 #6 0x0000000000fd14af in gimple_assign_set_rhs_with_ops(gimple_stmt_iterator*, tree_code, tree_node*, tree_node*, tree_node*) () at /home/eric/gnat/gnat-head/src/gcc/gimple.c:1616 #7 0x0000000000fe7af7 in replace_stmt_with_simplification (inplace=false, seq=0x7fffffffd818, ops=0x7fffffffd820, rcode=..., gsi=0x7fffffffd8e0) at /home/eric/gnat/gnat-head/src/gcc/gimple-fold.c:4151 #8 fold_stmt_1(gimple_stmt_iterator*, bool, tree_node* (*)(tree_node*)) () at /home/eric/gnat/gnat-head/src/gcc/gimple-fold.c:4462 #9 0x0000000000fe961a in fold_stmt (gsi=gsi@entry=0x7fffffffd8e0, valueize=valueize@entry=0x1331170 <fwprop_ssa_val(tree)>) at /home/eric/gnat/gnat-head/src/gcc/gimple-fold.c:4689 The folding is turning a TRUNC_DIV_EXPR into a COND_EXPR and the code does: /* If the new CODE needs more operands, allocate a new statement. */ if (gimple_num_ops (stmt) < new_rhs_ops + 1) { tree lhs = gimple_assign_lhs (stmt); gimple *new_stmt = gimple_alloc (gimple_code (stmt), new_rhs_ops + 1); memcpy (new_stmt, stmt, gimple_size (gimple_code (stmt))); gimple_init_singleton (new_stmt); gsi_replace (gsi, new_stmt, true); stmt = new_stmt; /* The LHS needs to be reset as this also changes the SSA name on the LHS. */ gimple_assign_set_lhs (stmt, lhs); } i.e it asks gsi_replace to update EH info, which doesn't work since the new statement is dummy at this point. Fixed by passing false instead of true. Bootstrapped/regtested on x86_64-suse-linux, applied on mainline as obvious. 2017-07-25 Eric Botcazou <ebotcazou@adacore.com> * gimple.c (gimple_assign_set_rhs_with_ops): Do not ask gsi_replace to update EH info here. 2017-07-25 Javier Miranda <miranda@adacore.com> ada/ * checks.adb (Apply_Divide_Checks): Ensure that operands are not evaluated twice. 2017-07-25 Eric Botcazou <ebotcazou@adacore.com> testsuite/ * gnat.dg/opt66.adb: New test.