[PATCH][RFC] Fix PRs 88882, 89905, 89892 (and more?)

Richard Biener rguenther@suse.de
Fri Apr 5 11:54:00 GMT 2019


On Fri, 5 Apr 2019, Alexandre Oliva wrote:

> On Apr  4, 2019, Richard Biener <rguenther@suse.de> wrote:
> 
> >> If there's any instruction or view that would be reached by the earlier
> >> bind (the one that precedes the one we'd drop or reset), it would get
> >> wrong debug information if we were to drop the bind rather than reset
> >> it.  If there isn't, whatever happens to the bind will have no effect.
> >> This suggests to me that resetting it is the right thing to do.
> 
> > Hmm.  I was thinking of
> 
> >   # i_1 = PHI <0(2), i_4(4)>
> >   # DEBUG i => NULL
> >   # DEBUG i => i_1
> >   # DEBUG BEGIN_STMT
> 
> > that might be OK(?) and in gdb I can't find it doing any harm.  What
> > I suggested was to, for example, notice that there's a PHI defining
> > 'i' in the destination and thus it would be safe to drop the debug-bind
> 
> *nod*.  It could be done.  I just meant a reset would also do.
> 
> > (it's now associated with a "wrong" stmt/view since we dropped the
> > DEBUG BEGIN_STMT as well?).
> 
> The view it would be associated with is the subsequent one, at the
> BEGIN_STMT marker, and it's overridden before that, so it doesn't
> matter.  That's why removing or resetting has the same effect in the
> end.

I see.

> >> Now, if we were to try to duplicate the logic that decides whether the
> >> bind might possibly be observable, in order to drop it on the floor
> >> instead of resetting it, we should look for another bind of the same
> >> variable before any real stmt or debug marker.  If we find one, it would
> >> be ok to drop the bind instead of resetting it.  I suppose we might even
> >> make this part of the debug bind resetting logic, or introduce separate
> >> but reusable functionality for this sort of cleanup.
> 
> > Hmm, but with the BEGIN_STMT markers still in place the views would
> > make the different values observable, no?
> 
> Not if there's another overriding bind before them, no.
> 
> >> /me mumbles something about PHI debug binds ;-)
> 
> > Eh.  But yes, sth like
> 
> > # DEBUG PHI i => <NULL(2), i(5)>
> 
> > meaning to reset on the edge into the loop and keep it "as is"
> > on the other edge would be equivalent to having the debug reset
> > on the edge.  That would of course just delay the issue to RTL
> > expansion where PHIs get replaced by copies (and that has to be
> > compare-debug safe).  OTOH in (non-CFG-layout-mode?) RTL we can
> > have insns between basic blocks (aka on edges).
> 
> Yeah, I've started (thought-)exploring these possibilities years ago,
> but didn't get very far.
> 
> > I'll see to add a guality testcase for my simple loop example above
> > (and try to trick it into going wrong with the patch some more) and
> > then apply the patch tomorrow.
> 
> Thanks!

The following is what I have applied.  I've also opened PR89983
because of debug issues in the new loop-1.c testcase.

I've checked .debug_loc size of cc1 and with the patch it's about
1% smaller.  That's probably not very useful information as
a intermediate reset might cause location ranges to be split
(larger .debug_loc) and we might just get less bogus debug info
or less useful debug info (both smaller .debug_loc).

I still think this is an important fix to correctness.

Bootstrapped / tested on x86_64-unknown-linux-gnu ontop of
r270154 to side-step the recent boostrap breakage.

Will cause

FAIL: gcc.dg/guality/loop-1.c   -O3 -fomit-frame-pointer -funroll-loops 
-fpeel-loops -ftracer -finline-functions  -DPREVENT_OPTIMIZATION  line 20 
i == 1

see the new PR for tracking this.

Richard.

2019-04-05  Richard Biener  <rguenther@suse.de>

	PR debug/89892
	PR debug/89905
	* tree-cfgcleanup.c (remove_forwarder_block): Always move
	debug bind stmts but reset them if they are not valid at the
	destination.

	* gcc.dg/guality/pr89892.c: New testcase.
	* gcc.dg/guality/pr89905.c: Likewise.
	* gcc.dg/guality/loop-1.c: Likewise.

Index: gcc/tree-cfgcleanup.c
===================================================================
--- gcc/tree-cfgcleanup.c	(revision 270114)
+++ gcc/tree-cfgcleanup.c	(working copy)
@@ -564,15 +564,39 @@ remove_forwarder_block (basic_block bb)
 	gsi_next (&gsi);
     }
 
