RFC: %ebp-based backtrace patch

RFC: %ebp-based backtrace patch

[Daniel Jacobowitz]

A number of Linux distributions are carrying the attached patch from
Michael Matz.  Frankly, I don't understand i386 frame layout issues
well enough to approve this; I'm posting this in hopes of starting a
discussion between Mark Kettenis and Michael Matz.  The patch
does seem to help in practice.

I took a 32-bit Debian system, with minimal packages installed, and
ran bash inside GDB.  I interrupted it while sitting at a prompt.
Results without the patch:

#0  0xf7fdf430 in __kernel_vsyscall ()
#1  0xf7f0ce93 in __read_nocancel () from /lib/i686/cmov/libc.so.6
#2  0x080cfca6 in rl_getc ()
#3  0x080d0103 in rl_read_key ()
#4  0x080be7d7 in readline_internal_char ()
#5  0x080bebc5 in readline ()
#6  0x080689d1 in ?? ()
#7  0x081b7808 in ?? ()
#8  0x080942c0 in ?? ()
#9  0xffffc5d8 in ?? ()
#10 0x08080e26 in notify_and_cleanup ()
#11 0x0806422b in ?? ()
#12 0x00000000 in ?? ()

With:

#0  0xf7fdf430 in __kernel_vsyscall ()
#1  0xf7f0ce93 in __read_nocancel () from /lib/i686/cmov/libc.so.6
#2  0x080cfca6 in rl_getc ()
#3  0x080d0103 in rl_read_key ()
#4  0x080be7d7 in readline_internal_char ()
#5  0x080bebc5 in readline ()
#6  0x080689d1 in ?? ()
#7  0x0806422b in ?? ()
#8  0x08065492 in ?? ()
#9  0x08068f57 in yyparse ()
#10 0x08061c98 in parse_command ()
#11 0x08061d7f in read_command ()
#12 0x08061fc3 in reader_loop ()
#13 0x08061ae9 in main ()

Michael's explanation of the patch is over here:

  https://bugzilla.novell.com/show_bug.cgi?id=390722#c25

My understanding is that this only affects frames we can't find a
symbol for.  Instead of assuming the frame starts at %esp - 4 (since
sp_offset is initialized to -4 and then not updated if we did not find
a symbol to analyze), assume that it uses and saved %ebp.  We have no
reliable information at this point and the new heuristic seems to be
right more often.

Mark, could you comment on this patch?

-- 
Daniel Jacobowitz
CodeSourcery

2009-07-06  Michael Matz  <matz@suse.de>

	* i386-tdep.c (i386_frame_cache): Assume anonymous functions use
	a frame pointer.

---
 gdb/i386-tdep.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Index: gdb-6.8.50.20090628/gdb/i386-tdep.c
===================================================================
--- gdb-6.8.50.20090628.orig/gdb/i386-tdep.c	2009-07-05 20:38:47.000000000 -0400
+++ gdb-6.8.50.20090628/gdb/i386-tdep.c	2009-07-05 20:39:17.000000000 -0400
@@ -1377,11 +1377,13 @@ i386_frame_cache (struct frame_info *thi
 	  /* This will be added back below.  */
 	  cache->saved_regs[I386_EIP_REGNUM] -= cache->base;
 	}
-      else
+      else if (cache->pc)
 	{
 	  get_frame_register (this_frame, I386_ESP_REGNUM, buf);
 	  cache->base = extract_unsigned_integer (buf, 4) + cache->sp_offset;
 	}
