[PATCH] Fix dead_debug_insert_before ICE (PR debug/49522, take 3)
On Wed, Jul 06, 2011 at 10:36:02PM +0200, Eric Botcazou wrote:
And here is a version that passed bootstrap/regtest on x86_64-linux and
i686-linux:
2011-07-06 Jakub Jelinek ja...@redhat.com
PR debug/49522
* df-problems.c (dead_debug_reset): Remove dead_debug_uses
referencing debug insns that have been reset.
(dead_debug_insert_before): Don't assert reg is non-NULL,
instead return immediately if it is NULL.
* gcc.dg/debug/pr49522.c: New test.
Sorry, our messages crossed. I'd set a flag in the first loop. In the end,
it's up to you.
Actually, looking at it some more, dead_debug_use structs referencing the
same insn are always adjacent due to the way how they are added using
dead_debug_add. While some of the dead_debug_use records might preceede
the record because of which it is reset, it isn't hard to remember a pointer
pointing to the pointer to the first entry for the current insn.
So, here is a new patch which doesn't need two loops, just might go a little
bit backwards to unchain dead_debug_use for the reset insn.
It still needs the change of the gcc_assert (reg) into if (reg == NULL)
return;, because the dead-used bitmap is with this sometimes a false
positive (saying that a regno is referenced even when it isn't).
But here it is IMHO better to occassionaly live with the false positives,
which just means we'll sometimes once walk the chain in dead_debug_reset
or dead_debug_insert_before before resetting it, than to recompute the
bitmap (we'd need a second loop for that, bitmap_clear (debug-used) and
populate it again).
Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
2011-07-07 Jakub Jelinek ja...@redhat.com
PR debug/49522
* df-problems.c (dead_debug_reset): Remove dead_debug_uses
referencing debug insns that have been reset.
(dead_debug_insert_before): Don't assert reg is non-NULL,
instead return immediately if it is NULL.
* gcc.dg/debug/pr49522.c: New test.
--- gcc/df-problems.c.jj2011-07-07 02:32:45.928547053 +0200
+++ gcc/df-problems.c 2011-07-07 09:57:34.846464573 +0200
@@ -3096,6 +3096,7 @@ static void
dead_debug_reset (struct dead_debug *debug, unsigned int dregno)
{
struct dead_debug_use **tailp = debug-head;
+ struct dead_debug_use **insnp = debug-head;
struct dead_debug_use *cur;
rtx insn;
@@ -3113,9 +3114,21 @@ dead_debug_reset (struct dead_debug *deb
debug-to_rescan = BITMAP_ALLOC (NULL);
bitmap_set_bit (debug-to_rescan, INSN_UID (insn));
XDELETE (cur);
+ if (tailp != insnp DF_REF_INSN ((*insnp)-use) == insn)
+ tailp = insnp;
+ while ((cur = *tailp) DF_REF_INSN (cur-use) == insn)
+ {
+ *tailp = cur-next;
+ XDELETE (cur);
+ }
+ insnp = tailp;
}
else
- tailp = (*tailp)-next;
+ {
+ if (DF_REF_INSN ((*insnp)-use) != DF_REF_INSN (cur-use))
+ insnp = tailp;
+ tailp = (*tailp)-next;
+ }
}
}
@@ -3174,7 +3187,8 @@ dead_debug_insert_before (struct dead_de
tailp = (*tailp)-next;
}
- gcc_assert (reg);
+ if (reg == NULL)
+return;
/* Create DEBUG_EXPR (and DEBUG_EXPR_DECL). */
dval = make_debug_expr_from_rtl (reg);
--- gcc/testsuite/gcc.dg/debug/pr49522.c.jj 2011-07-04 10:54:23.0
+0200
+++ gcc/testsuite/gcc.dg/debug/pr49522.c2011-07-04 10:54:02.0
+0200
@@ -0,0 +1,41 @@
+/* PR debug/49522 */
+/* { dg-do compile } */
+/* { dg-options -fcompare-debug } */
+
+int val1 = 0L;
+volatile int val2 = 7L;
+long long val3;
+int *ptr = val1;
+
+static int
+func1 ()
+{
+ return 0;
+}
+
+static short int
+func2 (short int a, unsigned int b)
+{
+ return !b ? a : a b;
+}
+
+static unsigned long long
+func3 (unsigned long long a, unsigned long long b)
+{
+ return !b ? a : a % b;
+}
+
+void
+func4 (unsigned short arg1, int arg2)
+{
+ for (arg2 = 0; arg2 2; arg2++)
+{
+ *ptr = func3 (func3 (10, func2 (val3, val2)), val3);
+ for (arg1 = -14; arg1 14; arg1 = func1 ())
+ {
+ *ptr = -1;
+ if (foo ())
+ ;
+ }
+}
+}
Jakub
Re: [PATCH] Fix dead_debug_insert_before ICE (PR debug/49522, take 3)
So, here is a new patch which doesn't need two loops, just might go a
little bit backwards to unchain dead_debug_use for the reset insn.
It still needs the change of the gcc_assert (reg) into if (reg == NULL)
return;, because the dead-used bitmap is with this sometimes a false
positive (saying that a regno is referenced even when it isn't).
But here it is IMHO better to occassionaly live with the false positives,
which just means we'll sometimes once walk the chain in dead_debug_reset
or dead_debug_insert_before before resetting it, than to recompute the
bitmap (we'd need a second loop for that, bitmap_clear (debug-used) and
populate it again).
Fine with me for both points, but move some bits of these explanations to the
code itself because this isn't obvious. For example see below.
Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
2011-07-07 Jakub Jelinek ja...@redhat.com
PR debug/49522
* df-problems.c (dead_debug_reset): Remove dead_debug_uses
referencing debug insns that have been reset.
(dead_debug_insert_before): Don't assert reg is non-NULL,
instead return immediately if it is NULL.
* gcc.dg/debug/pr49522.c: New test.
OK, thanks.
--- gcc/df-problems.c.jj 2011-07-07 02:32:45.928547053 +0200
+++ gcc/df-problems.c 2011-07-07 09:57:34.846464573 +0200
@@ -3096,6 +3096,7 @@ static void
dead_debug_reset (struct dead_debug *debug, unsigned int dregno)
{
struct dead_debug_use **tailp = debug-head;
+ struct dead_debug_use **insnp = debug-head;
struct dead_debug_use *cur;
rtx insn;
@@ -3113,9 +3114,21 @@ dead_debug_reset (struct dead_debug *deb
debug-to_rescan = BITMAP_ALLOC (NULL);
bitmap_set_bit (debug-to_rescan, INSN_UID (insn));
XDELETE (cur);
+ if (tailp != insnp DF_REF_INSN ((*insnp)-use) == insn)
+ tailp = insnp;
/* If the current use isn't the first one attached to INSN, go back to this
first use. We assume that the uses attached to an insn are adjacent. */
+ while ((cur = *tailp) DF_REF_INSN (cur-use) == insn)
+ {
+ *tailp = cur-next;
+ XDELETE (cur);
+ }
+ insnp = tailp;
/* Then remove all the other uses attached to INSN. */
}
else
- tailp = (*tailp)-next;
+ {
+ if (DF_REF_INSN ((*insnp)-use) != DF_REF_INSN (cur-use))
+ insnp = tailp;
+ tailp = (*tailp)-next;
+ }
}
}
@@ -3174,7 +3187,8 @@ dead_debug_insert_before (struct dead_de
tailp = (*tailp)-next;
}
- gcc_assert (reg);
+ if (reg == NULL)
+return;
/* We may have dangling bits in debug-used for registers that were part of
a multi-register use, one component of which has been reset. */
--
Eric Botcazou
[PATCH] Fix dead_debug_insert_before ICE (PR debug/49522, take 2)
On Tue, Jul 05, 2011 at 10:06:51PM +0200, Jakub Jelinek wrote:
On Tue, Jul 05, 2011 at 10:35:11AM +0200, Eric Botcazou wrote:
There are two kinds of changes we do on the debug insns without immediate
rescanning:
1) reset the debug insn
2) replace a reg use with DEBUG_EXPR of the same mode or
subreg of a larger DEBUG_EXPR with the same outer mode as the reg
In the attached testcase on arm a debug insn is reset, because a multi-reg
register has been used there and as the debug insn location was that
multi-reg register before, it is now VOIDmode after the reset - (clobber
(const_int 0)).
That can happen only in this case, right? Otherwise, for a single
register,
the debug insn would have been removed from debug-head already. If so,
how
simpler would it be to remove the other uses in dead_debug_reset instead?
So you prefer something like this (untested) instead?
Without the second loop I have no idea how to make it work in
dead_debug_reset, the other dead_debug_use referencing the same insn might
be earlier or later in the chain.
And here is a version that passed bootstrap/regtest on x86_64-linux and
i686-linux:
2011-07-06 Jakub Jelinek ja...@redhat.com
PR debug/49522
* df-problems.c (dead_debug_reset): Remove dead_debug_uses
referencing debug insns that have been reset.
(dead_debug_insert_before): Don't assert reg is non-NULL,
instead return immediately if it is NULL.
* gcc.dg/debug/pr49522.c: New test.
--- gcc/df-problems.c.jj2011-07-04 19:17:50.757435754 +0200
+++ gcc/df-problems.c 2011-07-06 17:20:06.264420868 +0200
@@ -3117,6 +3117,25 @@ dead_debug_reset (struct dead_debug *deb
else
tailp = (*tailp)-next;
}
+
+ /* If any other dead_debug_use structs refer to the debug insns
+ that have been reset above, remove them too. */
+ if (debug-to_rescan != NULL)
+{
+ tailp = debug-head;
+ while ((cur = *tailp))
+ {
+ insn = DF_REF_INSN (cur-use);
+ if (bitmap_bit_p (debug-to_rescan, INSN_UID (insn))
+ VAR_LOC_UNKNOWN_P (INSN_VAR_LOCATION_LOC (insn)))
+ {
+ *tailp = cur-next;
+ XDELETE (cur);
+ }
+ else
+ tailp = (*tailp)-next;
+ }
+}
}
/* Add USE to DEBUG. It must be a dead reference to UREGNO in a debug
@@ -3174,7 +3193,8 @@ dead_debug_insert_before (struct dead_de
tailp = (*tailp)-next;
}
- gcc_assert (reg);
+ if (reg == NULL)
+return;
/* Create DEBUG_EXPR (and DEBUG_EXPR_DECL). */
dval = make_debug_expr_from_rtl (reg);
--- gcc/testsuite/gcc.dg/debug/pr49522.c.jj 2011-07-04 10:54:23.0
+0200
+++ gcc/testsuite/gcc.dg/debug/pr49522.c2011-07-04 10:54:02.0
+0200
@@ -0,0 +1,41 @@
+/* PR debug/49522 */
+/* { dg-do compile } */
+/* { dg-options -fcompare-debug } */
+
+int val1 = 0L;
+volatile int val2 = 7L;
+long long val3;
+int *ptr = val1;
+
+static int
+func1 ()
+{
+ return 0;
+}
+
+static short int
+func2 (short int a, unsigned int b)
+{
+ return !b ? a : a b;
+}
+
+static unsigned long long
+func3 (unsigned long long a, unsigned long long b)
+{
+ return !b ? a : a % b;
+}
+
+void
+func4 (unsigned short arg1, int arg2)
+{
+ for (arg2 = 0; arg2 2; arg2++)
+{
+ *ptr = func3 (func3 (10, func2 (val3, val2)), val3);
+ for (arg1 = -14; arg1 14; arg1 = func1 ())
+ {
+ *ptr = -1;
+ if (foo ())
+ ;
+ }
+}
+}
Jakub
Re: [PATCH] Fix dead_debug_insert_before ICE (PR debug/49522)
So you prefer something like this (untested) instead? I think that, ideally, we should avoid leaving the dead_debug chain in the semi-broken state that we currently have. Without the second loop I have no idea how to make it work in dead_debug_reset, the other dead_debug_use referencing the same insn might be earlier or later in the chain. I guess I was somehow hoping that you could use one of the numerous DF links to get to the other uses; probably not, in the end, indeed. But you can set a flag in the first loop in order to decide whether to run the second loop. But I don't really have a strong opinon so, if you think that the original patch is good enough, fine with me. Maybe use gcc_checking_assert though. -- Eric Botcazou
Re: [PATCH] Fix dead_debug_insert_before ICE (PR debug/49522, take 2)
And here is a version that passed bootstrap/regtest on x86_64-linux and i686-linux: 2011-07-06 Jakub Jelinek ja...@redhat.com PR debug/49522 * df-problems.c (dead_debug_reset): Remove dead_debug_uses referencing debug insns that have been reset. (dead_debug_insert_before): Don't assert reg is non-NULL, instead return immediately if it is NULL. * gcc.dg/debug/pr49522.c: New test. Sorry, our messages crossed. I'd set a flag in the first loop. In the end, it's up to you. -- Eric Botcazou
Re: [PATCH] Fix dead_debug_insert_before ICE (PR debug/49522)
On Tue, Jul 05, 2011 at 10:35:11AM +0200, Eric Botcazou wrote:
There are two kinds of changes we do on the debug insns without immediate
rescanning:
1) reset the debug insn
2) replace a reg use with DEBUG_EXPR of the same mode or
subreg of a larger DEBUG_EXPR with the same outer mode as the reg
In the attached testcase on arm a debug insn is reset, because a multi-reg
register has been used there and as the debug insn location was that
multi-reg register before, it is now VOIDmode after the reset - (clobber
(const_int 0)).
That can happen only in this case, right? Otherwise, for a single register,
the debug insn would have been removed from debug-head already. If so, how
simpler would it be to remove the other uses in dead_debug_reset instead?
So you prefer something like this (untested) instead?
Without the second loop I have no idea how to make it work in
dead_debug_reset, the other dead_debug_use referencing the same insn might
be earlier or later in the chain.
--- gcc/df-problems.c 2011-07-04 19:17:50.757435754 +0200
+++ gcc/df-problems.c 2011-07-05 22:04:19.817464710 +0200
@@ -3117,6 +3117,25 @@ dead_debug_reset (struct dead_debug *deb
else
tailp = (*tailp)-next;
}
+
+ /* If any other dead_debug_use structs refer to the debug insns
+ that have been reset above, remove them too. */
+ if (debug-to_rescan != NULL)
+{
+ tailp = debug-head;
+ while ((cur = *tailp))
+ {
+ insn = DF_REF_INSN (cur-use);
+ if (bitmap_bit_p (debug-to_rescan, INSN_UID (insn))
+ VAR_LOC_UNKNOWN_P (INSN_VAR_LOCATION_LOC (insn)))
+ {
+ *tailp = cur-next;
+ XDELETE (cur);
+ }
+ else
+ tailp = (*tailp)-next;
+ }
+}
}
/* Add USE to DEBUG. It must be a dead reference to UREGNO in a debug
Jakub
Re: [PATCH] Fix dead_debug_insert_before ICE (PR debug/49522)
There are two kinds of changes we do on the debug insns without immediate rescanning: 1) reset the debug insn 2) replace a reg use with DEBUG_EXPR of the same mode or subreg of a larger DEBUG_EXPR with the same outer mode as the reg In the attached testcase on arm a debug insn is reset, because a multi-reg register has been used there and as the debug insn location was that multi-reg register before, it is now VOIDmode after the reset - (clobber (const_int 0)). That can happen only in this case, right? Otherwise, for a single register, the debug insn would have been removed from debug-head already. If so, how simpler would it be to remove the other uses in dead_debug_reset instead? -- Eric Botcazou
[PATCH] Fix dead_debug_insert_before ICE (PR debug/49522)
Hi!
In dead_debug_* we don't immediately rescan insns, because that kills all
the df links we need to use, only queue their rescanning.
There are two kinds of changes we do on the debug insns without immediate
rescanning:
1) reset the debug insn
2) replace a reg use with DEBUG_EXPR of the same mode or
subreg of a larger DEBUG_EXPR with the same outer mode as the reg
In the attached testcase on arm a debug insn is reset, because a multi-reg
register has been used there and as the debug insn location was that
multi-reg register before, it is now VOIDmode after the reset - (clobber
(const_int 0)). Fixed by disregarding the reset debug insns. Changes
of kind 2) that needed rescanning don't need this, as the mode doesn't
change in that case.
Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk/4.6?
2011-07-04 Jakub Jelinek ja...@redhat.com
PR debug/49522
* df-problems.c (dead_debug_insert_before): Ignore uses
where the use debug insn has been reset.
* gcc.dg/debug/pr49522.c: New test.
--- gcc/df-problems.c.jj2011-06-17 11:02:19.0 +0200
+++ gcc/df-problems.c 2011-07-04 10:46:42.0 +0200
@@ -3148,6 +3148,7 @@ dead_debug_insert_before (struct dead_de
struct dead_debug_use *cur;
struct dead_debug_use *uses = NULL;
struct dead_debug_use **usesp = uses;
+ bool no_reg_ok = false;
rtx reg = NULL;
rtx dval;
rtx bind;
@@ -3161,6 +3162,21 @@ dead_debug_insert_before (struct dead_de
{
if (DF_REF_REGNO (cur-use) == uregno)
{
+ /* If cur-use insn has been meanwhile reset, but hasn't been
+rescanned, just ignore that use. */
+ if (DF_REF_REAL_LOC (cur-use)
+ == INSN_VAR_LOCATION_LOC (DF_REF_INSN (cur-use))
+ VAR_LOC_UNKNOWN_P (*DF_REF_REAL_LOC (cur-use)))
+ {
+ gcc_assert (debug-to_rescan != NULL
+ bitmap_bit_p (debug-to_rescan,
+ INSN_UID (DF_REF_INSN (cur-use;
+ *tailp = cur-next;
+ XDELETE (cur);
+ if (!reg)
+ no_reg_ok = true;
+ continue;
+ }
*usesp = cur;
usesp = cur-next;
*tailp = cur-next;
@@ -3174,6 +3190,9 @@ dead_debug_insert_before (struct dead_de
tailp = (*tailp)-next;
}
+ if (no_reg_ok !reg)
+return;
+
gcc_assert (reg);
/* Create DEBUG_EXPR (and DEBUG_EXPR_DECL). */
--- gcc/testsuite/gcc.dg/debug/pr49522.c.jj 2011-07-04 10:54:23.0
+0200
+++ gcc/testsuite/gcc.dg/debug/pr49522.c2011-07-04 10:54:02.0
+0200
@@ -0,0 +1,41 @@
+/* PR debug/49522 */
+/* { dg-do compile } */
+/* { dg-options -fcompare-debug } */
+
+int val1 = 0L;
+volatile int val2 = 7L;
+long long val3;
+int *ptr = val1;
+
+static int
+func1 ()
+{
+ return 0;
+}
+
+static short int
+func2 (short int a, unsigned int b)
+{
+ return !b ? a : a b;
+}
+
+static unsigned long long
+func3 (unsigned long long a, unsigned long long b)
+{
+ return !b ? a : a % b;
+}
+
+void
+func4 (unsigned short arg1, int arg2)
+{
+ for (arg2 = 0; arg2 2; arg2++)
+{
+ *ptr = func3 (func3 (10, func2 (val3, val2)), val3);
+ for (arg1 = -14; arg1 14; arg1 = func1 ())
+ {
+ *ptr = -1;
+ if (foo ())
+ ;
+ }
+}
+}
Jakub
