Re: [PATCHv4] gdb: Change how frames are selected for 'frame' and 'info frame'.

[Andrew Burgess <andrew.burgess@embecosm.com>]

* Philippe Waroquiers <philippe.waroquiers@skynet.be> [2018-07-23 22:45:55 +0200]:

> On Tue, 2018-07-17 at 16:57 +0100, Andrew Burgess wrote:
> > Now that the 'frame apply' command has been merged to master, this is
> > a rebase of the v3 patch.  Merge conflicts have been resolved, but the
> > content is largely unchanged.
> FWIW, I like the idea of the patch, as it is too easy to
> (too silently) get a wrong frame with 'frame <a wrong frame level/number>'.
> 
> Find below some comments to discuss the terminology 'frame level'
> versus 'frame number'.
> 
> Philippe
> 
> > 
> > Thanks,
> > Andrew
> > 
> > ---
> > 
> > The 'frame' command, and thanks to code reuse the 'info frame' and
> > 'select-frame' commands, currently have an overloaded mechanism for
> > selecting a frame.
> > 
> > These commands take one or two parameters, if it's one parameter then
> > we first try to use the parameter as an integer to select a frame by
> > level (or depth in the stack).  If that fails then we treat the
> > parameter as an address and try to select a stack frame by
> > stack-address.  If we still have not selected a stack frame, or we
> > initially had two parameters, then GDB allows the user to view a stack
> > frame that is not part of the current backtrace.  Internally, a new
> > frame is created with the given stack and pc addresses, and this is
> > shown to the user.
> > 
> > The result of this is that a typo by the user, entering the wrong stack
> > frame level for example, can result in a brand new frame being viewed
> > rather than an error.
> > 
> > The purpose of this commit is to remove this overloading, while still
> > offering the same functionality through some new sub-commands.  By
> > making the default behaviour of 'frame' (and friends) be to select a
> > stack frame by level number, it is hoped that enough
> > backwards-compatibility is maintained that users will not be overly
> > inconvenienced.
> > 
> > The 'frame', 'select-frame', and 'info frame' commands now all take a
> > frame specification string as an argument, this string can be any of the
> > following:
> > 
> >   (1) An integer.  This is treated as a frame number.  If a frame for
> >   that number does not exist then the user gets an error.
> > 
> >   (2) A string like 'level <NUMBER>', where <NUMBER> is a frame number
> >   as in option (1) above.
> 
> I did a trial to scan the documentation for the usage of 'frame number',
> in order to replace it by 'frame level'.
> Note that Eli has some reserve about this idea/replacement,
> see thread
> https://sourceware.org/ml/gdb-patches/2018-07/msg00365.html.

I seem to have opened a can of works with the choice of level here.

It's been a while since I wrote the original patch (almost 3 years)
so, honestly, I can't recall why I picked 'level' over 'number'.  It
probably seemed more descriptive at the time, but honestly I would be
just as happy to use 'number' instead.

So, what I'd really like is some guidance from .... well .... anyone
who cares really .... level or number ?

Thanks,
Andrew








> 
> IMO, the sentences (1) and (2) above should preferably
> use consistently 'frame level' 'level <LEVEL>' : at the moment,
> we seem to semi-randomly use level or number, explaining at some places
> that level is a frame number. 
> 
> It would be good to clarify exactly what naming convention should
> be used, and apply it (more) systematically (in the existing
> documentation and/or in this patch).
> 
> 
> 
> >   (3) A string like 'address <ADDRESS>', where <ADDRESS> is a
> >   stack-frame address.  If there is no frame for this address then the
> >   user gets an error.
> Below, (5) and (6) are using STACK-ADDRESS instead of ADDRESS.
> I am not sure to understand if there is (or not) a difference of concept
> between this ADDRESS and the below STACK-ADDRESS.
> I am wondering which address we are speaking about.
> When doing 'info frame', I get something like:
>   (gdb) info frame
>   Stack level 1, frame at 0x7fffffffdf80:
>    rip = 0x555555554f5e in sleeper_or_burner (sleepers.c:86); saved rip = 0x55555555549d
>    called by frame at 0x7fffffffe040, caller of frame at 0x7fffffffdf40
>    source language c.
>    Arglist at 0x7fffffffdf70, args: v=0x7fffffffdf90
>    Locals at 0x7fffffffdf70, Previous frame's sp is 0x7fffffffdf80
>    Saved registers:
>     rbp at 0x7fffffffdf70, rip at 0x7fffffffdf78
>   (gdb) 
> Maybe it would be good to reference in the doc the field given
> in the above output ?
> 
> 
> Note that the above command output uses 'Stack level 1',
> and not 'Stack number 1' :).
> 
> 
> > 
> >   (4) A string like 'function <NAME>', where <NAME> is a function name,
> >   the inner most frame for function <NAME> is selected.  If there is no
> >   frame for function <NAME> then the user gets an error.
> > 
> >   (5) A string like 'view <STACK-ADDRESS>', this views a new frame
> >   with stack address <STACK-ADDRESS>.
> > 
> >   (6) A string like 'view <STACK-ADDRESS> <PC-ADDRESS>', this views
> >   a new frame with stack address <STACK-ADDRESS> an the pc <PC-ADDRESS>.
> > 
> > This change assumes that the most common use of the commands like
> > 'frame' is to select a frame by frame level number, it is for this
> > reason that this is the behaviour that is kept for backwards
> > compatibility.  Any of the alternative behaviours, which are assumed to
> > be less used, now require a change in user behaviour.
> > 
> > The MI command '-stack-select-frame' has also changed in the same way.
> > The default behaviour is to select a frame by level number, but the
> > other selection methods are also available.
> 
> In the rest of the patch, assuming there is an agreement about
> using frame level instead of frame number, there are some other
> occurrences of number to replace/clarify.
> 
> Philippe
>