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

The following patch tries to plug the CFG cleanup forwarder block
removal hole for wrong-debug PRs.  Currently when we cannot move
debug-stmts to the destination block (because that has multiple
predecessors and thus the debug stmt could possibly not be
valid on all paths into the block) we simply drop them on the
floor.  That is of course wrong since earlier live (and wrong)
values might now be visible at this point.

My solution is to instead of dropping the debug binds still
move them to the destination but then reset them.  This solves
the wrong-debug issue.

But it of course possibly degrades debug information for the
other paths into the destination block.

I'm less sure what would be the correct action for DEBUG_BEGIN
stmts (the patch contines to drop them on the floor, leaving
reset debug-binds possibly surrounded by wrong stmt context?).
Not sure what else debug stmt kinds we have and what the proper
action for those would be.

Somewhere I've written that debug-stmts living on edges would
also solve the issue on the GIMPLE (and RTL) side, not sure
if we can make sense of those in the end though.  Having stmts
permanently on edges is also sth new on GIMPLE so I'm staying
away from that at this moment...

I've noted that for the specific case where there is
(before the next DEBUG_BEGIN_STMT?  before the next real stmt?)
(debug-) definitions of the debug vars we reset in the destination
block then dropping the debug stmt might be better and avoids
the debug-info quality degadation.  Any opinions on the boundary
(DEBUG_BEGIN_STMT, real stmt, basic-block, post-domination region)
we can look up to for such definition?  I guess that at least
looking at all PHI nodes of the destination might be a good
idea because those defs happen before the "new" debug reset.

Jakub - I remember you posted debug-info quality summaries
(location counts / extents) previously, do you have a script
to extract those from ELF objects?

Meanwhile bootstrap & regtest is running on x86_64-unknown-linux-gnu.

Richard.

2019-04-03  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.

On Apr  3, 2019, Richard Biener <rguenther@suse.de> wrote:

> My solution is to instead of dropping the debug binds still
> move them to the destination but then reset them.  This solves
> the wrong-debug issue.

*nod*, yeah, that's probably the best we can do without major surgery.

> But it of course possibly degrades debug information for the
> other paths into the destination block.

If we could find a condition that took control flow through the
forwarding path vs other paths, we could try to preserve them with a
ternary expression that uses an unbound debug temp as the reset value,
the closest to a conditional debug bind we could have right now.  But I
guess it would make no difference once we attempted to resolve the
expression and found a subexpression to be reset.

> I'm less sure what would be the correct action for DEBUG_BEGIN
> stmts (the patch contines to drop them on the floor, leaving
> reset debug-binds possibly surrounded by wrong stmt context?).

Dropping them is the best we can do now.

> Not sure what else debug stmt kinds we have and what the proper
> action for those would be.

Inline entry point markers, also to be dropped for now.

> Somewhere I've written that debug-stmts living on edges would
> also solve the issue on the GIMPLE (and RTL) side, not sure
> if we can make sense of those in the end though.

Once you go down that path, I guess you'll soon find a need for control
flow graphs within edges, at which point it gets really ugly.

I'm slightly more hopeful about identifying guarding conditions to
introduce conditional debug stmts.

> Having stmts permanently on edges is also sth new on GIMPLE so I'm
> staying away from that at this moment...

*firm nod* :-)


> I've noted that for the specific case where there is
> (before the next DEBUG_BEGIN_STMT?  before the next real stmt?)
> (debug-) definitions of the debug vars we reset in the destination
> block then dropping the debug stmt might be better and avoids
> the debug-info quality degadation.

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.

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.  But didn't we have
something to that effect already?  I vaguely recall Jakub implemented
something that would drop binds that could not be observed, and that it
became a lot less effective after adjusting it to deal with view
markers.

> I guess that at least looking at all PHI nodes of the destination
> might be a good idea because those defs happen before the "new" debug
> reset.

/me mumbles something about PHI debug binds ;-)


> 2019-04-03  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.

LGTM, thanks.

Any reason to mention PR 88882 in the Subject: but not in the ChangeLog?

On Thu, 4 Apr 2019, Alexandre Oliva wrote:

