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

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

[Alan Modra]

This patch removes bogus debug info left around by shrink-wrapping,
which on some powerpc targets with just the right register allocation
led to assembly errors.

Bootstrapped and regression tested powerpc64-linux and x86_64-linux.
I did see some regressions, but completely unrelated to this patch.
See pr65810 for the powerpc64 regressions.  x86_64-linux showed fails
of
+FAIL: c-c++-common/ubsan/object-size-10.c   -O2  execution test
+FAIL: c-c++-common/ubsan/object-size-10.c   -O2 -flto -fno-use-linker-plugin -flto-partition=none  execution test
+FAIL: c-c++-common/ubsan/object-size-10.c   -O2 -flto -fuse-linker-plugin -fno-fat-lto-objects  execution test
+FAIL: gfortran.dg/class_allocate_18.f90   -O0  execution test
+FAIL: gfortran.dg/class_allocate_18.f90   -O1  execution test
 FAIL: gfortran.dg/class_allocate_18.f90   -O3 -fomit-frame-pointer -funroll-all-loops -finline-functions  execution test
-FAIL: gfortran.dg/class_allocate_18.f90   -Os  execution test
+FAIL: gfortran.dg/class_allocate_18.f90   -O3 -g  execution test

with the log for the ubsan fails
/src/gcc-5/gcc/testsuite/c-c++-common/ubsan/object-size-10.c:19:11: runtime error: index 128 out of bounds for type 'char [128]'
/src/gcc-5/gcc/testsuite/c-c++-common/ubsan/object-size-10.c:19:11: runtime error: load of address 0x0804a000 with insufficient space for an object of type 'char'
0x0804a000: note: pointer points here
<memory cannot be printed>
I assume I was thrashing my ubuntu 14.10 x86_64 box too hard and just
ran out of memory.  Running the test by hand resulted in the expected
output.

The class_allocate_18.f90 failure are intermittent, and occur
occasionally when running the testcase by hand.  :-(

gcc/
	PR debug/65779
	* shrink-wrap.c (insn_uses_reg): New function.
	(move_insn_for_shrink_wrap): Remove debug insns using regs set
	by the moved insn.
gcc/testsuite/
	* gcc.dg/pr65779.c: New.

Index: gcc/shrink-wrap.c
===================================================================
--- gcc/shrink-wrap.c	(revision 222160)
+++ gcc/shrink-wrap.c	(working copy)
@@ -182,6 +182,21 @@ live_edge_for_reg (basic_block bb, int regno, int
   return live_edge;
 }
 
+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
@@ -340,10 +355,15 @@ move_insn_for_shrink_wrap (basic_block bb, rtx_ins
       *split_p = true;
     }
 
+  vec<basic_block> live_bbs;
+  if (MAY_HAVE_DEBUG_INSNS)
+    live_bbs.create (5);
   /* At this point we are committed to moving INSN, but let's try to
      move it as far as we can.  */
   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 +446,34 @@ move_insn_for_shrink_wrap (basic_block bb, rtx_ins
 	SET_REGNO_REG_SET (bb_uses, i);
     }
 
+  /* Remove debug insns using 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 (*split_p)
+		    /* If split, then we will be moving insn into a
+		       newly created block immediately after the entry
+		       block.  Move the debug info there too.  */
+		    emit_debug_insn_after (PATTERN (dinsn), bb_note (bb));
+		  delete_insn (dinsn);
+		  break;
+		}
+	    }
+	}
+      live_bbs.release ();
+    }
+
   emit_insn_after (PATTERN (insn), bb_note (bb));
   delete_insn (insn);
   return true;
Index: gcc/testsuite/gcc.dg/pr65779.c
===================================================================
--- gcc/testsuite/gcc.dg/pr65779.c	(revision 0)
+++ gcc/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;
+}

-- 
Alan Modra
Australia Development Lab, IBM

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

[Jakub Jelinek]

On Mon, Apr 20, 2015 at 12:40:49PM +0930, Alan Modra wrote:
> with the log for the ubsan fails
> /src/gcc-5/gcc/testsuite/c-c++-common/ubsan/object-size-10.c:19:11: runtime error: index 128 out of bounds for type 'char [128]'
> /src/gcc-5/gcc/testsuite/c-c++-common/ubsan/object-size-10.c:19:11: runtime error: load of address 0x0804a000 with insufficient space for an object of type 'char'
> 0x0804a000: note: pointer points here
> <memory cannot be printed>

