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