[PATCH 1/2] Add an undwinder_name field to "struct frame_unwind" for use by "info frame".

[PATCH 1/2] Add an undwinder_name field to "struct frame_unwind" for use by "info frame".

[Samuel Bronson]

Have "info frame" display this field when present.

Fill in the field for unwinders (besides the sentinel) which are
normally built on i386 Linux.

Signed-off-by: Samuel Bronson <naesten@gmail.com>
---
 gdb/dummy-frame.c  |    2 ++
 gdb/dwarf2-frame.c |    8 ++++++--
 gdb/frame-unwind.h |    1 +
 gdb/frame.c        |    7 +++++++
 gdb/frame.h        |    3 +++
 gdb/i386-tdep.c    |    8 ++++++--
 gdb/stack.c        |   10 +++++++++-
 gdb/tramp-frame.c  |    1 +
 8 files changed, 35 insertions(+), 5 deletions(-)

diff --git a/gdb/dummy-frame.c b/gdb/dummy-frame.c
index 6e63686..59990c1 100644
--- a/gdb/dummy-frame.c
+++ b/gdb/dummy-frame.c
@@ -278,6 +278,8 @@ static const struct frame_unwind dummy_frame_unwinder =
   dummy_frame_prev_register,
   NULL,
   dummy_frame_sniffer,
+  NULL,
+  "dummy_frame_unwinder",
 };
 
 const struct frame_unwind *const dummy_frame_unwind = {
diff --git a/gdb/dwarf2-frame.c b/gdb/dwarf2-frame.c
index ce11d89..c90bd08 100644
--- a/gdb/dwarf2-frame.c
+++ b/gdb/dwarf2-frame.c
@@ -1183,7 +1183,9 @@ static const struct frame_unwind dwarf2_frame_unwind =
   dwarf2_frame_this_id,
   dwarf2_frame_prev_register,
   NULL,
-  dwarf2_frame_sniffer
+  dwarf2_frame_sniffer,
+  NULL,
+  "dwarf2_frame_unwind",
 };
 
 static const struct frame_unwind dwarf2_signal_frame_unwind =
@@ -1192,7 +1194,9 @@ static const struct frame_unwind dwarf2_signal_frame_unwind =
   dwarf2_frame_this_id,
   dwarf2_frame_prev_register,
   NULL,
-  dwarf2_frame_sniffer
+  dwarf2_frame_sniffer,
+  NULL,
+  "dwarf2_signal_frame_unwind",
 };
 
 /* Append the DWARF-2 frame unwinders to GDBARCH's list.  */
diff --git a/gdb/frame-unwind.h b/gdb/frame-unwind.h
index 35eeebf..4da7fcc 100644
--- a/gdb/frame-unwind.h
+++ b/gdb/frame-unwind.h
@@ -133,6 +133,7 @@ struct frame_unwind
   const struct frame_data *unwind_data;
   frame_sniffer_ftype *sniffer;
   frame_dealloc_cache_ftype *dealloc_cache;
+  const char *unwinder_name;
 };
 
 /* Register a frame unwinder, _prepending_ it to the front of the
diff --git a/gdb/frame.c b/gdb/frame.c
index b0a99fb..1319f3a 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -1824,6 +1824,13 @@ frame_stop_reason_string (enum unwind_stop_reason reason)
     }
 }
 
+/* Return unwinder name */
+
+const char *get_frame_unwinder_name (struct frame_info *frame)
+{
+  return frame->unwind->unwinder_name;
+}
+
 /* Clean up after a failed (wrong unwinder) attempt to unwind past
    FRAME.  */
 
diff --git a/gdb/frame.h b/gdb/frame.h
index 224aec9..b4a0871 100644
--- a/gdb/frame.h
+++ b/gdb/frame.h
@@ -422,6 +422,9 @@ enum unwind_stop_reason get_frame_unwind_stop_reason (struct frame_info *);
 
 const char *frame_stop_reason_string (enum unwind_stop_reason);
 
