Re: [PATCHv5 0/2] gdb: Change how frames are selected for 'frame' and 'info frame'.

[Philippe Waroquiers <philippe.waroquiers@skynet.be>]

On Mon, 2018-08-13 at 23:20 +0100, Andrew Burgess wrote:
> Given a lack of feedback on v4, I've put together two alternative
> versions to pick between...
> 
>   (A) In this version I took onboard the feedback from Eli and
>       Philippe that using "level" might be confusing to users, as
>       "number" is historically what has been used for the integer
>       label we give to frames.

What I find confusing is to have the same concept
('frame something_identifying_a_frame')
be called sometimes 'frame number' and sometimes 'frame level'.

My personal preference is to use 'frame level' because:
  * IMO, frame level better matches a stack/backtrace concept than the
    very general frame number (and number is overloaded e.g.
    in 'number of frames').
  * frame number is used for another concept (traceframe number,
    but without always using trace word.  E.g. the MI interface uses
    frame-number for 'frame something_identifying_a_traceframe'),
    which has (IIUC) not much to do with a backtrace frame.
  * there are a lot more usages of 'frame level' that 'frame number'
    (see below the result of analysing some grep 'frame/number/level'
    on the code and on the documentation).
    From what I can see, we have less than 20 occurrences of using
    number for a frame 'something', and we have a lot more places (80?)
    where the wording level is used for a frame 'something'
    (about 20 being the usage of frame_relative_level function).
  * I think we can (mostly?) fix all the inconsitencies by
    switching to frame level for a backtrace frame something,
    while I think it is (mostly?) desperate to fix the inconsistencies
    created by 'frame number' (due e.g. to MI using frame number for
    trace frame number').
    

> 
>       The command for selecting a frame by integer is now, 'frame
>       number <NUMBER>' and almost all references to "level" have now
>       been removed from the patch.
> 
>   (B) In this version I've taken onboard the advice from Philippe,
>       identifying places in the patch where I was mixing use of
>       "level" and "number".  I've doubled down on "level" and removed
>       most uses of "number" from the patch.
> 
> If we select patch 'A' then me might want to update 'frame apply level
> ...' to 'frame apply number ...' for consistency.
As indicated above, I prefer 'frame level', but no problem for me
to do a patch to change 'frame apply level' to 'frame apply number'
if that is what is decided).

> 
> If we select patch 'B' then we will probably need a follow up patch
> that goes through the documentation to tighten up references to
> "number" vs "level".
> 
> I really don't mind which approach we take, I guess I'd probably pick
> 'B' over 'A' given we already have 'frame apply level ...' in GDB, but
> if there's preference for 'A' then that's fine.
> 
> I'm also happy to do the follow up patches once we have some agreement
> on which way to go.
> 
> Thanks,
> Andrew
> 
> ---
> 
> Andrew Burgess (1):
>   gdb: Change how frames are selected for 'frame' and 'info frame'.
> 
>  gdb/ChangeLog                               |  36 ++
>  gdb/NEWS                                    |   8 +
>  gdb/cli/cli-decode.c                        |  44 ++-
>  gdb/command.h                               |  14 +
>  gdb/doc/ChangeLog                           |   8 +
>  gdb/doc/gdb.texinfo                         | 108 ++++--
>  gdb/mi/mi-cmd-stack.c                       |   4 +-
>  gdb/stack.c                                 | 535 +++++++++++++++++++---------
>  gdb/stack.h                                 |   2 +-
>  gdb/testsuite/ChangeLog                     |   7 +
>  gdb/testsuite/gdb.base/frame-selection.c    |  52 +++
>  gdb/testsuite/gdb.base/frame-selection.exp  | 157 ++++++++
>  gdb/testsuite/gdb.mi/mi-frame-selection.c   |  34 ++
>  gdb/testsuite/gdb.mi/mi-frame-selection.exp |  89 +++++
>  14 files changed, 896 insertions(+), 202 deletions(-)
>  create mode 100644 gdb/testsuite/gdb.base/frame-selection.c
>  create mode 100644 gdb/testsuite/gdb.base/frame-selection.exp
>  create mode 100644 gdb/testsuite/gdb.mi/mi-frame-selection.c
>  create mode 100644 gdb/testsuite/gdb.mi/mi-frame-selection.exp
> 

Usages of level:
---------------

It is used in the info frame output:
(gdb) info frame
Stack level 1, frame at 0x7fffffffdf90:
 rip = 0x555555554f5e in sleeper_or_burner (sleepers.c:86); saved rip = 0x55555555549d
 called by frame at 0x7fffffffe050, caller of frame at 0x7fffffffdf50
 source language c.
 Arglist at 0x7fffffffdf80, args: v=0x7fffffffdfa0
 Locals at 0x7fffffffdf80, Previous frame's sp is 0x7fffffffdf90
 Saved registers:
  rbp at 0x7fffffffdf80, rip at 0x7fffffffdf88
