frame.c backtrace limit issue and fix

frame.c backtrace limit issue and fix

[Jonathan Larmour]

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

On my embedded m68k target, where debugging appears problematic, sometimes 
frames called in frame.c:get_prev_frame() have their frame level set to 
-1. If you have a backtrace limit set (with "set backtrace limit") then 
this would mean the following test is done:


   if (this_frame->level > backtrace_limit)
     {
       error ("Backtrace limit of %d exceeded", backtrace_limit);
     }

This looks like it would be right even if the level is -1. However 
backtrace_limit is unsigned and this results in this_frame->level being 
promoted to an unsigned 0xffffffff value.

Therefore I'm attaching a patch to correct this, simply by making 
backtrace_limit now be signed. Anyone who needs more than 2 billion stack 
levels has bigger problems :-).

I do have write access so can check this in if someone approves.

Jifl

2005-05-23  Jonathan Larmour  <jifl@eCosCentric.com>

	* command.h: Declare new add_setshow_integer_cmd function.
	* cli/cli-decode.c (add_setshow_integer_cmd): New function. Cloned
	from add_setshow_uinteger_cmd.
	* frame.c: backtrace_limit should be signed to prevent matching against
	negative frame levels.
	(_initialize_frame): Call add_setshow_integer_cmd for backtrace limit
	now.



-- 
eCosCentric    http://www.eCosCentric.com/    The eCos and RedBoot experts
--["No sense being pessimistic, it wouldn't work anyway"]-- Opinions==mine

[-- Attachment #2: gdb-btlimit-6.3.patch --]
[-- Type: text/x-patch, Size: 3114 bytes --]

--- command.h.old	2004-07-28 20:42:00.000000000 +0100
+++ command.h	2005-05-23 19:00:44.000000000 +0100
@@ -270,6 +270,18 @@
 				    struct cmd_list_element **set_list,
 				    struct cmd_list_element **show_list);
 
+extern void add_setshow_integer_cmd (char *name,
+				     enum command_class class,
+				     int *var,
+				     const char *set_doc,
+				     const char *show_doc,
+				     const char *help_doc,
+				     const char *print,
+				     cmd_sfunc_ftype *set_func,
+				     cmd_sfunc_ftype *show_func,
+				     struct cmd_list_element **set_list,
+				     struct cmd_list_element **show_list);
+
 extern void add_setshow_uinteger_cmd (char *name,
 				      enum command_class class,
 				      unsigned int *var,
--- frame.c.old	2005-05-23 18:08:02.000000000 +0100
+++ frame.c	2005-05-23 19:08:25.000000000 +0100
@@ -115,7 +115,7 @@
 /* Flag to indicate whether backtraces should stop at main et.al.  */
 
 static int backtrace_past_main;
-static unsigned int backtrace_limit = UINT_MAX;
+static int backtrace_limit = INT_MAX;
 
 static void
 fprint_field (struct ui_file *file, const char *name, int p, CORE_ADDR addr)
@@ -1525,15 +1525,15 @@
 			   NULL, NULL, &set_backtrace_cmdlist,
 			   &show_backtrace_cmdlist);
 
-  add_setshow_uinteger_cmd ("limit", class_obscure,
-			    &backtrace_limit, "\
+  add_setshow_integer_cmd ("limit", class_obscure,
+			   &backtrace_limit, "\
 Set an upper bound on the number of backtrace levels.", "\
 Show the upper bound on the number of backtrace levels.", "\
 No more than the specified number of frames can be displayed or examined.\n\
 Zero is unlimited.", "\
 An upper bound on the number of backtrace levels is %s.",
-			    NULL, NULL, &set_backtrace_cmdlist,
-			    &show_backtrace_cmdlist);
+			   NULL, NULL, &set_backtrace_cmdlist,
+			   &show_backtrace_cmdlist);
 
   /* Debug this files internals. */
   deprecated_add_show_from_set
--- cli/cli-decode.c.old	2004-07-30 18:55:47.000000000 +0100
+++ cli/cli-decode.c	2005-05-23 19:00:03.000000000 +0100
@@ -493,6 +493,27 @@
    add_cmd.  VAR is address of the variable which will contain the
    value.  SET_DOC and SHOW_DOC are the documentation strings.  */
 void
+add_setshow_integer_cmd (char *name, enum command_class class,
+			  int *var,
+			  const char *set_doc, const char *show_doc,
+			  const char *help_doc, const char *print,
+			  cmd_sfunc_ftype *set_func,
+			  cmd_sfunc_ftype *show_func,
+			  struct cmd_list_element **set_list,
+			  struct cmd_list_element **show_list)
+{
+  add_setshow_cmd_full (name, class, var_integer, var,
+			set_doc, show_doc, help_doc, print,
+			set_func, show_func,
+			set_list, show_list,
+			NULL, NULL);
+}
+
+/* Add element named NAME to both the set and show command LISTs (the
+   list for set/show or some sublist thereof).  CLASS is as in
+   add_cmd.  VAR is address of the variable which will contain the
+   value.  SET_DOC and SHOW_DOC are the documentation strings.  */
+void
 add_setshow_uinteger_cmd (char *name, enum command_class class,
 			  unsigned int *var,
 			  const char *set_doc, const char *show_doc,

Re: frame.c backtrace limit issue and fix

[Daniel Jacobowitz]

On Mon, May 23, 2005 at 07:19:04PM +0100, Jonathan Larmour wrote:
> On my embedded m68k target, where debugging appears problematic, sometimes 
> frames called in frame.c:get_prev_frame() have their frame level set to 
> -1. If you have a backtrace limit set (with "set backtrace limit") then 
> this would mean the following test is done:
> 
> 
>   if (this_frame->level > backtrace_limit)
>     {
>       error ("Backtrace limit of %d exceeded", backtrace_limit);
>     }
> 
> This looks like it would be right even if the level is -1. However 
> backtrace_limit is unsigned and this results in this_frame->level being 
> promoted to an unsigned 0xffffffff value.
> 
> Therefore I'm attaching a patch to correct this, simply by making 
> backtrace_limit now be signed. Anyone who needs more than 2 billion stack 
> levels has bigger problems :-).
> 
> I do have write access so can check this in if someone approves.

