Fix -fcompare-debug due to DEBUG_BEGIN_STMTs (PR debug/83419)

Message ID 20171214200135.GY2353@tucnak
State New
Headers show
Series
  • Fix -fcompare-debug due to DEBUG_BEGIN_STMTs (PR debug/83419)
Related show

Commit Message

Jakub Jelinek Dec. 14, 2017, 8:01 p.m.
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

Comments

Richard Biener Dec. 15, 2017, 8:34 a.m. | #1
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

> 

> 


-- 
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)
Jakub Jelinek Dec. 15, 2017, 8:50 a.m. | #2
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
Jakub Jelinek Dec. 18, 2017, 1:02 p.m. | #3
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);
+}
Jakub Jelinek Dec. 18, 2017, 1:17 p.m. | #4
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
Jakub Jelinek Dec. 18, 2017, 1:35 p.m. | #5
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
Alexandre Oliva Dec. 21, 2017, 12:02 a.m. | #6
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);
+}


-- 
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer
Alexandre Oliva Dec. 21, 2017, 1:37 a.m. | #7
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)


-- 
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer
Jeff Law Dec. 21, 2017, 2:50 a.m. | #8
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.  */

> ---
Jeff Law Dec. 21, 2017, 4:31 a.m. | #9
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
Jakub Jelinek Dec. 21, 2017, 1:26 p.m. | #10
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
Alexandre Oliva Dec. 21, 2017, 6:09 p.m. | #11
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.

-- 
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer
Alexandre Oliva Dec. 21, 2017, 6:38 p.m. | #12
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.

-- 
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer
Alexandre Oliva Dec. 21, 2017, 8:29 p.m. | #13
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


-- 
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer
Jakub Jelinek Dec. 21, 2017, 8:30 p.m. | #14
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
Alexandre Oliva Dec. 21, 2017, 10:32 p.m. | #15
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);
+  }
+}


-- 
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer
Jakub Jelinek Dec. 21, 2017, 10:40 p.m. | #16
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

Patch

--- 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);
+}