[PATCH] Add support to control auto-display behavior

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] Add support to control auto-display behavior

clayne (Bugzilla)
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  <[hidden email]>

        * 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  <[hidden email]>

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

-cl

gdb.patch (7K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Eli Zaretskii
> Date: Fri, 6 Apr 2007 04:04:35 -0700
> From: Christopher Layne <[hidden email]>
>
> 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.
Reply | Threaded
Open this post in threaded view
|

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

Daniel Jacobowitz-2
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 <[hidden email]>
> >
> > 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
Reply | Threaded
Open this post in threaded view
|

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

clayne (Bugzilla)
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
Reply | Threaded
Open this post in threaded view
|

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

Daniel Jacobowitz-2
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
Reply | Threaded
Open this post in threaded view
|

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

clayne (Bugzilla)
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
Reply | Threaded
Open this post in threaded view
|

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

Daniel Jacobowitz-2
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
Reply | Threaded
Open this post in threaded view
|

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

Eli Zaretskii
In reply to this post by clayne (Bugzilla)
> Date: Sat, 7 Apr 2007 13:20:45 -0700
> From: Christopher Layne <[hidden email]>
> Cc: Daniel Jacobowitz <[hidden email]>
>
> 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.