(gdb)

infcmd.c:2023:      /* Print info on the selected frame, including level number but not
stack.c:2192:/* Find a frame a certain number of levels away from FRAME.
ada-lang.h:57:/* The maximum number of frame levels searched for non-local,
ada-lang.h:60:#define MAX_ENCLOSING_FRAME_LEVELS 7
defs.h:595:extern void (*selected_frame_level_changed_hook) (int);
    Obsolete ? I cannot find any reference to selected_frame_level_changed_hook.
extension.h:88:    /* Set this flag if frame level is to be printed.  */

frame.h:502:/* The frame's level: 0 for innermost, 1 for its caller, ...; or -1
frame.h:504:extern int frame_relative_level (struct frame_info *fi);
gdbthread.h:640:  int m_selected_frame_level;
stack.h:23:void select_frame_command (const char *level_exp, int from_tty);
ada-lang.c:12221:  int frame_level;
ada-lang.c:12230:  for (frame_level = 0; frame_level < 3; frame_level += 1)
ada-tasks.c:1339:                     frame_relative_level (get_selected_frame (NULL)),
amd64-tdep.c:2852:  if (frame_relative_level (this_frame) == 0)
annotate.c:435:annotate_frame_begin (int level, struct gdbarch *gdbarch, CORE_ADDR pc)
arm-tdep.c:2778:  if (frame_relative_level (this_frame) == 0)
dwarf2-frame-tailcall.c:175:  int retval = (frame_relative_level (this_frame)
dwarf2-frame-tailcall.c:176:		- frame_relative_level (cache->next_bottom_frame) - 1);

frame.c:87:  /* Level of this frame.  The inner-most (youngest) frame is at level
frame.c:88:     0.  As you move towards the outer-most (oldest) frame, the level
.... there are about 30 occurences of level in frame.c

hppa-tdep.c:1888:      frame_relative_level(this_frame));
hppa-tdep.c:2337:			frame_relative_level (this_frame));
i386-tdep.c:2236:  if (frame_relative_level (this_frame) == 0)
i386-tdep.c:2391:  if (frame_relative_level (this_frame) == 0)

infcmd.c:2023:      /* Print info on the selected frame, including level number but not
infcmd.c:2039:  /* Print info on the selected frame, including level number but not
microblaze-tdep.c:392:  if (frame_relative_level (next_frame) >= 0)
mips-tdep.c:1395:  if (frame_relative_level (next_frame) >= 0 && mips_in_frame_stub (pc))
nds32-tdep.c:1299:  if (frame_relative_level (this_frame) == 0)
or1k-tdep.c:539:			frame_relative_level (next_frame));
or1k-tdep.c:559:			frame_relative_level (next_frame));
rs6000-tdep.c:3443:  if (frame_relative_level (this_frame) == 0)

stack.c:1690:/* Return the innermost frame at level LEVEL.  */
stack.c:1693:leading_innermost_frame (int level)
... stack.c contains a lot of other occurences (some due to the new GDB 8.3
   frame apply level

value.c:3827:	 so that the frame level will be shown correctly.  */
value.c:3835:			  frame_relative_level (frame), regnum,

thread.c:1285:			       /* For MI output, print frame level.  */
thread.c:1403:restore_selected_frame (struct frame_id a_frame_id, int frame_level)
thread.c:1409:  if (frame_level == -1)
thread.c:1415:  gdb_assert (frame_level >= 0);
thread.c:1421:  count = frame_level;
thread.c:1449:  if (frame_level > 0 && !current_uiout->is_mi_like_p ())
thread.c:1453:	       frame_level);
thread.c:1487:    restore_selected_frame (m_selected_frame_id, m_selected_frame_level);
thread.c:1521:      m_selected_frame_level = frame_relative_level (frame);

