| Message ID | 20171214200135.GY2353@tucnak |
|---|---|
| State | New |
| Headers | show |
| Series | Fix -fcompare-debug due to DEBUG_BEGIN_STMTs (PR debug/83419) | expand |
On Thu, 14 Dec 2017, Jakub Jelinek wrote: > Hi! > > The following testcase FAILs -fcompare-debug, because one COND_EXPR > branch from the FE during gimplifications is just <nop_expr <integer_cst>> > which doesn't have TREE_SIDE_EFFECTS, but for -gstatement-frontiers it > is a STATEMENT_LIST which contains # DEBUG BEGIN_STMT and that <nop_expr > <integer_cst>>. Ugh... the issue is that this difference might have many other -fcompare-debug issues, like when folding things? Why is it a STATEMENT_LIST rather than a COMPOUND_EXPR? > Neither # DEBUG BEGIN_STMT nor that NOP_EXPR have > TREE_SIDE_EFFECTS, but STATEMENT_LIST has TREE_SIDE_EFFECTS already from > make_node and the gimplifier (and apparently the C++ FE too) checks > just that bit. With { { { 0; } { 1; } { 2; } { 3; } } } one can end up > with quite large STATEMENT_LIST subtrees which in reality still don't > have any side-effects. > Maintaining accurate TREE_SIDE_EFFECTS bit on STATEMENT_LIST would be hard, > if we would only add and never remove statements, then we could just clear > it during make_node and set it whenever adding TREE_SIDE_EFFECTS statement > into it, but as soon as we sometimes remove from STATEMENT_LIST, or merge > STATEMENT_LISTs etc., maintaining this is too IMHO expensive, especially > when we usually just will not care about it. > So, I think it is better to just compute this lazily in the few spots where > we are interested about this, in real-world testcases most of the > STATEMENT_LISTs will have side-effects and should find them pretty early > when walking the tree. > As a side-effect, this patch will handle those > { { { 0; } { 1; } { 2; } { 3; } } } and similar then/else statement lists > better. I don't like this too much. Iff then we should do "real" lazy computation, like adding a TREE_SIDE_EFFECTS_VALID flag on STATEMENT_LIST, keeping TREE_SIDE_EFFECTS up-to-date when easily possible and when doing the expensive thing cache the result. That said, I'm not convinced this will fix -fcompare-debug issues for good. Is it really necessary to introduce this IL difference so early and in such an intrusive way? Can't we avoid adding # DEBUG BEGIN_STMT when there's not already a STATEMENT_LIST around for example? Thanks, Richard. > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? > > 2017-12-14 Jakub Jelinek <jakub@redhat.com> > > PR debug/83419 > * tree.h (statement_with_side_effects_p): Declare. > * tree.c (statement_with_side_effects_p): New function. > * gimplify.c (shortcut_cond_expr, gimplify_cond_expr): Use it. > > * cp-gimplify.c (genericize_if_stmt, gimplify_expr_stmt, > cp_fold): Use statement_with_side_effects_p instead of > just TREE_SIDE_EFFECTS. > > * gcc.dg/pr83419.c: New test. > > --- gcc/tree.h.jj 2017-12-12 09:48:15.000000000 +0100 > +++ gcc/tree.h 2017-12-14 13:22:34.157858781 +0100 > @@ -4780,6 +4780,7 @@ extern tree obj_type_ref_class (const_tr > extern bool types_same_for_odr (const_tree type1, const_tree type2, > bool strict=false); > extern bool contains_bitfld_component_ref_p (const_tree); > +extern bool statement_with_side_effects_p (tree); > extern bool block_may_fallthru (const_tree); > extern void using_eh_for_cleanups (void); > extern bool using_eh_for_cleanups_p (void); > --- gcc/tree.c.jj 2017-12-12 09:48:15.000000000 +0100 > +++ gcc/tree.c 2017-12-14 13:21:25.857752480 +0100 > @@ -12296,6 +12296,26 @@ contains_bitfld_component_ref_p (const_t > return false; > } > > +/* Return true if STMT has side-effects. This is like > + TREE_SIDE_EFFECTS (stmt), except it returns false for NULL and if STMT > + is a STATEMENT_LIST, it recurses on the statements. */ > + > +bool > +statement_with_side_effects_p (tree stmt) > +{ > + if (stmt == NULL_TREE) > + return false; > + if (TREE_CODE (stmt) != STATEMENT_LIST) > + return TREE_SIDE_EFFECTS (stmt); > + > + for (tree_stmt_iterator i = tsi_start (stmt); > + !tsi_end_p (i); tsi_next (&i)) > + if (statement_with_side_effects_p (tsi_stmt (i))) > + return true; > + > + return false; > +} > + > /* Try to determine whether a TRY_CATCH expression can fall through. > This is a subroutine of block_may_fallthru. */ > > --- gcc/gimplify.c.jj 2017-12-14 11:53:34.907142223 +0100 > +++ gcc/gimplify.c 2017-12-14 13:18:19.261184074 +0100 > @@ -3637,8 +3637,8 @@ shortcut_cond_expr (tree expr) > tree *true_label_p; > tree *false_label_p; > bool emit_end, emit_false, jump_over_else; > - bool then_se = then_ && TREE_SIDE_EFFECTS (then_); > - bool else_se = else_ && TREE_SIDE_EFFECTS (else_); > + bool then_se = statement_with_side_effects_p (then_); > + bool else_se = statement_with_side_effects_p (else_); > > /* First do simple transformations. */ > if (!else_se) > @@ -3656,7 +3656,7 @@ shortcut_cond_expr (tree expr) > if (rexpr_has_location (pred)) > SET_EXPR_LOCATION (expr, rexpr_location (pred)); > then_ = shortcut_cond_expr (expr); > - then_se = then_ && TREE_SIDE_EFFECTS (then_); > + then_se = statement_with_side_effects_p (then_); > pred = TREE_OPERAND (pred, 0); > expr = build3 (COND_EXPR, void_type_node, pred, then_, NULL_TREE); > SET_EXPR_LOCATION (expr, locus); > @@ -3678,7 +3678,7 @@ shortcut_cond_expr (tree expr) > if (rexpr_has_location (pred)) > SET_EXPR_LOCATION (expr, rexpr_location (pred)); > else_ = shortcut_cond_expr (expr); > - else_se = else_ && TREE_SIDE_EFFECTS (else_); > + else_se = statement_with_side_effects_p (else_); > pred = TREE_OPERAND (pred, 0); > expr = build3 (COND_EXPR, void_type_node, pred, NULL_TREE, else_); > SET_EXPR_LOCATION (expr, locus); > @@ -3982,9 +3982,9 @@ gimplify_cond_expr (tree *expr_p, gimple > if (gimplify_ctxp->allow_rhs_cond_expr > /* If either branch has side effects or could trap, it can't be > evaluated unconditionally. */ > - && !TREE_SIDE_EFFECTS (then_) > + && !statement_with_side_effects_p (then_) > && !generic_expr_could_trap_p (then_) > - && !TREE_SIDE_EFFECTS (else_) > + && !statement_with_side_effects_p (else_) > && !generic_expr_could_trap_p (else_)) > return gimplify_pure_cond_expr (expr_p, pre_p); > > --- gcc/cp/cp-gimplify.c.jj 2017-12-05 10:16:48.000000000 +0100 > +++ gcc/cp/cp-gimplify.c 2017-12-14 13:24:08.373614768 +0100 > @@ -176,9 +176,9 @@ genericize_if_stmt (tree *stmt_p) > if (!else_) > else_ = build_empty_stmt (locus); > > - if (integer_nonzerop (cond) && !TREE_SIDE_EFFECTS (else_)) > + if (integer_nonzerop (cond) && !statement_with_side_effects_p (else_)) > stmt = then_; > - else if (integer_zerop (cond) && !TREE_SIDE_EFFECTS (then_)) > + else if (integer_zerop (cond) && !statement_with_side_effects_p (then_)) > stmt = else_; > else > stmt = build3 (COND_EXPR, void_type_node, cond, then_, else_); > @@ -424,7 +424,7 @@ gimplify_expr_stmt (tree *stmt_p) > gimplification. */ > if (stmt && warn_unused_value) > { > - if (!TREE_SIDE_EFFECTS (stmt)) > + if (!statement_with_side_effects_p (stmt)) > { > if (!IS_EMPTY_STMT (stmt) > && !VOID_TYPE_P (TREE_TYPE (stmt)) > @@ -2096,7 +2096,7 @@ cp_fold (tree x) > /* Strip CLEANUP_POINT_EXPR if the expression doesn't have side > effects. */ > r = cp_fold_rvalue (TREE_OPERAND (x, 0)); > - if (!TREE_SIDE_EFFECTS (r)) > + if (!statement_with_side_effects_p (r)) > x = r; > break; > > --- gcc/testsuite/gcc.dg/pr83419.c.jj 2017-12-14 13:14:45.520969384 +0100 > +++ gcc/testsuite/gcc.dg/pr83419.c 2017-12-14 13:14:45.520969384 +0100 > @@ -0,0 +1,16 @@ > +/* PR debug/83419 */ > +/* { dg-do compile } */ > +/* { dg-options "-O2 -fcompare-debug" } */ > + > +int a, b; > +void foo (int, ...); > + > +void > +bar (void) > +{ > + if (a || 1 == b) > + foo (1); > + else > + 0; > + foo (1, 0); > +} > > Jakub > >
On Fri, Dec 15, 2017 at 09:34:44AM +0100, Richard Biener wrote: > Ugh... the issue is that this difference might have many other > -fcompare-debug issues, like when folding things? Why is it a > STATEMENT_LIST rather than a COMPOUND_EXPR? I believe most of other foldings don't use TREE_SIDE_EFFECTS on whole statements, just on expressions. The possible exception is STATEMENT_EXPRESSIONs. As for COMPOUND_EXPR, you mean use it only if we actually don't create a STATEMENT_LIST for other reasons? Don't we optimize away COMPOUND_EXPR lhs if it doesn't have TREE_SIDE_EFFECTS, and we'd need COMPOUND_EXPR to have no TREE_SIDE_EFFECTS as whole. > > Neither # DEBUG BEGIN_STMT nor that NOP_EXPR have > > TREE_SIDE_EFFECTS, but STATEMENT_LIST has TREE_SIDE_EFFECTS already from > > make_node and the gimplifier (and apparently the C++ FE too) checks > > just that bit. With { { { 0; } { 1; } { 2; } { 3; } } } one can end up > > with quite large STATEMENT_LIST subtrees which in reality still don't > > have any side-effects. > > Maintaining accurate TREE_SIDE_EFFECTS bit on STATEMENT_LIST would be hard, > > if we would only add and never remove statements, then we could just clear > > it during make_node and set it whenever adding TREE_SIDE_EFFECTS statement > > into it, but as soon as we sometimes remove from STATEMENT_LIST, or merge > > STATEMENT_LISTs etc., maintaining this is too IMHO expensive, especially > > when we usually just will not care about it. > > So, I think it is better to just compute this lazily in the few spots where > > we are interested about this, in real-world testcases most of the > > STATEMENT_LISTs will have side-effects and should find them pretty early > > when walking the tree. > > As a side-effect, this patch will handle those > > { { { 0; } { 1; } { 2; } { 3; } } } and similar then/else statement lists > > better. > > I don't like this too much. Iff then we should do "real" lazy > computation, like adding a TREE_SIDE_EFFECTS_VALID flag on STATEMENT_LIST, > keeping TREE_SIDE_EFFECTS up-to-date when easily possible and when doing How would that possible? I have 3 nested STATEMENT_LISTs, and remove the only statement with TREE_SIDE_EFFECTS from the innermost one. I can clear TREE_SIDE_EFFECTS_VALID from that STATEMENT_LIST easily, but what would fix up the 2 parent ones? > the expensive thing cache the result. That said, I'm not convinced > this will fix -fcompare-debug issues for good. Is it really necessary > to introduce this IL difference so early and in such an intrusive way? > > Can't we avoid adding # DEBUG BEGIN_STMT when there's not already > a STATEMENT_LIST around for example? I'll defer that to Alex. Or we could surely just unset TREE_SIDE_EFFECTS when parsing a STATEMENT_LIST that contains just a single !TREE_SIDE_EFFECTS statement other than the # DEBUG BEGIN_STMT markers. The real question is what we do without -g when removing stuff from the STATEMENT_LISTs. Do we fold those into the only statement if we end up with just one, or optimize away completely if it contains none, or do we just keep around STATEMENT_LIST containing just the 0; or nothing at all? If that is the case and whether there is a STATEMENT_LIST or not depends purely on whether we've ever created one, then perhaps clearing the TREE_SIDE_EFFECTS during parsing, or starting with clear TREE_SIDE_EFFECTS in make_node for STATEMENT_LIST and updating it on additions to the STATEMENT_LIST would do the trick. Jakub
On Fri, Dec 15, 2017 at 09:34:44AM +0100, Richard Biener wrote: > I don't like this too much. Iff then we should do "real" lazy > computation, like adding a TREE_SIDE_EFFECTS_VALID flag on STATEMENT_LIST, > keeping TREE_SIDE_EFFECTS up-to-date when easily possible and when doing > the expensive thing cache the result. That said, I'm not convinced > this will fix -fcompare-debug issues for good. Is it really necessary > to introduce this IL difference so early and in such an intrusive way? > > Can't we avoid adding # DEBUG BEGIN_STMT when there's not already > a STATEMENT_LIST around for example? The STATEMENT_LIST handling is a complete mess. E.g. tree alloc_stmt_list (void) { tree list; if (!vec_safe_is_empty (stmt_list_cache)) { list = stmt_list_cache->pop (); memset (list, 0, sizeof (struct tree_base)); TREE_SET_CODE (list, STATEMENT_LIST); } else list = make_node (STATEMENT_LIST); Because make_node (STATEMENT_LIST) sets TREE_SIDE_EFFECTS, but the above memset clears it, we start with inconsistent TREE_SIDE_EFFECTS from the beginning. I've tried to track TREE_SIDE_EFFECTS precisely, as below, but that seems to break completely the statement expression handling, in particular make check-gcc check-g++ RUNTESTFLAGS="compile.exp=20030530-3.c debug.exp='20020224-1.c 20030605-1.c' dg.exp='Wduplicated-branches-1.c compare2.c gnu89-const-expr-1.c pr64637.c pr68412-2.c stmt-expr-*.c pr56419.C' lto.exp=pr83388* noncompile.exp=971104-1.c dg-torture.exp=pr51071.c tree-ssa.exp='20030711-2.c 20030714-1.c 20030731-1.c' vect.exp=pr33833.c tm.exp=pr56419.C" all FAILs because of that, e.g. gcc/testsuite/gcc.c-torture/compile/20030530-3.c:14:8: error: void value not ignored as it ought to be I've also tried to track instead of tracking TREE_SIDE_EFFECTS precisely track just ignore DEBUG_BEGIN_STMTs in the processing, but that unfortunately doesn't help for the testcase included in the patch (attached patch). 2017-12-18 Jakub Jelinek <jakub@redhat.com> PR debug/83419 * tree-iterator.c (alloc_stmt_list): Clear TREE_SIDE_EFFECTS for a new node. (tsi_link_after): Set TREE_SIDE_EFFECTS when adding t with TREE_SIDE_EFFECTS. (tsi_link_before): Likewise. Formatting fix. (tsi_delink): Recompute TREE_SIDE_EFFECTS on removal. * gimplify.c (gimplify_statement_list): Clear TREE_SIDE_EFFECTS. * gcc.dg/pr83419.c: New test. --- gcc/tree-iterator.c.jj 2017-12-12 09:48:26.000000000 +0100 +++ gcc/tree-iterator.c 2017-12-18 10:13:58.776212769 +0100 @@ -41,7 +41,10 @@ alloc_stmt_list (void) TREE_SET_CODE (list, STATEMENT_LIST); } else - list = make_node (STATEMENT_LIST); + { + list = make_node (STATEMENT_LIST); + TREE_SIDE_EFFECTS (list) = 0; + } TREE_TYPE (list) = void_type_node; return list; } @@ -137,7 +140,7 @@ tsi_link_before (tree_stmt_iterator *i, tail = head; } - if (TREE_CODE (t) != DEBUG_BEGIN_STMT) + if (TREE_SIDE_EFFECTS (t)) TREE_SIDE_EFFECTS (i->container) = 1; cur = i->ptr; @@ -157,9 +160,9 @@ tsi_link_before (tree_stmt_iterator *i, { head->prev = STATEMENT_LIST_TAIL (i->container); if (head->prev) - head->prev->next = head; + head->prev->next = head; else - STATEMENT_LIST_HEAD (i->container) = head; + STATEMENT_LIST_HEAD (i->container) = head; STATEMENT_LIST_TAIL (i->container) = tail; } @@ -214,7 +217,7 @@ tsi_link_after (tree_stmt_iterator *i, t tail = head; } - if (TREE_CODE (t) != DEBUG_BEGIN_STMT) + if (TREE_SIDE_EFFECTS (t)) TREE_SIDE_EFFECTS (i->container) = 1; cur = i->ptr; @@ -275,10 +278,28 @@ tsi_delink (tree_stmt_iterator *i) else STATEMENT_LIST_TAIL (i->container) = prev; - if (!next && !prev) - TREE_SIDE_EFFECTS (i->container) = 0; - i->ptr = next; + + if (TREE_SIDE_EFFECTS (i->container)) + { + while (next || prev) + { + if (next) + { + if (TREE_SIDE_EFFECTS (next->stmt)) + break; + next = next->next; + } + if (prev) + { + if (TREE_SIDE_EFFECTS (prev->stmt)) + break; + prev = prev->prev; + } + } + if (next == NULL && prev == NULL) + TREE_SIDE_EFFECTS (i->container) = 0; + } } /* Return the first expression in a sequence of COMPOUND_EXPRs, or in --- gcc/gimplify.c.jj 2017-12-14 21:11:40.000000000 +0100 +++ gcc/gimplify.c 2017-12-18 10:17:07.922556324 +0100 @@ -1763,6 +1763,9 @@ gimplify_statement_list (tree *expr_p, g tree_stmt_iterator i = tsi_start (*expr_p); + /* No need to recompute TREE_SIDE_EFFECTS on removal, we are going to + delink everything. */ + TREE_SIDE_EFFECTS (*expr_p) = 0; while (!tsi_end_p (i)) { gimplify_stmt (tsi_stmt_ptr (i), pre_p); --- gcc/testsuite/gcc.dg/pr83419.c.jj 2017-12-18 10:08:24.214911480 +0100 +++ gcc/testsuite/gcc.dg/pr83419.c 2017-12-18 10:08:24.214911480 +0100 @@ -0,0 +1,16 @@ +/* PR debug/83419 */ +/* { dg-do compile } */ +/* { dg-options "-O2 -fcompare-debug" } */ + +int a, b; +void foo (int, ...); + +void +bar (void) +{ + if (a || 1 == b) + foo (1); + else + 0; + foo (1, 0); +} Jakub 2017-12-18 Jakub Jelinek <jakub@redhat.com> PR debug/83419 * tree-iterator.c: Include options.h. (alloc_stmt_list): Clear TREE_SIDE_EFFECTS for a new node. (tsi_link_after): Set TREE_SIDE_EFFECTS when adding STATEMENT_LIST with TREE_SIDE_EFFECTS or non-DEBUG_BEGIN_STMT. (tsi_link_before): Likewise. Formatting fix. (tsi_delink): Recompute TREE_SIDE_EFFECTS on removal. * gimplify.c (gimplify_statement_list): Clear TREE_SIDE_EFFECTS. * gcc.dg/pr83419.c: New test. --- gcc/tree-iterator.c.jj 2017-12-12 09:48:26.000000000 +0100 +++ gcc/tree-iterator.c 2017-12-18 13:44:02.330719440 +0100 @@ -23,6 +23,7 @@ along with GCC; see the file COPYING3. #include "coretypes.h" #include "tree.h" #include "tree-iterator.h" +#include "options.h" /* This is a cache of STATEMENT_LIST nodes. We create and destroy them @@ -41,7 +42,10 @@ alloc_stmt_list (void) TREE_SET_CODE (list, STATEMENT_LIST); } else - list = make_node (STATEMENT_LIST); + { + list = make_node (STATEMENT_LIST); + TREE_SIDE_EFFECTS (list) = 0; + } TREE_TYPE (list) = void_type_node; return list; } @@ -127,6 +131,8 @@ tsi_link_before (tree_stmt_iterator *i, gcc_assert (head == tail); return; } + if (TREE_SIDE_EFFECTS (t)) + TREE_SIDE_EFFECTS (i->container) = 1; } else { @@ -135,11 +141,10 @@ tsi_link_before (tree_stmt_iterator *i, head->next = NULL; head->stmt = t; tail = head; + if (TREE_CODE (t) != DEBUG_BEGIN_STMT) + TREE_SIDE_EFFECTS (i->container) = 1; } - if (TREE_CODE (t) != DEBUG_BEGIN_STMT) - TREE_SIDE_EFFECTS (i->container) = 1; - cur = i->ptr; /* Link it into the list. */ @@ -157,9 +162,9 @@ tsi_link_before (tree_stmt_iterator *i, { head->prev = STATEMENT_LIST_TAIL (i->container); if (head->prev) - head->prev->next = head; + head->prev->next = head; else - STATEMENT_LIST_HEAD (i->container) = head; + STATEMENT_LIST_HEAD (i->container) = head; STATEMENT_LIST_TAIL (i->container) = tail; } @@ -204,6 +209,9 @@ tsi_link_after (tree_stmt_iterator *i, t gcc_assert (head == tail); return; } + + if (TREE_SIDE_EFFECTS (t)) + TREE_SIDE_EFFECTS (i->container) = 1; } else { @@ -212,10 +220,10 @@ tsi_link_after (tree_stmt_iterator *i, t head->next = NULL; head->stmt = t; tail = head; - } - if (TREE_CODE (t) != DEBUG_BEGIN_STMT) - TREE_SIDE_EFFECTS (i->container) = 1; + if (TREE_CODE (t) != DEBUG_BEGIN_STMT) + TREE_SIDE_EFFECTS (i->container) = 1; + } cur = i->ptr; @@ -275,10 +283,30 @@ tsi_delink (tree_stmt_iterator *i) else STATEMENT_LIST_TAIL (i->container) = prev; - if (!next && !prev) - TREE_SIDE_EFFECTS (i->container) = 0; - i->ptr = next; + + if (!MAY_HAVE_DEBUG_MARKER_STMTS) + TREE_SIDE_EFFECTS (i->container) = 0; + else if (TREE_SIDE_EFFECTS (i->container)) + { + while (next || prev) + { + if (next) + { + if (TREE_CODE (next->stmt) != DEBUG_BEGIN_STMT) + break; + next = next->next; + } + if (prev) + { + if (TREE_CODE (prev->stmt) != DEBUG_BEGIN_STMT) + break; + prev = prev->prev; + } + } + if (next == NULL && prev == NULL) + TREE_SIDE_EFFECTS (i->container) = 0; + } } /* Return the first expression in a sequence of COMPOUND_EXPRs, or in --- gcc/gimplify.c.jj 2017-12-14 21:11:40.000000000 +0100 +++ gcc/gimplify.c 2017-12-18 10:17:07.922556324 +0100 @@ -1763,6 +1763,9 @@ gimplify_statement_list (tree *expr_p, g tree_stmt_iterator i = tsi_start (*expr_p); + /* No need to recompute TREE_SIDE_EFFECTS on removal, we are going to + delink everything. */ + TREE_SIDE_EFFECTS (*expr_p) = 0; while (!tsi_end_p (i)) { gimplify_stmt (tsi_stmt_ptr (i), pre_p); --- gcc/testsuite/gcc.dg/pr83419.c.jj 2017-12-18 10:08:24.214911480 +0100 +++ gcc/testsuite/gcc.dg/pr83419.c 2017-12-18 10:08:24.214911480 +0100 @@ -0,0 +1,16 @@ +/* PR debug/83419 */ +/* { dg-do compile } */ +/* { dg-options "-O2 -fcompare-debug" } */ + +int a, b; +void foo (int, ...); + +void +bar (void) +{ + if (a || 1 == b) + foo (1); + else + 0; + foo (1, 0); +}
On Mon, Dec 18, 2017 at 02:02:52PM +0100, Jakub Jelinek wrote: > Because make_node (STATEMENT_LIST) sets TREE_SIDE_EFFECTS, but the above > memset clears it, we start with inconsistent TREE_SIDE_EFFECTS from the > beginning. > > I've tried to track TREE_SIDE_EFFECTS precisely, as below, but that seems to > break completely the statement expression handling, in particular > make check-gcc check-g++ RUNTESTFLAGS="compile.exp=20030530-3.c debug.exp='20020224-1.c 20030605-1.c' dg.exp='Wduplicated-branches-1.c compare2.c gnu89-const-expr-1.c pr64637.c pr68412-2.c stmt-expr-*.c pr56419.C' lto.exp=pr83388* noncompile.exp=971104-1.c dg-torture.exp=pr51071.c tree-ssa.exp='20030711-2.c 20030714-1.c 20030731-1.c' vect.exp=pr33833.c tm.exp=pr56419.C" > all FAILs because of that, e.g. > gcc/testsuite/gcc.c-torture/compile/20030530-3.c:14:8: error: void value not ignored as it ought to be > I've also tried to track instead of tracking TREE_SIDE_EFFECTS precisely > track just ignore DEBUG_BEGIN_STMTs in the processing, but that > unfortunately doesn't help for the testcase included in the patch (attached > patch). One option would be to deal with that in pop_stmt_list and the C++ caller thereof, where we right now have: /* If the statement list contained exactly one statement, then extract it immediately. */ if (tsi_one_before_end_p (i)) { u = tsi_stmt (i); tsi_delink (&i); free_stmt_list (t); t = u; } This part would need a MAY_HAVE_DEBUG_MARKER_STMTS variant that would check for the case where there are just DEBUG_BEGIN_STMT markers + one other stmt (+ maybe some further DEBUG_BEGIN_STMT markers) and nothing else. If that one other stmt doesn't have TREE_SIDE_EFFECTS, clear TREE_SIDE_EFFECTS on the whole statement list to get the same TREE_SIDE_EFFECTS from the returned expression as without -g. /* If the statement list contained a debug begin stmt and a statement list, move the debug begin stmt into the statement list and return it. */ else if (!tsi_end_p (i) && TREE_CODE (tsi_stmt (i)) == DEBUG_BEGIN_STMT) { u = tsi_stmt (i); tsi_next (&i); if (tsi_one_before_end_p (i) && TREE_CODE (tsi_stmt (i)) == STATEMENT_LIST) { tree l = tsi_stmt (i); tsi_prev (&i); tsi_delink (&i); tsi_delink (&i); i = tsi_start (l); free_stmt_list (t); t = l; tsi_link_before (&i, u, TSI_SAME_STMT); } } I don't understand how the above hack can work reliably, first of all, it handles only a single DEBUG_BEGIN_STMT before, none after, and doesn't recurse. Perhaps it wouldn't be needed at all if we do the above, or would be just an optimization for a common case, not a correctness fix? Jakub
On Mon, Dec 18, 2017 at 02:17:18PM +0100, Jakub Jelinek wrote: > One option would be to deal with that in pop_stmt_list and the C++ caller > thereof, where we right now have: > > /* If the statement list contained exactly one statement, then > extract it immediately. */ > if (tsi_one_before_end_p (i)) > { > u = tsi_stmt (i); > tsi_delink (&i); > free_stmt_list (t); > t = u; > } > > This part would need a MAY_HAVE_DEBUG_MARKER_STMTS variant that would > check for the case where there are just DEBUG_BEGIN_STMT markers + one other > stmt (+ maybe some further DEBUG_BEGIN_STMT markers) and nothing else. > If that one other stmt doesn't have TREE_SIDE_EFFECTS, clear > TREE_SIDE_EFFECTS on the whole statement list to get the same > TREE_SIDE_EFFECTS from the returned expression as without -g. Like this (the cp_parser_omp_for_loop_init function would need similar changes). Except while it works for this new testcase and some of the testcases I've listed, it still doesn't work for others. Alex, I'm giving up here. Jakub
On Dec 15, 2017, Richard Biener <rguenther@suse.de> wrote: > On Thu, 14 Dec 2017, Jakub Jelinek wrote: >> Hi! >> >> The following testcase FAILs -fcompare-debug, because one COND_EXPR >> branch from the FE during gimplifications is just <nop_expr <integer_cst>> >> which doesn't have TREE_SIDE_EFFECTS, but for -gstatement-frontiers it >> is a STATEMENT_LIST which contains # DEBUG BEGIN_STMT and that <nop_expr >> <integer_cst>>. > Ugh... the issue is that this difference might have many other > -fcompare-debug issues, like when folding things? I fixed a number of -fcompare-debug issues related with TREE_SIDE_EFFECTS in STATEMENT_LISTs during the development of SFN. It's not too hard, really: most surviving statement lists end up with TREE_SIDE_EFFECTS set. This case, that I had not caught, was one in which the non-debug case actually discards the STATEMENT_LIST (that had TREE_SIDE_EFFECTS set), and just uses a naked expr (that does NOT have TREE_SIDE_EFFECTS). It wasn't too hard to detect and fix this case, but of course there might be others still lurking: that's why we have -fcompare-debug, and every now and then some new case pops up. Some are new regressions, but others were just latent issues that hadn't been noticed. > Why is it a STATEMENT_LIST rather than a COMPOUND_EXPR? Since we introduce the begin stmt marker in the existing statement list long before we even start parsing the corresponding statement, using a stand-alone statement made sense to me. I did not even consider using COMPOUND_EXPRs. I suspect, however, that this could cause more complications, for it would hide any specific forms that might be searched for within the COMPOUND_EXPRs. Do you not think so? I guess it wouldn't be too hard to adjust the same spot I'm touching in this patch so as to turn begin stmt markers + stmt into COMPOUND_EXPRs. > like adding a TREE_SIDE_EFFECTS_VALID flag on STATEMENT_LIST, > keeping TREE_SIDE_EFFECTS up-to-date when easily possible and when doing > the expensive thing cache the result. I experimented a bit with that yesterday. Surprisingly, just deferring the computation of TREE_SIDE_EFFECTS for STATEMENT_LISTs caused trouble: pop_stmt_list expects STATEMENT_LISTs to have TREE_SIDE_EFFECTs set unless they're empty lists. Suspecting there might be other such issues lurking, I decided to go back to the strategy I used for earlier occurrences of such bugs, namely to get the behavior to match as closely as possible what we'd get if debug stmts weren't there. It didn't take me long to get a fix this way. > Is it really necessary to introduce this IL difference so early and in > such an intrusive way? The most reasonable way to introduce markers at statement boundaries is to have the parser do so. I don't see why this should be such a big deal, though; every time I introduce such IL debug-only changes, it took me significant effort to approach the state in which nearly all of the code behaves in the same way regardless of the debug-only stuff. That things work reasonably flawlessly now is not suggestive that it was easy, or not too intrusive, but rather that the work to make it seamless despite the intrusiveness was done, at first, and then over time. That's the reason for -fcompare-debug and the various bootstrap options that stress it further. > Can't we avoid adding # DEBUG BEGIN_STMT when there's not already > a STATEMENT_LIST around for example? We could, but that would drop most of the begin_stmt markers, or none. A STATEMENT_LIST is always there, ready to get stmts, and after parsing a statement we often extract it from the statement list containing it, and throw the list away. IMHO the way these markers are being introduced is just fine, it will just take us (me?) a bit more work to shake out the few remaining bugs, just like it did when VTA was introduced. A lot of work was done before the patch was submitted to this end, but a number of problems only showed up later, on other platforms or under different combinations of optimization flags. There's no reason to expect it to be different this time. The patch below fixes the PR83419 bug. I've bootstrapped it on x86_64-, i686-, ppc64-, ppc64le-, and aarch64-linux-gnu, with -fcompare-debug in all of stage3. sparc64-linux-gnu ran into -fcompare-debug failures early in stage3, the same I ran into before, and that I'll investigate next. Ok to install? [SFN] propagate single-nondebug-stmt's side effects to enclosing list Statements without side effects, preceded by debug begin stmt markers, would become a statement list with side effects, although the stmt on its own would be extracted from the list and remain not having side effects. This causes debug info and possibly codegen differences. This patch fixes it, identifying the situation in which the stmt would have been extracted from the stmt list, and propagating the side effects flag from the stmt to the list. for gcc/ChangeLog PR debug/83419 * c-family/c-semantics.c (pop_stmt_list): Propagate side effects from single nondebug stmt to container list. for gcc/testsuite/ChangeLog PR debug/83419 * gcc.dg/pr83419.c: New. --- gcc/c-family/c-semantics.c | 9 +++++++++ gcc/testsuite/gcc.dg/pr83419.c | 16 ++++++++++++++++ 2 files changed, 25 insertions(+) create mode 100644 gcc/testsuite/gcc.dg/pr83419.c diff --git a/gcc/c-family/c-semantics.c b/gcc/c-family/c-semantics.c index cd872d8ac740..3a972c32c8ea 100644 --- a/gcc/c-family/c-semantics.c +++ b/gcc/c-family/c-semantics.c @@ -96,6 +96,15 @@ pop_stmt_list (tree t) t = l; tsi_link_before (&i, u, TSI_SAME_STMT); } + while (!tsi_end_p (i) + && TREE_CODE (tsi_stmt (i)) == DEBUG_BEGIN_STMT) + tsi_next (&i); + /* If there's only one nondebug stmt in the list, we'd have + extracted the stmt and dropped the list, and we'd take + TREE_SIDE_EFFECTS from that statement, so keep the list's + TREE_SIDE_EFFECTS in sync. */ + if (tsi_one_before_end_p (i)) + TREE_SIDE_EFFECTS (t) = TREE_SIDE_EFFECTS (tsi_stmt (i)); } } diff --git a/gcc/testsuite/gcc.dg/pr83419.c b/gcc/testsuite/gcc.dg/pr83419.c new file mode 100644 index 000000000000..3d4f1d277011 --- /dev/null +++ b/gcc/testsuite/gcc.dg/pr83419.c @@ -0,0 +1,16 @@ +/* PR debug/83419 */ +/* { dg-do compile } */ +/* { dg-options "-O2 -fcompare-debug" } */ + +int a, b; +void foo (int, ...); + +void +bar (void) +{ + if (a || 1 == b) + foo (1); + else + 0; + foo (1, 0); +}
On Dec 20, 2017, Alexandre Oliva <aoliva@redhat.com> wrote: > things work reasonably flawlessly now is not suggestive that it was > easy, or not too intrusive, but rather that the work to make it seamless > despite the intrusiveness was done, at first, and then over time. > That's the reason for -fcompare-debug and the various bootstrap options > that stress it further. > sparc64-linux-gnu ran into -fcompare-debug failures early in stage3, > the same I ran into before, and that I'll investigate next. The problems I observed on sparc64 were all similar, caused by the delayed branch infrastructure. Here's a patch that fixes it. Testing on sparc64-linux-gnu underway, but I'm posting it right away because it's so obvious. And yet, I ask: ok to install? This fixes stage3 -fcompare-debug builds of at least the builds of libbacktrace/dwarf.o, zlib/deflate.o, and libiberty/cplus-dem.o. [-fcompare-debug] retain insn locations when turning dbr seq into return A number of -fcompare-debug errors on sparc arise as we split a dbr SEQUENCE back into separate insns to turn the branch into a return. If we just take the location from the PREV_INSN, it might be a debug insn without INSN_LOCATION, or an insn with an unrelated location. But that's silly: each of the SEQUENCEd insns is still an insn with its own INSN_LOCATION, so use that instead, even though some may have been adjusted while constructing the SEQUENCE. for gcc/ChangeLog * reorg.c (make_return_insns): Reemit each insn with its own location. --- gcc/reorg.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/gcc/reorg.c b/gcc/reorg.c index 96778addce52..02d8adc61808 100644 --- a/gcc/reorg.c +++ b/gcc/reorg.c @@ -3612,9 +3612,14 @@ make_return_insns (rtx_insn *first) delete_related_insns (insn); for (i = 1; i < XVECLEN (pat, 0); i++) - prev = emit_insn_after (PATTERN (XVECEXP (pat, 0, i)), prev); + { + rtx_insn *in_seq_insn = as_a<rtx_insn *> (XVECEXP (pat, 0, i)); + prev = emit_insn_after_setloc (PATTERN (in_seq_insn), prev, + INSN_LOCATION (in_seq_insn)); + } - insn = emit_jump_insn_after (PATTERN (jump_insn), prev); + insn = emit_jump_insn_after_setloc (PATTERN (jump_insn), prev, + INSN_LOCATION (jump_insn)); emit_barrier_after (insn); if (slots)
On 12/20/2017 06:37 PM, Alexandre Oliva wrote: > On Dec 20, 2017, Alexandre Oliva <aoliva@redhat.com> wrote: > >> things work reasonably flawlessly now is not suggestive that it was >> easy, or not too intrusive, but rather that the work to make it seamless >> despite the intrusiveness was done, at first, and then over time. >> That's the reason for -fcompare-debug and the various bootstrap options >> that stress it further. > >> sparc64-linux-gnu ran into -fcompare-debug failures early in stage3, >> the same I ran into before, and that I'll investigate next. > > The problems I observed on sparc64 were all similar, caused by the > delayed branch infrastructure. Here's a patch that fixes it. Testing > on sparc64-linux-gnu underway, but I'm posting it right away because > it's so obvious. And yet, I ask: ok to install? > > This fixes stage3 -fcompare-debug builds of at least the builds of > libbacktrace/dwarf.o, zlib/deflate.o, and libiberty/cplus-dem.o. > > > [-fcompare-debug] retain insn locations when turning dbr seq into return > > A number of -fcompare-debug errors on sparc arise as we split a dbr > SEQUENCE back into separate insns to turn the branch into a return. > If we just take the location from the PREV_INSN, it might be a debug > insn without INSN_LOCATION, or an insn with an unrelated location. > But that's silly: each of the SEQUENCEd insns is still an insn with > its own INSN_LOCATION, so use that instead, even though some may have > been adjusted while constructing the SEQUENCE. > > for gcc/ChangeLog > > * reorg.c (make_return_insns): Reemit each insn with its own > location. OK. FWIW I wouldn't be surprised if there's other places in reorg that are going to need similar fixes. FOr example, look at the code after these two comments: /* Delete the RETURN and just execute the delay list insns. We do this by deleting the INSN containing the SEQUENCE, then re-emitting the insns separately, and then deleting the RETURN. This allows the count of the jump target to be properly decremented. Note that we need to change the INSN_UID of the re-emitted insns since it is used to hash the insns for mark_target_live_regs and the re-emitted insns will no longer be wrapped up in a SEQUENCE. Clear the from target bit, since these insns are no longer in delay slots. */ /* All this insn does is execute its delay list and jump to the following insn. So delete the jump and just execute the delay list insns. We do this by deleting the INSN containing the SEQUENCE, then re-emitting the insns separately, and then deleting the jump. This allows the count of the jump target to be properly decremented. Note that we need to change the INSN_UID of the re-emitted insns since it is used to hash the insns for mark_target_live_regs and the re-emitted insns will no longer be wrapped up in a SEQUENCE. Clear the from target bit, since these insns are no longer in delay slots. */ > ---
On 12/20/2017 05:02 PM, Alexandre Oliva wrote: > On Dec 15, 2017, Richard Biener <rguenther@suse.de> wrote: > >> On Thu, 14 Dec 2017, Jakub Jelinek wrote: >>> Hi! >>> >>> The following testcase FAILs -fcompare-debug, because one COND_EXPR >>> branch from the FE during gimplifications is just <nop_expr <integer_cst>> >>> which doesn't have TREE_SIDE_EFFECTS, but for -gstatement-frontiers it >>> is a STATEMENT_LIST which contains # DEBUG BEGIN_STMT and that <nop_expr >>> <integer_cst>>. > >> Ugh... the issue is that this difference might have many other >> -fcompare-debug issues, like when folding things? > > I fixed a number of -fcompare-debug issues related with > TREE_SIDE_EFFECTS in STATEMENT_LISTs during the development of SFN. > It's not too hard, really: most surviving statement lists end up with > TREE_SIDE_EFFECTS set. > > This case, that I had not caught, was one in which the non-debug case > actually discards the STATEMENT_LIST (that had TREE_SIDE_EFFECTS set), > and just uses a naked expr (that does NOT have TREE_SIDE_EFFECTS). It > wasn't too hard to detect and fix this case, but of course there might > be others still lurking: that's why we have -fcompare-debug, and every > now and then some new case pops up. Some are new regressions, but > others were just latent issues that hadn't been noticed. > >> Why is it a STATEMENT_LIST rather than a COMPOUND_EXPR? > > Since we introduce the begin stmt marker in the existing statement list > long before we even start parsing the corresponding statement, using a > stand-alone statement made sense to me. I did not even consider using > COMPOUND_EXPRs. > > I suspect, however, that this could cause more complications, for it > would hide any specific forms that might be searched for within the > COMPOUND_EXPRs. Do you not think so? I guess it wouldn't be too hard > to adjust the same spot I'm touching in this patch so as to turn begin > stmt markers + stmt into COMPOUND_EXPRs. > >> like adding a TREE_SIDE_EFFECTS_VALID flag on STATEMENT_LIST, >> keeping TREE_SIDE_EFFECTS up-to-date when easily possible and when doing >> the expensive thing cache the result. > > I experimented a bit with that yesterday. Surprisingly, just deferring > the computation of TREE_SIDE_EFFECTS for STATEMENT_LISTs caused trouble: > pop_stmt_list expects STATEMENT_LISTs to have TREE_SIDE_EFFECTs set > unless they're empty lists. Suspecting there might be other such issues > lurking, I decided to go back to the strategy I used for earlier > occurrences of such bugs, namely to get the behavior to match as closely > as possible what we'd get if debug stmts weren't there. It didn't take > me long to get a fix this way. > >> Is it really necessary to introduce this IL difference so early and in >> such an intrusive way? > > The most reasonable way to introduce markers at statement boundaries is > to have the parser do so. I don't see why this should be such a big > deal, though; every time I introduce such IL debug-only changes, it took > me significant effort to approach the state in which nearly all of the > code behaves in the same way regardless of the debug-only stuff. That > things work reasonably flawlessly now is not suggestive that it was > easy, or not too intrusive, but rather that the work to make it seamless > despite the intrusiveness was done, at first, and then over time. > That's the reason for -fcompare-debug and the various bootstrap options > that stress it further. > >> Can't we avoid adding # DEBUG BEGIN_STMT when there's not already >> a STATEMENT_LIST around for example? > > We could, but that would drop most of the begin_stmt markers, or none. > A STATEMENT_LIST is always there, ready to get stmts, and after parsing > a statement we often extract it from the statement list containing it, > and throw the list away. > > IMHO the way these markers are being introduced is just fine, it will > just take us (me?) a bit more work to shake out the few remaining bugs, > just like it did when VTA was introduced. A lot of work was done before > the patch was submitted to this end, but a number of problems only > showed up later, on other platforms or under different combinations of > optimization flags. There's no reason to expect it to be different this > time. > > > > The patch below fixes the PR83419 bug. I've bootstrapped it on x86_64-, > i686-, ppc64-, ppc64le-, and aarch64-linux-gnu, with -fcompare-debug in > all of stage3. sparc64-linux-gnu ran into -fcompare-debug failures > early in stage3, the same I ran into before, and that I'll investigate > next. > > Ok to install? > > > [SFN] propagate single-nondebug-stmt's side effects to enclosing list > > Statements without side effects, preceded by debug begin stmt markers, > would become a statement list with side effects, although the stmt on > its own would be extracted from the list and remain not having side > effects. This causes debug info and possibly codegen differences. > This patch fixes it, identifying the situation in which the stmt would > have been extracted from the stmt list, and propagating the side > effects flag from the stmt to the list. > > for gcc/ChangeLog > > PR debug/83419 > * c-family/c-semantics.c (pop_stmt_list): Propagate side > effects from single nondebug stmt to container list. > > for gcc/testsuite/ChangeLog > > PR debug/83419 > * gcc.dg/pr83419.c: New. OK. jeff
On Wed, Dec 20, 2017 at 10:02:24PM -0200, Alexandre Oliva wrote: > --- a/gcc/c-family/c-semantics.c > +++ b/gcc/c-family/c-semantics.c > @@ -96,6 +96,15 @@ pop_stmt_list (tree t) > t = l; > tsi_link_before (&i, u, TSI_SAME_STMT); > } > + while (!tsi_end_p (i) > + && TREE_CODE (tsi_stmt (i)) == DEBUG_BEGIN_STMT) > + tsi_next (&i); > + /* If there's only one nondebug stmt in the list, we'd have > + extracted the stmt and dropped the list, and we'd take > + TREE_SIDE_EFFECTS from that statement, so keep the list's > + TREE_SIDE_EFFECTS in sync. */ > + if (tsi_one_before_end_p (i)) > + TREE_SIDE_EFFECTS (t) = TREE_SIDE_EFFECTS (tsi_stmt (i)); So DEBUG_BEGIN_STMTs after the single non-DEBUG_BEGIN_STMT stmt in the STATEMENT_LIST can't happen (or just we don't have a testcase for it)? Jakub
On Dec 21, 2017, Jakub Jelinek <jakub@redhat.com> wrote: > On Wed, Dec 20, 2017 at 10:02:24PM -0200, Alexandre Oliva wrote: >> --- a/gcc/c-family/c-semantics.c >> +++ b/gcc/c-family/c-semantics.c >> @@ -96,6 +96,15 @@ pop_stmt_list (tree t) >> + while (!tsi_end_p (i) >> + && TREE_CODE (tsi_stmt (i)) == DEBUG_BEGIN_STMT) >> + tsi_next (&i); >> + /* If there's only one nondebug stmt in the list, we'd have >> + extracted the stmt and dropped the list, and we'd take >> + TREE_SIDE_EFFECTS from that statement, so keep the list's >> + TREE_SIDE_EFFECTS in sync. */ >> + if (tsi_one_before_end_p (i)) >> + TREE_SIDE_EFFECTS (t) = TREE_SIDE_EFFECTS (tsi_stmt (i)); > So DEBUG_BEGIN_STMTs after the single non-DEBUG_BEGIN_STMT stmt in > the STATEMENT_LIST can't happen (or just we don't have a testcase for it)? AFAICT, if they do at this point, it implies multiple nondebug stmts, thus nonzero TREE_SIDE_EFFECTS for the list. I'm not even sure we need to skip multiple being stmt markers at this point, but I couldn't rule that out easily, so I put the loop in.
On Dec 21, 2017, Jeff Law <law@redhat.com> wrote: > FWIW I wouldn't be surprised if there's other places in reorg that are > going to need similar fixes. *nod*. I looked around, and the only place that seemed suspicious was find_end_label, but I'm not sure we might actually use a debug insn as the insertion point there. I could conservatively tweak it to avoid the possibility, but I'm usually more comfortable when I have a testcase that shows the problem for sure. The other places that emit insns seem to be (re)emitting actual insns, or emitting copies thereof, and in these cases their preexisting locations are retained/copied. That's unlike the just-fixed fragment, that took just a pattern and emitted it as a new insn before or after another: this copied the INSN_LOCATION from the other insn, which is undesirable.
On Dec 20, 2017, Alexandre Oliva <aoliva@redhat.com> wrote: > for gcc/ChangeLog > PR debug/83419 > * c-family/c-semantics.c (pop_stmt_list): Propagate side > effects from single nondebug stmt to container list. Oops, this belonged in gcc/c-family/ChangeLog. I'm checking this in to fix it. diff --git a/gcc/ChangeLog b/gcc/ChangeLog index 61ae2b6eaa27..126f505184d1 100644 --- a/gcc/ChangeLog +++ b/gcc/ChangeLog @@ -41,12 +41,6 @@ * reorg.c (make_return_insns): Reemit each insn with its own location. -2017-12-21 Alexandre Oliva <aoliva@redhat.com> - - PR debug/83419 - * c-family/c-semantics.c (pop_stmt_list): Propagate side - effects from single nondebug stmt to container list. - 2017-12-21 James Greenhalgh <james.greenhalgh@arm.com> * config/aarch64/aarch64.c (aarch64_expand_vector_init): Modify code diff --git a/gcc/c-family/ChangeLog b/gcc/c-family/ChangeLog index 996e7b8eaf5d..f0132f46ad8a 100644 --- a/gcc/c-family/ChangeLog +++ b/gcc/c-family/ChangeLog @@ -1,3 +1,9 @@ +2017-12-21 Alexandre Oliva <aoliva@redhat.com> + + PR debug/83419 + * c-family/c-semantics.c (pop_stmt_list): Propagate side + effects from single nondebug stmt to container list. + 2017-12-19 Jakub Jelinek <jakub@redhat.com> * known-headers.cc (get_stdlib_header_for_name): Replace Yoda
On Thu, Dec 21, 2017 at 06:29:11PM -0200, Alexandre Oliva wrote: > > PR debug/83419 > > * c-family/c-semantics.c (pop_stmt_list): Propagate side > > effects from single nondebug stmt to container list. > > Oops, this belonged in gcc/c-family/ChangeLog. I'm checking this in to > fix it. > > diff --git a/gcc/ChangeLog b/gcc/ChangeLog > index 61ae2b6eaa27..126f505184d1 100644 > --- a/gcc/ChangeLog > +++ b/gcc/ChangeLog > @@ -41,12 +41,6 @@ > > * reorg.c (make_return_insns): Reemit each insn with its own location. > > -2017-12-21 Alexandre Oliva <aoliva@redhat.com> > - > - PR debug/83419 > - * c-family/c-semantics.c (pop_stmt_list): Propagate side > - effects from single nondebug stmt to container list. > - > 2017-12-21 James Greenhalgh <james.greenhalgh@arm.com> > > * config/aarch64/aarch64.c (aarch64_expand_vector_init): Modify code > diff --git a/gcc/c-family/ChangeLog b/gcc/c-family/ChangeLog > index 996e7b8eaf5d..f0132f46ad8a 100644 > --- a/gcc/c-family/ChangeLog > +++ b/gcc/c-family/ChangeLog > @@ -1,3 +1,9 @@ > +2017-12-21 Alexandre Oliva <aoliva@redhat.com> > + > + PR debug/83419 > + * c-family/c-semantics.c (pop_stmt_list): Propagate side Still not right. c-family/ prefix shouldn't be there. > + effects from single nondebug stmt to container list. > + > 2017-12-19 Jakub Jelinek <jakub@redhat.com> > > * known-headers.cc (get_stdlib_header_for_name): Replace Yoda Jakub
On Dec 21, 2017, Jakub Jelinek <jakub@redhat.com> wrote: > On Wed, Dec 20, 2017 at 10:02:24PM -0200, Alexandre Oliva wrote: >> + if (tsi_one_before_end_p (i)) >> + TREE_SIDE_EFFECTS (t) = TREE_SIDE_EFFECTS (tsi_stmt (i)); > So DEBUG_BEGIN_STMTs after the single non-DEBUG_BEGIN_STMT stmt in > the STATEMENT_LIST can't happen (or just we don't have a testcase for it)? I know I said it couldn't, but while looking into PR83527 I decided to actually check that, and my conclusion quickly proved to be false. So the patch below fixes that, as well as the newer PR. This has completed -fcompare-debug bootstrap on i686- and ppc64le-linux-gnu; x86_64-linux-gnu is almost done, and regression testing for them all is underway or about to start. Ok to install if it all passes? for gcc/c-family/ChangeLog PR debug/83527 PR debug/83419 * c-semantics.c (only_debug_stmts_after_p): New. (pop_stmt_list): Clear side effects in debug-only stmt list. Check for single nondebug stmt followed by debug stmts only. for gcc/testsuite/ChangeLog PR debug/83527 PR debug/83419 * gcc.dg/pr83527.c: New. --- gcc/c-family/c-semantics.c | 23 +++++++++++++++++++---- gcc/testsuite/gcc.dg/pr83527.c | 26 ++++++++++++++++++++++++++ 2 files changed, 45 insertions(+), 4 deletions(-) create mode 100644 gcc/testsuite/gcc.dg/pr83527.c diff --git a/gcc/c-family/c-semantics.c b/gcc/c-family/c-semantics.c index 3a972c32c8ea..21d908ed06c1 100644 --- a/gcc/c-family/c-semantics.c +++ b/gcc/c-family/c-semantics.c @@ -35,6 +35,17 @@ push_stmt_list (void) return t; } +/* Return TRUE if, after I, there are any nondebug stmts. */ + +static inline bool +only_debug_stmts_after_p (tree_stmt_iterator i) +{ + for (tsi_next (&i); !tsi_end_p (i); tsi_next (&i)) + if (TREE_CODE (tsi_stmt (i)) != DEBUG_BEGIN_STMT) + return false; + return true; +} + /* Finish the statement tree rooted at T. */ tree @@ -99,11 +110,15 @@ pop_stmt_list (tree t) while (!tsi_end_p (i) && TREE_CODE (tsi_stmt (i)) == DEBUG_BEGIN_STMT) tsi_next (&i); - /* If there's only one nondebug stmt in the list, we'd have - extracted the stmt and dropped the list, and we'd take - TREE_SIDE_EFFECTS from that statement, so keep the list's + /* If there are only debug stmts in the list, without them + we'd have an empty stmt without side effects. If there's + only one nondebug stmt, we'd have extracted the stmt and + dropped the list, and we'd take TREE_SIDE_EFFECTS from + that statement. In either case, keep the list's TREE_SIDE_EFFECTS in sync. */ - if (tsi_one_before_end_p (i)) + if (tsi_end_p (i)) + TREE_SIDE_EFFECTS (t) = 0; + else if (only_debug_stmts_after_p (i)) TREE_SIDE_EFFECTS (t) = TREE_SIDE_EFFECTS (tsi_stmt (i)); } } diff --git a/gcc/testsuite/gcc.dg/pr83527.c b/gcc/testsuite/gcc.dg/pr83527.c new file mode 100644 index 000000000000..effef439ac07 --- /dev/null +++ b/gcc/testsuite/gcc.dg/pr83527.c @@ -0,0 +1,26 @@ +/* PR debug/83527 */ +/* { dg-do compile } */ +/* { dg-options "-O2 -fcompare-debug" } */ + +extern void fn2(void); +extern void fn3(void); +int a, b; +void fn1() { + int c; + short d; + switch (a) { + case 32800: + fn2(); + case 32769: + b = 0; + case 32771: + case 32772: + case 32782: + fn3(); + } + if (d || c) { + do + ; + while (0); + } +}
On Thu, Dec 21, 2017 at 08:32:28PM -0200, Alexandre Oliva wrote: > for gcc/c-family/ChangeLog > > PR debug/83527 > PR debug/83419 > * c-semantics.c (only_debug_stmts_after_p): New. > (pop_stmt_list): Clear side effects in debug-only stmt list. > Check for single nondebug stmt followed by debug stmts only. > > for gcc/testsuite/ChangeLog > > PR debug/83527 > PR debug/83419 > * gcc.dg/pr83527.c: New. Ok, thanks. Jakub
--- gcc/tree.h.jj 2017-12-12 09:48:15.000000000 +0100 +++ gcc/tree.h 2017-12-14 13:22:34.157858781 +0100 @@ -4780,6 +4780,7 @@ extern tree obj_type_ref_class (const_tr extern bool types_same_for_odr (const_tree type1, const_tree type2, bool strict=false); extern bool contains_bitfld_component_ref_p (const_tree); +extern bool statement_with_side_effects_p (tree); extern bool block_may_fallthru (const_tree); extern void using_eh_for_cleanups (void); extern bool using_eh_for_cleanups_p (void); --- gcc/tree.c.jj 2017-12-12 09:48:15.000000000 +0100 +++ gcc/tree.c 2017-12-14 13:21:25.857752480 +0100 @@ -12296,6 +12296,26 @@ contains_bitfld_component_ref_p (const_t return false; } +/* Return true if STMT has side-effects. This is like + TREE_SIDE_EFFECTS (stmt), except it returns false for NULL and if STMT + is a STATEMENT_LIST, it recurses on the statements. */ + +bool +statement_with_side_effects_p (tree stmt) +{ + if (stmt == NULL_TREE) + return false; + if (TREE_CODE (stmt) != STATEMENT_LIST) + return TREE_SIDE_EFFECTS (stmt); + + for (tree_stmt_iterator i = tsi_start (stmt); + !tsi_end_p (i); tsi_next (&i)) + if (statement_with_side_effects_p (tsi_stmt (i))) + return true; + + return false; +} + /* Try to determine whether a TRY_CATCH expression can fall through. This is a subroutine of block_may_fallthru. */ --- gcc/gimplify.c.jj 2017-12-14 11:53:34.907142223 +0100 +++ gcc/gimplify.c 2017-12-14 13:18:19.261184074 +0100 @@ -3637,8 +3637,8 @@ shortcut_cond_expr (tree expr) tree *true_label_p; tree *false_label_p; bool emit_end, emit_false, jump_over_else; - bool then_se = then_ && TREE_SIDE_EFFECTS (then_); - bool else_se = else_ && TREE_SIDE_EFFECTS (else_); + bool then_se = statement_with_side_effects_p (then_); + bool else_se = statement_with_side_effects_p (else_); /* First do simple transformations. */ if (!else_se) @@ -3656,7 +3656,7 @@ shortcut_cond_expr (tree expr) if (rexpr_has_location (pred)) SET_EXPR_LOCATION (expr, rexpr_location (pred)); then_ = shortcut_cond_expr (expr); - then_se = then_ && TREE_SIDE_EFFECTS (then_); + then_se = statement_with_side_effects_p (then_); pred = TREE_OPERAND (pred, 0); expr = build3 (COND_EXPR, void_type_node, pred, then_, NULL_TREE); SET_EXPR_LOCATION (expr, locus); @@ -3678,7 +3678,7 @@ shortcut_cond_expr (tree expr) if (rexpr_has_location (pred)) SET_EXPR_LOCATION (expr, rexpr_location (pred)); else_ = shortcut_cond_expr (expr); - else_se = else_ && TREE_SIDE_EFFECTS (else_); + else_se = statement_with_side_effects_p (else_); pred = TREE_OPERAND (pred, 0); expr = build3 (COND_EXPR, void_type_node, pred, NULL_TREE, else_); SET_EXPR_LOCATION (expr, locus); @@ -3982,9 +3982,9 @@ gimplify_cond_expr (tree *expr_p, gimple if (gimplify_ctxp->allow_rhs_cond_expr /* If either branch has side effects or could trap, it can't be evaluated unconditionally. */ - && !TREE_SIDE_EFFECTS (then_) + && !statement_with_side_effects_p (then_) && !generic_expr_could_trap_p (then_) - && !TREE_SIDE_EFFECTS (else_) + && !statement_with_side_effects_p (else_) && !generic_expr_could_trap_p (else_)) return gimplify_pure_cond_expr (expr_p, pre_p); --- gcc/cp/cp-gimplify.c.jj 2017-12-05 10:16:48.000000000 +0100 +++ gcc/cp/cp-gimplify.c 2017-12-14 13:24:08.373614768 +0100 @@ -176,9 +176,9 @@ genericize_if_stmt (tree *stmt_p) if (!else_) else_ = build_empty_stmt (locus); - if (integer_nonzerop (cond) && !TREE_SIDE_EFFECTS (else_)) + if (integer_nonzerop (cond) && !statement_with_side_effects_p (else_)) stmt = then_; - else if (integer_zerop (cond) && !TREE_SIDE_EFFECTS (then_)) + else if (integer_zerop (cond) && !statement_with_side_effects_p (then_)) stmt = else_; else stmt = build3 (COND_EXPR, void_type_node, cond, then_, else_); @@ -424,7 +424,7 @@ gimplify_expr_stmt (tree *stmt_p) gimplification. */ if (stmt && warn_unused_value) { - if (!TREE_SIDE_EFFECTS (stmt)) + if (!statement_with_side_effects_p (stmt)) { if (!IS_EMPTY_STMT (stmt) && !VOID_TYPE_P (TREE_TYPE (stmt)) @@ -2096,7 +2096,7 @@ cp_fold (tree x) /* Strip CLEANUP_POINT_EXPR if the expression doesn't have side effects. */ r = cp_fold_rvalue (TREE_OPERAND (x, 0)); - if (!TREE_SIDE_EFFECTS (r)) + if (!statement_with_side_effects_p (r)) x = r; break; --- gcc/testsuite/gcc.dg/pr83419.c.jj 2017-12-14 13:14:45.520969384 +0100 +++ gcc/testsuite/gcc.dg/pr83419.c 2017-12-14 13:14:45.520969384 +0100 @@ -0,0 +1,16 @@ +/* PR debug/83419 */ +/* { dg-do compile } */ +/* { dg-options "-O2 -fcompare-debug" } */ + +int a, b; +void foo (int, ...); + +void +bar (void) +{ + if (a || 1 == b) + foo (1); + else + 0; + foo (1, 0); +}
Hi! The following testcase FAILs -fcompare-debug, because one COND_EXPR branch from the FE during gimplifications is just <nop_expr <integer_cst>> which doesn't have TREE_SIDE_EFFECTS, but for -gstatement-frontiers it is a STATEMENT_LIST which contains # DEBUG BEGIN_STMT and that <nop_expr <integer_cst>>. Neither # DEBUG BEGIN_STMT nor that NOP_EXPR have TREE_SIDE_EFFECTS, but STATEMENT_LIST has TREE_SIDE_EFFECTS already from make_node and the gimplifier (and apparently the C++ FE too) checks just that bit. With { { { 0; } { 1; } { 2; } { 3; } } } one can end up with quite large STATEMENT_LIST subtrees which in reality still don't have any side-effects. Maintaining accurate TREE_SIDE_EFFECTS bit on STATEMENT_LIST would be hard, if we would only add and never remove statements, then we could just clear it during make_node and set it whenever adding TREE_SIDE_EFFECTS statement into it, but as soon as we sometimes remove from STATEMENT_LIST, or merge STATEMENT_LISTs etc., maintaining this is too IMHO expensive, especially when we usually just will not care about it. So, I think it is better to just compute this lazily in the few spots where we are interested about this, in real-world testcases most of the STATEMENT_LISTs will have side-effects and should find them pretty early when walking the tree. As a side-effect, this patch will handle those { { { 0; } { 1; } { 2; } { 3; } } } and similar then/else statement lists better. Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? 2017-12-14 Jakub Jelinek <jakub@redhat.com> PR debug/83419 * tree.h (statement_with_side_effects_p): Declare. * tree.c (statement_with_side_effects_p): New function. * gimplify.c (shortcut_cond_expr, gimplify_cond_expr): Use it. * cp-gimplify.c (genericize_if_stmt, gimplify_expr_stmt, cp_fold): Use statement_with_side_effects_p instead of just TREE_SIDE_EFFECTS. * gcc.dg/pr83419.c: New test. Jakub