[PATCH] Add support to control auto-display behavior

[PATCH] Add support to control auto-display behavior

[Christopher Layne]

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

This patch is to allow setting how auto-displays are affected when memory is
not accessible. It's very common to, when single-stepping, or stepping over,
initially setup some auto-displays and then begin stepping. I don't see
any reason why, in the event an object is not accessible, the auto-display
should be disabled. I know that the actual error is "To prevent infinite
recursion" but I'm not finding how that will happen based on the already
present code. Is it something from the past?  

Basically, it does the following:

(gdb) b main
Breakpoint 1 at 0x8048371: file null.c, line 5.
(gdb) r

Breakpoint 1, main () at null.c:5
5               char buf[][64] = { "foo", "bar" };
(gdb) set display strict off
(gdb) disp q
(gdb) disp *q
(gdb) n
6               char *q = NULL;
2: *q = -115 '\215'
1: q = 0x8048426 "\215\203 ÿÿÿ\215"
(gdb)
8               q = NULL;
2: *q = Cannot access memory at address 0x0
1: q = 0x0
(gdb)
9               q = buf[0];
2: *q = Cannot access memory at address 0x0
1: q = 0x0
(gdb)
10              q = buf[1];
2: *q = 102 'f'
1: q = 0xbfeec3fc "foo"
(gdb)
11              q = (void *)0xDEAD;
2: *q = 98 'b'
1: q = 0xbfeec43c "bar"
(gdb)
13              return 0;
2: *q = Cannot access memory at address 0xdead
1: q = 0xdead <Address 0xdead out of bounds>
(gdb)


Additionally, in the exercising of implementing it, it also fixes a "bug"
where if a display is disabled, as per default behavior, the rest
of the valid displays will still be displayed. Previously things would
just stop at the first exception caught.

(gdb) set display strict on
(gdb) n
8               q = NULL;
Disabling display 2 to avoid infinite recursion.
2: *q = Cannot access memory at address 0x0
1: q = 0x0


gdb/ChangeLog:

2007-03-30  Christopher Layne  <clayne@anodized.com>

	* cli/cli-cmds.c (setdisplaylist): Define.
	(showdisplaylist): Define.
	(init_cmd_lists): Initialize both.
	* cli/cli-cmds.h (setdisplaylist): Declare.
	(showdisplaylist): Declare.
	* gdbcmd.h (setdisplaylist): Declare.
	(showdisplaylist): Declare.
	* printcmd.c (exceptions.h): Include.
	(display_strict): Define and initialize.
	(show_display_strict): New function.
	(show_display): New function.
	(set_display): New function.
	(do_one_display): Change prototype to be compatible with
	catch_errors(). Change return values to fit new prototype.
	(do_displays): Wrap do_one_display in catch_errors().
	(disable_current_display): Check value of display_strict first.
	(_initialize_printcmd): Add prefix commands for controlling
	auto-displays and value of display_strict. Default display_strict to 1.

gdb/doc/ChangeLog:

2007-03-30  Christopher Layne  <clayne@anodized.com>

	* gdb.texinfo (Display Settings): Add documentation for manipulating
	auto-displays.

-cl

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

diff -U3 -r gdb-6.6/gdb/cli/cli-cmds.c /usr/local/build/gdb-6.6/gdb/cli/cli-cmds.c
--- gdb-6.6/gdb/cli/cli-cmds.c	2006-10-27 15:23:20.000000000 -0700
+++ /usr/local/build/gdb-6.6/gdb/cli/cli-cmds.c	2007-03-30 00:58:40.000000000 -0700
@@ -178,6 +178,10 @@
 
 struct cmd_list_element *showchecklist;
 
+struct cmd_list_element *setdisplaylist;
+
+struct cmd_list_element *showdisplaylist;
+
 /* Command tracing state.  */
 
 int source_verbose = 0;
@@ -1108,6 +1112,8 @@
   showprintlist = NULL;
   setchecklist = NULL;
   showchecklist = NULL;
+  setdisplaylist = NULL;
+  showdisplaylist = NULL;
 }
 
 static void
diff -U3 -r gdb-6.6/gdb/cli/cli-cmds.h /usr/local/build/gdb-6.6/gdb/cli/cli-cmds.h
--- gdb-6.6/gdb/cli/cli-cmds.h	2006-10-27 15:23:20.000000000 -0700
+++ /usr/local/build/gdb-6.6/gdb/cli/cli-cmds.h	2007-03-30 00:57:39.000000000 -0700
@@ -103,6 +103,10 @@
 
 extern struct cmd_list_element *showchecklist;
 
+extern struct cmd_list_element *setdisplaylist;
+
+extern struct cmd_list_element *showdisplaylist;
+
 /* Exported to gdb/top.c */
 
 void init_cmd_lists (void);
