[PATCH] Fix dead_debug_insert_before ICE (PR debug/49522)

[PATCH] Fix dead_debug_insert_before ICE (PR debug/49522)

[Jakub Jelinek]

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  <jakub@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.jj	2011-06-17 11:02:19.000000000 +0200
+++ gcc/df-problems.c	2011-07-04 10:46:42.000000000 +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.000000000 +0200
+++ gcc/testsuite/gcc.dg/debug/pr49522.c	2011-07-04 10:54:02.000000000 +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)

[Eric Botcazou]

> 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

Re: [PATCH] Fix dead_debug_insert_before ICE (PR debug/49522)

[Jakub Jelinek]

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

[PATCH] Fix dead_debug_insert_before ICE (PR debug/49522, take 2)

[Jakub Jelinek]

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  <jakub@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.jj	2011-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.000000000 +0200
+++ gcc/testsuite/gcc.dg/debug/pr49522.c	2011-07-04 10:54:02.000000000 +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)

[Eric Botcazou]

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

[Eric Botcazou]

> And here is a version that passed bootstrap/regtest on x86_64-linux and
> i686-linux:
>
> 2011-07-06  Jakub Jelinek  <jakub@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

[PATCH] Fix dead_debug_insert_before ICE (PR debug/49522, take 3)

[Jakub Jelinek]

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  <jakub@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  <jakub@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.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;
+	  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.000000000 +0200
+++ gcc/testsuite/gcc.dg/debug/pr49522.c	2011-07-04 10:54:02.000000000 +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)

[Eric Botcazou]

> 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  <jakub@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