This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [patch][python] 2 of 5 - Frame filter MI code changes.
- From: Phil Muldoon <pmuldoon at redhat dot com>
- To: Tom Tromey <tromey at redhat dot com>
- Cc: "gdb-patches at sourceware dot org" <gdb-patches at sourceware dot org>
- Date: Fri, 07 Dec 2012 13:55:58 +0000
- Subject: Re: [patch][python] 2 of 5 - Frame filter MI code changes.
- References: <50B8C333.4070008@redhat.com> <87ip8gtoex.fsf@fleche.redhat.com>
On 12/05/2012 05:11 PM, Tom Tromey wrote:
>>>>>> "Phil" == Phil Muldoon <pmuldoon@redhat.com> writes:
> Phil> +/* True if we want to allow Python-based frame filters. */
> Phil> +static int frame_filters = 0;
> Phil> +
> Phil> +void
> Phil> +stack_enable_frame_filters (void)
> Phil> +{
> Phil> + frame_filters = 1;
> Phil> +}
>
> I don't think you need this function, see below.
>
> Phil> +static int
> Phil> +parse_no_frames_option (char *arg)
> Phil> +{
> Phil> + if (arg && (strcmp (arg, "--no-frame-filters") == 0))
> Phil> + return 1;
> Phil> +
> Phil> + return 0;
>
> I'd prefer it if the various callers were changed to use mi_getopt.
> This provides uniformity and lets us add options later.
If there was uniformity then I would agree, but as far as I looked
there wasn't. Some MI commands use mi_getopt, some parse their own
options, some allow long options ("--"), others do not, and mi_getopt
does not handle long options in any case (and huge amounts of other
useful getopt functions too). I wrote a patch for mi_getopts to
handle long options, but why do we even need another implementation of
getopt like functionality?
So I decided to just leave be, and parse options as each command has
previously done so. Maybe I should have written a cleanup patch
before hand.
I wanted to mention something else about MI. I recently discovered in
the GDB manual that -stack-list-locals, -stack-list-arguments are
considered depreciated. Not even sure if we should add frame filter
logic to them. What do you think?
> Phil> + if (! raw_arg && frame_filters)
> Phil> + {
> Phil> + int count = frame_high;
> Phil> + int flags = PRINT_LEVEL | PRINT_FRAME_INFO;
> Phil> +
> Phil> + if (frame_high != -1)
> Phil> + count = (frame_high - frame_low) + 1;
> Phil> +
> Phil> + result = apply_frame_filter (fi, flags, 0, NULL, current_uiout,
> Phil> + count);
>
> I don't think I follow the high/low logic here.
>
> How does this code strip off the first 'frame_low' frames?
fi is unwound to the position of frame_low in a loop preceding this
call. This is existing code, and not in the patch context. It is as
follows:
/* Let's position fi on the frame at which to start the
display. Could be the innermost frame if the whole stack needs
displaying, or if frame_low is 0. */
for (i = 0, fi = get_current_frame ();
fi && i < frame_low;
i++, fi = get_prev_frame (fi));
>
> Also, Do frame_low and frame_high refer to "raw" or "cooked" frames?
> I tend to think they should refer to cooked ones, but I think at least
> the answer should be explicit and documented.
In the existing mi sense, they just refer to frames on the stack. I
followed this logic, but something I am still unsure of is if a frame
is elided between frame low, and frame high, if that should be
counted. I think it should.
> Phil> void
> Phil> +mi_cmd_enable_frame_filters (char *command, char **argv, int argc)
> Phil> +{
> Phil> + if (argc != 0)
> Phil> + error (_("-enable-frame-filters: no arguments allowed"));
> Phil> +
> Phil> + stack_enable_frame_filters ();
>
> I think just put this into mi-cmd-stack.c and remove
> stack_enable_frame_filters.
I was curious about this, I just followed how pretty printing is done.
I have no objection though.
Cheers,
Phil