Re: [PATCH] Fix -fcompare-debug due to DEBUG_BEGIN_STMTs (PR debug/83419)

[Jakub Jelinek <jakub at redhat dot com>]

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

Attachment: U917
Description: Text document