From: Phil Muldoon <pmuldoon@redhat.com>
To: Tom Tromey <tromey@redhat.com>
Cc: "gdb-patches@sourceware.org" <gdb-patches@sourceware.org>
Subject: Re: [patch][python] 2 of 5 - Frame filter MI code changes.
Date: Tue, 05 Feb 2013 12:08:00 -0000 [thread overview]
Message-ID: <5110F64C.2040907@redhat.com> (raw)
In-Reply-To: <87sj7dd3i7.fsf@fleche.redhat.com>
On 12/10/2012 09:02 PM, Tom Tromey wrote:
>>>>>> "Phil" == Phil Muldoon <pmuldoon@redhat.com> writes:
>
> Tom> I'd prefer it if the various callers were changed to use mi_getopt.
> Tom> This provides uniformity and lets us add options later.
>
> Phil> If there was uniformity then I would agree, but as far as I looked
> Phil> there wasn't. Some MI commands use mi_getopt, some parse their own
> Phil> options, some allow long options ("--"), others do not, and mi_getopt
> Phil> does not handle long options in any case (and huge amounts of other
> Phil> useful getopt functions too). I wrote a patch for mi_getopts to
> Phil> handle long options, but why do we even need another implementation of
> Phil> getopt like functionality?
>
> Ok, I see the problem now.
>
> Right now, though. -stack-list-frames doesn't take any options.
> I think it would be better if commands like this were converted to use
> mi_getopt instead of doing the parsing by hand.
> So I suggest having all the functions use a short option name.
>
> I think the changes like in -stack-list-locals are strange, too, in that
> they make options order-dependent. This will be surprising to users.
I spent the last couple of weeks trying to fix this, and I have
reached a dead end with your review requirements.
First, these commands are already option placement dependent. Lets
look at an example before the frame filters patch:
mi_cmd_stack_list_args:
stack-list-arguments: Usage: PRINT_VALUES [FRAME_LOW FRAME_HIGH]
You can see in that command PRINT_VALUES is mandatory and is an
option. Its placement in the argument order is dependent on being
the first argument. The existing MI code already expects this to be
at argv[0].
Here is an example instantiation of this command for a slice of
frames:
-stack-list-arguments --all-values 0 24
Or this for the whole backtrace:
-stack-list-arguments --all-values
If these were the only cases this would be fine. However the commands
highlighted in the review also accept integer representations for
options (and also why order placement is important to this (and other)
commands):
-stack-list-arguments 1 0 24
Where the first "1" is an integer value replacement for the
--all-values option. Some MI commands, for whatever reason, allow
these integer replacements. (As an aside, some MI commands parse
their own argv and opt-out of using mi_getopt for this reason).
So this is all ok so far. I patched mi_getopt locally to process
"--foo" style options. This would still work:
-stack-list-arguments --no-frame-filters 1 0 24
But this would not:
-stack-list-arguments 1 --no-frame-filters 0 24
mi_getopt (quite rightly) detects the "1" as not an option and returns
-1 as an indication that we have reached the end of the options in the
command line. At this point --no-frame-filters has not been
processed). I followed the logic that if the review does not want
option placement dependency, then all options should not be placement
dependent.
I thought about running a substitution for the command line that
would substitute integer options with their long equivalent (IE,
substitute 1 for --all-values). I am uncomfortable with this as it
reallocates the size of argv for every command that allows integer
option substitution.
I also thought about just running mi_getopt manually on every index in
argv and ignoring the return value, thereby forcing mi_getopt to
process each element in the argv array. The problem with this is then
I lose access to oind which indicates the end of the arguments, and
the beginning of the rest of the command line (in the examples case,
the slice).
The next thing I thought about was just removing integer option
replacement. That turned out to be a non-starter as we would lose MI
compatibility with previous versions.
So I'm at a loss. I do not know how to write this code with the
review requirements that allows:
-- options not being order dependent;
-- not heaping more hacks (like -- argv pre-substitution) on top of
existing bad behavior.
This is all somewhat tangential to the frame filters patch. While I
do not mind fixing issues as I go, this is becoming an issue far more
contentious than the patch content.
What do you think we should do?
Cheers,
Phil
>
> Phil> I wanted to mention something else about MI. I recently discovered in
> Phil> the GDB manual that -stack-list-locals, -stack-list-arguments are
> Phil> considered depreciated. Not even sure if we should add frame filter
> Phil> logic to them. What do you think?
>
> I think we still ought to. It doesn't seem like much work and it is
> friendlier in case some MI user hasn't updated.
>
> Tom> I don't think I follow the high/low logic here.
>
> Tom> How does this code strip off the first 'frame_low' frames?
>
> Phil> fi is unwound to the position of frame_low in a loop preceding this
> Phil> call. This is existing code, and not in the patch context.
>
> Tom> Also, Do frame_low and frame_high refer to "raw" or "cooked" frames?
> Tom> I tend to think they should refer to cooked ones, but I think at least
> Tom> the answer should be explicit and documented.
>
> Phil> In the existing mi sense, they just refer to frames on the stack. I
> Phil> followed this logic, but something I am still unsure of is if a frame
> Phil> is elided between frame low, and frame high, if that should be
> Phil> counted. I think it should.
>
> I see. I think this is wrong then. I think the arguments have to be
> applied after filtering. Here's why:
>
> One use for these arguments to the stack commands is so that a GUI can
> display just part of the stack, and then, if the user scrolls the
> display, it can request more of the stack to show. This way the GUI
> doesn't have to unwind the whole stack just to show the first 5 lines or
> whatever.
>
> Suppose you have a frame filter that notices a sentinel frame and then
> elides the following 3 frames. This isn't necessarily an obscure
> situation, maybe some interpreter takes a few function calls to
> implement a call in the interpreted language and the filter wants to
> collapse those frames to just show what is happening in the interpreted
> code.
>
> Suppose this sentinel frame occurs at stack position 3.
>
> Now suppose the MI client requests frames 0..4. Ok, the filter will
> work (since it can request more frames than the MI client did) and will
> present the right thing.
>
> But now the user scrolls the window and the MI client requests frames
> starting at frame 5.
>
> Whoops -- the output will be inconsistent; the filter won't trigger
> because the sentinel appears at frame 3, which the existing code already
> iterated past.
>
> Tom
>
next prev parent reply other threads:[~2013-02-05 12:08 UTC|newest]
Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-11-30 14:31 Phil Muldoon
2012-12-05 17:11 ` Tom Tromey
2012-12-07 13:56 ` Phil Muldoon
2012-12-10 21:03 ` Tom Tromey
2013-02-05 12:08 ` Phil Muldoon [this message]
2013-02-07 21:32 ` Tom Tromey
2013-02-20 15:17 ` Phil Muldoon
2013-02-20 20:37 ` Tom Tromey
2013-03-11 22:13 Phil Muldoon
2013-03-12 20:43 ` Tom Tromey
2013-03-12 20:52 ` Phil Muldoon
2013-03-13 12:15 ` Phil Muldoon
2013-03-13 17:48 ` Tom Tromey
2013-03-13 19:41 ` Phil Muldoon
2013-03-13 20:27 ` Tom Tromey
2013-03-13 20:53 ` Phil Muldoon
2013-03-13 20:56 ` Tom Tromey
2013-03-13 21:10 ` Phil Muldoon
2013-03-14 17:54 ` Tom Tromey
2013-03-14 19:35 ` Phil Muldoon
2013-04-22 16:01 Phil Muldoon
2013-04-26 11:19 ` Tom Tromey
2013-05-06 8:23 Phil Muldoon
2013-05-06 20:42 ` Tom Tromey
2013-05-07 8:23 ` Phil Muldoon
2013-05-07 14:02 ` Tom Tromey
2013-05-08 10:18 ` Phil Muldoon
2013-05-08 19:47 ` Tom Tromey
2013-05-10 10:45 ` Phil Muldoon
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5110F64C.2040907@redhat.com \
--to=pmuldoon@redhat.com \
--cc=gdb-patches@sourceware.org \
--cc=tromey@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox