This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [patch][python] 2 of 5 - Frame filter MI code changes.


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
> 


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]