+/* Return the name of the frame's unwinder (or NULL) */
+const char *get_frame_unwinder_name (struct frame_info *);
+
 /* Unwind the stack frame so that the value of REGNUM, in the previous
    (up, older) frame is returned.  If VALUEP is NULL, don't
    fetch/compute the value.  Instead just return the location of the
diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c
index c2515fe..bc35279 100644
--- a/gdb/i386-tdep.c
+++ b/gdb/i386-tdep.c
@@ -1467,7 +1467,9 @@ static const struct frame_unwind i386_frame_unwind =
   i386_frame_this_id,
   i386_frame_prev_register,
   NULL,
-  default_frame_sniffer
+  default_frame_sniffer,
+  NULL,
+  "i386_frame_unwind",
 };
 \f
 
@@ -1567,7 +1569,9 @@ static const struct frame_unwind i386_sigtramp_frame_unwind =
   i386_sigtramp_frame_this_id,
   i386_sigtramp_frame_prev_register,
   NULL,
-  i386_sigtramp_frame_sniffer
+  i386_sigtramp_frame_sniffer,
+  NULL,
+  "i386_sigtramp_frame_unwind",
 };
 \f
 
diff --git a/gdb/stack.c b/gdb/stack.c
index cc56125..1169d59 100644
--- a/gdb/stack.c
+++ b/gdb/stack.c
@@ -1151,7 +1151,8 @@ frame_info (char *addr_exp, int from_tty)
     int count;
     int i;
     int need_nl = 1;
-
+    const char *unwinder;
+    
     /* The sp is special; what's displayed isn't the save address, but
        the value of the previous frame's sp.  This is a legacy thing,
        at one stage the frame cached the previous frame's SP instead
@@ -1198,6 +1199,13 @@ frame_info (char *addr_exp, int from_tty)
 	/* else keep quiet.  */
       }
 
+    unwinder = get_frame_unwinder_name(fi);
+    if (unwinder)
+      {
+	printf_filtered(" Unwinder: %s\n", unwinder);
+	need_nl = 0;
+      }
+
     count = 0;
     numregs = gdbarch_num_regs (gdbarch)
 	      + gdbarch_num_pseudo_regs (gdbarch);
diff --git a/gdb/tramp-frame.c b/gdb/tramp-frame.c
index 46d32fd..0880113 100644
--- a/gdb/tramp-frame.c
+++ b/gdb/tramp-frame.c
@@ -157,5 +157,6 @@ tramp_frame_prepend_unwinder (struct gdbarch *gdbarch,
   unwinder->sniffer = tramp_frame_sniffer;
   unwinder->this_id = tramp_frame_this_id;
   unwinder->prev_register = tramp_frame_prev_register;
+  unwinder->unwinder_name = "tramp-frame.c";
   frame_unwind_prepend_unwinder (gdbarch, unwinder);
 }
-- 
1.6.3.1.169.g33fd.dirty

[PATCH 2/2] Add "set/show debug unwinder" prefix commands.

[Samuel Bronson]

Also add one subcommand for tracing the sniffing of stack frames by
unwinders.

Signed-off-by: Samuel Bronson <naesten@gmail.com>
---
 gdb/frame-unwind.c |   57 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 gdb/frame-unwind.h |    7 ++++++
 2 files changed, 64 insertions(+), 0 deletions(-)

diff --git a/gdb/frame-unwind.c b/gdb/frame-unwind.c
index 98d6b43..5e893bb 100644
--- a/gdb/frame-unwind.c
+++ b/gdb/frame-unwind.c
@@ -23,6 +23,7 @@
 #include "dummy-frame.h"
 #include "value.h"
 #include "regcache.h"
+#include "gdbcmd.h"
 
 #include "gdb_assert.h"
 #include "gdb_obstack.h"