You seem to be submitting patches against an old version; there already
is a function named add_setshow_integer_cmd.  Andrew added it in
February.  Also, someone wrote a testcase for this problem, but I think
it's still in my queue somewhere... ah, no, it's on Paul's plate at the
moment.

So only about half this patch is needed.  I took that, stuck a properly
formatted ChangeLog and PR number on it, and tested it.
Unfortunately... it didn't work.  The errors went away, but backtraces
didn't stop in the right place.

(gdb) bt
#0  factorial (value=4) at /big/fsf/commit/src/gdb/testsuite/gdb.base/break.c:111
#1  0x080484e3 in factorial (value=5) at /big/fsf/commit/src/gdb/testsuite/gdb.base/break.c:112
#2  0x080484e3 in factorial (value=6) at /big/fsf/commit/src/gdb/testsuite/gdb.base/break.c:112
Backtrace limit of 1 exceeded

Oops, that's not a backtrace limit of 1, that's a backtrace limit of
3... the documentation suggests that "bt 3" and "set backtrace limit 3"
should behave similarly in this regard.

I also don't think that the error() is appropriate.  "set backtrace
past-main" doesn't give an error at main.

With all those fixed, I get the attached patch, which I've tested,
regression tested, and checked in.  Thanks.

-- 
Daniel Jacobowitz
CodeSourcery, LLC

2005-05-28  Daniel Jacobowitz  <dan@codesourcery.com>
	    Jonathan Larmour  <jifl@eCosCentric.com>

	PR backtrace/1760
	* frame.c (backtrace_limit): Change type to int.
	(get_prev_frame): Update backtrace limit support.
	(_initialize_frame): Use add_setshow_integer_cmd for backtrace_limit.

Index: frame.c
===================================================================
RCS file: /cvs/src/src/gdb/frame.c,v
retrieving revision 1.209
diff -u -p -r1.209 frame.c
--- frame.c	22 May 2005 14:53:33 -0000	1.209
+++ frame.c	28 May 2005 22:41:17 -0000
@@ -141,7 +141,7 @@ Whether backtraces should continue past 
 		    value);
 }
 
-static unsigned int backtrace_limit = UINT_MAX;
+static int backtrace_limit = INT_MAX;
 static void
 show_backtrace_limit (struct ui_file *file, int from_tty,
 		      struct cmd_list_element *c, const char *value)
@@ -1258,9 +1258,16 @@ get_prev_frame (struct frame_info *this_
       return NULL;
     }
 
-  if (this_frame->level > backtrace_limit)
+  /* If the user's backtrace limit has been exceeded, stop.  We must
+     add two to the current level; one of those accounts for backtrace_limit
+     being 1-based and the level being 0-based, and the other accounts for
+     the level of the new frame instead of the level of the current
+     frame.  */
+  if (this_frame->level + 2 > backtrace_limit)
     {
-      error (_("Backtrace limit of %d exceeded"), backtrace_limit);
+      frame_debug_got_null_frame (gdb_stdlog, this_frame,
+				  "backtrace limit exceeded");
+      return NULL;
     }
 
   /* If we're already inside the entry function for the main objfile,
@@ -1610,16 +1617,16 @@ the rest of the stack trace."),
 			   &set_backtrace_cmdlist,
 			   &show_backtrace_cmdlist);
 
-  add_setshow_uinteger_cmd ("limit", class_obscure,
-			    &backtrace_limit, _("\
+  add_setshow_integer_cmd ("limit", class_obscure,
+			   &backtrace_limit, _("\
 Set an upper bound on the number of backtrace levels."), _("\
 Show the upper bound on the number of backtrace levels."), _("\
 No more than the specified number of frames can be displayed or examined.\n\
 Zero is unlimited."),
-			    NULL,
-			    show_backtrace_limit,
-			    &set_backtrace_cmdlist,
-			    &show_backtrace_cmdlist);
+			   NULL,
+			   show_backtrace_limit,
+			   &set_backtrace_cmdlist,
+			   &show_backtrace_cmdlist);
 
   /* Debug this files internals. */
   add_setshow_zinteger_cmd ("frame", class_maintenance, &frame_debug,  _("\

Re: frame.c backtrace limit issue and fix

[Jonathan Larmour]

Daniel Jacobowitz wrote:
> You seem to be submitting patches against an old version; there already
> is a function named add_setshow_integer_cmd.  Andrew added it in
> February.

Oops, yes it was against the most recent official release (6.3) not CVS. I 
had to do the patch against 6.3 for my work, and didn't remember to switch 
to CVS for the submission, my mistake.

[ker-snip]
> With all those fixed, I get the attached patch, which I've tested,
> regression tested, and checked in.  Thanks.

Looks like you spent more effort on it than I did, so thank *you*!

Jifl
-- 
eCosCentric    http://www.eCosCentric.com/    The eCos and RedBoot experts
--["No sense being pessimistic, it wouldn't work anyway"]-- Opinions==mine