diff mbox

pr65779 - [5/6 Regression] undefined local symbol on powerpc

Message ID 20150421113804.GH12627@bubble.grove.modra.org
State New
Headers show

Commit Message

Alan Modra April 21, 2015, 11:38 a.m. UTC 
On Mon, Apr 20, 2015 at 03:17:21PM +0200, Jakub Jelinek wrote:
> On Mon, Apr 20, 2015 at 10:30:32PM +0930, Alan Modra wrote:
> Zapping is conservatively correct, if you don't know where the var lives in
> or how to compute it, you tell the debugger you don't know it.
> Of course, it is a QoI issue, if there is an easy way how to reconstruct the
> value otherwise, it is always better to do so.

That's what this revised patch does, fix the easy cases.

> > Of course, all this moving for shrink-wrap is senseless in a block
> > that contains a call.
> 
> Yeah, such blocks clearly aren't going to be shrink-wrapped, so there is no
> point to move it that far, right?

It's not where we're moving to, but from.  The first block in the
function has a call, but prepare_shrink_wrap goes ahead regardless,
moving reg copies and initialization out of the block.  Ideally none
of the moves would be committed until we decide that we can shrink
wrap.  The tricky part is that we need to perform the moves in order
to update dataflow info used to decide whether other moves can
happen.  So I think the only way to get back to the original insn
stream is keep info around for an undo.

Anyway, here's the current patch.  The debug_loc info looks much
better, so we should see fewer of those <optimized out> messages from
gdb.  Cures a dozen quality fails on powerpc64 too (all in one
testcase).  Bootstrapped and regression tested powerpc64-linux and
x86_64-linux.

gcc/
	PR debug/65779
	* shrink-wrap.c (insn_uses_reg): New function.
	(move_insn_for_shrink_wrap): Try to fix up debug insns related
	to the moved insn.
gcc/testsuite/
	* gcc.dg/pr65779.c: New.

Comments

