From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 36608 invoked by alias); 14 Aug 2018 10:31:57 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 36592 invoked by uid 89); 14 Aug 2018 10:31:56 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-18.6 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RCVD_IN_DNSWL_LOW,SPF_PASS autolearn=ham version=3.3.2 spammy=N, 2391, Recall, **** X-HELO: mailsec109.isp.belgacom.be Received: from mailsec109.isp.belgacom.be (HELO mailsec109.isp.belgacom.be) (195.238.20.105) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 14 Aug 2018 10:31:53 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=skynet.be; i=@skynet.be; q=dns/txt; s=securemail; t=1534242712; x=1565778712; h=message-id:subject:from:to:cc:date:in-reply-to: references:mime-version:content-transfer-encoding; bh=tdA1l/gHPUmOUrtV20+NDkCHG85axyKWJt/zkOO2SHA=; b=YR83nBZA9/Oxdbe4icWVYvxnYGRWpgHiv8wbRba6ICFJzvevLxzOBzk0 IhpeRO2U4S+JjQVwNeIx9gRVNixCQw==; Received: from 217.24-133-109.adsl-dyn.isp.belgacom.be (HELO md) ([109.133.24.217]) by relay.skynet.be with ESMTP/TLS/AES256-GCM-SHA384; 14 Aug 2018 12:31:49 +0200 Message-ID: <1534242709.15655.4.camel@skynet.be> Subject: Re: [PATCHv5 0/2] gdb: Change how frames are selected for 'frame' and 'info frame'. From: Philippe Waroquiers To: Andrew Burgess , gdb-patches@sourceware.org Cc: Eli Zaretskii Date: Tue, 14 Aug 2018 10:31:00 -0000 In-Reply-To: References: <20180725181406.GA3155@embecosm.com> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: 8bit X-IsSubscribed: yes X-SW-Source: 2018-08/txt/msg00349.txt.bz2 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 ' 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    (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;