[rfa/mips] Stop backtraces when we've lost the PC

[rfa/mips] Stop backtraces when we've lost the PC

[Daniel Jacobowitz]

Here's an updated version of a little hack I've been using since GDB 6.0.
If we are in a nested normal frame, i.e. something whose next frame is a
function that it called in the normal way, and we didn't find a saved PC,
we're going to be stuck in a loop.  We might have been able to figure out
the frame size, but not where the return address was stored; as the comment
says, this happens in glibc's clone function.  Of course the problem there
is that it _doesn't_ save $ra in the normal fashion; it won't return.

Without this patch schedlock.exp falls apart, because backtraces continue
forever printing "clone()" on every line.

OK?  Or a better way to do this?

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

2004-03-06  Daniel Jacobowitz  <drow@mvista.com>

	* mips-tdep.c (mips_mdebug_frame_this_id): Terminate unwinding if
	we haven't found a saved PC.

Index: mips-tdep.c
===================================================================
RCS file: /big/fsf/rsync/src-cvs/src/gdb/mips-tdep.c,v
retrieving revision 1.283
diff -u -p -r1.283 mips-tdep.c
--- mips-tdep.c	17 Feb 2004 15:21:21 -0000	1.283
+++ mips-tdep.c	6 Mar 2004 22:52:29 -0000
@@ -1672,6 +1672,24 @@ mips_mdebug_frame_this_id (struct frame_
 {
   struct mips_frame_cache *info = mips_mdebug_frame_cache (next_frame,
 							   this_cache);
+
+  /* If the return address is not saved for two frames in a row,
+     then we are probably hosed.  Not necessarily - it's possible to
+     write working assembly that violates this rule - but we can't
+     backtrace through that either.  Eventually MIPS will support
+     DWARF2 unwind information, allowing assembly programmers to
+     avoid this problem.
+
+     One place this check triggers is in the GNU/Linux clone syscall
+     wrapper.  */
+  if (frame_relative_level (next_frame) >= 0
+      && get_frame_type (next_frame) == NORMAL_FRAME
+      && !trad_frame_addr_p (info->saved_regs, NUM_REGS + PC_REGNUM))
+    {
+      (*this_id) = null_frame_id;
+      return;
+    }
+
   (*this_id) = frame_id_build (info->base, frame_func_unwind (next_frame));
 }
 

Re: [rfa/mips] Stop backtraces when we've lost the PC

[Andrew Cagney]

> Here's an updated version of a little hack I've been using since GDB 6.0.
> If we are in a nested normal frame, i.e. something whose next frame is a
> function that it called in the normal way, and we didn't find a saved PC,
> we're going to be stuck in a loop.  We might have been able to figure out
> the frame size, but not where the return address was stored; as the comment
> says, this happens in glibc's clone function.  Of course the problem there
> is that it _doesn't_ save $ra in the normal fashion; it won't return.
> 
> Without this patch schedlock.exp falls apart, because backtraces continue
> forever printing "clone()" on every line.
> 
> OK?

No!

> +  if (frame_relative_level (next_frame) >= 0
> +      && get_frame_type (next_frame) == NORMAL_FRAME
> +      && !trad_frame_addr_p (info->saved_regs, NUM_REGS + PC_REGNUM))

The whole point of this recursive code is that you don't need to add 
hacks that look down the stack at the type of more inner frames :-/

Can you post the relevant assembler so that we can determine why the 
prologue analyzer is failing.

Andrew

Re: [rfa/mips] Stop backtraces when we've lost the PC

[Daniel Jacobowitz]

On Sun, Mar 07, 2004 at 07:56:18PM -0500, Andrew Cagney wrote:
> >Here's an updated version of a little hack I've been using since GDB 6.0.
> >If we are in a nested normal frame, i.e. something whose next frame is a
> >function that it called in the normal way, and we didn't find a saved PC,
> >we're going to be stuck in a loop.  We might have been able to figure out
> >the frame size, but not where the return address was stored; as the comment
> >says, this happens in glibc's clone function.  Of course the problem there
> >is that it _doesn't_ save $ra in the normal fashion; it won't return.
> >
> >Without this patch schedlock.exp falls apart, because backtraces continue
> >forever printing "clone()" on every line.
> >
> >OK?
> 
> No!
> 
> >+  if (frame_relative_level (next_frame) >= 0
> >+      && get_frame_type (next_frame) == NORMAL_FRAME
> >+      && !trad_frame_addr_p (info->saved_regs, NUM_REGS + PC_REGNUM))
> 
> The whole point of this recursive code is that you don't need to add 
> hacks that look down the stack at the type of more inner frames :-/
> 
> Can you post the relevant assembler so that we can determine why the 
> prologue analyzer is failing.

This is clone.  It can't be unwound past - there is no stack behind it
(we're in the child, here).

00113730 <__clone>:
  113730:       3c1c000c        lui     gp,0xc
  113734:       279ca5c0        addiu   gp,gp,-23104
  113738:       0399e021        addu    gp,gp,t9
  11373c:       27bdffe8        addiu   sp,sp,-24
  113740:       afbc0014        sw      gp,20(sp)
  113744:       10800010        beqz    a0,113788 <__clone+0x58>
  113748:       24020016        li      v0,22
  11374c:       10a0000e        beqz    a1,113788 <__clone+0x58>
  113750:       00000000        nop
  113754:       24a5ffe0        addiu   a1,a1,-32
  113758:       aca40000        sw      a0,0(a1)
  11375c:       aca70004        sw      a3,4(a1)
  113760:       00c02021        move    a0,a2
  113764:       24021018        li      v0,4120
  113768:       0000000c        syscall
  11376c:       14e00006        bnez    a3,113788 <__clone+0x58>
  113770:       00000000        nop
  113774:       10400008        beqz    v0,113798 <__thread_start>
  113778:       00000000        nop
  11377c:       03e00008        jr      ra
  113780:       27bd0018        addiu   sp,sp,24
  113784:       00000000        nop
  113788:       8f998290        lw      t9,-32112(gp)
  11378c:       27bd0018        addiu   sp,sp,24
  113790:       03200008        jr      t9
  113794:       00000000        nop
00113798 <__thread_start>:
  113798:       afbc0014        sw      gp,20(sp)
  11379c:       8fb90000        lw      t9,0(sp)
  1137a0:       8fa40004        lw      a0,4(sp)
  1137a4:       0320f809        jalr    t9
  1137a8:       00000000        nop
  1137ac:       8fbc0014        lw      gp,20(sp)
  1137b0:       00402021        move    a0,v0
  1137b4:       8f9991f4        lw      t9,-28172(gp)
  1137b8:       00000000        nop
  1137bc:       0320f809        jalr    t9
  1137c0:       00000000        nop

The last jalr is exit.

And here's its PDR (little endian):
 0e0c0 30371100 00000000 00000000 00000000  07..............
 0e0d0 00000000 10000000 1d000000 1d000000  ................
 0e0e0 98371100 00000000 00000000 00000000  .7..............
 0e0f0 00000000 00000000 00000000 00000000  ................

Frame register is sp, stack size is 16, no registers saved for clone. 
Then stack size 0, no registers saved, frame register is zero for
__thread_start, which is where we are.  I don't know why that didn't
terminate the stack trace right away, I'll bring the board back up
tomorrow to find out.

I'm pretty sure I had another case in mind when I wrote the first
version of this patch, but I can't think what it was now.  So maybe
this is the only one.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: [rfa/mips] Stop backtraces when we've lost the PC

[Daniel Jacobowitz]

On Sun, Mar 07, 2004 at 10:23:24PM -0500, Daniel Jacobowitz wrote:
> On Sun, Mar 07, 2004 at 07:56:18PM -0500, Andrew Cagney wrote:
> > >Here's an updated version of a little hack I've been using since GDB 6.0.
> > >If we are in a nested normal frame, i.e. something whose next frame is a
> > >function that it called in the normal way, and we didn't find a saved PC,
> > >we're going to be stuck in a loop.  We might have been able to figure out
> > >the frame size, but not where the return address was stored; as the comment
> > >says, this happens in glibc's clone function.  Of course the problem there
> > >is that it _doesn't_ save $ra in the normal fashion; it won't return.
> > >
> > >Without this patch schedlock.exp falls apart, because backtraces continue
> > >forever printing "clone()" on every line.
> > >
> > >OK?
> > 
> > No!
> > 
> > >+  if (frame_relative_level (next_frame) >= 0
> > >+      && get_frame_type (next_frame) == NORMAL_FRAME
> > >+      && !trad_frame_addr_p (info->saved_regs, NUM_REGS + PC_REGNUM))
> > 
> > The whole point of this recursive code is that you don't need to add 
> > hacks that look down the stack at the type of more inner frames :-/
> > 
> > Can you post the relevant assembler so that we can determine why the 
> > prologue analyzer is failing.
> 
> This is clone.  It can't be unwound past - there is no stack behind it
> (we're in the child, here).
> 
> 00113730 <__clone>:
>   113730:       3c1c000c        lui     gp,0xc
>   113734:       279ca5c0        addiu   gp,gp,-23104
>   113738:       0399e021        addu    gp,gp,t9
>   11373c:       27bdffe8        addiu   sp,sp,-24
>   113740:       afbc0014        sw      gp,20(sp)
>   113744:       10800010        beqz    a0,113788 <__clone+0x58>
>   113748:       24020016        li      v0,22
>   11374c:       10a0000e        beqz    a1,113788 <__clone+0x58>
>   113750:       00000000        nop
>   113754:       24a5ffe0        addiu   a1,a1,-32
>   113758:       aca40000        sw      a0,0(a1)
>   11375c:       aca70004        sw      a3,4(a1)
>   113760:       00c02021        move    a0,a2
>   113764:       24021018        li      v0,4120
>   113768:       0000000c        syscall
>   11376c:       14e00006        bnez    a3,113788 <__clone+0x58>
>   113770:       00000000        nop
>   113774:       10400008        beqz    v0,113798 <__thread_start>
>   113778:       00000000        nop
>   11377c:       03e00008        jr      ra
>   113780:       27bd0018        addiu   sp,sp,24
>   113784:       00000000        nop
>   113788:       8f998290        lw      t9,-32112(gp)
>   11378c:       27bd0018        addiu   sp,sp,24
>   113790:       03200008        jr      t9
>   113794:       00000000        nop
> 00113798 <__thread_start>:
>   113798:       afbc0014        sw      gp,20(sp)
>   11379c:       8fb90000        lw      t9,0(sp)
>   1137a0:       8fa40004        lw      a0,4(sp)
>   1137a4:       0320f809        jalr    t9
>   1137a8:       00000000        nop
>   1137ac:       8fbc0014        lw      gp,20(sp)
>   1137b0:       00402021        move    a0,v0
>   1137b4:       8f9991f4        lw      t9,-28172(gp)
>   1137b8:       00000000        nop
>   1137bc:       0320f809        jalr    t9
>   1137c0:       00000000        nop
> 
> The last jalr is exit.
> 
> And here's its PDR (little endian):
>  0e0c0 30371100 00000000 00000000 00000000  07..............
>  0e0d0 00000000 10000000 1d000000 1d000000  ................
>  0e0e0 98371100 00000000 00000000 00000000  .7..............
>  0e0f0 00000000 00000000 00000000 00000000  ................
> 
> Frame register is sp, stack size is 16, no registers saved for clone. 
> Then stack size 0, no registers saved, frame register is zero for
> __thread_start, which is where we are.  I don't know why that didn't
> terminate the stack trace right away, I'll bring the board back up
> tomorrow to find out.

OK, two things.


The first problem is that __thread_start is a local symbol, and the
copy of the library I'm using on my target is stripped of local symbols
but has a .pdr section.  We pass the PC to find_proc_desc, which is
fine, but non_heuristic_proc_desc does:
  find_pc_partial_function (pc, NULL, &startaddr, NULL);
and looks for a PDR associated with startaddr.  Oopsie.

The problem is, GAS's .pdr information does not include a size or an
end address.  It's slightly risky to search for the latest .pdr entry
preceding PC, because that might be for a completely different
function, but not very risky - .frame will generally be used
consistently if it was used at all.

I believe the attached patch is safe in a superset of circumstances to
where the previous code was safe.  It causes us to select the correct
.pdr entry in this circumstance.  What do you think of it?

[BTW: I verified that without a .pdr section we notice that we can't
find a prologue for clone, and give up unwinding semi-gracefully.  I
also verified that with local symbols, we find the correct .pdr.]


The other thing:

I hypothesize that if two consecutive frames, regardless of their type,
claim to save the PC register at the same location, then unwinding is
hosed.  If we changed the interface for gdbarch_unwind_pc to return
information on where, if anywhere, the PC was unwound from, we could
check this.  This handles sigtramp frames (which save the PC at a
particular memory buffer), dummy frames (which would say not_lval, i.e.
the PC is not saved in a simple fashion, i.e. skip this test), typical
normal frames (the sentinel frame says it's saved in a register, the
outermost frame says it's saved in the return address register or in a
local stack slot, normal frames which called other normal frames will
have it in a local stack slot.

Obviously for some architecture with a reconstructed not_lval PC this
wouldn't work, but I think it could still be useful as a safety check
on the unwinder.  Not a safe termination - this sort of hack doesn't
have any business being relied on - but at least a call to error() to
prevent spiralling off into infinity.

Does my hypothesis make sense?  Shall I try to implement it?

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

2004-03-08  Daniel Jacobowitz  <drow@mvista.com>

	* mips-tdep.c (non_heuristic_proc_desc): Search using the specified
	PC rather than the partial function start address.  Use the start
	address to sanity check the found PDR.

Index: mips-tdep.c
===================================================================
RCS file: /big/fsf/rsync/src-cvs/src/gdb/mips-tdep.c,v
retrieving revision 1.283
diff -u -p -r1.283 mips-tdep.c
--- mips-tdep.c	17 Feb 2004 15:21:21 -0000	1.283
+++ mips-tdep.c	8 Mar 2004 15:33:25 -0000
@@ -2289,10 +2289,6 @@ non_heuristic_proc_desc (CORE_ADDR pc, C
   if (DEPRECATED_PC_IN_CALL_DUMMY (pc, 0, 0))
     return NULL;
 
-  find_pc_partial_function (pc, NULL, &startaddr, NULL);
-  if (addrptr)
-    *addrptr = startaddr;
-
   priv = NULL;
 
   sec = find_pc_section (pc);
@@ -2352,38 +2348,60 @@ non_heuristic_proc_desc (CORE_ADDR pc, C
 	{
 	  int low, mid, high;
 	  char *ptr;
+	  CORE_ADDR pdr_pc;
 
 	  low = 0;
 	  high = priv->size / 32;
 
+	  /* First, find the last .pdr entry starting at or before PC.  */
 	  do
 	    {
-	      CORE_ADDR pdr_pc;
-
 	      mid = (low + high) / 2;
 
 	      ptr = priv->contents + mid * 32;
 	      pdr_pc = bfd_get_signed_32 (sec->objfile->obfd, ptr);
 	      pdr_pc += ANOFFSET (sec->objfile->section_offsets,
 				  SECT_OFF_TEXT (sec->objfile));
-	      if (pdr_pc == startaddr)
+	      if (pdr_pc == pc)
 		break;
-	      if (pdr_pc > startaddr)
+	      if (pdr_pc > pc)
 		high = mid;
 	      else
 		low = mid + 1;
 	    }
 	  while (low != high);
 
-	  if (low != high)
+	  /* If we got an exact match on PC, low != high.  Otherwise,
+	     both low and high point one past the PDR of interest.  */
+	  if (low == high && low > 0)
+	    {
+	      ptr = priv->contents + (low - 1) * 32;
+	      pdr_pc = bfd_get_signed_32 (sec->objfile->obfd, ptr);
+	      pdr_pc += ANOFFSET (sec->objfile->section_offsets,
+				  SECT_OFF_TEXT (sec->objfile));
+	    }
+
+	  /* We don't have a range, so we have no way to know for sure
+	     whether we're in the correct PDR or a PDR for a preceding
+	     function and the current function was a stripped local
+	     symbol.  But if the PDR's PC is at least as great as the
+	     best guess from the symbol table, assume that it does cover
+	     the right area; if a .pdr section is present at all then
+	     nearly every function will have an entry.  */
+
+	  find_pc_partial_function (pc, NULL, &startaddr, NULL);
+	  if (startaddr <= pdr_pc)
 	    {
 	      struct symbol *sym = find_pc_function (pc);
 
+	      if (addrptr)
+		*addrptr = pdr_pc;
+
 	      /* Fill in what we need of the proc_desc.  */
 	      proc_desc = (mips_extra_func_info_t)
 		obstack_alloc (&sec->objfile->objfile_obstack,
 			       sizeof (struct mips_extra_func_info));
-	      PROC_LOW_ADDR (proc_desc) = startaddr;
+	      PROC_LOW_ADDR (proc_desc) = pdr_pc;
 
 	      /* Only used for dummy frames.  */
 	      PROC_HIGH_ADDR (proc_desc) = 0;
@@ -2412,6 +2430,10 @@ non_heuristic_proc_desc (CORE_ADDR pc, C
 
   if (b == NULL)
     return NULL;
+
+  find_pc_partial_function (pc, NULL, &startaddr, NULL);
+  if (addrptr)
+    *addrptr = startaddr;
 
   if (startaddr > BLOCK_START (b))
     {

Re: [rfa/mips] Stop backtraces when we've lost the PC

[Andrew Cagney]

> This is clone.  It can't be unwound past - there is no stack behind it
> (we're in the child, here).

Briefly, MarkK and I hit a problem with the exact same function on 
GNU/Linux amd64.  The root cause was identified as glibc:
http://sources.redhat.com/ml/gdb-patches/2004-02/msg00567.html

(I'll get to your other follow up but will take a bit of time to digest)

Andrew

Re: [rfa/mips] Stop backtraces when we've lost the PC

[Daniel Jacobowitz]

On Mon, Mar 08, 2004 at 10:48:14AM -0500, Daniel Jacobowitz wrote:
> I believe the attached patch is safe in a superset of circumstances to
> where the previous code was safe.  It causes us to select the correct
> .pdr entry in this circumstance.  What do you think of it?

The patch is not safe; testing showed regressions in break.exp and
other places.

I think the theory is sound, but I need to analyze the regressions
to figure out what's going on.  Sorry 'bout that.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: [rfa/mips] Stop backtraces when we've lost the PC

[Daniel Jacobowitz]

On Mon, Mar 08, 2004 at 10:48:14AM -0500, Daniel Jacobowitz wrote:
> OK, two things.
> 
> 
> The first problem is that __thread_start is a local symbol, and the
> copy of the library I'm using on my target is stripped of local symbols
> but has a .pdr section.  We pass the PC to find_proc_desc, which is
> fine, but non_heuristic_proc_desc does:
>   find_pc_partial_function (pc, NULL, &startaddr, NULL);
> and looks for a PDR associated with startaddr.  Oopsie.
> 
> The problem is, GAS's .pdr information does not include a size or an
> end address.  It's slightly risky to search for the latest .pdr entry
> preceding PC, because that might be for a completely different
> function, but not very risky - .frame will generally be used
> consistently if it was used at all.
> 
> I believe the attached patch is safe in a superset of circumstances to
> where the previous code was safe.  It causes us to select the correct
> .pdr entry in this circumstance.  What do you think of it?
> 
> [BTW: I verified that without a .pdr section we notice that we can't
> find a prologue for clone, and give up unwinding semi-gracefully.  I
> also verified that with local symbols, we find the correct .pdr.]
> 
> 
> The other thing:
> 
> I hypothesize that if two consecutive frames, regardless of their type,
> claim to save the PC register at the same location, then unwinding is
> hosed.  If we changed the interface for gdbarch_unwind_pc to return
> information on where, if anywhere, the PC was unwound from, we could
> check this.  This handles sigtramp frames (which save the PC at a
> particular memory buffer), dummy frames (which would say not_lval, i.e.
> the PC is not saved in a simple fashion, i.e. skip this test), typical
> normal frames (the sentinel frame says it's saved in a register, the
> outermost frame says it's saved in the return address register or in a
> local stack slot, normal frames which called other normal frames will
> have it in a local stack slot.
> 
> Obviously for some architecture with a reconstructed not_lval PC this
> wouldn't work, but I think it could still be useful as a safety check
> on the unwinder.  Not a safe termination - this sort of hack doesn't
> have any business being relied on - but at least a call to error() to
> prevent spiralling off into infinity.
> 
> Does my hypothesis make sense?  Shall I try to implement it?

I stand by the theory but there was a problem in the implementation;
several, actually, related to dynamic resolution stubs.  Which are next
on my list of things that need to get taken care of.  Right now, with
or without my patch, they pick up the .pdr entry describing the last
full section before the PLT.  An unfortunate consequence of their being
in .text on MIPS.  So a little care is needed to mitigate the
consequences.

Corrected patch attached.  Fully tested on mips-linux this time.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

2004-03-08  Daniel Jacobowitz  <drow@mvista.com>

	* mips-tdep.c (non_heuristic_proc_desc): Search using the specified
	PC rather than the partial function start address.  Use the start
	address to sanity check the found PDR.

Index: mips-tdep.c
===================================================================
RCS file: /big/fsf/rsync/src-cvs/src/gdb/mips-tdep.c,v
retrieving revision 1.283
diff -u -p -r1.283 mips-tdep.c
--- mips-tdep.c	17 Feb 2004 15:21:21 -0000	1.283
+++ mips-tdep.c	8 Mar 2004 18:50:58 -0000
@@ -2352,38 +2352,63 @@ non_heuristic_proc_desc (CORE_ADDR pc, C
 	{
 	  int low, mid, high;
 	  char *ptr;
+	  CORE_ADDR pdr_pc;
 
 	  low = 0;
 	  high = priv->size / 32;
 
+	  /* First, find the last .pdr entry starting at or before PC.  */
 	  do
 	    {
-	      CORE_ADDR pdr_pc;
-
 	      mid = (low + high) / 2;
 
 	      ptr = priv->contents + mid * 32;
 	      pdr_pc = bfd_get_signed_32 (sec->objfile->obfd, ptr);
 	      pdr_pc += ANOFFSET (sec->objfile->section_offsets,
 				  SECT_OFF_TEXT (sec->objfile));
-	      if (pdr_pc == startaddr)
+	      if (pdr_pc == pc)
 		break;
-	      if (pdr_pc > startaddr)
+	      if (pdr_pc > pc)
 		high = mid;
 	      else
 		low = mid + 1;
 	    }
 	  while (low != high);
 
-	  if (low != high)
+	  /* If we got an exact match on PC, low != high.  Otherwise,
+	     both low and high point one past the PDR of interest.
+	     If both are zero, that means this PC is before any
+	     region covered by a PDR.  */
+	  if (low == high && low > 0)
+	    {
+	      ptr = priv->contents + (low - 1) * 32;
+	      pdr_pc = bfd_get_signed_32 (sec->objfile->obfd, ptr);
+	      pdr_pc += ANOFFSET (sec->objfile->section_offsets,
+				  SECT_OFF_TEXT (sec->objfile));
+	    }
+
+	  /* We don't have a range, so we have no way to know for sure
+	     whether we're in the correct PDR or a PDR for a preceding
+	     function and the current function was a stripped local
+	     symbol.  But if the PDR's PC is at least as great as the
+	     best guess from the symbol table, assume that it does cover
+	     the right area; if a .pdr section is present at all then
+	     nearly every function will have an entry.  The biggest exception
+	     will be the dynamic linker stubs; conveniently these are
+	     placed before .text instead of after.  */
+
+	  if (pc >= pdr_pc && pdr_pc >= startaddr)
 	    {
 	      struct symbol *sym = find_pc_function (pc);
 
+	      if (addrptr)
+		*addrptr = pdr_pc;
+
 	      /* Fill in what we need of the proc_desc.  */
 	      proc_desc = (mips_extra_func_info_t)
 		obstack_alloc (&sec->objfile->objfile_obstack,
 			       sizeof (struct mips_extra_func_info));
-	      PROC_LOW_ADDR (proc_desc) = startaddr;
+	      PROC_LOW_ADDR (proc_desc) = pdr_pc;
 
 	      /* Only used for dummy frames.  */
 	      PROC_HIGH_ADDR (proc_desc) = 0;

Re: [rfa/mips] Stop backtraces when we've lost the PC

[Andrew Cagney]

> I hypothesize that if two consecutive frames, regardless of their type,
> claim to save the PC register at the same location, then unwinding is
> hosed.

It would need to do a deep analysis of the location (think about a 
register window architecture), hence I don't know that there's that much 
cost benefit.  Something simpler such as a list of functions known to 
terminate the stack might be more useful.

Andrew

Re: [rfa/mips] Stop backtraces when we've lost the PC

[Daniel Jacobowitz]

On Thu, Mar 11, 2004 at 03:51:11PM -0500, Andrew Cagney wrote:
> >I hypothesize that if two consecutive frames, regardless of their type,
> >claim to save the PC register at the same location, then unwinding is
> >hosed.
> 
> It would need to do a deep analysis of the location (think about a 
> register window architecture), hence I don't know that there's that much 
> cost benefit.  Something simpler such as a list of functions known to 
> terminate the stack might be more useful.

Er, no.  frame_unwind_register tells us where, relative to the current
machine state, the register is saved.  If it returns lval_register and
real_regnum == O7_REGNUM, then that means it leaves in
read_register(O7_REGNUM) at this moment, not that it did at some point
in the past.  Isn't that the point of the recursive unwinder?

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: [rfa/mips] Stop backtraces when we've lost the PC

[Andrew Cagney]

> On Thu, Mar 11, 2004 at 03:51:11PM -0500, Andrew Cagney wrote:
> 
>>>> >I hypothesize that if two consecutive frames, regardless of their type,
>>>> >claim to save the PC register at the same location, then unwinding is
>>>> >hosed.
>>
>>> 
>>> It would need to do a deep analysis of the location (think about a 
>>> register window architecture), hence I don't know that there's that much 
>>> cost benefit.

 >>> Something simpler such as a list of functions known to
>>> terminate the stack might be more useful.
> 
> 
> Er, no.  frame_unwind_register tells us where, relative to the current
> machine state, the register is saved.  If it returns lval_register and
> real_regnum == O7_REGNUM, then that means it leaves in
> read_register(O7_REGNUM) at this moment, not that it did at some point
> in the past.  Isn't that the point of the recursive unwinder?

"Er, no". to which part?  I'll assume the first half of the first half.

I suspect you're violently agreeing with me here - you're describing 
what I ment by a deep analysis of the location - tracking things all the 
way back to where in the inferior the value is.   The architecture 
vector will need to be changed, the existing function deprecated, and 
new methods implemented.  The introduction of "struct location" (or 
whatever) would then see it changed again. Given it is all for a 
marginal edge case (and to cover up breakage in glibc), I don't see any 
cost benefit in doing this.

I think a more useful mechanism is for there to be a table of "start" 
functions that the user could manipulate (but would default to values 
specified by the OSABI).

Andrew

Re: [rfa/mips] Stop backtraces when we've lost the PC

[Daniel Jacobowitz]

On Thu, Mar 11, 2004 at 06:47:04PM -0500, Andrew Cagney wrote:
> >On Thu, Mar 11, 2004 at 03:51:11PM -0500, Andrew Cagney wrote:
> >
> >>>>>I hypothesize that if two consecutive frames, regardless of their type,
> >>>>>claim to save the PC register at the same location, then unwinding is
> >>>>>hosed.
> >>
> >>>
> >>>It would need to do a deep analysis of the location (think about a 
> >>>register window architecture), hence I don't know that there's that much 
> >>>cost benefit.
> 
> >>> Something simpler such as a list of functions known to
> >>>terminate the stack might be more useful.
> >
> >
> >Er, no.  frame_unwind_register tells us where, relative to the current
> >machine state, the register is saved.  If it returns lval_register and
> >real_regnum == O7_REGNUM, then that means it leaves in
> >read_register(O7_REGNUM) at this moment, not that it did at some point
> >in the past.  Isn't that the point of the recursive unwinder?
> 
> "Er, no". to which part?  I'll assume the first half of the first half.
> 
> I suspect you're violently agreeing with me here - you're describing 
> what I ment by a deep analysis of the location - tracking things all the 
> way back to where in the inferior the value is.   The architecture 
> vector will need to be changed, the existing function deprecated, and 
> new methods implemented.  The introduction of "struct location" (or 
> whatever) would then see it changed again. Given it is all for a 
> marginal edge case (and to cover up breakage in glibc), I don't see any 
> cost benefit in doing this.

OK.  It was just a thought :)  It seems reasonable that whatever kind
of location frame_unwind_register returns (which you're right, is
likely to change) could naturally be returned by frame_unwind_pc also.
But it would require playing with the interfaces pretty severely, so
I'll just table the idea unless I run into this again somewhere else.

> I think a more useful mechanism is for there to be a table of "start" 
> functions that the user could manipulate (but would default to values 
> specified by the OSABI).

I'm not sure how useful that would really be; we seem to handle the
entry points OK at the moment.  And it couldn't be used for this case
since we do want to backtrace past clone in some circumstances.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: [rfa/mips] Stop backtraces when we've lost the PC

[Andrew Cagney]

>  	{
>  	  int low, mid, high;
>  	  char *ptr;
> +	  CORE_ADDR pdr_pc;
>  
>  	  low = 0;
>  	  high = priv->size / 32;
>  
> +	  /* First, find the last .pdr entry starting at or before PC.  */

A summary here of what the objective is and what / why the follow on 
code is doing.

>  	  do
>  	    {
> -	      CORE_ADDR pdr_pc;
> -
>  	      mid = (low + high) / 2;
>  
>  	      ptr = priv->contents + mid * 32;
>  	      pdr_pc = bfd_get_signed_32 (sec->objfile->obfd, ptr);
>  	      pdr_pc += ANOFFSET (sec->objfile->section_offsets,
>  				  SECT_OFF_TEXT (sec->objfile));

> -	      if (pdr_pc == startaddr)
> +	      if (pdr_pc == pc)
>  		break;

> -	      if (pdr_pc > startaddr)
> +	      if (pdr_pc > pc)
>  		high = mid;
>  	      else
>  		low = mid + 1;

>  	    }

>  	  while (low != high);

Other than that, just an observation that the binary search is [already] 
pretty messed up.  In all likelyhood the test:

	if (pdr_pc == pc)

will never fire and having it gains little if anything (one less 
iteration VS logN extra compares).  Eliminating it means cleaning up the 
binary search though.   Can you attach a fixme to that test indicating 
that it should be eliminated.

I also suspect that STARTADDR's computation can be delayed until it is 
needed (the latter reference that goes with the "pathological", should 
no longer occure, I think it has been moved into symbol reading. 
However, leave that for the moment.

Otherwize ok, and way better than the original patch, thanks,

Andrew

[rfa/mips] Stop backtraces when we've lost the PC

[Daniel Jacobowitz]

Here's an updated version of a little hack I've been using since GDB 6.0.
If we are in a nested normal frame, i.e. something whose next frame is a
function that it called in the normal way, and we didn't find a saved PC,
we're going to be stuck in a loop.  We might have been able to figure out
the frame size, but not where the return address was stored; as the comment
says, this happens in glibc's clone function.  Of course the problem there
is that it _doesn't_ save $ra in the normal fashion; it won't return.

Without this patch schedlock.exp falls apart, because backtraces continue
forever printing "clone()" on every line.

OK?  Or a better way to do this?

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

2004-03-06  Daniel Jacobowitz  <drow@mvista.com>

	* mips-tdep.c (mips_mdebug_frame_this_id): Terminate unwinding if
	we haven't found a saved PC.

Index: mips-tdep.c
===================================================================
RCS file: /big/fsf/rsync/src-cvs/src/gdb/mips-tdep.c,v
retrieving revision 1.283
diff -u -p -r1.283 mips-tdep.c
--- mips-tdep.c	17 Feb 2004 15:21:21 -0000	1.283
+++ mips-tdep.c	6 Mar 2004 22:52:29 -0000
@@ -1672,6 +1672,24 @@ mips_mdebug_frame_this_id (struct frame_
 {
   struct mips_frame_cache *info = mips_mdebug_frame_cache (next_frame,
 							   this_cache);
+
+  /* If the return address is not saved for two frames in a row,
+     then we are probably hosed.  Not necessarily - it's possible to
+     write working assembly that violates this rule - but we can't
+     backtrace through that either.  Eventually MIPS will support
+     DWARF2 unwind information, allowing assembly programmers to
+     avoid this problem.
+
+     One place this check triggers is in the GNU/Linux clone syscall
+     wrapper.  */
+  if (frame_relative_level (next_frame) >= 0
+      && get_frame_type (next_frame) == NORMAL_FRAME
+      && !trad_frame_addr_p (info->saved_regs, NUM_REGS + PC_REGNUM))
+    {
+      (*this_id) = null_frame_id;
+      return;
+    }
+
   (*this_id) = frame_id_build (info->base, frame_func_unwind (next_frame));
 }
 

Re: [rfa/mips] Stop backtraces when we've lost the PC

[Andrew Cagney]

> On Thu, Mar 11, 2004 at 03:51:11PM -0500, Andrew Cagney wrote:
> 
>>>> >I hypothesize that if two consecutive frames, regardless of their type,
>>>> >claim to save the PC register at the same location, then unwinding is
>>>> >hosed.
>>
>>> 
>>> It would need to do a deep analysis of the location (think about a 
>>> register window architecture), hence I don't know that there's that much 
>>> cost benefit.

 >>> Something simpler such as a list of functions known to
>>> terminate the stack might be more useful.
> 
> 
> Er, no.  frame_unwind_register tells us where, relative to the current
> machine state, the register is saved.  If it returns lval_register and
> real_regnum == O7_REGNUM, then that means it leaves in
> read_register(O7_REGNUM) at this moment, not that it did at some point
> in the past.  Isn't that the point of the recursive unwinder?

"Er, no". to which part?  I'll assume the first half of the first half.

I suspect you're violently agreeing with me here - you're describing 
what I ment by a deep analysis of the location - tracking things all the 
way back to where in the inferior the value is.   The architecture 
vector will need to be changed, the existing function deprecated, and 
new methods implemented.  The introduction of "struct location" (or 
whatever) would then see it changed again. Given it is all for a 
marginal edge case (and to cover up breakage in glibc), I don't see any 
cost benefit in doing this.

I think a more useful mechanism is for there to be a table of "start" 
functions that the user could manipulate (but would default to values 
specified by the OSABI).

Andrew

Re: [rfa/mips] Stop backtraces when we've lost the PC

[Andrew Cagney]

> This is clone.  It can't be unwound past - there is no stack behind it
> (we're in the child, here).

Briefly, MarkK and I hit a problem with the exact same function on 
GNU/Linux amd64.  The root cause was identified as glibc:
http://sources.redhat.com/ml/gdb-patches/2004-02/msg00567.html

(I'll get to your other follow up but will take a bit of time to digest)

Andrew

Re: [rfa/mips] Stop backtraces when we've lost the PC

[Andrew Cagney]

> Here's an updated version of a little hack I've been using since GDB 6.0.
> If we are in a nested normal frame, i.e. something whose next frame is a
> function that it called in the normal way, and we didn't find a saved PC,
> we're going to be stuck in a loop.  We might have been able to figure out
> the frame size, but not where the return address was stored; as the comment
> says, this happens in glibc's clone function.  Of course the problem there
> is that it _doesn't_ save $ra in the normal fashion; it won't return.
> 
> Without this patch schedlock.exp falls apart, because backtraces continue
> forever printing "clone()" on every line.
> 
> OK?

No!

> +  if (frame_relative_level (next_frame) >= 0
> +      && get_frame_type (next_frame) == NORMAL_FRAME
> +      && !trad_frame_addr_p (info->saved_regs, NUM_REGS + PC_REGNUM))

The whole point of this recursive code is that you don't need to add 
hacks that look down the stack at the type of more inner frames :-/

Can you post the relevant assembler so that we can determine why the 
prologue analyzer is failing.

Andrew

Re: [rfa/mips] Stop backtraces when we've lost the PC

[Andrew Cagney]

>  	{
>  	  int low, mid, high;
>  	  char *ptr;
> +	  CORE_ADDR pdr_pc;
>  
>  	  low = 0;
>  	  high = priv->size / 32;
>  
> +	  /* First, find the last .pdr entry starting at or before PC.  */

A summary here of what the objective is and what / why the follow on 
code is doing.

>  	  do
>  	    {
> -	      CORE_ADDR pdr_pc;
> -
>  	      mid = (low + high) / 2;
>  
>  	      ptr = priv->contents + mid * 32;
>  	      pdr_pc = bfd_get_signed_32 (sec->objfile->obfd, ptr);
>  	      pdr_pc += ANOFFSET (sec->objfile->section_offsets,
>  				  SECT_OFF_TEXT (sec->objfile));

> -	      if (pdr_pc == startaddr)
> +	      if (pdr_pc == pc)
>  		break;

> -	      if (pdr_pc > startaddr)
> +	      if (pdr_pc > pc)
>  		high = mid;
>  	      else
>  		low = mid + 1;

>  	    }

>  	  while (low != high);

Other than that, just an observation that the binary search is [already] 
pretty messed up.  In all likelyhood the test:

	if (pdr_pc == pc)

will never fire and having it gains little if anything (one less 
iteration VS logN extra compares).  Eliminating it means cleaning up the 
binary search though.   Can you attach a fixme to that test indicating 
that it should be eliminated.

I also suspect that STARTADDR's computation can be delayed until it is 
needed (the latter reference that goes with the "pathological", should 
no longer occure, I think it has been moved into symbol reading. 
However, leave that for the moment.

Otherwize ok, and way better than the original patch, thanks,

Andrew

Re: [rfa/mips] Stop backtraces when we've lost the PC

[Daniel Jacobowitz]

On Sun, Mar 07, 2004 at 10:23:24PM -0500, Daniel Jacobowitz wrote:
> On Sun, Mar 07, 2004 at 07:56:18PM -0500, Andrew Cagney wrote:
> > >Here's an updated version of a little hack I've been using since GDB 6.0.
> > >If we are in a nested normal frame, i.e. something whose next frame is a
> > >function that it called in the normal way, and we didn't find a saved PC,
> > >we're going to be stuck in a loop.  We might have been able to figure out
> > >the frame size, but not where the return address was stored; as the comment
> > >says, this happens in glibc's clone function.  Of course the problem there
> > >is that it _doesn't_ save $ra in the normal fashion; it won't return.
> > >
> > >Without this patch schedlock.exp falls apart, because backtraces continue
> > >forever printing "clone()" on every line.
> > >
> > >OK?
> > 
> > No!
> > 
> > >+  if (frame_relative_level (next_frame) >= 0
> > >+      && get_frame_type (next_frame) == NORMAL_FRAME
> > >+      && !trad_frame_addr_p (info->saved_regs, NUM_REGS + PC_REGNUM))
> > 
> > The whole point of this recursive code is that you don't need to add 
> > hacks that look down the stack at the type of more inner frames :-/
> > 
> > Can you post the relevant assembler so that we can determine why the 
> > prologue analyzer is failing.
> 
> This is clone.  It can't be unwound past - there is no stack behind it
> (we're in the child, here).
> 
> 00113730 <__clone>:
>   113730:       3c1c000c        lui     gp,0xc
>   113734:       279ca5c0        addiu   gp,gp,-23104
>   113738:       0399e021        addu    gp,gp,t9
>   11373c:       27bdffe8        addiu   sp,sp,-24
>   113740:       afbc0014        sw      gp,20(sp)
>   113744:       10800010        beqz    a0,113788 <__clone+0x58>
>   113748:       24020016        li      v0,22
>   11374c:       10a0000e        beqz    a1,113788 <__clone+0x58>
>   113750:       00000000        nop
>   113754:       24a5ffe0        addiu   a1,a1,-32
>   113758:       aca40000        sw      a0,0(a1)
>   11375c:       aca70004        sw      a3,4(a1)
>   113760:       00c02021        move    a0,a2
>   113764:       24021018        li      v0,4120
>   113768:       0000000c        syscall
>   11376c:       14e00006        bnez    a3,113788 <__clone+0x58>
>   113770:       00000000        nop
>   113774:       10400008        beqz    v0,113798 <__thread_start>
>   113778:       00000000        nop
>   11377c:       03e00008        jr      ra
>   113780:       27bd0018        addiu   sp,sp,24
>   113784:       00000000        nop
>   113788:       8f998290        lw      t9,-32112(gp)
>   11378c:       27bd0018        addiu   sp,sp,24
>   113790:       03200008        jr      t9
>   113794:       00000000        nop
> 00113798 <__thread_start>:
>   113798:       afbc0014        sw      gp,20(sp)
>   11379c:       8fb90000        lw      t9,0(sp)
>   1137a0:       8fa40004        lw      a0,4(sp)
>   1137a4:       0320f809        jalr    t9
>   1137a8:       00000000        nop
>   1137ac:       8fbc0014        lw      gp,20(sp)
>   1137b0:       00402021        move    a0,v0
>   1137b4:       8f9991f4        lw      t9,-28172(gp)
>   1137b8:       00000000        nop
>   1137bc:       0320f809        jalr    t9
>   1137c0:       00000000        nop
> 
> The last jalr is exit.
> 
> And here's its PDR (little endian):
>  0e0c0 30371100 00000000 00000000 00000000  07..............
>  0e0d0 00000000 10000000 1d000000 1d000000  ................
>  0e0e0 98371100 00000000 00000000 00000000  .7..............
>  0e0f0 00000000 00000000 00000000 00000000  ................
> 
> Frame register is sp, stack size is 16, no registers saved for clone. 
> Then stack size 0, no registers saved, frame register is zero for
> __thread_start, which is where we are.  I don't know why that didn't
> terminate the stack trace right away, I'll bring the board back up
> tomorrow to find out.

OK, two things.


The first problem is that __thread_start is a local symbol, and the
copy of the library I'm using on my target is stripped of local symbols
but has a .pdr section.  We pass the PC to find_proc_desc, which is
fine, but non_heuristic_proc_desc does:
  find_pc_partial_function (pc, NULL, &startaddr, NULL);
and looks for a PDR associated with startaddr.  Oopsie.

The problem is, GAS's .pdr information does not include a size or an
end address.  It's slightly risky to search for the latest .pdr entry
preceding PC, because that might be for a completely different
function, but not very risky - .frame will generally be used
consistently if it was used at all.

I believe the attached patch is safe in a superset of circumstances to
where the previous code was safe.  It causes us to select the correct
.pdr entry in this circumstance.  What do you think of it?

[BTW: I verified that without a .pdr section we notice that we can't
find a prologue for clone, and give up unwinding semi-gracefully.  I
also verified that with local symbols, we find the correct .pdr.]


The other thing:

I hypothesize that if two consecutive frames, regardless of their type,
claim to save the PC register at the same location, then unwinding is
hosed.  If we changed the interface for gdbarch_unwind_pc to return
information on where, if anywhere, the PC was unwound from, we could
check this.  This handles sigtramp frames (which save the PC at a
particular memory buffer), dummy frames (which would say not_lval, i.e.
the PC is not saved in a simple fashion, i.e. skip this test), typical
normal frames (the sentinel frame says it's saved in a register, the
outermost frame says it's saved in the return address register or in a
local stack slot, normal frames which called other normal frames will
have it in a local stack slot.

Obviously for some architecture with a reconstructed not_lval PC this
wouldn't work, but I think it could still be useful as a safety check
on the unwinder.  Not a safe termination - this sort of hack doesn't
have any business being relied on - but at least a call to error() to
prevent spiralling off into infinity.

Does my hypothesis make sense?  Shall I try to implement it?

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

2004-03-08  Daniel Jacobowitz  <drow@mvista.com>

	* mips-tdep.c (non_heuristic_proc_desc): Search using the specified
	PC rather than the partial function start address.  Use the start
	address to sanity check the found PDR.

Index: mips-tdep.c
===================================================================
RCS file: /big/fsf/rsync/src-cvs/src/gdb/mips-tdep.c,v
retrieving revision 1.283
diff -u -p -r1.283 mips-tdep.c
--- mips-tdep.c	17 Feb 2004 15:21:21 -0000	1.283
+++ mips-tdep.c	8 Mar 2004 15:33:25 -0000
@@ -2289,10 +2289,6 @@ non_heuristic_proc_desc (CORE_ADDR pc, C
   if (DEPRECATED_PC_IN_CALL_DUMMY (pc, 0, 0))
     return NULL;
 
-  find_pc_partial_function (pc, NULL, &startaddr, NULL);
-  if (addrptr)
-    *addrptr = startaddr;
-
   priv = NULL;
 
   sec = find_pc_section (pc);
@@ -2352,38 +2348,60 @@ non_heuristic_proc_desc (CORE_ADDR pc, C
 	{
 	  int low, mid, high;
 	  char *ptr;
+	  CORE_ADDR pdr_pc;
 
 	  low = 0;
 	  high = priv->size / 32;
 
+	  /* First, find the last .pdr entry starting at or before PC.  */
 	  do
 	    {
-	      CORE_ADDR pdr_pc;
-
 	      mid = (low + high) / 2;
 
 	      ptr = priv->contents + mid * 32;
 	      pdr_pc = bfd_get_signed_32 (sec->objfile->obfd, ptr);
 	      pdr_pc += ANOFFSET (sec->objfile->section_offsets,
 				  SECT_OFF_TEXT (sec->objfile));
-	      if (pdr_pc == startaddr)
+	      if (pdr_pc == pc)
 		break;
-	      if (pdr_pc > startaddr)
+	      if (pdr_pc > pc)
 		high = mid;
 	      else
 		low = mid + 1;
 	    }
 	  while (low != high);
 
-	  if (low != high)
+	  /* If we got an exact match on PC, low != high.  Otherwise,
+	     both low and high point one past the PDR of interest.  */
+	  if (low == high && low > 0)
+	    {
+	      ptr = priv->contents + (low - 1) * 32;
+	      pdr_pc = bfd_get_signed_32 (sec->objfile->obfd, ptr);
+	      pdr_pc += ANOFFSET (sec->objfile->section_offsets,
+				  SECT_OFF_TEXT (sec->objfile));
+	    }
+
+	  /* We don't have a range, so we have no way to know for sure
+	     whether we're in the correct PDR or a PDR for a preceding
+	     function and the current function was a stripped local
+	     symbol.  But if the PDR's PC is at least as great as the
+	     best guess from the symbol table, assume that it does cover
+	     the right area; if a .pdr section is present at all then
+	     nearly every function will have an entry.  */
+
+	  find_pc_partial_function (pc, NULL, &startaddr, NULL);
+	  if (startaddr <= pdr_pc)
 	    {
 	      struct symbol *sym = find_pc_function (pc);
 
+	      if (addrptr)
+		*addrptr = pdr_pc;
+
 	      /* Fill in what we need of the proc_desc.  */
 	      proc_desc = (mips_extra_func_info_t)
 		obstack_alloc (&sec->objfile->objfile_obstack,
 			       sizeof (struct mips_extra_func_info));
-	      PROC_LOW_ADDR (proc_desc) = startaddr;
+	      PROC_LOW_ADDR (proc_desc) = pdr_pc;
 
 	      /* Only used for dummy frames.  */
 	      PROC_HIGH_ADDR (proc_desc) = 0;
@@ -2412,6 +2430,10 @@ non_heuristic_proc_desc (CORE_ADDR pc, C
 
   if (b == NULL)
     return NULL;
+
+  find_pc_partial_function (pc, NULL, &startaddr, NULL);
+  if (addrptr)
+    *addrptr = startaddr;
 
   if (startaddr > BLOCK_START (b))
     {

Re: [rfa/mips] Stop backtraces when we've lost the PC

[Daniel Jacobowitz]

On Sun, Mar 07, 2004 at 07:56:18PM -0500, Andrew Cagney wrote:
> >Here's an updated version of a little hack I've been using since GDB 6.0.
> >If we are in a nested normal frame, i.e. something whose next frame is a
> >function that it called in the normal way, and we didn't find a saved PC,
> >we're going to be stuck in a loop.  We might have been able to figure out
> >the frame size, but not where the return address was stored; as the comment
> >says, this happens in glibc's clone function.  Of course the problem there
> >is that it _doesn't_ save $ra in the normal fashion; it won't return.
> >
> >Without this patch schedlock.exp falls apart, because backtraces continue
> >forever printing "clone()" on every line.
> >
> >OK?
> 
> No!
> 
> >+  if (frame_relative_level (next_frame) >= 0
> >+      && get_frame_type (next_frame) == NORMAL_FRAME
> >+      && !trad_frame_addr_p (info->saved_regs, NUM_REGS + PC_REGNUM))
> 
> The whole point of this recursive code is that you don't need to add 
> hacks that look down the stack at the type of more inner frames :-/
> 
> Can you post the relevant assembler so that we can determine why the 
> prologue analyzer is failing.

This is clone.  It can't be unwound past - there is no stack behind it
(we're in the child, here).

00113730 <__clone>:
  113730:       3c1c000c        lui     gp,0xc
  113734:       279ca5c0        addiu   gp,gp,-23104
  113738:       0399e021        addu    gp,gp,t9
  11373c:       27bdffe8        addiu   sp,sp,-24
  113740:       afbc0014        sw      gp,20(sp)
  113744:       10800010        beqz    a0,113788 <__clone+0x58>
  113748:       24020016        li      v0,22
  11374c:       10a0000e        beqz    a1,113788 <__clone+0x58>
  113750:       00000000        nop
  113754:       24a5ffe0        addiu   a1,a1,-32
  113758:       aca40000        sw      a0,0(a1)
  11375c:       aca70004        sw      a3,4(a1)
  113760:       00c02021        move    a0,a2
  113764:       24021018        li      v0,4120
  113768:       0000000c        syscall
  11376c:       14e00006        bnez    a3,113788 <__clone+0x58>
  113770:       00000000        nop
  113774:       10400008        beqz    v0,113798 <__thread_start>
  113778:       00000000        nop
  11377c:       03e00008        jr      ra
  113780:       27bd0018        addiu   sp,sp,24
  113784:       00000000        nop
  113788:       8f998290        lw      t9,-32112(gp)
  11378c:       27bd0018        addiu   sp,sp,24
  113790:       03200008        jr      t9
  113794:       00000000        nop
00113798 <__thread_start>:
  113798:       afbc0014        sw      gp,20(sp)
  11379c:       8fb90000        lw      t9,0(sp)
  1137a0:       8fa40004        lw      a0,4(sp)
  1137a4:       0320f809        jalr    t9
  1137a8:       00000000        nop
  1137ac:       8fbc0014        lw      gp,20(sp)
  1137b0:       00402021        move    a0,v0
  1137b4:       8f9991f4        lw      t9,-28172(gp)
  1137b8:       00000000        nop
  1137bc:       0320f809        jalr    t9
  1137c0:       00000000        nop

The last jalr is exit.

And here's its PDR (little endian):
 0e0c0 30371100 00000000 00000000 00000000  07..............
 0e0d0 00000000 10000000 1d000000 1d000000  ................
 0e0e0 98371100 00000000 00000000 00000000  .7..............
 0e0f0 00000000 00000000 00000000 00000000  ................

Frame register is sp, stack size is 16, no registers saved for clone. 
Then stack size 0, no registers saved, frame register is zero for
__thread_start, which is where we are.  I don't know why that didn't
terminate the stack trace right away, I'll bring the board back up
tomorrow to find out.

I'm pretty sure I had another case in mind when I wrote the first
version of this patch, but I can't think what it was now.  So maybe
this is the only one.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: [rfa/mips] Stop backtraces when we've lost the PC

[Daniel Jacobowitz]

On Thu, Mar 11, 2004 at 03:51:11PM -0500, Andrew Cagney wrote:
> >I hypothesize that if two consecutive frames, regardless of their type,
> >claim to save the PC register at the same location, then unwinding is
> >hosed.
> 
> It would need to do a deep analysis of the location (think about a 
> register window architecture), hence I don't know that there's that much 
> cost benefit.  Something simpler such as a list of functions known to 
> terminate the stack might be more useful.

Er, no.  frame_unwind_register tells us where, relative to the current
machine state, the register is saved.  If it returns lval_register and
real_regnum == O7_REGNUM, then that means it leaves in
read_register(O7_REGNUM) at this moment, not that it did at some point
in the past.  Isn't that the point of the recursive unwinder?

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: [rfa/mips] Stop backtraces when we've lost the PC

[Daniel Jacobowitz]

On Thu, Mar 11, 2004 at 06:47:04PM -0500, Andrew Cagney wrote:
> >On Thu, Mar 11, 2004 at 03:51:11PM -0500, Andrew Cagney wrote:
> >
> >>>>>I hypothesize that if two consecutive frames, regardless of their type,
> >>>>>claim to save the PC register at the same location, then unwinding is
> >>>>>hosed.
> >>
> >>>
> >>>It would need to do a deep analysis of the location (think about a 
> >>>register window architecture), hence I don't know that there's that much 
> >>>cost benefit.
> 
> >>> Something simpler such as a list of functions known to
> >>>terminate the stack might be more useful.
> >
> >
> >Er, no.  frame_unwind_register tells us where, relative to the current
> >machine state, the register is saved.  If it returns lval_register and
> >real_regnum == O7_REGNUM, then that means it leaves in
> >read_register(O7_REGNUM) at this moment, not that it did at some point
> >in the past.  Isn't that the point of the recursive unwinder?
> 
> "Er, no". to which part?  I'll assume the first half of the first half.
> 
> I suspect you're violently agreeing with me here - you're describing 
> what I ment by a deep analysis of the location - tracking things all the 
> way back to where in the inferior the value is.   The architecture 
> vector will need to be changed, the existing function deprecated, and 
> new methods implemented.  The introduction of "struct location" (or 
> whatever) would then see it changed again. Given it is all for a 
> marginal edge case (and to cover up breakage in glibc), I don't see any 
> cost benefit in doing this.

OK.  It was just a thought :)  It seems reasonable that whatever kind
of location frame_unwind_register returns (which you're right, is
likely to change) could naturally be returned by frame_unwind_pc also.
But it would require playing with the interfaces pretty severely, so
I'll just table the idea unless I run into this again somewhere else.

> I think a more useful mechanism is for there to be a table of "start" 
> functions that the user could manipulate (but would default to values 
> specified by the OSABI).

I'm not sure how useful that would really be; we seem to handle the
entry points OK at the moment.  And it couldn't be used for this case
since we do want to backtrace past clone in some circumstances.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: [rfa/mips] Stop backtraces when we've lost the PC

[Daniel Jacobowitz]

On Mon, Mar 08, 2004 at 10:48:14AM -0500, Daniel Jacobowitz wrote:
> OK, two things.
> 
> 
> The first problem is that __thread_start is a local symbol, and the
> copy of the library I'm using on my target is stripped of local symbols
> but has a .pdr section.  We pass the PC to find_proc_desc, which is
> fine, but non_heuristic_proc_desc does:
>   find_pc_partial_function (pc, NULL, &startaddr, NULL);
> and looks for a PDR associated with startaddr.  Oopsie.
> 
> The problem is, GAS's .pdr information does not include a size or an
> end address.  It's slightly risky to search for the latest .pdr entry
> preceding PC, because that might be for a completely different
> function, but not very risky - .frame will generally be used
> consistently if it was used at all.
> 
> I believe the attached patch is safe in a superset of circumstances to
> where the previous code was safe.  It causes us to select the correct
> .pdr entry in this circumstance.  What do you think of it?
> 
> [BTW: I verified that without a .pdr section we notice that we can't
> find a prologue for clone, and give up unwinding semi-gracefully.  I
> also verified that with local symbols, we find the correct .pdr.]
> 
> 
> The other thing:
> 
> I hypothesize that if two consecutive frames, regardless of their type,
> claim to save the PC register at the same location, then unwinding is
> hosed.  If we changed the interface for gdbarch_unwind_pc to return
> information on where, if anywhere, the PC was unwound from, we could
> check this.  This handles sigtramp frames (which save the PC at a
> particular memory buffer), dummy frames (which would say not_lval, i.e.
> the PC is not saved in a simple fashion, i.e. skip this test), typical
> normal frames (the sentinel frame says it's saved in a register, the
> outermost frame says it's saved in the return address register or in a
> local stack slot, normal frames which called other normal frames will
> have it in a local stack slot.
> 
> Obviously for some architecture with a reconstructed not_lval PC this
> wouldn't work, but I think it could still be useful as a safety check
> on the unwinder.  Not a safe termination - this sort of hack doesn't
> have any business being relied on - but at least a call to error() to
> prevent spiralling off into infinity.
> 
> Does my hypothesis make sense?  Shall I try to implement it?

I stand by the theory but there was a problem in the implementation;
several, actually, related to dynamic resolution stubs.  Which are next
on my list of things that need to get taken care of.  Right now, with
or without my patch, they pick up the .pdr entry describing the last
full section before the PLT.  An unfortunate consequence of their being
in .text on MIPS.  So a little care is needed to mitigate the
consequences.

Corrected patch attached.  Fully tested on mips-linux this time.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

2004-03-08  Daniel Jacobowitz  <drow@mvista.com>

	* mips-tdep.c (non_heuristic_proc_desc): Search using the specified
	PC rather than the partial function start address.  Use the start
	address to sanity check the found PDR.

Index: mips-tdep.c
===================================================================
RCS file: /big/fsf/rsync/src-cvs/src/gdb/mips-tdep.c,v
retrieving revision 1.283
diff -u -p -r1.283 mips-tdep.c
--- mips-tdep.c	17 Feb 2004 15:21:21 -0000	1.283
+++ mips-tdep.c	8 Mar 2004 18:50:58 -0000
@@ -2352,38 +2352,63 @@ non_heuristic_proc_desc (CORE_ADDR pc, C
 	{
 	  int low, mid, high;
 	  char *ptr;
+	  CORE_ADDR pdr_pc;
 
 	  low = 0;
 	  high = priv->size / 32;
 
+	  /* First, find the last .pdr entry starting at or before PC.  */
 	  do
 	    {
-	      CORE_ADDR pdr_pc;
-
 	      mid = (low + high) / 2;
 
 	      ptr = priv->contents + mid * 32;
 	      pdr_pc = bfd_get_signed_32 (sec->objfile->obfd, ptr);
 	      pdr_pc += ANOFFSET (sec->objfile->section_offsets,
 				  SECT_OFF_TEXT (sec->objfile));
-	      if (pdr_pc == startaddr)
+	      if (pdr_pc == pc)
 		break;
-	      if (pdr_pc > startaddr)
+	      if (pdr_pc > pc)
 		high = mid;
 	      else
 		low = mid + 1;
 	    }
 	  while (low != high);
 
-	  if (low != high)
+	  /* If we got an exact match on PC, low != high.  Otherwise,
+	     both low and high point one past the PDR of interest.
+	     If both are zero, that means this PC is before any
+	     region covered by a PDR.  */
+	  if (low == high && low > 0)
+	    {
+	      ptr = priv->contents + (low - 1) * 32;
+	      pdr_pc = bfd_get_signed_32 (sec->objfile->obfd, ptr);
+	      pdr_pc += ANOFFSET (sec->objfile->section_offsets,
+				  SECT_OFF_TEXT (sec->objfile));
+	    }
+
+	  /* We don't have a range, so we have no way to know for sure
+	     whether we're in the correct PDR or a PDR for a preceding
+	     function and the current function was a stripped local
+	     symbol.  But if the PDR's PC is at least as great as the
+	     best guess from the symbol table, assume that it does cover
+	     the right area; if a .pdr section is present at all then
+	     nearly every function will have an entry.  The biggest exception
+	     will be the dynamic linker stubs; conveniently these are
+	     placed before .text instead of after.  */
+
+	  if (pc >= pdr_pc && pdr_pc >= startaddr)
 	    {
 	      struct symbol *sym = find_pc_function (pc);
 
+	      if (addrptr)
+		*addrptr = pdr_pc;
+
 	      /* Fill in what we need of the proc_desc.  */
 	      proc_desc = (mips_extra_func_info_t)
 		obstack_alloc (&sec->objfile->objfile_obstack,
 			       sizeof (struct mips_extra_func_info));
-	      PROC_LOW_ADDR (proc_desc) = startaddr;
+	      PROC_LOW_ADDR (proc_desc) = pdr_pc;
 
 	      /* Only used for dummy frames.  */
 	      PROC_HIGH_ADDR (proc_desc) = 0;

Re: [rfa/mips] Stop backtraces when we've lost the PC

[Daniel Jacobowitz]

On Mon, Mar 08, 2004 at 10:48:14AM -0500, Daniel Jacobowitz wrote:
> I believe the attached patch is safe in a superset of circumstances to
> where the previous code was safe.  It causes us to select the correct
> .pdr entry in this circumstance.  What do you think of it?

The patch is not safe; testing showed regressions in break.exp and
other places.

I think the theory is sound, but I need to analyze the regressions
to figure out what's going on.  Sorry 'bout that.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: [rfa/mips] Stop backtraces when we've lost the PC

[Andrew Cagney]

> I hypothesize that if two consecutive frames, regardless of their type,
> claim to save the PC register at the same location, then unwinding is
> hosed.

It would need to do a deep analysis of the location (think about a 
register window architecture), hence I don't know that there's that much 
cost benefit.  Something simpler such as a list of functions known to 
terminate the stack might be more useful.

Andrew

Re: [rfa/mips] Stop backtraces when we've lost the PC

[Daniel Jacobowitz]

On Wed, Mar 17, 2004 at 03:07:02PM -0500, Andrew Cagney wrote:
> Other than that, just an observation that the binary search is [already] 
> pretty messed up.  In all likelyhood the test:
> 
> 	if (pdr_pc == pc)
> 
> will never fire and having it gains little if anything (one less 
> iteration VS logN extra compares).  Eliminating it means cleaning up the 
> binary search though.   Can you attach a fixme to that test indicating 
> that it should be eliminated.

It's pretty easy to clean up the binary search instead, so I did that.

> I also suspect that STARTADDR's computation can be delayed until it is 
> needed (the latter reference that goes with the "pathological", should 
> no longer occure, I think it has been moved into symbol reading. 
> However, leave that for the moment.

I'm pretty sure I tried that, and it meant we stopped using the partial
symbol table to find the beginnings of functions in some cases, leading
to loud failures from heuristic_proc_start.  startaddr is returned via
*addrptr to the caller.

> Otherwize ok, and way better than the original patch, thanks,

Thanks.  Here's what I'll checked in after rerunning some tests.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

2004-03-22  Daniel Jacobowitz  <drow@mvista.com>

	* mips-tdep.c (non_heuristic_proc_desc): Search using the specified
	PC rather than the partial function start address.  Use the start
	address to sanity check the found PDR.

Index: mips-tdep.c
===================================================================
RCS file: /big/fsf/rsync/src-cvs/src/gdb/mips-tdep.c,v
retrieving revision 1.283
diff -u -p -r1.283 mips-tdep.c
--- mips-tdep.c	17 Feb 2004 15:21:21 -0000	1.283
+++ mips-tdep.c	22 Mar 2004 20:35:32 -0000
@@ -2352,38 +2352,70 @@ non_heuristic_proc_desc (CORE_ADDR pc, C
 	{
 	  int low, mid, high;
 	  char *ptr;
+	  CORE_ADDR pdr_pc;
 
 	  low = 0;
 	  high = priv->size / 32;
 
+	  /* We've found a .pdr section describing this objfile.  We want to
+	     find the entry which describes this code address.  The .pdr
+	     information is not very descriptive; we have only a function
+	     start address.  We have to look for the closest entry, because
+	     the local symbol at the beginning of this function may have
+	     been stripped - so if we ask the symbol table for the start
+	     address we may get a preceding global function.  */
+
+	  /* First, find the last .pdr entry starting at or before PC.  */
 	  do
 	    {
-	      CORE_ADDR pdr_pc;
-
 	      mid = (low + high) / 2;
 
 	      ptr = priv->contents + mid * 32;
 	      pdr_pc = bfd_get_signed_32 (sec->objfile->obfd, ptr);
 	      pdr_pc += ANOFFSET (sec->objfile->section_offsets,
 				  SECT_OFF_TEXT (sec->objfile));
-	      if (pdr_pc == startaddr)
-		break;
-	      if (pdr_pc > startaddr)
+
+	      if (pdr_pc > pc)
 		high = mid;
 	      else
 		low = mid + 1;
 	    }
 	  while (low != high);
 
-	  if (low != high)
+	  /* Both low and high point one past the PDR of interest.  If
+	     both are zero, that means this PC is before any region
+	     covered by a PDR, i.e. pdr_pc for the first PDR entry is
+	     greater than PC.  */
+	  if (low > 0)
+	    {
+	      ptr = priv->contents + (low - 1) * 32;
+	      pdr_pc = bfd_get_signed_32 (sec->objfile->obfd, ptr);
+	      pdr_pc += ANOFFSET (sec->objfile->section_offsets,
+				  SECT_OFF_TEXT (sec->objfile));
+	    }
+
+	  /* We don't have a range, so we have no way to know for sure
+	     whether we're in the correct PDR or a PDR for a preceding
+	     function and the current function was a stripped local
+	     symbol.  But if the PDR's PC is at least as great as the
+	     best guess from the symbol table, assume that it does cover
+	     the right area; if a .pdr section is present at all then
+	     nearly every function will have an entry.  The biggest exception
+	     will be the dynamic linker stubs; conveniently these are
+	     placed before .text instead of after.  */
+
+	  if (pc >= pdr_pc && pdr_pc >= startaddr)
 	    {
 	      struct symbol *sym = find_pc_function (pc);
 
+	      if (addrptr)
+		*addrptr = pdr_pc;
+
 	      /* Fill in what we need of the proc_desc.  */
 	      proc_desc = (mips_extra_func_info_t)
 		obstack_alloc (&sec->objfile->objfile_obstack,
 			       sizeof (struct mips_extra_func_info));
-	      PROC_LOW_ADDR (proc_desc) = startaddr;
+	      PROC_LOW_ADDR (proc_desc) = pdr_pc;
 
 	      /* Only used for dummy frames.  */
 	      PROC_HIGH_ADDR (proc_desc) = 0;