> On Apr  3, 2019, Richard Biener <rguenther@suse.de> wrote:
> 
> > My solution is to instead of dropping the debug binds still
> > move them to the destination but then reset them.  This solves
> > the wrong-debug issue.
> 
> *nod*, yeah, that's probably the best we can do without major surgery.
> 
> > But it of course possibly degrades debug information for the
> > other paths into the destination block.
> 
> If we could find a condition that took control flow through the
> forwarding path vs other paths, we could try to preserve them with a
> ternary expression that uses an unbound debug temp as the reset value,
> the closest to a conditional debug bind we could have right now.  But I
> guess it would make no difference once we attempted to resolve the
> expression and found a subexpression to be reset.
> 
> > I'm less sure what would be the correct action for DEBUG_BEGIN
> > stmts (the patch contines to drop them on the floor, leaving
> > reset debug-binds possibly surrounded by wrong stmt context?).
> 
> Dropping them is the best we can do now.
> 
> > Not sure what else debug stmt kinds we have and what the proper
> > action for those would be.
> 
> Inline entry point markers, also to be dropped for now.
> 
> > Somewhere I've written that debug-stmts living on edges would
> > also solve the issue on the GIMPLE (and RTL) side, not sure
> > if we can make sense of those in the end though.
> 
> Once you go down that path, I guess you'll soon find a need for control
> flow graphs within edges, at which point it gets really ugly.
> 
> I'm slightly more hopeful about identifying guarding conditions to
> introduce conditional debug stmts.
> 
> > Having stmts permanently on edges is also sth new on GIMPLE so I'm
> > staying away from that at this moment...
> 
> *firm nod* :-)
> 
> 
> > I've noted that for the specific case where there is
> > (before the next DEBUG_BEGIN_STMT?  before the next real stmt?)
> > (debug-) definitions of the debug vars we reset in the destination
> > block then dropping the debug stmt might be better and avoids
> > the debug-info quality degadation.
> 
> 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

void foo(int n)
{
  if (n == 0)
    return;
  int i = 0;
  do
    {
      ++i;
    }
  while (i < n);
}

where before CCP we have

  <bb 2> :
  # DEBUG BEGIN_STMT
  if (n_2(D) == 0)
    goto <bb 3>; [INV]
  else
    goto <bb 4>; [INV]

  <bb 4> :
  # DEBUG BEGIN_STMT
  i_3 = 0;
  # DEBUG i => i_3

  <bb 5> :
  # i_1 = PHI <i_3(4), i_4(5)>
  # DEBUG i => i_1
  # DEBUG BEGIN_STMT
  # DEBUG BEGIN_STMT
  i_4 = i_1 + 1;
  # DEBUG i => i_4
  if (i_4 < n_2(D))
    goto <bb 5>; [INV]
  else
    goto <bb 6>; [INV]

  <bb 3> :
  # DEBUG BEGIN_STMT
  // predicted unlikely by early return (on trees) predictor.
  goto <bb 6>; [INV]

  <bb 6> :
  return;

and then CCP propagates out i_3 = 0 resulting in

# DEBUG i => 0

and bb 4 degrading into a forwarder block CFG cleanup removes.  Now
before the patch we'd drop the above but afterwards we end up with

  <bb 4> :
  # i_1 = PHI <0(2), i_4(4)>
  # DEBUG i => NULL
  # DEBUG i => i_1
  # DEBUG BEGIN_STMT
  # DEBUG BEGIN_STMT
  i_4 = i_1 + 1;
  # DEBUG i => i_4
  if (i_4 < n_2(D))
    goto <bb 4>; [INV]
  else
    goto <bb 5>; [INV]

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
(it's now associated with a "wrong" stmt/view since we dropped the
DEBUG BEGIN_STMT as well?).  Similarly we immediately arrive at
# DEBUG i => i_1 and thus dopping would be OK even if the actual
IV were replaced by another one?

Since I've now done the above experiment in gdb I feel safer with
the idea btw - at least this kind of simple cases do not regress
visibly ;)

> 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?

>  But didn't we have
> something to that effect already?  I vaguely recall Jakub implemented
> something that would drop binds that could not be observed, and that it
> became a lot less effective after adjusting it to deal with view
> markers.
> 
> > I guess that at least looking at all PHI nodes of the destination
> > might be a good idea because those defs happen before the "new" debug
> > reset.
> 
> /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).

> 
> > 2019-04-03  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.
> 
> LGTM, thanks.
> 
> Any reason to mention PR 88882 in the Subject: but not in the ChangeLog?

I've not added its testcase (and meanwhile closed all of them as dups).

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,
Richard.

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.


>> 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!

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