diff mbox

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

Message ID 20110707101935.GM2687@tyan-ft48-01.lab.bos.redhat.com
State New
Headers show

Commit Message

Jakub Jelinek July 7, 2011, 10:19 a.m. UTC 
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.



	Jakub

Comments

Eric Botcazou July 7, 2011, 3:50 p.m. UTC | #1 
> 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.  */
diff mbox

Patch

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