Jakub Jelinek April 21, 2015, 12:30 p.m. UTC | #1 
On Tue, Apr 21, 2015 at 09:08:04PM +0930, Alan Modra wrote:
> +	      if (DEBUG_INSN_P (dinsn)
> +		  && insn_uses_reg (dinsn, dregno, end_dregno))
> +		{
> +		  if (live_bbs.is_empty ())
> +		    /* Put debug info for the insn we'll be moving
> +		       into the destination block.  */
> +		    {
> +		      rtx_insn *newdinsn
> +			= emit_debug_insn_after (copy_rtx (PATTERN (dinsn)),
> +						 bb_note (bb));
> +		      df_insn_rescan (newdinsn);
> +		    }

This isn't safe.  There could be a debug_insn for the same decl anywhere in
between the dinsn and bb_note (bb) on the chosen live path, if there is,
this change will break stuff.

> +		  /* If the insn is a simple reg-reg copy, then reset
> +		     the debug insn to point to src.  */
> +		  if (REG_P (src) && GET_MODE (src) == GET_MODE (dest))
> +		    {
> +		      INSN_VAR_LOCATION_LOC (dinsn)
> +			= simplify_replace_rtx (INSN_VAR_LOCATION_LOC (dinsn),
> +						dest, src);
> +		      df_insn_rescan (dinsn);
> +		    }
> +		  else
> +		    {
> +		      /* Otherwise remove anything about this variable.  */
> +		      INSN_VAR_LOCATION_LOC (dinsn)
> +			= gen_rtx_UNKNOWN_VAR_LOC ();
> +		      df_insn_rescan_debug_internal (dinsn);
> +		    }

This works (though the simplify_replace_rtx alone is dangerous, you'd better
use propagate_for_debug), but is unnecessarily limitting.  You could just insert a debug
insn with a debug temp before the original insn and replace all the uses of
the reg with the debug temporary.
And, as you are walking all the bbs on the path insn by insn anyway,
supposedly you could instead use the valtrack APIs for that.
Thus, call
  dead_debug_local_init (&debug, NULL, NULL);
before walking the first bb, then call
  dead_debug_add on each FOR_EACH_INSN_INFO_USE of the debug insns that
overlaps the dest REG, and finally
  dead_debug_insert_temp with DEBUG_TEMP_BEFORE_WITH_VALUE and
finally dead_debug_local_finish.  Of course all this guarded with
MAY_HAVE_DEBUG_INSNS.

	Jakub
diff mbox

Patch

Index: shrink-wrap.c
===================================================================
--- shrink-wrap.c	(revision 222227)
+++ shrink-wrap.c	(working copy)
@@ -182,6 +182,24 @@  live_edge_for_reg (basic_block bb, int regno, int
   return live_edge;
 }
 
+/* Return true if INSN df shows a use of a reg in the range
+   [REGNO,END_REGNO).  */
+
+static bool
+insn_uses_reg (rtx_insn *insn, unsigned int regno, unsigned int end_regno)
+{
+  df_ref use;
+
+  FOR_EACH_INSN_USE (use, insn)
+    {
+      rtx reg = DF_REF_REG (use);
+
+      if (REG_P (reg) && REGNO (reg) >= regno && REGNO (reg) < end_regno)
+	return true;
+    }
+  return false;
+}
+
 /* Try to move INSN from BB to a successor.  Return true on success.
    USES and DEFS are the set of registers that are used and defined
    after INSN in BB.  SPLIT_P indicates whether a live edge from BB
@@ -342,8 +360,11 @@  move_insn_for_shrink_wrap (basic_block bb, rtx_ins
 
   /* At this point we are committed to moving INSN, but let's try to
      move it as far as we can.  */
+  auto_vec<basic_block, 5> live_bbs;
   do
     {
+      if (MAY_HAVE_DEBUG_INSNS)
+	live_bbs.safe_push (bb);
       live_out = df_get_live_out (bb);
       live_in = df_get_live_in (next_block);
       bb = next_block;
@@ -426,6 +447,54 @@  move_insn_for_shrink_wrap (basic_block bb, rtx_ins
 	SET_REGNO_REG_SET (bb_uses, i);
     }
 
+  /* Try to fix up debug insns in the tail of the entry block and any
+     intervening blocks that use regs set by the insn we are moving.  */
+  if (MAY_HAVE_DEBUG_INSNS)
+    {
+      while (!live_bbs.is_empty ())
+	{
+	  rtx_insn *dinsn;
+	  basic_block tmp_bb = live_bbs.pop ();
+
+	  FOR_BB_INSNS_REVERSE (tmp_bb, dinsn)
+	    {
+	      if (dinsn == insn)
+		break;
+	      if (DEBUG_INSN_P (dinsn)
+		  && insn_uses_reg (dinsn, dregno, end_dregno))
+		{
+		  if (live_bbs.is_empty ())
+		    /* Put debug info for the insn we'll be moving
+		       into the destination block.  */
+		    {
+		      rtx_insn *newdinsn
+			= emit_debug_insn_after (copy_rtx (PATTERN (dinsn)),
+						 bb_note (bb));
+		      df_insn_rescan (newdinsn);
+		    }
+
+		  /* If the insn is a simple reg-reg copy, then reset
+		     the debug insn to point to src.  */
+		  if (REG_P (src) && GET_MODE (src) == GET_MODE (dest))
+		    {
+		      INSN_VAR_LOCATION_LOC (dinsn)
+			= simplify_replace_rtx (INSN_VAR_LOCATION_LOC (dinsn),
+						dest, src);
+		      df_insn_rescan (dinsn);
+		    }
+		  else
+		    {
+		      /* Otherwise remove anything about this variable.  */
+		      INSN_VAR_LOCATION_LOC (dinsn)
+			= gen_rtx_UNKNOWN_VAR_LOC ();
+		      df_insn_rescan_debug_internal (dinsn);
+		    }
+		  break;
+		}
+	    }
+	}
+    }
+
   emit_insn_after (PATTERN (insn), bb_note (bb));
   delete_insn (insn);
   return true;
Index: testsuite/gcc.dg/pr65779.c
===================================================================
--- testsuite/gcc.dg/pr65779.c	(revision 0)
+++ testsuite/gcc.dg/pr65779.c	(working copy)
@@ -0,0 +1,64 @@ 
+/* { dg-do run } */
+/* { dg-options "-O2 -g" } */
+/* { dg-additional-options "-mrelocatable" { target powerpc-*-rtems* } } */
+
+unsigned long __attribute__ ((noinline))
+adler32 (unsigned long adler, unsigned char *buf, unsigned int len)
+{
+  unsigned long s1 = adler & 0xffff;
+  unsigned long s2 = (adler >> 16) & 0xffff;
+  int k;
+
+  if (buf == 0)
+    return 1L;
+
+  while (len > 0)
+    {
+      k = len < 5552 ? len : 5552;
+      len -= k;
+      while (k >= 16)
+	{
+	  s1 += *buf++; s2 += s1;
+	  s1 += *buf++; s2 += s1;
+	  s1 += *buf++; s2 += s1;
+	  s1 += *buf++; s2 += s1;
+	  s1 += *buf++; s2 += s1;
+	  s1 += *buf++; s2 += s1;
+	  s1 += *buf++; s2 += s1;
+	  s1 += *buf++; s2 += s1;
+	  s1 += *buf++; s2 += s1;
+	  s1 += *buf++; s2 += s1;
+	  s1 += *buf++; s2 += s1;
+	  s1 += *buf++; s2 += s1;
+	  s1 += *buf++; s2 += s1;
+	  s1 += *buf++; s2 += s1;
+	  s1 += *buf++; s2 += s1;
+	  s1 += *buf++; s2 += s1;
+	  k -= 16;
+	}
+      if (k != 0)
+	do
+	  {
+	    s1 += *buf++; s2 += s1;
+	  } while (--k);
+      s1 &= 0xffffffffUL;
+      s2 &= 0xffffffffUL;
+      s1 %= 65521L;
+      s2 %= 65521L;
+    }
+  return (s2 << 16) | s1;
+}
+
+unsigned char buf[] = { 0, 1, 2, 3, 4, 5, 6, 7,
+			8, 9, 10, 11, 12, 13, 14, 15,
+			0x80, 0x81, 0x82, 0x83, 0x84, 0x85, 0x86, 0x87,
+			0x88, 0x89, 0x8a, 0x8b, 0x8c, 0x8d, 0x8e, 0x8f,
+			0x55, 0xaa };
+int
+main ()
+{
+  unsigned long x = adler32 (0, buf, sizeof buf);
+  if (x != 0x640409efUL)
+    __builtin_abort ();
+  return 0;
+}