diff -U3 -r gdb-6.6/gdb/doc/gdb.texinfo /usr/local/build/gdb-6.6/gdb/doc/gdb.texinfo
--- gdb-6.6/gdb/doc/gdb.texinfo	2006-11-14 13:53:59.000000000 -0800
+++ /usr/local/build/gdb-6.6/gdb/doc/gdb.texinfo	2007-04-06 03:05:16.000000000 -0700
@@ -5391,6 +5391,7 @@
 * Output Formats::              Output formats
 * Memory::                      Examining memory
 * Auto Display::                Automatic display
+* Display Settings::            Display settings
 * Print Settings::              Print settings
 * Value History::               Value history
 * Convenience Vars::            Convenience variables
@@ -5963,6 +5964,32 @@
 automatically.  The next time your program stops where @code{last_char}
 is meaningful, you can enable the display expression once again.
 
+@node Display Settings
+@section Display settings
+
+@cindex display options
+@cindex display settings
+@value{GDBN} provides the following ways to control how auto-displays are
+handled.
+
+@table @code
+@kindex set display
+@item set display strict
+@itemx set display strict on
+@cindex disable/permit auto-displays on memory access error
+@value{GDBN} automatically disables any auto-display which encounters a
+memory access error during display. The default is @code{on}.
+
+@item set display strict off
+Do not disable any auto-display which encounters a memory access error,
+but instead print a general error message rather than displaying a
+non-accessible value.
+
+@kindex show display
+@item show display strict
+Show whether or not auto-displays are disabled on memory access error.
+@end table
+
 @node Print Settings
 @section Print settings
 
diff -U3 -r gdb-6.6/gdb/gdbcmd.h /usr/local/build/gdb-6.6/gdb/gdbcmd.h
--- gdb-6.6/gdb/gdbcmd.h	2006-10-27 15:23:20.000000000 -0700
+++ /usr/local/build/gdb-6.6/gdb/gdbcmd.h	2007-03-30 00:56:48.000000000 -0700
@@ -122,6 +122,10 @@
 
 extern struct cmd_list_element *showchecklist;
 
+extern struct cmd_list_element *setdisplaylist;
+
+extern struct cmd_list_element *showdisplaylist;
+
 extern void execute_command (char *, int);
 
 enum command_control_type execute_control_command (struct command_line *);
diff -U3 -r gdb-6.6/gdb/printcmd.c /usr/local/build/gdb-6.6/gdb/printcmd.c
--- gdb-6.6/gdb/printcmd.c	2006-10-17 13:17:44.000000000 -0700
+++ /usr/local/build/gdb-6.6/gdb/printcmd.c	2007-03-30 03:24:16.000000000 -0700
@@ -43,6 +43,7 @@
 #include "gdb_assert.h"
 #include "block.h"
 #include "disasm.h"
+#include "exceptions.h"		/* to handle auto-display access errors */
 
 #ifdef TUI
 #include "tui/tui.h"		/* For tui_active et.al.   */
@@ -104,6 +105,32 @@
 		    value);
 }
 
+/* Enable disabling of an auto-display which cannot be displayed
+   due to a memory error. */
+static int display_strict = 0;
+static void
+show_display_strict (struct ui_file *file, int from_tty,
+			    struct cmd_list_element *c, const char *value)
+{
+  fprintf_filtered (file, _("\
+Disabling of display on memory access error is %s.\n"),
+		    value);
+}
+
+static void
+set_display (char *arg, int from_tty)
+{
+  printf_unfiltered (
+     "\"set display\" must be followed by the name of a display subcommand.\n");
+  help_list (setdisplaylist, "set display ", -1, gdb_stdout);
+}
+
+static void
+show_display (char *args, int from_tty)
+{
+  cmd_show_list (showdisplaylist, from_tty, "");
+}
+
 /* Number of auto-display expression currently being displayed.
    So that we can disable it if we get an error or a signal within it.
    -1 when not doing one.  */
@@ -148,7 +175,7 @@
 
 /* Prototypes for local functions. */
 
-static void do_one_display (struct display *);
+static int do_one_display (void *);
 \f
 
 /* Decode a format specification.  *STRING_PTR should point to it.
@@ -1468,20 +1495,21 @@
    Do nothing if the display cannot be printed in the current context,
    or if the display is disabled. */
 