python/py-framefilter.c:734:    (in the case of elided frames), and LEVELS_PRINTED is a hash-table
python/py-framefilter.c:735:    containing all the frames level that have already been printed.
python/py-framefilter.c:736:    If a frame level has been printed, do not print it again (in the
python/py-framefilter.c:818:  /* Print frame level.  MI does not require the level if
python/py-framefilter.c:828:      level = frame_relative_level (frame);
python/py-framefilter.c:839:	  annotate_frame_begin (print_level ? level : 0,


It looks like btrace.h uses level for a btrace frame, but the outermost frame
is numbered 0 (so the other way around as in backtrace command).
  /* The function level in a back trace across the entire branch trace.
     A caller's level is one lower than the level of its callee.
record-btrace.c:1759:  DEBUG ("[frame] unwound PC in %s on level %d: %s",
record-btrace.c:1809:  DEBUG ("[frame] sniffed frame for %s on level %d",
record-btrace.c:1851:  DEBUG ("[frame] sniffed tailcall frame for %s on level %d",

gdb.texinfo:5536:control reaches a different line of code at the original stack level
gdb.texinfo:7564:If you need to examine the startup code, or limit the number of levels
in a backtrace, you can change this behavior:
gdb.texinfo:7598:Limit the backtrace to @var{n} levels.  A value of @code{unlimited}
gdb.texinfo:7599:or zero means unlimited levels.
gdb.texinfo:7602:Display the current limit on backtrace levels.

gdb.texinfo:7778:@item frame apply [all | @var{count} | @var{-count} | level @var{level}@dots{}] [@var{flag}]@dots{} @var{command}
   (and 5 other occurences due to the new 'frame apply level')
gdb.texinfo:12510:Stack level 1, frame at 0x7fffffffda30:
gdb.texinfo:27863:The level of the stack frame.  The innermost frame has the level of
gdb.texinfo:29202:   frame=@{level="0",addr="0xffffe410",func="__kernel_vsyscall",
  and a whole bunch of references to level in the mi documentation/examples
  e.g.
gdb.texinfo:29941:For a stack with frame levels 0 through 11:
gdb.texinfo:29977:at the corresponding level.  It is an error if @var{low-frame} is
gdb.texinfo:30009:frame=@{level="0",addr="0x00010734",func="callee4",
gdb.texinfo:30012:frame=@{level="1",addr="0x0001076c",func="callee3",
gdb.texinfo:30015:frame=@{level="2",addr="0x0001078c",func="callee2",
gdb.texinfo:30018:frame=@{level="3",addr="0x000107b4",func="callee1",


The below might  be more clear as 'a low level unique identifier for a frame' ?
frame.h:442:   get_frame_id: A low level frame unique identifier, that consists of

Usages of number
----------------
stack.c:1765:	  /* The argument to apply_ext_lang_frame_filter is the number
stack.c:2750:It can be a stack frame number or the address of the frame."),
stack.c:2796:It can be a stack frame number or the address of the frame."),
cli/cli-cmds.c:1688:The stack is made up of stack frames.  Gdb assigns numbers to stack frames\n\
cli/cli-cmds.c:1693:The commands below can be used to select other frames by number or address."),
mi/mi-cmd-stack.c:76:   specifying the frame numbers at which to start and stop the


The below should be (or are in fact) traceframe number (but some cannot be changed
as they are part of the mi interface).
remote.c:13266:  /* Lookups other than by absolute frame number depend on the current
tracepoint.c:2196:   of information: a frame number, a tracepoint number, and an
tracepoint.c:2200:   target does not give us a frame number or a tracepoint number).
tracepoint.c:2203:   F<hexnum>    (gives the selected frame number)
gdbserver/tracepoint.c:949:   not need to keep a frame number, because they are all sequential
gdbserver/tracepoint.c:951:   always frame number N.  */
mi/mi-main.c:2330:  if (strcmp (mode, "frame-number") == 0)
mi/mi-main.c:2333:	error (_("frame number is required"));

gdb.texinfo:7396:@cindex frame number
@value{GDBN} assigns numbers to all existing stack frames, starting with
and so on upward.  These numbers do not really exist in your program;

**** In the below, the positive number is not a frame number
gdb.texinfo:7447:@itemx @var{n}
Print only the innermost @var{n} frames, where @var{n} is a positive
number.
@item -@var{n}
@itemx -@var{n}
Print only the outermost @var{n} frames, where @var{n} is a positive
number.
gdb.texinfo:7458:with a number to limit the number of frames shown.



gdb.texinfo:7489:Each line in the backtrace shows the frame number and the function name.

*** in the below, this is rather level that is being used ... as otherwise we should
say 'limit the number of numbers' :).
gdb.texinfo:7564:If you need to examine the startup code, or limit the number of levels


gdb.texinfo:7636:Select frame number @var{n}.  Recall that frame zero is the innermost
gdb.texinfo:7666:frame.  The first line shows the frame number, the function name, the

*** also in the below, this is using level (explaining that a level is a number)
gdb.texinfo:7797:levels as @var{level1}-@var{level2}.  The frame level is the number shown


*** the below is really a number, not a 'frame number'
gdb.texinfo:11204:it is the number of frames up in the stack to look.
gdb.texinfo:11231:it is the number of frames up in the stack to look.
gdb.texinfo:11240:it is the number of frames up in the stack to look.
gdb.texinfo:11254:it is the number of frames up in the stack to look.


*** I think the below should be traceframe.
gdb.texinfo:14099:The current trace snapshot (a.k.a.@: @dfn{frame}) number, or -1 if no


*** the below tells that frame level is frame number
@item @var{level}
The frame number, 0 being the topmost frame, i.e., the innermost function.

*** the below should be traceframe number, but that cannot really be changed
as this is part of MI interface.
gdb.texinfo:31858:@item frame-number
*** the below should be traceframe number
gdb.texinfo:39503:The selected frame is number @var{n} in the trace frame buffer;