@@ -42,6 +43,9 @@ struct frame_unwind_table
   struct frame_unwind_table_entry **osabi_head;
 };
 
+/* trace unwinders called */
+static int trace_unwinders;
+
 static void *
 frame_unwind_init (struct obstack *obstack)
 {
@@ -91,14 +95,33 @@ frame_unwind_find_by_frame (struct frame_info *this_frame, void **this_cache)
   struct frame_unwind_table *table = gdbarch_data (gdbarch, frame_unwind_data);
   struct frame_unwind_table_entry *entry;
   struct cleanup *old_cleanup;
+
+  if (trace_unwinders) {
+    fprintf_unfiltered (gdb_stdlog, "[ Searching for unwinder for frame ...\n");
+  }
+
   for (entry = table->list; entry != NULL; entry = entry->next)
     {
       struct cleanup *old_cleanup;
+      const char *uname = entry->unwinder->unwinder_name;
+
+      if (trace_unwinders) {
+	if (!uname)
+	  uname = "<unknown>";
+	
+	fprintf_unfiltered (gdb_stdlog, "... trying unwinder \"%s\" (at %p) ...\n",
+			    uname, entry->unwinder);
+      }
 
       old_cleanup = frame_prepare_for_sniffer (this_frame, entry->unwinder);
       if (entry->unwinder->sniffer (entry->unwinder, this_frame,
 				    this_cache))
 	{
+	  if (trace_unwinders) {
+	    fprintf_unfiltered(gdb_stdlog,
+			       "... unwinder \"%s\"'s sniffer succeeded. Search over.]\n",
+			       uname);
+	  }
 	  discard_cleanups (old_cleanup);
 	  return entry->unwinder;
 	}
@@ -200,8 +223,42 @@ frame_unwind_got_address (struct frame_info *frame, int regnum,
 
 extern initialize_file_ftype _initialize_frame_unwind; /* -Wmissing-prototypes */
 
+/* "set debug unwinder" command list */
+struct cmd_list_element *set_debug_unwinder_list;
+/* "show debug unwinder" command list */
+struct cmd_list_element *show_debug_unwinder_list;
+
+static void
+set_debug_unwinder_cmd (char *args, int from_tty)
+{
+  help_list (set_debug_unwinder_list, "set debug unwinder ", -1, gdb_stdout);
+}
+
+static void
+show_debug_unwinder_cmd (char *args, int from_tty)
+{
+  help_list (show_debug_unwinder_list, "show debug unwinder ", -1, gdb_stdout);
+}
+
 void
 _initialize_frame_unwind (void)
 {
   frame_unwind_data = gdbarch_data_register_pre_init (frame_unwind_init);
+
+  add_prefix_cmd ("unwinder", class_maintenance, set_debug_unwinder_cmd, _("\
+Set stack frame unwinder debugging variables."),
+		  &set_debug_unwinder_list, "set debug unwinder ",
+		  0, &setdebuglist);
+
+  add_prefix_cmd ("unwinder", class_maintenance, show_debug_unwinder_cmd, _("\
+Show stack frame unwinder debugging variables."),
+		  &show_debug_unwinder_list, "set debug unwinder ",
+		  0, &showdebuglist);
+
+  add_setshow_boolean_cmd ("trace-tried", class_maintenance, &trace_unwinders,
+			   _("Set tracing of unwinders tried."),
+			   _("Show tracing of unwinders tried."),
+			   NULL /* main doc */,
+			   NULL, NULL,
+			   &set_debug_unwinder_list, &show_debug_unwinder_list);
 }
diff --git a/gdb/frame-unwind.h b/gdb/frame-unwind.h
index 4da7fcc..d8dfc30 100644
--- a/gdb/frame-unwind.h
+++ b/gdb/frame-unwind.h
@@ -152,6 +152,13 @@ extern void frame_unwind_prepend_unwinder (struct gdbarch *gdbarch,
 extern void frame_unwind_append_unwinder (struct gdbarch *gdbarch,
 					  const struct frame_unwind *unwinder);
 
+/* Command lists for "set/show debug unwinder" subcommands.  Most of
+   these are expected to be flags to request an unwinder or
+   closely-related group of unwinders to give a blow-by-blow account
+   of why it's sniffer did or did not succeed. */
+extern struct cmd_list_element *set_debug_unwinder_list;
+extern struct cmd_list_element *show_debug_unwinder_list;
+
 /* Iterate through sniffers for THIS frame until one returns with an
    unwinder implementation.  Possibly initialize THIS_CACHE.  */
 
-- 
1.6.3.1.169.g33fd.dirty

Re: [PATCH 2/2] Add "set/show debug unwinder" prefix commands.

[Eli Zaretskii]

> From: Samuel Bronson <naesten@gmail.com>
> Cc: Samuel Bronson <naesten@gmail.com>
> Date: Fri, 29 May 2009 19:16:27 -0400
> 
> Also add one subcommand for tracing the sniffing of stack frames by
> unwinders.

Thanks.

If the code is accepted, we will need a suitable change for the manual
describing this new command.

Re: [PATCH 2/2] Add "set/show debug unwinder" prefix commands.

[Doug Evans]

On Fri, May 29, 2009 at 4:16 PM, Samuel Bronson <naesten@gmail.com> wrote:
> Also add one subcommand for tracing the sniffing of stack frames by
> unwinders.
>
> [...]
> +/* trace unwinders called */
> +static int trace_unwinders;

[N.B. None of this is binding.  Just "fwiw".]

Just some nits:

I don't care *too* much about the naming, but I do care about
following existing conventions and gdb seems to have a pretty
consistent convention of using "debug" in these situations instead of
"trace".  E.g., debug_unwinders?

>  void
>  _initialize_frame_unwind (void)
>  {
>   frame_unwind_data = gdbarch_data_register_pre_init (frame_unwind_init);
> +
> +  add_prefix_cmd ("unwinder", class_maintenance, set_debug_unwinder_cmd, _("\
> +Set stack frame unwinder debugging variables."),
> +                 &set_debug_unwinder_list, "set debug unwinder ",
> +                 0, &setdebuglist);
> +
> +  add_prefix_cmd ("unwinder", class_maintenance, show_debug_unwinder_cmd, _("\
> +Show stack frame unwinder debugging variables."),
> +                 &show_debug_unwinder_list, "set debug unwinder ",
> +                 0, &showdebuglist);
> +
> +  add_setshow_boolean_cmd ("trace-tried", class_maintenance, &trace_unwinders,
> +                          _("Set tracing of unwinders tried."),
> +                          _("Show tracing of unwinders tried."),
> +                          NULL /* main doc */,
> +                          NULL, NULL,
> +                          &set_debug_unwinder_list, &show_debug_unwinder_list);
>  }

Adding another level of prefixes just for debugging the unwinder seems
excessive.

Re: [PATCH 2/2] Add "set/show debug unwinder" prefix commands.

[Samuel Bronson]

On Sat, May 30, 2009 at 3:24 PM, Doug Evans <dje@google.com> wrote:
> On Fri, May 29, 2009 at 4:16 PM, Samuel Bronson <naesten@gmail.com> wrote:
>> Also add one subcommand for tracing the sniffing of stack frames by
>> unwinders.
>>
>> [...]
>> +/* trace unwinders called */
>> +static int trace_unwinders;
>
> [N.B. None of this is binding.  Just "fwiw".]
>
> Just some nits:
>
> I don't care *too* much about the naming, but I do care about
> following existing conventions and gdb seems to have a pretty
> consistent convention of using "debug" in these situations instead of
> "trace".  E.g., debug_unwinders?

Ah. Didn't realize it made much difference for static vars.

> Adding another level of prefixes just for debugging the unwinder seems
> excessive.

Well, I had been planning to add another subcommand to trace the
activities of the dawrf2 unwinder. I did start it, but then I
discovered that the CFI for the code I was wondering about was being
left out of the kernel image due to an incorrect linker script. I
still think it might be useful to be able to add others, but I'm not
going to be stubborn about it.

Re: [PATCH 1/2] Add an undwinder_name field to "struct frame_unwind" for use by "info frame".

[Tom Tromey]

>>>>> "Samuel" == Samuel Bronson <naesten@gmail.com> writes:

Samuel> Have "info frame" display this field when present.
Samuel> Fill in the field for unwinders (besides the sentinel) which are
Samuel> normally built on i386 Linux.

Funnily enough, I wanted this yesterday :)

This mostly looks ok.  It needs a ChangeLog entry.  And I have some
formatting nits...

Samuel> +const char *get_frame_unwinder_name (struct frame_info *frame)

Newline after the first "*".

Samuel> +    unwinder = get_frame_unwinder_name(fi);

Space before the open paren.

Also, do you have copyright assignment papers in place?  Those are a
prerequisite for us to accept a non-trivial patch.

Tom

Re: [PATCH 2/2] Add "set/show debug unwinder" prefix commands.

[Tom Tromey]

>>>>> "Samuel" == Samuel Bronson <naesten@gmail.com> writes:

Samuel> Also add one subcommand for tracing the sniffing of stack
Samuel> frames by unwinders.

This is also basically ok; just needs a ChangeLog and some formatting
fixlets, nothing serious.

Samuel> +/* trace unwinders called */
Samuel> +static int trace_unwinders;

In the GNU style, comments should be sentences, so should start with a
capital letter and end with a period followed by two spaces.  Yes,
we're that nit picky ;)

There are a few instances of this.

Samuel> +  if (trace_unwinders) {
Samuel> +    fprintf_unfiltered (gdb_stdlog, "[ Searching for unwinder for frame ...\n");
Samuel> +  }

Drop the braces.

Samuel>    for (entry = table->list; entry != NULL; entry = entry->next)
Samuel>      {
Samuel>        struct cleanup *old_cleanup;
Samuel> +      const char *uname = entry->unwinder->unwinder_name;
Samuel> +
Samuel> +      if (trace_unwinders) {

The opening brace is misplaced here, in the GNU style it goes on the
next line.  There are a couple instances of this.

Samuel> +  add_setshow_boolean_cmd ("trace-tried", class_maintenance, &trace_unwinders,

I think this could just be "set debug unwinder", rather than
"set debug unwinder trace-tried".

If you do plan to send another "debug unwinder" sub-command, maybe we
could discuss the naming now, before the patch.

Tom

Re: [PATCH 1/2] Add an undwinder_name field to "struct frame_unwind" for use by "info frame".

[Samuel Bronson]

At Thu, 04 Jun 2009 15:59:57 -0600,
Tom Tromey wrote:
> 
> >>>>> "Samuel" == Samuel Bronson <naesten@gmail.com> writes:
> 
> Samuel> Have "info frame" display this field when present.
> Samuel> Fill in the field for unwinders (besides the sentinel) which are
> Samuel> normally built on i386 Linux.
> 
> Funnily enough, I wanted this yesterday :)
> 
> This mostly looks ok.  It needs a ChangeLog entry.  And I have some
> formatting nits...

Hmm. How is that different from the thing above the patch (aka "commit
message")?
 
> Samuel> +const char *get_frame_unwinder_name (struct frame_info *frame)
> 
> Newline after the first "*".

Ah.

> Samuel> +    unwinder = get_frame_unwinder_name(fi);
> 
> Space before the open paren.

Yeah, I've noticed that you always put those in and have been doing so
myself more recently.

> Also, do you have copyright assignment papers in place?  Those are a
> prerequisite for us to accept a non-trivial patch.

Ah, no, not yet. I assume this doesn't qualify?  I'd be fairly happy
to do so, my concerns at this point being:
 
  a) what if I write some fairly generic code and end up wanting to BSD it?

  b) how do I know you aren't going to add front- or back-cover texts
     or invariant sections to the manual?

Re: [PATCH 2/2] Add "set/show debug unwinder" prefix commands.

[Samuel Bronson]

At Thu, 04 Jun 2009 16:05:06 -0600,
Tom Tromey wrote:
> 
> >>>>> "Samuel" == Samuel Bronson <naesten@gmail.com> writes:
> 
> Samuel> Also add one subcommand for tracing the sniffing of stack
> Samuel> frames by unwinders.
> 
> This is also basically ok; just needs a ChangeLog and some formatting
> fixlets, nothing serious.
> 
> Samuel> +/* trace unwinders called */
> Samuel> +static int trace_unwinders;
> 
> In the GNU style, comments should be sentences, so should start with a
> capital letter and end with a period followed by two spaces.  Yes,
> we're that nit picky ;)

Strange definition of sentence you have at GNU.

> There are a few instances of this.
> 
> Samuel> +  if (trace_unwinders) {
> Samuel> +    fprintf_unfiltered (gdb_stdlog, "[ Searching for unwinder for frame ...\n");
> Samuel> +  }

Point, even the Linux style guide says you only need those if you want
to match the other body in an if/else, or are in the midst of an
if/else-if/.../else chain...

> Samuel>    for (entry = table->list; entry != NULL; entry = entry->next)
> Samuel>      {
> Samuel>        struct cleanup *old_cleanup;
> Samuel> +      const char *uname = entry->unwinder->unwinder_name;
> Samuel> +
> Samuel> +      if (trace_unwinders) {
> 
> The opening brace is misplaced here, in the GNU style it goes on the
> next line.  There are a couple instances of this.

Grrr... why did RMS have to abandon the prophets Kernighan and
Ritchie?

> Samuel> +  add_setshow_boolean_cmd ("trace-tried", class_maintenance, &trace_unwinders,
> 
> I think this could just be "set debug unwinder", rather than
> "set debug unwinder trace-tried".
>
> If you do plan to send another "debug unwinder" sub-command, maybe we
> could discuss the naming now, before the patch.

Actually, I ended up deciding the one I was thinking of wouldn't be
that useful, especially when I figured out it was the kernel link
process that was causing my actual problem, so I guess "set debug
unwinder" would be fine.

Re: [PATCH 1/2] Add an undwinder_name field to "struct frame_unwind" for use by "info frame".

[Tom Tromey]

>>>>> "Samuel" == Samuel Bronson <naesten@gmail.com> writes:

Tom> This mostly looks ok.  It needs a ChangeLog entry.  And I have some
Tom> formatting nits...

Samuel> Hmm. How is that different from the thing above the patch (aka "commit
Samuel> message")?
 
There's an extensive, nit-picky standard for ChangeLog formatting.

http://www.gnu.org/prep/standards/standards.html#Change-Logs

A lot of the style manual is worth reading, just because we review
patches for conformance to it.

[ assignment ]
Samuel> Ah, no, not yet. I assume this doesn't qualify?  I'd be fairly
Samuel> happy to do so, my concerns at this point being:
 
I will get you started.  We can't answer your questions, but you can
ask the FSF assignment clerk and see what he says.

Tom

Re: [PATCH 2/2] Add "set/show debug unwinder" prefix commands.

[Tom Tromey]

>>>>> "Samuel" == Samuel Bronson <naesten@gmail.com> writes:

Samuel> Strange definition of sentence you have at GNU.

:)

Samuel> Point, even the Linux style guide says you only need those if you want
Samuel> to match the other body in an if/else, or are in the midst of an
Samuel> if/else-if/.../else chain...

Yeah.  gdb has used a different style since time immemorial.

http://www.gnu.org/prep/standards/standards.html#Writing-C

Tom