[SH][PATCH] Disable ABI frame sniffer

[SH][PATCH] Disable ABI frame sniffer

[Andrew STUBBS]

[-- Attachment #1: Type: text/plain, Size: 1334 bytes --]

Hi,

The attached patch adds a new sh-specific command:

set backtrace abi-sniffer

This command allows the ABI frame sniffer to be disabled. The default 
state remains 'on', as it is now.

It is useful to disable this sniffer because it has an irritating habit 
of causing (non-fatal) errors when it runs out of frames. Specifically 
it often says 'Previous frame identical to this frame (corrupt stack?)'.

This message is worrying if you do not know what it means, but it is 
normally easily ignored. Unfortunately, the problem is more serious when 
used in conjunction with 'thread apply' as the first error kills the 
output from the rest of the threads. This command is used by some GUIs 
to populate their displays.

It is difficult to see how the ABI sniffer can be made more robust 
(although I have not looked into it in great detail), and it certainly 
can't be made totally reliable, so the best option would seem to be to 
disable it altogether.

With the ABI sniffer 'off' it is not possible to backtrace through code 
that does not have CFI. On sh-elf, at least, there is always CFI when 
there is debug info, so it is relatively rare that the ABI sniffer is 
actually required.

I have not set the default to 'off' because that causes quite a number 
of FAILs in the testsuite.

Is this all OK?

Thanks

Andrew Stubbs

[-- Attachment #2: abi_sniffer.patch --]
[-- Type: text/plain, Size: 4448 bytes --]

2005-11-09  Andrew Stubbs  <andrew.stubbs@st.com>

	* sh-tdep.c (backtrace_abi_sniffer): New variable.
	(sh_frame_this_id): Do nothing if backtrace_abi_sniffer zero.
	(sh_gdbarch_init): Add set/show abi-sniffer command.
	* frame.c (set_backtrace_cmdlist): Remove static.
	(show_backtrace_cmdlist): Likewise.
	* frame.h (set_backtrace_cmdlist): Add extern declaration.
	(show_backtrace_cmdlist): Likewise.

doc/
	* gdb.texinfo (Super-H): Add 'set/show backtrace abi-sniffer'.

Index: src/gdb/sh-tdep.c
===================================================================
--- src.orig/gdb/sh-tdep.c	2005-11-01 11:43:29.000000000 +0000
+++ src/gdb/sh-tdep.c	2005-11-01 12:26:24.000000000 +0000
@@ -2377,12 +2377,18 @@ sh_frame_prev_register (struct frame_inf
     frame_unwind_register (next_frame, (*realnump), valuep);
 }
 
+static int backtrace_abi_sniffer = 1;
+
 static void
 sh_frame_this_id (struct frame_info *next_frame, void **this_cache,
 		  struct frame_id *this_id)
 {
   struct sh_frame_cache *cache = sh_frame_cache (next_frame, this_cache);
 
+  /* Is this sniffer enabled? */
+  if (!backtrace_abi_sniffer)
+    return;
+
   /* This marks the outermost frame.  */
   if (cache->base == 0)
     return;
@@ -2726,6 +2732,22 @@ sh_gdbarch_init (struct gdbarch_info inf
   frame_unwind_append_sniffer (gdbarch, dwarf2_frame_sniffer);
   frame_unwind_append_sniffer (gdbarch, sh_frame_sniffer);
 
+  add_setshow_boolean_cmd ("abi-sniffer", class_obscure,
+                           &backtrace_abi_sniffer, "\
+Set whether backtraces should use the ABI where there is no debug info.", "\
+Show whether backtraces should use the ABI where there is no debug info.", "\
+Normally there is debug information from which to construct a backtrace\n\
+but sometimes is is not available (e.g. in assembly code). In this case the\n\
+ABI sniffer can attempt to construct a backtrace. It is disabled by default\n\
+because it can cause inconvenient errors. Note that it does not disable the\n\
+Dwarf debug information sniffer.",
+                           NULL, NULL, &set_backtrace_cmdlist,
+                           &show_backtrace_cmdlist);
+
+#ifdef SVR4_SHARED_LIBS
+  set_solib_svr4_fetch_link_map_offsets (gdbarch, svr4_ilp32_fetch_link_map_offsets);
+#endif
+
   return gdbarch;
 }
 
Index: src/gdb/frame.c
===================================================================
--- src.orig/gdb/frame.c	2005-11-01 11:16:55.000000000 +0000
+++ src/gdb/frame.c	2005-11-01 11:43:29.000000000 +0000
@@ -1558,8 +1558,8 @@ frame_sp_unwind (struct frame_info *next
 
 extern initialize_file_ftype _initialize_frame; /* -Wmissing-prototypes */
 
-static struct cmd_list_element *set_backtrace_cmdlist;
-static struct cmd_list_element *show_backtrace_cmdlist;
+struct cmd_list_element *set_backtrace_cmdlist;
+struct cmd_list_element *show_backtrace_cmdlist;
 
 static void
 set_backtrace_cmd (char *args, int from_tty)
Index: src/gdb/frame.h
===================================================================
--- src.orig/gdb/frame.h	2005-11-01 11:16:55.000000000 +0000
+++ src/gdb/frame.h	2005-11-01 11:43:29.000000000 +0000
@@ -660,4 +660,9 @@ extern void deprecated_update_frame_pc_h
 extern void deprecated_update_frame_base_hack (struct frame_info *frame,
 					       CORE_ADDR base);
 
+/* Export 'set backtrace' so that new subcommands may be added elsewhere. */
+
+extern struct cmd_list_element *set_backtrace_cmdlist;
+extern struct cmd_list_element *show_backtrace_cmdlist;
+
 #endif /* !defined (FRAME_H)  */
Index: src/gdb/doc/gdb.texinfo
===================================================================
--- src.orig/gdb/doc/gdb.texinfo	2005-11-01 11:43:29.000000000 +0000
+++ src/gdb/doc/gdb.texinfo	2005-11-01 12:56:00.000000000 +0000
@@ -14790,8 +14790,21 @@ commands:
 @item regs
 @kindex regs@r{, Super-H}
 Show the values of all Super-H registers.
+
+@item set backtrace abi-sniffer
+@kindex set backtrace abi-sniffer
+This command enables or disables the SH ABI-based frame sniffer. This
+`sniffer' attempts to find the frames of a backtrace when there is no
+debug information (specifically CFI) to do the job properly. Sniffing
+frames is problematic so it is often best to switch this @samp{off}. It
+is @samp{on} by default.
+
+@item show backtrace abi-sniffer
+@kindex show backtrace abi-sniffer
+Show whether the abi-sniffer is enabled, or not.
 @end table
 
+
 @node WinCE
 @subsection Windows CE
 @cindex Windows CE

Re: [SH][PATCH] Disable ABI frame sniffer

[Andreas Schwab]

Andrew STUBBS <andrew.stubbs@st.com> writes:

> This message is worrying if you do not know what it means, but it is 
> normally easily ignored. Unfortunately, the problem is more serious when 
> used in conjunction with 'thread apply' as the first error kills the 
> output from the rest of the threads.

Does it?  That's not what I get.  I always see "Previous frame inner to
this frame" in the backtrace, but that does not stop thread apply to
continue with the next thread.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [SH][PATCH] Disable ABI frame sniffer

[Daniel Jacobowitz]

On Wed, Nov 09, 2005 at 09:43:03PM +0100, Andreas Schwab wrote:
> Andrew STUBBS <andrew.stubbs@st.com> writes:
> 
> > This message is worrying if you do not know what it means, but it is 
> > normally easily ignored. Unfortunately, the problem is more serious when 
> > used in conjunction with 'thread apply' as the first error kills the 
> > output from the rest of the threads.
> 
> Does it?  That's not what I get.  I always see "Previous frame inner to
> this frame" in the backtrace, but that does not stop thread apply to
> continue with the next thread.

It used to.  I bet Andrew is submitting patches off of a GDB 6.3 or
earlier port.  In HEAD, errors during backtrace are non-fatal.

-- 
Daniel Jacobowitz
CodeSourcery, LLC

Re: [SH][PATCH] Disable ABI frame sniffer

[Daniel Jacobowitz]

On Wed, Nov 09, 2005 at 05:12:15PM +0000, Andrew STUBBS wrote:
> Hi,
> 
> The attached patch adds a new sh-specific command:
> 
> set backtrace abi-sniffer
> 
> This command allows the ABI frame sniffer to be disabled. The default 
> state remains 'on', as it is now.

I had to look up what you meant by "ABI frame sniffer"; we normally
call this the prologue analyzer.

> With the ABI sniffer 'off' it is not possible to backtrace through code 
> that does not have CFI. On sh-elf, at least, there is always CFI when 
> there is debug info, so it is relatively rare that the ABI sniffer is 
> actually required.
> 
> I have not set the default to 'off' because that causes quite a number 
> of FAILs in the testsuite.

That should be a hint...

Obviously the SH maintainers have final say, but I strongly discourage
any patch along these lines.  If you want to improve the fallback
unwinder, go ahead.  If you want to add more generic tests to terminate
backtraces without annoying messages, we can discuss that.  But turning
it off is not viable.

-- 
Daniel Jacobowitz
CodeSourcery, LLC

Re: [SH][PATCH] Disable ABI frame sniffer

[Eli Zaretskii]

> Date: Wed, 09 Nov 2005 17:12:15 +0000
> From: Andrew STUBBS <andrew.stubbs@st.com>
> 
> It is useful to disable this sniffer because it has an irritating habit 
> of causing (non-fatal) errors when it runs out of frames. Specifically 
> it often says 'Previous frame identical to this frame (corrupt stack?)'.
> 
> This message is worrying if you do not know what it means, but it is 
> normally easily ignored. Unfortunately, the problem is more serious when 
> used in conjunction with 'thread apply' as the first error kills the 
> output from the rest of the threads. This command is used by some GUIs 
> to populate their displays.

I think this explanation should be in the manual, because it explains
very clearly when this command is useful.

> +  add_setshow_boolean_cmd ("abi-sniffer", class_obscure,
> +                           &backtrace_abi_sniffer, "\
> +Set whether backtraces should use the ABI where there is no debug info.", "\
> +Show whether backtraces should use the ABI where there is no debug info.", "\
> +Normally there is debug information from which to construct a backtrace\n\
> +but sometimes is is not available (e.g. in assembly code). In this case the\n\
> +ABI sniffer can attempt to construct a backtrace. It is disabled by default\n\
> +because it can cause inconvenient errors. Note that it does not disable the\n\
> +Dwarf debug information sniffer.",

These strings should be in _(), to allow for their translation to
other languages.  See the other examples of using
add_setshow_boolean_cmd in GDB's sources.

> Index: src/gdb/doc/gdb.texinfo
> ===================================================================
> --- src.orig/gdb/doc/gdb.texinfo	2005-11-01 11:43:29.000000000 +0000
> +++ src/gdb/doc/gdb.texinfo	2005-11-01 12:56:00.000000000 +0000
> @@ -14790,8 +14790,21 @@ commands:
>  @item regs
>  @kindex regs@r{, Super-H}
>  Show the values of all Super-H registers.
> +
> +@item set backtrace abi-sniffer
> +@kindex set backtrace abi-sniffer
> +This command enables or disables the SH ABI-based frame sniffer. This
> +`sniffer' attempts to find the frames of a backtrace when there is no
> +debug information (specifically CFI) to do the job properly. Sniffing
> +frames is problematic so it is often best to switch this @samp{off}. It
> +is @samp{on} by default.
> +
> +@item show backtrace abi-sniffer
> +@kindex show backtrace abi-sniffer
> +Show whether the abi-sniffer is enabled, or not.

This part is fine with me, provided that you fix the following
gotchas:

 . Make sure there are 2 blanks after a period that ends a sentence.

 . Use ``sniffer'' instead of `sniffer' (two quote characters).

 . Replace the "Sniffing is problematic..." sentence with text similar
   to the one you included in your message that explains when this
   command would be useful.

>  @end table
>  
> +
>  @node WinCE

Please don't add this gratuitous whitespace.

Re: [SH][PATCH] Disable ABI frame sniffer

[Andrew STUBBS]

Daniel Jacobowitz wrote:
> It used to.  I bet Andrew is submitting patches off of a GDB 6.3 or
> earlier port.  In HEAD, errors during backtrace are non-fatal.

Quite so. I have not forward ported our target backend and so cannot 
test threads with HEAD. Not with SH OS21 anyway.

Is it possible that the message is still present for SH and/or 
non-standard threading implementations?

Andrew Stubbs

Re: [SH][PATCH] Disable ABI frame sniffer

[Andrew STUBBS]

Daniel Jacobowitz wrote:
> I had to look up what you meant by "ABI frame sniffer"; we normally
> call this the prologue analyzer.

One registers the analyser using frame_unwind_append_sniffer(). The 
currently configured analysers are called dwarf2_frame_sniffer() and 
sh_frame_sniffer().

I can't imagine where I got the name from ;-)

>>With the ABI sniffer 'off' it is not possible to backtrace through code 
>>that does not have CFI. On sh-elf, at least, there is always CFI when 
>>there is debug info, so it is relatively rare that the ABI sniffer is 
>>actually required.
>>
>>I have not set the default to 'off' because that causes quite a number 
>>of FAILs in the testsuite.
> 
> 
> That should be a hint...

Naturally you can't unwind ASM or anything without CFI without the 
fallback unwinder. That's why it is there. That doesn't mean it isn't 
irritating when it trys to unwind frames that aren't there. Most of the 
time you just don't need it.

> Obviously the SH maintainers have final say, but I strongly discourage
> any patch along these lines.  If you want to improve the fallback
> unwinder, go ahead.  If you want to add more generic tests to terminate
> backtraces without annoying messages, we can discuss that.  But turning
> it off is not viable.

It may be possible to improve the unwinder. However, it is dependent on 
the values in registers and in memory. It would be impossible to prevent 
it getting occasionally confused.

I did, when I origininally encountered the problem, try to detect the 
problem to avoid the messages, but all I ended up doing was detecting if 
it would give an error and declaring there were no more frames, which 
seemed pointless - it would hide the message when it really did mean 
something.

Besides which, the other problem I am trying to fix is that, before the 
error, the unwinder would typically produce at least one bogus frame. 
Even if the error has changed to a warning (as you say in the other 
message) this would still be an irritant.

I much prefer the option to say 'My program has CFI, I know it does, I 
put it there, so stop trying to confuse yourself'.

As to the SH maintainers, there are none listed in MAINTAINERS.

Andrew Stubbs

Re: [SH][PATCH] Disable ABI frame sniffer

[Daniel Jacobowitz]

On Thu, Nov 10, 2005 at 10:12:33AM +0000, Andrew STUBBS wrote:
> Daniel Jacobowitz wrote:
> >It used to.  I bet Andrew is submitting patches off of a GDB 6.3 or
> >earlier port.  In HEAD, errors during backtrace are non-fatal.
> 
> Quite so. I have not forward ported our target backend and so cannot 
> test threads with HEAD. Not with SH OS21 anyway.
> 
> Is it possible that the message is still present for SH and/or 
> non-standard threading implementations?

The message is still _present_.  It is no longer _fatal_, which is what
you were complaining about with this patch.

-- 
Daniel Jacobowitz
CodeSourcery, LLC

Re: [SH][PATCH] Disable ABI frame sniffer

[Daniel Jacobowitz]

On Thu, Nov 10, 2005 at 10:33:18AM +0000, Andrew STUBBS wrote:
> Daniel Jacobowitz wrote:
> >I had to look up what you meant by "ABI frame sniffer"; we normally
> >call this the prologue analyzer.
> 
> One registers the analyser using frame_unwind_append_sniffer(). The 
> currently configured analysers are called dwarf2_frame_sniffer() and 
> sh_frame_sniffer().
> 
> I can't imagine where I got the name from ;-)

I was complaning about the "ABI" part.  If you want to talk about what
kind of object it is, it's an unwinder, not a sniffer; sniffers are
used to select unwinders.

> Naturally you can't unwind ASM or anything without CFI without the 
> fallback unwinder. That's why it is there. That doesn't mean it isn't 
> irritating when it trys to unwind frames that aren't there. Most of the 
> time you just don't need it.

Quite on the contrary, my experience is that in most programs you'll
need it at least a couple of times, e.g. to get out of bits of libc or
linker-generated code.

> It may be possible to improve the unwinder. However, it is dependent on 
> the values in registers and in memory. It would be impossible to prevent 
> it getting occasionally confused.

Just like the rest of GDB on both counts.

> Besides which, the other problem I am trying to fix is that, before the 
> error, the unwinder would typically produce at least one bogus frame. 
> Even if the error has changed to a warning (as you say in the other 
> message) this would still be an irritant.

I know it's tempting to try to fix this with a broad stroke.  But
really, it is not appropriate.  Cases need to be handled one at a time
if you want the unwinder to work.

> I much prefer the option to say 'My program has CFI, I know it does, I 
> put it there, so stop trying to confuse yourself'.

For any frame with CFI, the CFI is used in preference.  If you don't
want the fallback unwinder to come into play, if as you claim you
really don't need it, then find out why it's being triggered in the
first place.

> As to the SH maintainers, there are none listed in MAINTAINERS.

Yes, my mistake; there used to be.

-- 
Daniel Jacobowitz
CodeSourcery, LLC

Re: [SH][PATCH] Disable ABI frame sniffer

[Andrew STUBBS]

Daniel Jacobowitz wrote:
> I was complaning about the "ABI" part.  If you want to talk about what
> kind of object it is, it's an unwinder, not a sniffer; sniffers are
> used to select unwinders.

Hmm, confusing. Well, I can change the name easily enough. It is the 
unwinder based on the ABI though. How come the unwind function is called 
sniffer?

>>Naturally you can't unwind ASM or anything without CFI without the 
>>fallback unwinder. That's why it is there. That doesn't mean it isn't 
>>irritating when it trys to unwind frames that aren't there. Most of the 
>>time you just don't need it.
> 
> 
> Quite on the contrary, my experience is that in most programs you'll
> need it at least a couple of times, e.g. to get out of bits of libc or
> linker-generated code.

Bare-machine (or OS21) sh-elf doesn't use glibc with all its 
complications. We use newlib and we always compile it with debug info 
and CFI so there is it should never need to fall back.

>>It may be possible to improve the unwinder. However, it is dependent on 
>>the values in registers and in memory. It would be impossible to prevent 
>>it getting occasionally confused.
> 
> 
> Just like the rest of GDB on both counts.

Maybe other bits of GDB need an option to switch them off when they turn 
problematic. Maybe they already do. In fact, maybe I should make this 
feature more generic and change the error message so that it says 'if 
this isn't a real problem try switching ... off'.

>>I much prefer the option to say 'My program has CFI, I know it does, I 
>>put it there, so stop trying to confuse yourself'.
> 
> 
> For any frame with CFI, the CFI is used in preference.  If you don't
> want the fallback unwinder to come into play, if as you claim you
> really don't need it, then find out why it's being triggered in the
> first place.

The problem only occurs in threads (unless 'set backtrace pastmain' is 
set, in which case it happens in any program) when the backtrace falls 
off the end of the program. There are no more frames because there's no 
more stack, but there's no way to know that unless you assume the CFI 
and program run out at the same time.

>>As to the SH maintainers, there are none listed in MAINTAINERS.
> 
> 
> Yes, my mistake; there used to be.

BTW, were you (and others) waiting for them to do something about the SH 
specific patches I have been sending? I ask because I have had no 
response on a few.

I'm currently in the process of trying to patch GDB HEAD sufficiently to 
run OS21 threaded programs on ST targets like I do with our 6.3 based 
tools. When I've got that done I'll have a better idea whether anything 
needs to be done any more. I suspect we will still _like_ to.

Thanks

Andrew Stubbs

Re: [SH][PATCH] Disable ABI frame sniffer

[Andrew STUBBS]

> I'm currently in the process of trying to patch GDB HEAD sufficiently to 
> run OS21 threaded programs on ST targets like I do with our 6.3 based 
> tools. When I've got that done I'll have a better idea whether anything 
> needs to be done any more. I suspect we will still _like_ to.

The following shows the difference between the unwinder enabled and 
disabled:

(gdb) thread apply all bt

Thread 7 (Thread 7):
#0  _md_kernel_task_launch (entry_point=0x9, datap=0x8)
     at src/st40/kernel/kernel.c:164
#1  0xdeaddead in ?? ()
#2  0xdeaddead in ?? ()
Previous frame identical to this frame (corrupt stack?)

Thread 6 (Thread 6):
#0  _md_kernel_task_launch (entry_point=0x9, datap=0x8)
     at src/st40/kernel/kernel.c:164
#1  0xdeaddead in ?? ()
#2  0xdeaddead in ?? ()
Previous frame identical to this frame (corrupt stack?)

Thread 5 (Thread 5):
#0  _md_kernel_task_launch (entry_point=0x9, datap=0x8)
     at src/st40/kernel/kernel.c:164
#1  0xdeaddead in ?? ()
#2  0xdeaddead in ?? ()
Previous frame identical to this frame (corrupt stack?)

Thread 4 (Thread 4):
#0  _md_kernel_task_launch (entry_point=0x9, datap=0x8)
     at src/st40/kernel/kernel.c:164
#1  0xdeaddead in ?? ()
#2  0xdeaddead in ?? ()
Previous frame identical to this frame (corrupt stack?)

Thread 3 (Thread 3):
#0  _md_kernel_task_launch (entry_point=0x9, datap=0x8)
     at src/st40/kernel/kernel.c:164
#1  0xdeaddead in ?? ()
#2  0xdeaddead in ?? ()
Previous frame identical to this frame (corrupt stack?)

Thread 2 (Thread 2):
#0  _md_kernel_task_launch (entry_point=0x9, datap=0x8)
     at src/st40/kernel/kernel.c:164
#1  0xdeaddead in ?? ()
#2  0xdeaddead in ?? ()
Previous frame identical to this frame (corrupt stack?)

Thread 1 (Thread 1):
#0  main () at /home/afra/users/stubbsa/os21test.c:30
(gdb) set backtrace abi-sniffer off
(gdb) thread apply all bt

Thread 7 (Thread 7):
#0  _md_kernel_task_launch (entry_point=0x9, datap=0x8)
     at src/st40/kernel/kernel.c:164
#1  0xdeaddead in ?? ()

Thread 6 (Thread 6):
#0  _md_kernel_task_launch (entry_point=0x9, datap=0x8)
     at src/st40/kernel/kernel.c:164
#1  0xdeaddead in ?? ()

Thread 5 (Thread 5):
#0  _md_kernel_task_launch (entry_point=0x9, datap=0x8)
     at src/st40/kernel/kernel.c:164
#1  0xdeaddead in ?? ()

Thread 4 (Thread 4):
#0  _md_kernel_task_launch (entry_point=0x9, datap=0x8)
     at src/st40/kernel/kernel.c:164
#1  0xdeaddead in ?? ()

Thread 3 (Thread 3):
#0  _md_kernel_task_launch (entry_point=0x9, datap=0x8)
     at src/st40/kernel/kernel.c:164
#1  0xdeaddead in ?? ()

Thread 2 (Thread 2):
#0  _md_kernel_task_launch (entry_point=0x9, datap=0x8)
     at src/st40/kernel/kernel.c:164
#1  0xdeaddead in ?? ()

Thread 1 (Thread 1):
#0  main () at /home/afra/users/stubbsa/os21test.c:30



The situation clearly has improved since 6.3, but I'm still not happy 
about giving a debugger with this behaviour to our customers.

Unfortunately I have been unable to test how eclipse reacts to the error 
message due to a mysterious seg fault in ui_file_put() when using -i=mi1 
(but not mi2) after the first 'Loading section' message. Apparently 
'0x19' isn't a useful value for 'file'. Something to look forward to 
tomorrow.

BFN

Andrew

Re: [SH][PATCH] Disable ABI frame sniffer

[Mark Kettenis]

> Date: Thu, 10 Nov 2005 14:33:01 +0000
> From: Andrew STUBBS <andrew.stubbs@st.com>
> 
> Daniel Jacobowitz wrote:
> > Quite on the contrary, my experience is that in most programs you'll
> > need it at least a couple of times, e.g. to get out of bits of libc or
> > linker-generated code.
> 
> Bare-machine (or OS21) sh-elf doesn't use glibc with all its 
> complications. We use newlib and we always compile it with debug info 
> and CFI so there is it should never need to fall back.

But can you guarantee that other people won't need the fallback?

> > For any frame with CFI, the CFI is used in preference.  If you don't
> > want the fallback unwinder to come into play, if as you claim you
> > really don't need it, then find out why it's being triggered in the
> > first place.
> 
> The problem only occurs in threads (unless 'set backtrace pastmain' is 
> set, in which case it happens in any program) when the backtrace falls 
> off the end of the program. There are no more frames because there's no 
> more stack, but there's no way to know that unless you assume the CFI 
> and program run out at the same time.

Ah there's your real problem.  The unwinder walks off the stack and at
that point concludes the stack is thrashed.  This is a recurring
problem on many platforms, especially when threads are involved.  It
might be possible to detect the end of the stack on your platform and
teach the fallback unwinder about it.  That might involve some changes
to the threads library and/or crt0 though, to make it mark the end of
the stack properly.  It'd also be great to have a way to encode the
end of the stack in CFI.  Unfortunately there has been quite a bit of
talk about this, but nobody has implemeneted anything yet.

So I mostly agree with Daniel, and think your patch is a bad idea.  Sorry.

Mark

Re: [SH][PATCH] Disable ABI frame sniffer

[Daniel Jacobowitz]

On Thu, Nov 10, 2005 at 11:59:39PM +0100, Mark Kettenis wrote:
> Ah there's your real problem.  The unwinder walks off the stack and at
> that point concludes the stack is thrashed.  This is a recurring
> problem on many platforms, especially when threads are involved.  It
> might be possible to detect the end of the stack on your platform and
> teach the fallback unwinder about it.  That might involve some changes
> to the threads library and/or crt0 though, to make it mark the end of
> the stack properly.  It'd also be great to have a way to encode the
> end of the stack in CFI.  Unfortunately there has been quite a bit of
> talk about this, but nobody has implemeneted anything yet.

How short your memory... :-)

  http://sources.redhat.com/ml/gdb-patches/2005-03/msg00021.html

There's no way in GCC to annotate a function with this property, and it
appears that the thread-start routine in Andrew's OS is written in C,
so I don't know if it helps.

The fp == 0 and pc == 0 checks are one level off for stopping this, as
I recall.  A cleverer solution to that might be possible.

-- 
Daniel Jacobowitz
CodeSourcery, LLC

Re: [SH][PATCH] Disable ABI frame sniffer

[Daniel Jacobowitz]

On Thu, Nov 10, 2005 at 02:33:01PM +0000, Andrew STUBBS wrote:
> Hmm, confusing. Well, I can change the name easily enough. It is the 
> unwinder based on the ABI though. How come the unwind function is called 
> sniffer?

It's not.  That doesn't _do_ the unwinding, it _selects_ which unwinder
to use.  Sniffs around for the right one, I suppose.

> >Quite on the contrary, my experience is that in most programs you'll
> >need it at least a couple of times, e.g. to get out of bits of libc or
> >linker-generated code.
> 
> Bare-machine (or OS21) sh-elf doesn't use glibc with all its 
> complications. We use newlib and we always compile it with debug info 
> and CFI so there is it should never need to fall back.

There are assembly routines in newlib too, you know.  I didn't say
anything about glibc.  You may encounter linker generated branch
trampolines in some cases, too.

> >>It may be possible to improve the unwinder. However, it is dependent on 
> >>the values in registers and in memory. It would be impossible to prevent 
> >>it getting occasionally confused.
> >
> >
> >Just like the rest of GDB on both counts.

In case you missed my sarcasm here, I was responding to "dependent on
the values in registers and memory".  Of course it is!

> The problem only occurs in threads (unless 'set backtrace pastmain' is 
> set, in which case it happens in any program) when the backtrace falls 
> off the end of the program. There are no more frames because there's no 
> more stack, but there's no way to know that unless you assume the CFI 
> and program run out at the same time.

See Mark's reply for more on this.  Had you said what your problem
really was, we could have saved a couple of back-and-forth exchanges;
I'm extremely familiar with the problem of finding clean ways to
terminate the backtrace.

> BTW, were you (and others) waiting for them to do something about the SH 
> specific patches I have been sending? I ask because I have had no 
> response on a few.

I review patches as fast as I am able.  I am more likely to review
patches to code I know something about, which does not include the SH
backend.

-- 
Daniel Jacobowitz
CodeSourcery, LLC

Re: [SH][PATCH] Disable ABI frame sniffer

[Andrew STUBBS]

Mark Kettenis wrote:
> But can you guarantee that other people won't need the fallback?

It remains on be default. I'm not suggesting removing the unwinder, 
merely adding an option to tell it to 'get lost' when it gets annoying.

> So I mostly agree with Daniel, and think your patch is a bad idea.  Sorry.

Fair enough. I'm happy to look into an alternative, but don't I see one yet.

Andrew

Re: [SH][PATCH] Disable ABI frame sniffer

[Andrew STUBBS]

Hi Daniel,

Daniel Jacobowitz wrote:
> There are assembly routines in newlib too, you know.  I didn't say
> anything about glibc.  You may encounter linker generated branch
> trampolines in some cases, too.

I know you didn't say anything about glibc. I was just citing that as an 
example of a complex library.

Newlib does have assembly routines, but very few, on SH anyway. The only 
ones I know of off hand are memset and friends. There's the crt stuff, 
of course, but you have to go 'pastmain' to find that.

> In case you missed my sarcasm here, I was responding to "dependent on
> the values in registers and memory".  Of course it is!

I didn't miss it as such, but seriously, most of what GDB does doesn't 
involve following (possibly phantom) trails of pointers through target 
memory. This about the least certain data that GDB presents, simply 
because the uncertainties mount up.

> See Mark's reply for more on this.  Had you said what your problem
> really was, we could have saved a couple of back-and-forth exchanges;
> I'm extremely familiar with the problem of finding clean ways to
> terminate the backtrace.

OK, I'm listening. If we can get it so that both the 0xdeadbeef frames 
(one which comes from the dwarf unwinder - see my posting yesterday) are 
ignored then that would be a bonus.

>>BTW, were you (and others) waiting for them to do something about the SH 
>>specific patches I have been sending? I ask because I have had no 
>>response on a few.
> 
> 
> I review patches as fast as I am able.  I am more likely to review
> patches to code I know something about, which does not include the SH
> backend.
> 

I can understand that. I'm sure you have your own real work to be 
getting on with too. However, I have a selection of SH specific patches, 
submitted and queued, so this situation worries me a little. About half 
of sh-tdep.c is a total mystery to me - mostly the bits that have never 
caused me any trouble. My patches to that area are all quite simple, so 
it probably won't be too bad.

Thanks for all your time

Andrew Stubbs

Re: [SH][PATCH] Disable ABI frame sniffer

[Daniel Jacobowitz]

On Fri, Nov 11, 2005 at 11:53:33AM +0000, Andrew STUBBS wrote:
> >See Mark's reply for more on this.  Had you said what your problem
> >really was, we could have saved a couple of back-and-forth exchanges;
> >I'm extremely familiar with the problem of finding clean ways to
> >terminate the backtrace.
> 
> OK, I'm listening. If we can get it so that both the 0xdeadbeef frames 
> (one which comes from the dwarf unwinder - see my posting yesterday) are 
> ignored then that would be a bonus.

What we need is to examine the alternatives for cleanly marking the end
of a stack frame - there are quite a few - and find one which will work
for your situation.

Here's the easiest, in that it only requires changing one of the three
pieces of software involved (runtime, compiler, debugger):

Instead of whatever C language routine you use to start threads, start
them using an assembly wrapper.  Have it set up a non-unwindable frame
using DWARF2 CFI as mentioned in one of my earlier postings in this
thread.  You can do this easily by marking the return address column as
undefined.  Then have it call the C thread start routine.  GDB will
cleanly terminate the backtrace right there  You'll have one more frame
in your stack, but it will be a real frame.

Other alternatives:
  - Add a GCC attribute to mark a function as "never called directly,
    not backtraceable" in the CFI.  Annotate the runtime library to
    mark the thread start routine with this attribute.  I'm not real
    fond of this solution, though.
  - Add a way for a sniffer to indicate "I do recognize how to unwind
    this frame, but I recognize it as the end of the stack" to solve
    the current off-by-one problem.  This could be quite nice...

-- 
Daniel Jacobowitz
CodeSourcery, LLC

Re: [SH][PATCH] Disable ABI frame sniffer

[Jim Blandy]

On 11/13/05, Daniel Jacobowitz <drow@false.org> wrote:
> Instead of whatever C language routine you use to start threads, start
> them using an assembly wrapper.  Have it set up a non-unwindable frame
> using DWARF2 CFI as mentioned in one of my earlier postings in this
> thread.  You can do this easily by marking the return address column as
> undefined.  Then have it call the C thread start routine.  GDB will
> cleanly terminate the backtrace right there  You'll have one more frame
> in your stack, but it will be a real frame.

There's an example of how to do this in libgloss/m32c/crt0.S (and
other places, I'm sure).

Re: [SH][PATCH] Disable ABI frame sniffer

[Andrew STUBBS]

Daniel Jacobowitz wrote:
>   - Add a way for a sniffer to indicate "I do recognize how to unwind
>     this frame, but I recognize it as the end of the stack" to solve
>     the current off-by-one problem.  This could be quite nice...

I don't want to mess with the OS or compiler to fix this problem so I 
have been looking into this suggestion (or at least something in GDB alone).

I started by trying to figure out why the dwarf unwinder appeared to be 
giving:

   #2  0xdeaddead in ?? ()

It turns out that this does not come from here at all - the dwarf 
sniffer rejects the frame, and yet I thought I had the SH unwinder 
disabled, so why did it still come out?

In fact, the order of events is (and apologise to all those who know 
this stuff backwards):

   dwarf sniffer
   sh sniffer
   print message
   sh 'frame this id' (where I had disabled the unwinder).

The obvious thing to do appears to be to disable it in the sh sniffer, 
but if I have that return NULL, as the dwarf unwinder does, I get an 
internal error. Not only may this sniffer never reject *any* frame, it 
is not possible to remove it altogether (i.e. not register it in the 
first place), because you still get the internal error.

I must conclude therefore that it is not currently possible to reject 
any 'frame' which precedes (from the point of view of the program) a 
frame with debug information.

My question is 'where to go from here?'

The above suggestion would require modifying the dwarf sniffer (since 
the last frame on the stack has dwarf). I can't easily see how to do 
this because the dwarf data doesn't know it is the end of the stack and 
unwinding any further seems problematic.

I could modify the sh sniffer such that it actually does something. 
However, this would require modifying frame_unwind_find_by_frame() and 
family such that it can accept a NULL answer from every sniffer.

There is, of course, the question of how to detect the end of the stack, 
short of just searching for the specific case. I could check if it is in 
memory, but that seems problematic at best - for one thing it assumes we 
know where memory is. Perhaps I could see if it is in a loaded section?

Any other suggestions?

Thanks

Andrew Stubbs

Re: [SH][PATCH] Disable ABI frame sniffer

[Andrew STUBBS]

[-- Attachment #1: Type: text/plain, Size: 1466 bytes --]

Andrew Stubbs wrote:
> I could modify the sh sniffer such that it actually does something. 
> However, this would require modifying frame_unwind_find_by_frame() and 
> family such that it can accept a NULL answer from every sniffer.

This won't work. The whole unwinding mechanism is lazy and does not 
happen until print_frame_info() has already committed to printing 
something, it just doesn't know what yet.

The reason for this is that get_prev_frame() and get_prev_frame_1() have 
already effectively promised that there will be another frame, but 
haven't actually ventured to find out what it is - that is done lazily 
as required.

It isn't possible to have the sniffer say 'I know this is the last frame 
on the stack' because, unless the compiler says so, all it really could 
tell is that this is the last frame as it knows it. It might work if the 
code asked all the sniffers and returned 'end of stack' if all of them 
fail to recognise a frame.

But, all is not lost ....

The attached patch to get_prev_frame_1() *does* fix the problem. With 
this I get exactly what I want.

I have no doubt that it is somehow horribly flawed and totally 
unacceptable, but hopefully something will emerge from all this.

BTW, the file has a comment that says 'Allocate the new frame but do not 
wire it in to the frame chain', but then it appears to go ahead and wire 
up the frame chain. Is this a mistake or am I just misunderstanding it?

Thanks

Andrew Stubbs

[-- Attachment #2: unwinder.patch --]
[-- Type: text/plain, Size: 977 bytes --]

2005-11-24  Andrew Stubbs  <andrew.stubbs@st.com>

	* frame.c (get_prev_frame_1): Check the PC is within the program
	before allowing the frame to be created.

Index: src/gdb/frame.c
===================================================================
--- src.orig/gdb/frame.c	2005-11-24 16:45:22.000000000 +0000
+++ src/gdb/frame.c	2005-11-24 17:24:09.000000000 +0000
@@ -1123,6 +1123,16 @@ get_prev_frame_1 (struct frame_info *thi
   this_frame->prev = prev_frame;
   prev_frame->next = this_frame;
 
+  /* Check that the new frame would refer to a location within the
+     program.  This has to be done after it is linked in or the
+     function calls will not work.  If the location is junk then
+     we have probably dropped off the bottom of the stack.  */
+  if (!find_pc_section (frame_unwind_address_in_block (this_frame)))
+    {
+      this_frame->prev = NULL;
+      return NULL;
+    }
+
   if (frame_debug)
     {
       fprintf_unfiltered (gdb_stdlog, "-> ");

Re: [SH][PATCH] Disable ABI frame sniffer

[Mark Kettenis]

> Date: Thu, 24 Nov 2005 17:25:52 +0000
> From: Andrew STUBBS <andrew.stubbs@st.com>
>
> The attached patch to get_prev_frame_1() *does* fix the problem. With 
> this I get exactly what I want.
> 
> I have no doubt that it is somehow horribly flawed and totally 
> unacceptable, but hopefully something will emerge from all this.

Indeed it is.  Several operating systems map bits of code into the
program's address space and your patch prevents gdb from unwinding
through such bits.

Mark