-  /* Move debug statements if the destination has a single predecessor.  */
-  if (can_move_debug_stmts && !gsi_end_p (gsi))
+  /* Move debug statements.  Reset them if the destination does not
+     have a single predecessor.  */
+  if (!gsi_end_p (gsi))
     {
       gsi_to = gsi_after_labels (dest);
       do
 	{
 	  gimple *debug = gsi_stmt (gsi);
 	  gcc_assert (is_gimple_debug (debug));
-	  gsi_move_before (&gsi, &gsi_to);
+	  /* Move debug binds anyway, but not anything else
+	     like begin-stmt markers unless they are always
+	     valid at the destination.  */
+	  if (can_move_debug_stmts
+	      || gimple_debug_bind_p (debug))
+	    {
+	      gsi_move_before (&gsi, &gsi_to);
+	      /* Reset debug-binds that are not always valid at the
+		 destination.  Simply dropping them can cause earlier
+		 values to become live, generating wrong debug information.
+		 ???  There are several things we could improve here.  For
+		 one we might be able to move stmts to the predecessor.
+		 For anther, if the debug stmt is immediately followed
+		 by a (debug) definition in the destination (on a
+		 post-dominated path?) we can elide it without any bad
+		 effects.  */
+	      if (!can_move_debug_stmts)
+		{
+		  gimple_debug_bind_reset_value (debug);
+		  update_stmt (debug);
+		}
+	    }
+	  else
+	    gsi_next (&gsi);
 	}
       while (!gsi_end_p (gsi));
     }
Index: gcc/testsuite/gcc.dg/guality/pr89892.c
===================================================================
--- gcc/testsuite/gcc.dg/guality/pr89892.c	(nonexistent)
+++ gcc/testsuite/gcc.dg/guality/pr89892.c	(working copy)
@@ -0,0 +1,34 @@
+/* { dg-do run } */
+/* { dg-options "-g" } */
+
+void __attribute__((noinline))
+optimize_me_not ()
+{
+  __asm__ volatile ("" : : : "memory");
+}
+volatile int a;
+static short b[3][9][1] = {5};
+int c;
+int main() {
+    int i, d;
+    i = 0;
+    for (; i < 3; i++) {
+	c = 0;
+	for (; c < 9; c++) {
+	    d = 0;
+	    for (; d < 1; d++)
+	      a = b[i][c][d];
+	}
+    }
+    i = c = 0;
+    for (; c < 7; c++)
+      for (; d < 6; d++)
+	a;
+    /* i may very well be optimized out, so we cannot test for i == 0.
+       Instead test i + 1 which will make the test UNSUPPORTED if i
+       is optimized out.  Since the test previously had wrong debug
+       with i == 2 this is acceptable.  Optimally we'd produce a
+       debug stmt for the final value of the loop which would fix
+       the UNSUPPORTED cases.  */
+    optimize_me_not(); /* { dg-final { gdb-test . "i + 1" "1" } } */
+}
Index: gcc/testsuite/gcc.dg/guality/pr89905.c
===================================================================
--- gcc/testsuite/gcc.dg/guality/pr89905.c	(nonexistent)
+++ gcc/testsuite/gcc.dg/guality/pr89905.c	(working copy)
@@ -0,0 +1,39 @@
+/* { dg-do run } */
+/* { dg-options "-g" } */
+
+void __attribute__((noinline))
+optimize_me_not ()
+{
+  __asm__ volatile ("" : : : "memory");
+}
+char c, d = 22, f;
+short e, g;
+int h;
+char(a)() {}
+char(b)() { return 0; }
+void i() {
+    char j;
+    for (; h < 1;) {
+	short k[9] = {1, 1, 1, 1, 1, 1, 1, 1, 1};
+	int l, i = 5;
+	short m[3] = {0, 0, 0};
+	for (; h < 7; h++)
+	  for (; d >= 33;) {
+	      ++k[8];
+	      f = (c || a()) && g;
+	  }
+	i++;
+	j = b() || m[2];
+	l = 0;
+	for (; l <= 6; l = d)
+	  e = k[8];
+	/* i may very well be optimized out, so we cannot test for i == 6.
+	   Instead test i + 1 which will make the test UNSUPPORTED if i
+	   is optimized out.  Since the test previously had wrong debug
+	   with i == 5 this is acceptable.  Optimally we'd produce a
+	   debug stmt for the final value of the loop which would fix
+	   the UNSUPPORTED cases.  */
+	optimize_me_not(); /* { dg-final { gdb-test . "i + 1" "7" } } */
+    }
+}
+int main() { i(); }
Index: gcc/testsuite/gcc.dg/guality/loop-1.c
===================================================================
--- gcc/testsuite/gcc.dg/guality/loop-1.c	(nonexistent)
+++ gcc/testsuite/gcc.dg/guality/loop-1.c	(working copy)
@@ -0,0 +1,22 @@
+/* { dg-do run } */
+/* { dg-options "-fno-tree-scev-cprop -fno-tree-vectorize -g" } */
+
+#include "../nop.h"
+
+void __attribute__((noipa,noinline))
+foo (int n)
+{
+  if (n == 0)
+    return;
+  int i = 0;
+  do
+    {
+      ++i; /* { dg-final { gdb-test . "i" "0" } } */
+    }
+  while (i < n);
+  /* The following works only with final value replacement or with the NOP
+     but not without (which means -Og).  Vectorization breaks it, so disable
+     that.  At -O3 it currently fails, PR89983.  */
+  __asm__ volatile (NOP : : "g" (i) : "memory"); /* { dg-final { gdb-test . "i" "1" } } */
+}
+int main() { foo(1); }



More information about the Gcc-patches mailing list