The issue here is that libsanitizer wants to print some context around the
variable, and doesn't try too hard, so if the variable is too close to the
end of the RW PT_LOAD, you get different message from what is expected.
In your case, most likely the end of the array happens to be exactly at the
end of the PT_LOAD segment.

So, the fix is either to try harder in ubsan renderMemorySnippet function
(it first computes the region it wishes to print, then has
  if (!IsAccessibleMemoryRange(Min, Max - Min)) {
    Printf("<memory cannot be printed>\n");
    return;
  }
).  Supposedly it could, if there are any page boundary crosses in the
Min .. Max region lower a little bit (to the page boundary) the end and/or
increase to the page boundary the start, and retry with that if it is
accessible.

Or we'd need to make the testcases that suffer from this accept also
the <memory cannot be printed> in place of the memory content line, line
with ^ marker (don't remember if there is yet another one).

> gcc/
> 	PR debug/65779
> 	* shrink-wrap.c (insn_uses_reg): New function.
> 	(move_insn_for_shrink_wrap): Remove debug insns using regs set
> 	by the moved insn.
> gcc/testsuite/
> 	* gcc.dg/pr65779.c: New.
> 
> Index: gcc/shrink-wrap.c
> ===================================================================
> --- gcc/shrink-wrap.c	(revision 222160)
> +++ gcc/shrink-wrap.c	(working copy)
> @@ -182,6 +182,21 @@ live_edge_for_reg (basic_block bb, int regno, int
>    return live_edge;
>  }
>  
> +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
> @@ -340,10 +355,15 @@ move_insn_for_shrink_wrap (basic_block bb, rtx_ins
>        *split_p = true;
>      }
>  
> +  vec<basic_block> live_bbs;
> +  if (MAY_HAVE_DEBUG_INSNS)
> +    live_bbs.create (5);

Just wonder if using an
  auto_vec<basic_block, 5> live_bbs;

> +	  FOR_BB_INSNS_REVERSE (tmp_bb, dinsn)
> +	    {
> +	      if (dinsn == insn)
> +		break;
> +	      if (DEBUG_INSN_P (dinsn)
> +		  && insn_uses_reg (dinsn, dregno, end_dregno))
> +		{
> +		  if (*split_p)
> +		    /* If split, then we will be moving insn into a
> +		       newly created block immediately after the entry
> +		       block.  Move the debug info there too.  */
> +		    emit_debug_insn_after (PATTERN (dinsn), bb_note (bb));
> +		  delete_insn (dinsn);

Debug insns should never be deleted, nor moved.  You should either
reset them
(INSN_VAR_LOCATION_LOC (insn) = gen_rtx_UNKNOWN_VAR_LOC (); plus
df_insn_rescan_debug_internal (insn);), or try to adjust them based on the
instruction setting the register (say, if insn sets the register to
some other register + 10 and the other register is still live, you could
replace the uses of the register with (plus (the other register) (const_int 10)).

> +      live_bbs.release ();

If live_bbs is auto_vec, this would not be needed.

	Jakub

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

[Alan Modra]

On Mon, Apr 20, 2015 at 09:35:07AM +0200, Jakub Jelinek wrote:
> On Mon, Apr 20, 2015 at 12:40:49PM +0930, Alan Modra wrote:
> > with the log for the ubsan fails
> > /src/gcc-5/gcc/testsuite/c-c++-common/ubsan/object-size-10.c:19:11: runtime error: index 128 out of bounds for type 'char [128]'
> > /src/gcc-5/gcc/testsuite/c-c++-common/ubsan/object-size-10.c:19:11: runtime error: load of address 0x0804a000 with insufficient space for an object of type 'char'
> > 0x0804a000: note: pointer points here
> > <memory cannot be printed>
> 
> The issue here is that libsanitizer wants to print some context around the
> variable, and doesn't try too hard, so if the variable is too close to the
> end of the RW PT_LOAD, you get different message from what is expected.

Thanks for the info.  I don't tend to run sanitizer tests, so this was
the first time I'd seen such a failure.

> Just wonder if using an
>   auto_vec<basic_block, 5> live_bbs;

OK, done.

> > +	  FOR_BB_INSNS_REVERSE (tmp_bb, dinsn)
> > +	    {
> > +	      if (dinsn == insn)
> > +		break;
> > +	      if (DEBUG_INSN_P (dinsn)
> > +		  && insn_uses_reg (dinsn, dregno, end_dregno))
> > +		{
> > +		  if (*split_p)
> > +		    /* If split, then we will be moving insn into a
> > +		       newly created block immediately after the entry
> > +		       block.  Move the debug info there too.  */
> > +		    emit_debug_insn_after (PATTERN (dinsn), bb_note (bb));
> > +		  delete_insn (dinsn);
> 
> Debug insns should never be deleted, nor moved.  You should either
> reset them
> (INSN_VAR_LOCATION_LOC (insn) = gen_rtx_UNKNOWN_VAR_LOC (); plus
> df_insn_rescan_debug_internal (insn);), or

I had it that way in my first patch, then decided to try deleting..

I can certainly change it back even if only to do it the standard way
for safety's sake, but I'm curious as to why they can't be deleted in
this special case.  My thinking was that we're on a chain of blocks
starting at the entry where there is a single outgoing live edge for
the register being used.  So there shouldn't be any need for a debug
insn to mark info about the variable as invalid.

-- 
Alan Modra
Australia Development Lab, IBM

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

[Jakub Jelinek]

On Mon, Apr 20, 2015 at 06:12:26PM +0930, Alan Modra wrote:
> I had it that way in my first patch, then decided to try deleting..
> 
> I can certainly change it back even if only to do it the standard way
> for safety's sake, but I'm curious as to why they can't be deleted in
> this special case.  My thinking was that we're on a chain of blocks
> starting at the entry where there is a single outgoing live edge for
> the register being used.  So there shouldn't be any need for a debug
> insn to mark info about the variable as invalid.

The debug insns can be for arbitrary variables, there is no "the variable",
and there could be other debug insns for the same variable on that path,
say saying that decl lives in some other register, or can be computed using
an expression involving other registers, or memory etc.  Say you could have
(set regX (whatever))
...
(debug_insn var5 (some expression not referring to regX))
...
(debug_insn var5 (some expression referring to regX))
...
(debug_insn var5 (other expression not referring to regX))
...
(use regX)

where ... contains unrelated insns (not referring to regX) and edges live
for regX.  If shrink wrapping attempts to move the first set somewhere into
the last ..., if you delete debug insns referring to regX, you extend the
lifetime of the previous debug_insn, which is wrong, the registers
referenced in the first debug_insn might not contain the right values
afterwards.  And if you move the debug insn later, you might shorten the
lifetime of the third debug_insn, while regX is supposed to contain the same
value, perhaps some other register referenced there might have been changed
already.

	Jakub

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

[Alan Modra]

On Mon, Apr 20, 2015 at 10:55:56AM +0200, Jakub Jelinek wrote:
> On Mon, Apr 20, 2015 at 06:12:26PM +0930, Alan Modra wrote:
> > I had it that way in my first patch, then decided to try deleting..
> > 
> > I can certainly change it back even if only to do it the standard way
> > for safety's sake, but I'm curious as to why they can't be deleted in
> > this special case.  My thinking was that we're on a chain of blocks
> > starting at the entry where there is a single outgoing live edge for
> > the register being used.  So there shouldn't be any need for a debug
> > insn to mark info about the variable as invalid.
> 
> The debug insns can be for arbitrary variables, there is no "the variable",

Sure.

> and there could be other debug insns for the same variable on that path,
> say saying that decl lives in some other register, or can be computed using
> an expression involving other registers, or memory etc.  Say you could have

Yes, that's true in the general case.  For the shrink-wrap case, any
bb (or tail of the entry block) that we move over has no use or def of
the register.  So I'm left wondering how it would be possible for the
var to live in some other register or memory?  Probably lack of
imagination on my part, but the only scenarios I see would involve a
failure of cse.

Anyway, I rewrote the patch to do as you suggested, and started
looking at .debug_loc in gcc/*.o for files that differed between the
two approaches.  (Only 32 files differed, besides the expected
shrinkwrap.o and checksum files.)  That was a bit of a revelation,
and no wonder powerpc debugging is such a pain..

The first file I looked at was reload.o, and the first difference is
in location lists for get_secondary_mem mode param.  It looks like

virgin:
    000086c5 0000000000005c90 0000000000005cd7 (DW_OP_reg4 (r4))
    000086d8 0000000000005cd7 0000000000005cd8 (DW_OP_GNU_entry_value: (DW_OP_reg4 (r4)); DW_OP_stack_value)
    000086ee 0000000000005cd8 0000000000005cf8 (DW_OP_reg30 (r30))
    00008701 0000000000005d2c 0000000000005d38 (DW_OP_reg30 (r30))

delete:
    000086ae 0000000000005c90 0000000000005cd7 (DW_OP_reg4 (r4))
    000086c1 0000000000005cd7 0000000000005ec0 (DW_OP_GNU_entry_value: (DW_OP_reg4 (r4)); DW_OP_stack_value)

zap:
    000086ae 0000000000005c90 0000000000005cd7 (DW_OP_reg4 (r4))
    000086c1 0000000000005cd7 0000000000005cd8 (DW_OP_GNU_entry_value: (DW_OP_reg4 (r4)); DW_OP_stack_value)

and the code:
    5cd0:	48 00 00 01 	bl      5cd0 <._Z17get_secondary_memP7rtx_def12machine_modei11reload_type+0x40>
			5cd0: R_PPC64_REL24	_Z35rs6000_secondary_memory_needed_mode12machine_mode
    5cd4:	60 00 00 00 	nop
    5cd8:	7c 7d 07 b4 	extsw   r29,r3
    5cdc:	1f fd 00 1e 	mulli   r31,r29,30
    5ce0:	7d 3f da 14 	add     r9,r31,r27
    5ce4:	79 29 1f 24 	rldicr  r9,r9,3,60
    5ce8:	7d 3c 4a 14 	add     r9,r28,r9
    5cec:	e9 29 55 a8 	ld      r9,21928(r9)
    5cf0:	2f a9 00 00 	cmpdi   cr7,r9,0
    5cf4:	41 9e 00 3c 	beq     cr7,5d30 <._Z17get_secondary_memP7rtx_def12machine_modei11reload_type+0xa0>
    5cf8:	38 21 00 c0 	addi    r1,r1,192
    5cfc:	7d 23 4b 78 	mr	r3,r9
..rest of epilogue

    5d30:	7b b9 1f 24 	rldicr  r25,r29,3,60
    5d34:	7c 7e 1b 78 	mr      r30,r3
    5d38:	7f 1c ca 14 	add     r24,r28,r25

Arrgh!  In the first place the ranges are wrong since r4 dies after
the bl, not the toc restoring nop.  Worse, both deleting and zapping
(ie UNKNOWN_VAR_LOC) the debug insn is wrong.  My simplistic patch
isn't correct.  In fact it makes the debug info worse.  However,
leaving the debug insn alone says "mode" lives in r30, but that is
wrong since the copy to r30 insn has been moved, to 5d34.  Apparently
the move causes the virgin location lists to say "mode" disappears at
that point too.  What a mess!

Of course, all this moving for shrink-wrap is senseless in a block
that contains a call.

-- 
Alan Modra
Australia Development Lab, IBM

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

[Jakub Jelinek]

On Mon, Apr 20, 2015 at 10:30:32PM +0930, Alan Modra wrote:
> > and there could be other debug insns for the same variable on that path,
> > say saying that decl lives in some other register, or can be computed using
> > an expression involving other registers, or memory etc.  Say you could have
> 
> Yes, that's true in the general case.  For the shrink-wrap case, any
> bb (or tail of the entry block) that we move over has no use or def of
> the register.  So I'm left wondering how it would be possible for the
> var to live in some other register or memory?  Probably lack of
> imagination on my part, but the only scenarios I see would involve a
> failure of cse.

E.g. the variable might have different values in the source code.
You can have
  int var1 = 5;
  // some statements
  var1 = parmx + 20;
  // some statements
  var1 = 30;
  // ...
in the source and consider that the shrink wrapping insn is attempting to move
(set (regX) (plus (reg parmx) (const_int 20)))
later.  You could have different debug insns for the same var1, and trying
to move the debug_insn later would break things.  Usually, gcc only adds
further debug stmts (or insns), e.g. if in a range between earlier two debug
stmts (or insns) the var is known to contain some particular value, but
expression having that value or something used in it (SSA_NAME, pseudo REG),
is optimized away, if there is some other way to get the same value, the
range could be split into two with two different expressions etc.

> Arrgh!  In the first place the ranges are wrong since r4 dies after
> the bl, not the toc restoring nop.  Worse, both deleting and zapping
> (ie UNKNOWN_VAR_LOC) the debug insn is wrong.  My simplistic patch
> isn't correct.  In fact it makes the debug info worse.  However,

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.

> leaving the debug insn alone says "mode" lives in r30, but that is
> wrong since the copy to r30 insn has been moved, to 5d34.  Apparently
> the move causes the virgin location lists to say "mode" disappears at
> that point too.  What a mess!
> 
> 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?

	Jakub

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

[Jeff Law]

On 04/19/2015 09:10 PM, Alan Modra wrote:
> This patch removes bogus debug info left around by shrink-wrapping,
> which on some powerpc targets with just the right register allocation
> led to assembly errors.
>
> Bootstrapped and regression tested powerpc64-linux and x86_64-linux.
> I did see some regressions, but completely unrelated to this patch.
> See pr65810 for the powerpc64 regressions.  x86_64-linux showed fails
> of
> +FAIL: c-c++-common/ubsan/object-size-10.c   -O2  execution test
> +FAIL: c-c++-common/ubsan/object-size-10.c   -O2 -flto -fno-use-linker-plugin -flto-partition=none  execution test
> +FAIL: c-c++-common/ubsan/object-size-10.c   -O2 -flto -fuse-linker-plugin -fno-fat-lto-objects  execution test
> +FAIL: gfortran.dg/class_allocate_18.f90   -O0  execution test
> +FAIL: gfortran.dg/class_allocate_18.f90   -O1  execution test
>   FAIL: gfortran.dg/class_allocate_18.f90   -O3 -fomit-frame-pointer -funroll-all-loops -finline-functions  execution test
> -FAIL: gfortran.dg/class_allocate_18.f90   -Os  execution test
> +FAIL: gfortran.dg/class_allocate_18.f90   -O3 -g  execution test
>
> with the log for the ubsan fails
> /src/gcc-5/gcc/testsuite/c-c++-common/ubsan/object-size-10.c:19:11: runtime error: index 128 out of bounds for type 'char [128]'
> /src/gcc-5/gcc/testsuite/c-c++-common/ubsan/object-size-10.c:19:11: runtime error: load of address 0x0804a000 with insufficient space for an object of type 'char'
> 0x0804a000: note: pointer points here
> <memory cannot be printed>
> I assume I was thrashing my ubuntu 14.10 x86_64 box too hard and just
> ran out of memory.  Running the test by hand resulted in the expected
> output.
>
> The class_allocate_18.f90 failure are intermittent, and occur
> occasionally when running the testcase by hand.  :-(
Yea, I think folks are still trying to sort out what's happening with 
class_allocate_18.f90.

>
> gcc/
> 	PR debug/65779
> 	* shrink-wrap.c (insn_uses_reg): New function.
> 	(move_insn_for_shrink_wrap): Remove debug insns using regs set
> 	by the moved insn.
> gcc/testsuite/
> 	* gcc.dg/pr65779.c: New.
>
> Index: gcc/shrink-wrap.c
> ===================================================================
> --- gcc/shrink-wrap.c	(revision 222160)
> +++ gcc/shrink-wrap.c	(working copy)
> @@ -182,6 +182,21 @@ live_edge_for_reg (basic_block bb, int regno, int
>     return live_edge;
>   }
>
> +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;
> +}
Need a comment for this function.

So just one question.  Why handle the split case differently?  In the 
split case you effectively move the debug insn to the new block.  In the 
!split case, you just delete the debug insn.

I'm sure there's a reason, it would be worth noting it as a comment in 
this code.

OK with the comments added.

jeff

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

[Alan Modra]

On Mon, Apr 20, 2015 at 12:17:08PM -0600, Jeff Law wrote:
> So just one question.  Why handle the split case differently?  In the split
> case you effectively move the debug insn to the new block.  In the !split
> case, you just delete the debug insn.

The idea was that when split we have a new block with no debug info,
but !split the block already has debug info about the var/reg.  Now
that I've looked at a lot more .debug_loc sections than I ever wanted,
and the debug_insns that generate that info, I realize my assumption
about the !split case is wrong.  Working on a new patch to fix some of
the things I've discovered.

-- 
Alan Modra
Australia Development Lab, IBM

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

[Alan Modra]

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.

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

-- 
Alan Modra
Australia Development Lab, IBM

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

[Jakub Jelinek]

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

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

[Alan Modra]

On Tue, Apr 21, 2015 at 02:30:15PM +0200, Jakub Jelinek wrote:
> 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.

Yeah, I see now that what I was trying to do can't be done so easily.
In fact, the more I look at debug_insns the less sure I am of any
solution other than simply zapping them with UNKNOWN_VAR_LOC.
However, I'm not going to write such a patch.  Simply because I don't
want to be responsible for even more cases where "print var" in gdb
gets "<optimized out>" and upon examing the disassembly you can easily
determine that "var" is live in some register.

One of the potential problems I've discovered is that debug_insns do
not satisfy an assumption made by propagate_for_debug, that they are
always located immediately after their associated insn.  For example,
gcc-5 branch on x86_64 varasm.c:hashtab_entry_note_pointers has this
in vregs pass:

(code_label 18 17 19 5 57 "" [1 uses])
(note 19 18 7 5 [bb 5] NOTE_INSN_BASIC_BLOCK)
(insn 7 19 40 5 (set (reg/v:DI 91 [ i ])
        (const_int 0 [0])) /src/gcc-5-cur/gcc/hash-table.h:1723 89 {*movdi_internal}
     (nil))
;;  succ:       6 [100.0%]  (FALLTHRU)

;; basic block 6, loop depth 1, count 0, freq 9100, maybe hot
;;  prev block 5, next block 7, flags: (NEW, REACHABLE, RTL)
;;  pred:       5 [100.0%]  (FALLTHRU)
;;              8 [100.0%] 
(code_label 40 7 20 6 60 "" [1 uses])
(note 20 40 21 6 [bb 6] NOTE_INSN_BASIC_BLOCK)
(debug_insn 21 20 22 6 (var_location:DI i (reg/v:DI 91 [ i ])) -1
     (nil))

OK, so that particular example can't cause a problem with combine's
use of propagate_for_debug but something like the above would break
what I'm trying to do in shrink_wrap.


Here's another problem, x86_64 varasm.c again, last occurrence of
valid0 in debug, cprop1:

(debug_insn 858 629 630 104 (var_location:DI D#293 (reg:DI 0 ax)) -1
     (nil))
(insn 630 858 631 104 (set (reg/v/f:DI 195 [ ret ])
        (reg:DI 0 ax)) /src/gcc-5-cur/gcc/varasm.c:4541 89 {*movdi_internal}
     (expr_list:REG_DEAD (reg:DI 0 ax)
        (nil)))
(debug_insn 631 630 633 104 (var_location:DI valid0 (debug_expr:DI D#293)) /src/gcc-5-cur/gcc/varasm.c:4541 -1
     (nil))

Now that seems just plain wrong to me.  This is just after a call.
After insn 630 valid0 lives in reg 195, not reg 0 as the debug_insn
says.  What I'm concerned about as far as fixing shrink_wrap is
whether this sort of thing can occur in correct debug expressions,
ie. if debug_isns 858 said D#293 is equal to reg 195.  We'd have part
of the debug info about valid0 *before* the insn setting reg 195.
As far as I can tell, sched doesn't create this sort of problem.
Note that prior to cprop1, we just had
(insn 630 629 631 93 (set (reg/v/f:DI 195 [ ret ])
        (reg:DI 0 ax)) /src/gcc-5-cur/gcc/varasm.c:4541 89 {*movdi_internal}
     (expr_list:REG_DEAD (reg:DI 0 ax)
        (nil)))
(debug_insn 631 630 633 93 (var_location:DI valid0 (reg/v/f:DI 195 [ ret ])) /src/gcc-5-cur/gcc/varasm.c:4541 -1
     (nil))


Anyway, here's my best attempt at fixing PR65779.  If it isn't
acceptable I'm going to have to leave this problem to someone else,
not because I'm uninterested in fixing it properly, but because I'm
being asked to work on other things by my employer..

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.

Index: gcc/shrink-wrap.c
===================================================================
--- gcc/shrink-wrap.c	(revision 222708)
+++ gcc/shrink-wrap.c	(working copy)
@@ -79,6 +79,7 @@
 #include "shrink-wrap.h"
 #include "regcprop.h"
 #include "rtl-iter.h"
+#include "valtrack.h"
 
 #ifdef HAVE_simple_return
 
@@ -182,6 +183,24 @@
   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
@@ -193,7 +212,7 @@
 			   const HARD_REG_SET defs,
 			   bool *split_p)
 {
-  rtx set, src, dest;
+  rtx set, src, orig_src, dest;
   bitmap live_out, live_in, bb_uses, bb_defs;
   unsigned int i, dregno, end_dregno;
   unsigned int sregno = FIRST_PSEUDO_REGISTER;
@@ -209,7 +228,7 @@
   set = PATTERN (insn);
   if (GET_CODE (set) != SET)
     return false;
-  src = SET_SRC (set);
+  orig_src = src = SET_SRC (set);
   dest = SET_DEST (set);
 
   /* For the destination, we want only a register.  Also disallow STACK
@@ -342,8 +361,11 @@
 
   /* 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 +448,71 @@
 	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)
+    {
+      basic_block *elt;
+      unsigned int indx;
+      bool do_prop = false;
+
+      FOR_EACH_VEC_ELT (live_bbs, indx, elt)
+	{
+	  rtx_insn *dinsn;
+	  basic_block tmp_bb = *elt;
+
+	  if (indx == 0)
+	    {
+	      /* First check that we have some debug info relevant to
+		 the insn being moved.  It ought to be just after
+		 insn.  */
+	      dinsn = insn;
+	      while (dinsn != BB_END (tmp_bb))
+		{
+		  dinsn = NEXT_INSN (dinsn);
+		  if (!DEBUG_INSN_P (dinsn))
+		    break;
+		  if (insn_uses_reg (dinsn, dregno, end_dregno))
+		    {
+		      do_prop = true;
+		      break;
+		    }
+		}
+
+	      if (do_prop)
+		{
+		  /* Put debug info for the insn we'll be moving
+		     into the destination block.  */
+		  rtx_insn *newdinsn = bb_note (bb);
+
+		  dinsn = insn;
+		  while (dinsn != BB_END (tmp_bb))
+		    {
+		      dinsn = NEXT_INSN (dinsn);
+		      if (!DEBUG_INSN_P (dinsn))
+			break;
+		      newdinsn
+			= emit_debug_insn_after (copy_rtx (PATTERN (dinsn)),
+						 newdinsn);
+		      df_insn_rescan (newdinsn);
+		    }
+		}
+	      if (dump_file)
+		fprintf (dump_file,
+			 "Propagating %sdebug for insn %d.\n",
+			 do_prop ? "" : "no ", INSN_UID (insn));
+	    }
+	  dinsn = insn;
+	  if (indx != 0)
+	    dinsn = BB_HEAD (tmp_bb);
+
+	  if (do_prop)
+	    propagate_for_debug (dinsn, BB_END (tmp_bb),
+				 dest, orig_src,
+				 tmp_bb);
+	}
+    }
+
   emit_insn_after (PATTERN (insn), bb_note (bb));
   delete_insn (insn);
   return true;
Index: gcc/testsuite/gcc.dg/pr65779.c
===================================================================
--- gcc/testsuite/gcc.dg/pr65779.c	(revision 0)
+++ gcc/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;
+}


-- 
Alan Modra
Australia Development Lab, IBM