On Fri, 5 Apr 2019, Alexandre Oliva wrote:
> On Apr 4, 2019, Richard Biener <rguent...@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 <rguent...@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); }