-static void
-do_one_display (struct display *d)
+static int
+do_one_display (void *p)
 {
+  struct display *d = p;
   int within_current_scope;
 
   if (d->enabled_p == 0)
-    return;
+    return 0;
 
   if (d->block)
     within_current_scope = contained_in (get_selected_block (0), d->block);
   else
     within_current_scope = 1;
   if (!within_current_scope)
-    return;
+    return 0;
 
   current_display_number = d->number;
 
@@ -1548,6 +1576,8 @@
 
   gdb_flush (gdb_stdout);
   current_display_number = -1;
+
+  return 0;
 }
 
 /* Display all of the values on the auto-display chain which can be
@@ -1559,7 +1589,7 @@
   struct display *d;
 
   for (d = display_chain; d; d = d->next)
-    do_one_display (d);
+    catch_errors (do_one_display, d, "", RETURN_MASK_ALL);
 }
 
 /* Delete the auto-display which we were in the process of displaying.
@@ -1582,6 +1612,7 @@
 void
 disable_current_display (void)
 {
+  if (display_strict == 0) return;
   if (current_display_number >= 0)
     {
       disable_display (current_display_number);
@@ -2165,6 +2196,23 @@
 No argument means cancel all automatic-display expressions.\n\
 Do \"info display\" to see current list of code numbers."), &deletelist);
 
+  add_prefix_cmd ("display", no_class, set_display,
+		  _("Generic command for setting how auto-displays work."),
+		  &setdisplaylist, "set display ", 0, &setlist);
+
+  add_prefix_cmd ("display", no_class, show_display,
+		  _("Generic command for showing auto-display settings."),
+		  &showdisplaylist, "show display ", 0, &showlist);
+
+  add_setshow_boolean_cmd ("strict", class_vars, 
+			   &display_strict, _("\
+Set disabling of display on memory access error."), _("\
+Show disabling of display on memory access error."),
+			   NULL,
+			   NULL,
+			   show_display_strict,
+			   &setdisplaylist, &showdisplaylist);
+
   add_com ("printf", class_vars, printf_command, _("\
 printf \"printf format string\", arg1, arg2, arg3, ..., argn\n\
 This is useful for formatted output in user-defined commands."));
@@ -2267,4 +2315,5 @@
   examine_w_type = init_type (TYPE_CODE_INT, 4, 0, "examine_w_type", NULL);
   examine_g_type = init_type (TYPE_CODE_INT, 8, 0, "examine_g_type", NULL);
 
+  display_strict = 1;
 }

Re: [PATCH] Add support to control auto-display behavior

[Eli Zaretskii]

> Date: Fri, 6 Apr 2007 04:04:35 -0700
> From: Christopher Layne <clayne@anodized.com>
> 
> This patch is to allow setting how auto-displays are affected when memory is
> not accessible. It's very common to, when single-stepping, or stepping over,
> initially setup some auto-displays and then begin stepping. I don't see
> any reason why, in the event an object is not accessible, the auto-display
> should be disabled.

Thanks.  I agree that it's a good idea to have an option to control
this, unless we agree that what you want as an option should be how
GDB behaves in all cases.

I have a few comments for your patch.

> 	* printcmd.c (exceptions.h): Include.

A minor formatting issue: exceptions.h should not be in parentheses,
since it's not a function or macro.  Instead, use this:

	* printcmd.c: Include exceptions.h.

> +@cindex display options
> +@cindex display settings

It is better to have this as a single index entry, because they both
begin with the same string and both point to the same place in the
manual.  So having two of them is mostly redundant.  How about this
instead:

  @cindex display, optional settings

> +@value{GDBN} provides the following ways to control how auto-displays are
> +handled.

This sentence is better ended with a colon, don't you think?

> +@cindex disable/permit auto-displays on memory access error

When users look for such features in the manual, they don't think of
"disable" as the first word, I think.  They think about
"auto-display".  So I would change this index entry to say this
instead:

  @cindex auto-display, and memory access errors

> +@value{GDBN} automatically disables any auto-display which encounters a
> +memory access error during display. The default is @code{on}.
                                      ^
Two spaces after a period, please.

> +@item set display strict off
> +Do not disable any auto-display which encounters a memory access error,
> +but instead print a general error message rather than displaying a
> +non-accessible value.

How can one display a non-accessible value?

I'd rephrase the whole section as follows:

    +@table @code
    +@kindex set display
    +@item set display strict
    +@itemx set display strict on
    +@cindex disable/permit auto-displays on memory access error
    +When auto-displays is set to @samp{strict}, @value{GDBN}
    +automatically disables any auto-display that encounters a memory
    +access error during display. This is the default behavior.
    +
    +@item set display strict off
    +When strict display is turned off, @value{GDBN} prints an error
    +message, but does not disable an auto-display that encounters a memory
    +access error.

Btw, is it a good idea to call this ``strict'' display?  I wouldn't
characterize the current behavior as ``strict''.  How about "set
display disable-on-error on/off" instead?

> +  add_setshow_boolean_cmd ("strict", class_vars, 
> +			   &display_strict, _("\
> +Set disabling of display on memory access error."), _("\
> +Show disabling of display on memory access error."),

I'd prefer more help text here to explain in more detail what happens
under each setting.

Re: [PATCH] Add support to control auto-display behavior

[Daniel Jacobowitz]

On Fri, Apr 06, 2007 at 04:52:59PM +0300, Eli Zaretskii wrote:
> > Date: Fri, 6 Apr 2007 04:04:35 -0700
> > From: Christopher Layne <clayne@anodized.com>
> > 
> > This patch is to allow setting how auto-displays are affected when memory is
> > not accessible. It's very common to, when single-stepping, or stepping over,
> > initially setup some auto-displays and then begin stepping. I don't see
> > any reason why, in the event an object is not accessible, the auto-display
> > should be disabled.
> 
> Thanks.  I agree that it's a good idea to have an option to control
> this, unless we agree that what you want as an option should be how
> GDB behaves in all cases.

I think changing the behavior unconditionally would be a good idea.
Showing the error is more sensible; it's always bugged me that my
displays get turned off (and sometimes deleted instead of disabled - I
do not know why that happens).

-- 
Daniel Jacobowitz
CodeSourcery

Re: [PATCH] Add support to control auto-display behavior

[Christopher Layne]

On Sat, Apr 07, 2007 at 01:25:39PM -0400, Daniel Jacobowitz wrote:
> On Fri, Apr 06, 2007 at 04:52:59PM +0300, Eli Zaretskii wrote:
> > > initially setup some auto-displays and then begin stepping. I don't see
> > > any reason why, in the event an object is not accessible, the auto-display
> > > should be disabled.
> > 
> > Thanks.  I agree that it's a good idea to have an option to control
> > this, unless we agree that what you want as an option should be how
> > GDB behaves in all cases.
> 
> I think changing the behavior unconditionally would be a good idea.
> Showing the error is more sensible; it's always bugged me that my
> displays get turned off (and sometimes deleted instead of disabled - I
> do not know why that happens).

My thinking is the same as well - it bugged me enough that I decided to
do something about it. Kinda funny, I went on a research hunt for exactly
where the line of output "avoid infinite recursion" had been added, hoping
to find some rationale, and it looks like it's been there since the early 90s.

I originally had it as "disable-on-error" but changed it to "strict" and
non-default as I wasn't sure if the original intended behavior had a real
reason for being there. If it's okay with you guys, I'll restructure the
patch to fix some of the documentation issues and semantics to assume it
as default behavior.

-cl

Re: [PATCH] Add support to control auto-display behavior

[Daniel Jacobowitz]

On Sat, Apr 07, 2007 at 01:20:45PM -0700, Christopher Layne wrote:
> I originally had it as "disable-on-error" but changed it to "strict" and
> non-default as I wasn't sure if the original intended behavior had a real
> reason for being there. If it's okay with you guys, I'll restructure the
> patch to fix some of the documentation issues and semantics to assume it
> as default behavior.

I'd recommend hanging on a few days and seeing what others think.  I
am actually against the knob - I think GDB is developing too many user
interface knobs because we (the developers) can't make up our minds,
and there's a point at which additional customizability does more harm
than good.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [PATCH] Add support to control auto-display behavior

[Christopher Layne]

On Sat, Apr 07, 2007 at 06:18:41PM -0400, Daniel Jacobowitz wrote:
> I'd recommend hanging on a few days and seeing what others think.  I
> am actually against the knob - I think GDB is developing too many user
> interface knobs because we (the developers) can't make up our minds,
> and there's a point at which additional customizability does more harm
> than good.

Sure. I also didn't think a knob was needed for the behavior - but also
considered how it may affect gdb output scrapers / automating tools if
something changed, so just went ahead and made it one.

-cl

Re: [PATCH] Add support to control auto-display behavior

[Daniel Jacobowitz]

On Sat, Apr 07, 2007 at 03:27:15PM -0700, Christopher Layne wrote:
> Sure. I also didn't think a knob was needed for the behavior - but also
> considered how it may affect gdb output scrapers / automating tools if
> something changed, so just went ahead and made it one.

This is a reasonable concern, and I'm glad you thought about it.  But
I have a very simple belief: at one point, parsing the GDB CLI's
output was the only reasonable way to control it, but that hasn't been
true for years.  So, I'm worried about compatibility where it will
affect GDB/MI and where it will confuse human beings, but not where it
will change the CLI output confusingly to programs.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [PATCH] Add support to control auto-display behavior

[Eli Zaretskii]

> Date: Sat, 7 Apr 2007 13:20:45 -0700
> From: Christopher Layne <clayne@anodized.com>
> Cc: Daniel Jacobowitz <drow@false.org>
> 
> If it's okay with you guys, I'll restructure the patch to fix some
> of the documentation issues and semantics to assume it as default
> behavior.

If in a few days no one objects, please do that.