+      else
+	cache->saved_regs[I386_EBP_REGNUM] = 0;
     }
 
   /* Now that we have the base address for the stack frame we can

Re: RFC: %ebp-based backtrace patch

[Frank Ch. Eigler]

Daniel Jacobowitz <drow@false.org> writes:

> A number of Linux distributions are carrying the attached patch from
> Michael Matz.  [...]
>   https://bugzilla.novell.com/show_bug.cgi?id=390722#c25
>[...]
> 	* i386-tdep.c (i386_frame_cache): Assume anonymous functions use
> 	a frame pointer.
>[...]

Could gdb partially disassemble such functions to check
whether %ebp is likely being used as this heuristic expects?


- FChE

Re: RFC: %ebp-based backtrace patch

[Daniel Jacobowitz]

On Mon, Jul 06, 2009 at 04:10:23PM -0400, Frank Ch. Eigler wrote:
> Daniel Jacobowitz <drow@false.org> writes:
> 
> > A number of Linux distributions are carrying the attached patch from
> > Michael Matz.  [...]
> >   https://bugzilla.novell.com/show_bug.cgi?id=390722#c25
> >[...]
> > 	* i386-tdep.c (i386_frame_cache): Assume anonymous functions use
> > 	a frame pointer.
> >[...]
> 
> Could gdb partially disassemble such functions to check
> whether %ebp is likely being used as this heuristic expects?

I don't think so, but maybe someone else will come up with a way.
Since we don't know the function's start address, we could be anywhere
in the function.

-- 
Daniel Jacobowitz
CodeSourcery

Re: RFC: %ebp-based backtrace patch

[Mark Kettenis]

> Date: Mon, 6 Jul 2009 14:33:16 -0400
> From: Daniel Jacobowitz <drow@false.org>
> 
> A number of Linux distributions are carrying the attached patch from
> Michael Matz.  Frankly, I don't understand i386 frame layout issues
> well enough to approve this; I'm posting this in hopes of starting a
> discussion between Mark Kettenis and Michael Matz.  The patch
> does seem to help in practice.
> 
> I took a 32-bit Debian system, with minimal packages installed, and
> ran bash inside GDB.  I interrupted it while sitting at a prompt.
> Results without the patch:
> 
> #0  0xf7fdf430 in __kernel_vsyscall ()
> #1  0xf7f0ce93 in __read_nocancel () from /lib/i686/cmov/libc.so.6
> #2  0x080cfca6 in rl_getc ()
> #3  0x080d0103 in rl_read_key ()
> #4  0x080be7d7 in readline_internal_char ()
> #5  0x080bebc5 in readline ()
> #6  0x080689d1 in ?? ()
> #7  0x081b7808 in ?? ()
> #8  0x080942c0 in ?? ()
> #9  0xffffc5d8 in ?? ()
> #10 0x08080e26 in notify_and_cleanup ()
> #11 0x0806422b in ?? ()
> #12 0x00000000 in ?? ()
> 
> With:
> 
> #0  0xf7fdf430 in __kernel_vsyscall ()
> #1  0xf7f0ce93 in __read_nocancel () from /lib/i686/cmov/libc.so.6
> #2  0x080cfca6 in rl_getc ()
> #3  0x080d0103 in rl_read_key ()
> #4  0x080be7d7 in readline_internal_char ()
> #5  0x080bebc5 in readline ()
> #6  0x080689d1 in ?? ()
> #7  0x0806422b in ?? ()
> #8  0x08065492 in ?? ()
> #9  0x08068f57 in yyparse ()
> #10 0x08061c98 in parse_command ()
> #11 0x08061d7f in read_command ()
> #12 0x08061fc3 in reader_loop ()
> #13 0x08061ae9 in main ()
> 
> Michael's explanation of the patch is over here:
> 
>   https://bugzilla.novell.com/show_bug.cgi?id=390722#c25
> 
> My understanding is that this only affects frames we can't find a
> symbol for.  Instead of assuming the frame starts at %esp - 4 (since
> sp_offset is initialized to -4 and then not updated if we did not find
> a symbol to analyze), assume that it uses and saved %ebp.  We have no
> reliable information at this point and the new heuristic seems to be
> right more often.
> 
> Mark, could you comment on this patch?

Makes sense to me.  If we have no clue where we are anymore, taking
the gamble that %ebp is a valid frame pointer probably has better odds
than that %esp points to a valid frame.  This will need a comment
though.  I'll take care of that.

There is one potential problem though.  IIRC early versions of the
vsyscall DSO did not have embedded debug information.  This will
probably make backtraces from interrupted system calls on systems with
those kernels fail miserably.  Not sure if that's something we really
care about though.

> Daniel Jacobowitz
> CodeSourcery
> 
> 2009-07-06  Michael Matz  <matz@suse.de>
> 
> 	* i386-tdep.c (i386_frame_cache): Assume anonymous functions use
> 	a frame pointer.
> 
> ---
>  gdb/i386-tdep.c |    4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> Index: gdb-6.8.50.20090628/gdb/i386-tdep.c
> ===================================================================
> --- gdb-6.8.50.20090628.orig/gdb/i386-tdep.c	2009-07-05 20:38:47.000000000 -0400
> +++ gdb-6.8.50.20090628/gdb/i386-tdep.c	2009-07-05 20:39:17.000000000 -0400
> @@ -1377,11 +1377,13 @@ i386_frame_cache (struct frame_info *thi
>  	  /* This will be added back below.  */
>  	  cache->saved_regs[I386_EIP_REGNUM] -= cache->base;
>  	}
> -      else
> +      else if (cache->pc)
>  	{
>  	  get_frame_register (this_frame, I386_ESP_REGNUM, buf);
>  	  cache->base = extract_unsigned_integer (buf, 4) + cache->sp_offset;
>  	}
> +      else
> +	cache->saved_regs[I386_EBP_REGNUM] = 0;
>      }
>  
>    /* Now that we have the base address for the stack frame we can
> 

Re: RFC: %ebp-based backtrace patch

[Michael Matz]

Hi,

On Mon, 6 Jul 2009, Mark Kettenis wrote:

> Makes sense to me.  If we have no clue where we are anymore, taking
> the gamble that %ebp is a valid frame pointer probably has better odds
> than that %esp points to a valid frame.  This will need a comment
> though.  I'll take care of that.
> 
> There is one potential problem though.  IIRC early versions of the
> vsyscall DSO did not have embedded debug information.

That was indeed quite some time ago, probably not useful to care anymore.  
But I took a quick look at the current implementations.  Some of them use 
%ebp frames, so those would be fine even without debug info (and broken 
with the current heuristic), and some use no frame at all (hence the 
heuristic of %esp-4 would work).  The latter will use the frame of the 
caller with the new heuristic, which is not ideal but somewhat okay when 
that caller itself has a %ebp frame.  If it hasn't, everything is off.  
All of that is problematic only for non-debug-info vsyscalls/vDSOs, which 
I'd hope don't exist in practice anymore.

One thing of note: we meanwhile use the Fedora package of gdb, mostly 
unchanged.  One patch of it interacts with the new heuristic, namely 
gdb-6.5-bz218379-ppc-solib-trampoline-fix.patch .  That patch isn't 
necessary anymore and in any case never really did what it intended.  The 
result of it is that the frame unwinder magically always finds a 
"function" (the fake minimal symbols for trampolines, not to be confused 
with "function@plt" symbols) for each program counter, even of length 
zero.  That breaks the heuristic again.


Ciao,
Michael.

Re: RFC: %ebp-based backtrace patch

[Michael Matz]

Hi,

On Mon, 6 Jul 2009, Frank Ch. Eigler wrote:

> Daniel Jacobowitz <drow@false.org> writes:
> 
> > A number of Linux distributions are carrying the attached patch from
> > Michael Matz.  [...]
> >   https://bugzilla.novell.com/show_bug.cgi?id=390722#c25
> >[...]
> > 	* i386-tdep.c (i386_frame_cache): Assume anonymous functions use
> > 	a frame pointer.
> >[...]
> 
> Could gdb partially disassemble such functions to check
> whether %ebp is likely being used as this heuristic expects?

Nope.  We don't know the function borders.  But there's another heuristic 
that could potentially be useful: check if %ebp points not too far away 
from %esp (and has a higher value).  That way we would at least reject 
functions that use %ebp to hold some low integer values.

Then we of course have the problem again of having to rely on %esp-4 
containing the return address.  That assumption doesn't really hold very 
often.  In fact it holds _only_ for frameless functions, of which there 
aren't that many on x86.  So we're screwed either way.


Ciao,
Michael.

Re: RFC: %ebp-based backtrace patch

[Daniel Jacobowitz]

On Mon, Jul 06, 2009 at 11:57:29PM +0200, Mark Kettenis wrote:
> Makes sense to me.  If we have no clue where we are anymore, taking
> the gamble that %ebp is a valid frame pointer probably has better odds
> than that %esp points to a valid frame.  This will need a comment
> though.  I'll take care of that.

Thank you.  Would you like me to check in the code, or will you do
both together?

-- 
Daniel Jacobowitz
CodeSourcery

Re: RFC: %ebp-based backtrace patch

[Mark Kettenis]

> Date: Tue, 7 Jul 2009 09:00:40 -0400
> From: Daniel Jacobowitz <drow@false.org>
> 
> On Mon, Jul 06, 2009 at 11:57:29PM +0200, Mark Kettenis wrote:
> > Makes sense to me.  If we have no clue where we are anymore, taking
> > the gamble that %ebp is a valid frame pointer probably has better odds
> > than that %esp points to a valid frame.  This will need a comment
> > though.  I'll take care of that.
> 
> Thank you.  Would you like me to check in the code, or will you do
> both together?

I'll take care of it.

Re: RFC: %ebp-based backtrace patch

[Frank Ch. Eigler]

Hi -

On Tue, Jul 07, 2009 at 02:15:19PM +0200, Michael Matz wrote:
> [...]
> > Could gdb partially disassemble such functions to check
> > whether %ebp is likely being used as this heuristic expects?
> 
> Nope.  We don't know the function borders.  [...]

I'm feeling dense, so can you explain why?  We have at least a
PC value, which one could map in the symbol table, no?

- FChE

Re: RFC: %ebp-based backtrace patch

[Michael Matz]

Hi,

On Tue, 7 Jul 2009, Frank Ch. Eigler wrote:

> On Tue, Jul 07, 2009 at 02:15:19PM +0200, Michael Matz wrote:
> > [...]
> > > Could gdb partially disassemble such functions to check
> > > whether %ebp is likely being used as this heuristic expects?
> > 
> > Nope.  We don't know the function borders.  [...]
> 
> I'm feeling dense, so can you explain why?  We have at least a
> PC value, which one could map in the symbol table, no?

There is no complete symbol table anymore, only the exported symbol table 
which is empty for executable by default and doesn't contain e.g. static 
functions for shared libs.  That's the whole point.  We are talking about 
stripped objects for which we still want to generate useful backtraces (or 
at least not disturb backtracing when even just one shared lib in the call 
stack was stripped) so that bugreports of crashes are not completely 
useless and misleading.


Ciao,
Michael.

Re: RFC: %ebp-based backtrace patch

[Mark Kettenis]

> Date: Tue, 7 Jul 2009 09:00:40 -0400
> From: Daniel Jacobowitz <drow@false.org>
> 
> On Mon, Jul 06, 2009 at 11:57:29PM +0200, Mark Kettenis wrote:
> > Makes sense to me.  If we have no clue where we are anymore, taking
> > the gamble that %ebp is a valid frame pointer probably has better odds
> > than that %esp points to a valid frame.  This will need a comment
> > though.  I'll take care of that.
> 
> Thank you.  Would you like me to check in the code, or will you do
> both together?

Before comitting this diff, I ran the testsuite, and noticed a
regression.  The problem is that if you do a function call through a
null-function-pointer or an otherwise corrupt function pointer, and
get a SIGSEGV, the backtrace no longer shows the frame that did the
function call.

Until we come up with a way to fix this issue, I'm not going to commit
the diff.

Re: RFC: %ebp-based backtrace patch

[Daniel Jacobowitz]

On Wed, Jul 08, 2009 at 11:01:16AM +0200, Mark Kettenis wrote:
> Before comitting this diff, I ran the testsuite, and noticed a
> regression.  The problem is that if you do a function call through a
> null-function-pointer or an otherwise corrupt function pointer, and
> get a SIGSEGV, the backtrace no longer shows the frame that did the
> function call.
> 
> Until we come up with a way to fix this issue, I'm not going to commit
> the diff.

Hmm.  if (target_read_memory (pc, buf, 1)) ?  That's how a similar
issue is handled on ARM, in arm_stub_unwind_sniffer.

-- 
Daniel Jacobowitz
CodeSourcery

Re: RFC: %ebp-based backtrace patch

[Daniel Jacobowitz]

On Wed, Jul 08, 2009 at 08:53:30AM -0400, Daniel Jacobowitz wrote:
> On Wed, Jul 08, 2009 at 11:01:16AM +0200, Mark Kettenis wrote:
> > Before comitting this diff, I ran the testsuite, and noticed a
> > regression.  The problem is that if you do a function call through a
> > null-function-pointer or an otherwise corrupt function pointer, and
> > get a SIGSEGV, the backtrace no longer shows the frame that did the
> > function call.
> > 
> > Until we come up with a way to fix this issue, I'm not going to commit
> > the diff.
> 
> Hmm.  if (target_read_memory (pc, buf, 1)) ?  That's how a similar
> issue is handled on ARM, in arm_stub_unwind_sniffer.

Hi Mark,

I was looking through Ubuntu's local GDB patches and noticed this one
is still outstanding.  Will the above work?  I can test and
(hopefully) commit it, if you'd like.

-- 
Daniel Jacobowitz
CodeSourcery

Re: RFC: %ebp-based backtrace patch

[Mark Kettenis]

> Date: Sun, 27 Dec 2009 11:59:16 -0500
> From: Daniel Jacobowitz <drow@false.org>
> 
> On Wed, Jul 08, 2009 at 08:53:30AM -0400, Daniel Jacobowitz wrote:
> > On Wed, Jul 08, 2009 at 11:01:16AM +0200, Mark Kettenis wrote:
> > > Before comitting this diff, I ran the testsuite, and noticed a
> > > regression.  The problem is that if you do a function call through a
> > > null-function-pointer or an otherwise corrupt function pointer, and
> > > get a SIGSEGV, the backtrace no longer shows the frame that did the
> > > function call.
> > > 
> > > Until we come up with a way to fix this issue, I'm not going to commit
> > > the diff.
> > 
> > Hmm.  if (target_read_memory (pc, buf, 1)) ?  That's how a similar
> > issue is handled on ARM, in arm_stub_unwind_sniffer.
> 
> Hi Mark,
> 
> I was looking through Ubuntu's local GDB patches and noticed this one
> is still outstanding.  Will the above work?  I can test and
> (hopefully) commit it, if you'd like.

Sorry, but I don't see how this would solve things.  Do you have a
diff for me to look at?

Re: RFC: %ebp-based backtrace patch

[Daniel Jacobowitz]

On Sun, Dec 27, 2009 at 10:37:19PM +0100, Mark Kettenis wrote:
> Sorry, but I don't see how this would solve things.  Do you have a
> diff for me to look at?

Sure.  Here's a version that passes signull.exp for me.

I also noticed that Ubuntu has a version of this applied to
amd64-tdep.c.  I don't know if that has merit or not; I wouldn't
expect it to matter much, given that the ABI mandates .eh_frame.

-- 
Daniel Jacobowitz
CodeSourcery

2009-12-27  Michael Matz  <matz@suse.de>
	    Daniel Jacobowitz  <dan@codesourcery.com>

        * i386-tdep.c (i386_frame_cache): Assume valid anonymous
	functions use a frame pointer.

Index: i386-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/i386-tdep.c,v
retrieving revision 1.290
diff -u -p -r1.290 i386-tdep.c
--- i386-tdep.c	12 Oct 2009 15:52:28 -0000	1.290
+++ i386-tdep.c	27 Dec 2009 21:59:51 -0000
@@ -1394,12 +1394,24 @@ i386_frame_cache (struct frame_info *thi
 	  /* This will be added back below.  */
 	  cache->saved_regs[I386_EIP_REGNUM] -= cache->base;
 	}
-      else
+      else if (cache->pc != 0
+	       || target_read_memory (get_frame_pc (this_frame), buf, 1))
 	{
+	  /* We're in a known function, but did not find a frame
+	     setup.  Assume that the function does not use %ebp.
+	     Alternatively, we may have jumped to an invalid
+	     address; in that case there is definitely no new
+	     frame in %ebp.  */
 	  get_frame_register (this_frame, I386_ESP_REGNUM, buf);
 	  cache->base = extract_unsigned_integer (buf, 4, byte_order)
 			+ cache->sp_offset;
 	}
+      else
+	/* We're in an unknown function.  We could not find the start
+	   of the function to analyze the prologue; our best option is
+	   to assume a typical frame layout with the caller's %ebp
+	   saved.  */
+	cache->saved_regs[I386_EBP_REGNUM] = 0;
     }
 
   /* Now that we have the base address for the stack frame we can

Re: RFC: %ebp-based backtrace patch

[Daniel Jacobowitz]

On Sun, Dec 27, 2009 at 05:03:11PM -0500, Daniel Jacobowitz wrote:
> Sure.  Here's a version that passes signull.exp for me.

I've tested this version and checked it in; there are no longer any
regressions.

-- 
Daniel Jacobowitz
CodeSourcery