From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id 292C63858D37 for ; Thu, 22 Jun 2023 09:54:39 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 292C63858D37 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1687427678; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=TLRsLEpdTSsfFdOwJkkTpkLju0tHQ5fmlim7vsvpX60=; b=YAz4IDLZQs2J0nORy6dBl/2fLrh/CxCRvaVcGJK8YFpRkVSR0aHstNdTIhsurwVZhFG3GB uDjY2jOELyQD1us1V8Q0fMu+dhBYs+3mj1DNnJGYUPVWWS5atK62VbvSDb+t33DJ3NKI6h XBYPIlFOHRD3/dSGjd/AXK9pnMbQTio= Received: from mail-qv1-f70.google.com (mail-qv1-f70.google.com [209.85.219.70]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-256-FcyuHeseNYq464z49lCIPw-1; Thu, 22 Jun 2023 05:54:37 -0400 X-MC-Unique: FcyuHeseNYq464z49lCIPw-1 Received: by mail-qv1-f70.google.com with SMTP id 6a1803df08f44-62ff6a6b4f4so82592766d6.0 for ; Thu, 22 Jun 2023 02:54:37 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1687427677; x=1690019677; h=content-transfer-encoding:in-reply-to:from:references:to :content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=TLRsLEpdTSsfFdOwJkkTpkLju0tHQ5fmlim7vsvpX60=; b=fCyupqmgXfdOAnZ+Ms8hvNcKNEN/pmVJCxigNyL4Zg2iIkHv/qYlACWXAW+UquDmjn xE8HBKdMuoaXPzMFEY/4s8AlOxI6EZWqzo2il5PKQlOO0jGB1hBmvEQL+0X7gUHYcMGR HmZHcEph4uf2tcAPrZl3SZYs2IBekIs4A8jcYESgzOTMdPGtV3CSMqXL/GuOhSmNBLww J+ZDbjGGc2r4O704kk16X+Ggu4pGiDu52/7o/MAqm8PDwOfSznWeaVEQFOnPXa2WwWBN 74IDM3i7saEZppOVwU21pHt3b0gvugqVZMCdippqyaHFONTCZz5SA98TQiKNyJWnQqrh R1kQ== X-Gm-Message-State: AC+VfDwRAgoKXlzIKd1mI/onODKB3RLFLnY8pVdvg+xEcjqS1QbrXvIx AY4WFYYXcKcbyuMGmtclOMF1fPGtq/M53gMgnsKthnfvo07boWWz7VYlRkjeapkN7u3czueqouI bqY9ykQlv0gUlpi7Q326poSdfi2yF2w== X-Received: by 2002:ad4:5ba2:0:b0:61d:be1e:7c47 with SMTP id 2-20020ad45ba2000000b0061dbe1e7c47mr20035144qvq.11.1687427676659; Thu, 22 Jun 2023 02:54:36 -0700 (PDT) X-Google-Smtp-Source: ACHHUZ751plSUZYJJ4zRpW9qjaZFUwKl2+ss++9wprRto+7T5o+z6/UkNOdUH/GnWHNnRWhKykijLA== X-Received: by 2002:ad4:5ba2:0:b0:61d:be1e:7c47 with SMTP id 2-20020ad45ba2000000b0061dbe1e7c47mr20035131qvq.11.1687427676315; Thu, 22 Jun 2023 02:54:36 -0700 (PDT) Received: from [192.168.0.129] (ip-94-112-225-44.bb.vodafone.cz. [94.112.225.44]) by smtp.gmail.com with ESMTPSA id m1-20020a0ce6e1000000b006166d870243sm3641681qvn.43.2023.06.22.02.54.35 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 22 Jun 2023 02:54:36 -0700 (PDT) Message-ID: Date: Thu, 22 Jun 2023 11:54:29 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.11.0 Subject: Re: [PATCH v3 2/4] gdb/cli: Improve UX when using list with no args To: Keith Seitz , gdb-patches@sourceware.org References: <20230621104545.2530552-1-blarsen@redhat.com> <20230621104545.2530552-3-blarsen@redhat.com> <2c50787b-6aa3-382e-37c4-7494da0a26c5@redhat.com> From: Bruno Larsen In-Reply-To: <2c50787b-6aa3-382e-37c4-7494da0a26c5@redhat.com> X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-8.3 required=5.0 tests=BAYES_00,BODY_8BITS,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,KAM_SHORT,NICE_REPLY_A,RCVD_IN_BARRACUDACENTRAL,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H5,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On 21/06/2023 18:07, Keith Seitz wrote: > Hi, > > Apologies, I'm late to the review here, so if I'm stepping on other's > toes, please ignore me! > > On 6/21/23 03:45, Bruno Larsen via Gdb-patches wrote: >> When using "list" with no arguments, GDB will first print the lines >> around where the inferior is stopped, then print the next N lines until >> reaching the end of file, at which point it wanrs the user "Line X out >> of range, file Y only has X-1 lines.".  This is usually desireable, but >> if the user can no longer see the original line, they may have forgotten >> the current line or that a list command was used at all, making GDB's >> error message look cryptic. It was reported in bugzilla as PR cli/30497. > > I've run into this myself over the years. While I've adapted to it, you > (and the original bz reporter) have a very valid point. This is simply > not very user-friendly. So thank you for posting something to address > this. > >> This commit improves the user experince by changing the behavior of >> "list" slightly when a user passes no arguments.  If the line that would >> be printed is past the end of the file, GDB will now warn that the >> previous list command has reached the end of file, and the current line >> wil be listed again. > > [If this is to be your commit message, please note there are several > typos in these two paragraphs ("desireable", "experince").] oof, I think my keyboard or USB hub is on its way out o7 > > An example before(?) / after would help frame the discussion (see below). Sure! > >> diff --git a/gdb/cli/cli-cmds.c b/gdb/cli/cli-cmds.c >> index b0b9c08c2ec..5973aebfad3 100644 >> --- a/gdb/cli/cli-cmds.c >> +++ b/gdb/cli/cli-cmds.c >> @@ -1244,8 +1244,40 @@ list_command (const char *arg, int from_tty) >>         list_around_line (arg, cursal); >>       } >>   -      /* "l" or "l +" lists next ten lines.  */ >> -      else if (arg == NULL || arg[0] == '+') >> +      /* "l" lists the next few lines, unless we're listing past the >> end of >> +     the file.  If it would be past the end, re-print the current >> line.  */ >> +      else if (arg == nullptr) >> +    { >> +      if (can_print_line (cursal.symtab, cursal.line)) >> +        print_source_lines (cursal.symtab, >> +                source_lines_range (cursal.line), 0); >> +      else >> +        { >> +          warning (_("previous list command has already reached the >> end " >> +             "of the file. Listing default location again")); > > [Warning: You may want to skip this nit-picking and jump to "HOWEVER" > below!] > > Nit: Grep'ping through the code, I see that some warnings are implemented > as full sentences and some as incomplete sentences/phrases. This is both. > I'm not a fan. I would prefer full sentences or "previous list command > has > already reached the end of the file -- listing default location again". That's fair. I have some more suggestions for the warning message below. > > On that front, I'm not sure I like the whole "Listing default location > again" phrase. Do users typically know what a "default" location is? > > I don't see this term specifically defined in either the manual or the > proposed patches? [I see there is a definition in NEWS which seems > insufficient.] > > HOWEVER, if I may offer up an example around which to discuss things... > > Current behavior, using "list main.c:0" on gdb's sources, and simply > hitting > [Enter] until the behavior of interest appears: > > (top-gdb) > 1481        gdb_printf (stream, _("\n\ > 1482    Report bugs to %ps.\n\ > 1483    "), styled_string (file_name_style.style (), REPORT_BUGS_TO)); > 1484      if (stream == gdb_stdout) > 1485        gdb_printf (stream, _("\n\ > 1486    You can ask GDB-related questions on the GDB users mailing > list\n\ > 1487    (gdb@sourceware.org) or on GDB's IRC channel (#gdb on > Libera.Chat).\n")); > 1488    } > (top-gdb) > > Line number 1489 out of range; ../../src/gdb/main.c has 1488 lines. > (top-gdb) > > The proposal here changes this to: > > (top-gdb) > 1481        gdb_printf (stream, _("\n\ > 1482    Report bugs to %ps.\n\ > 1483    "), styled_string (file_name_style.style (), REPORT_BUGS_TO)); > 1484      if (stream == gdb_stdout) > 1485        gdb_printf (stream, _("\n\ > 1486    You can ask GDB-related questions on the GDB users mailing > list\n\ > 1487    (gdb@sourceware.org) or on GDB's IRC channel (#gdb on > Libera.Chat).\n")); > 1488    } > (top-gdb) > warning: previous list command has already reached the end of the > file. Listing default location again > 14       GNU General Public License for more details. > 15 > 16       You should have received a copy of the GNU General Public > License > 17       along with this program.  If not, see > .  */ > 18 > 19    #include "defs.h" > 20    #include "main.h" > 21    #include "interps.h" > 22 > 23    int > (top-gdb) > > TBH, I was surprised by this. If a user was looking at the end of a > source > file, would they really want to suddenly jump to the top? [They could > always > "list 0" to easily accomplish that.] This is the difficult part of this change proposal: If the last command for GDB was "list", we can be reasonably sure that the user would prefer the error message, and if the last command was something else (especially info locals, backtrace or some other thing that generates a lot of output), we can guess that they want to be back at the current location. Unfortunately, there is no way (to my knowledge) to get that info, so I went with always assuming the user wants to be back at where they were. Also, about the "stuck" behavior, and the confusion about "default position" i guess: Since you haven't started the inferior yet, the default position is lines around the main function. You're not listing gdb/main.c anymore, you're now listing gdb/gdb.c, which only has 33 lines. The fact that GDB isn't actually listing the function itself, ending at the "int" line, is old behavior already I guess the warning message could be changed to indicate where is being listed, saying something like "previous list command has already reached the end of file -- inferior not running, listing main again" or "previous list (...) -- User examining inferior at line X" (The awkward wording of "user examining" is there because we'll list lines around the selected frame, so if the user moved up some frames we'll list around that instead which is the current behavior for "list" already). Or I could change it so if the inferior is not running, the command errors out, and if it is running, the new behavior is in place. I think its better to error out than repeating the last few lines if we can assume that the user is just examining the source code (which in this case would be when the inferior hasn't been started). > > Also note that as implemented in these patches, hitting [Enter] AGAIN > gets "stuck" > in a weird way [this scenario needs tests]: > > (top-gdb) > 1481        gdb_printf (stream, _("\n\ > 1482    Report bugs to %ps.\n\ > 1483    "), styled_string (file_name_style.style (), REPORT_BUGS_TO)); > 1484      if (stream == gdb_stdout) > 1485        gdb_printf (stream, _("\n\ > 1486    You can ask GDB-related questions on the GDB users mailing > list\n\ > 1487    (gdb@sourceware.org) or on GDB's IRC channel (#gdb on > Libera.Chat).\n")); > 1488    } > (top-gdb) > warning: previous list command has already reached the end of the > file. Listing default location again > 14       GNU General Public License for more details. > 15 > 16       You should have received a copy of the GNU General Public > License > 17       along with this program.  If not, see > .  */ > 18 > 19    #include "defs.h" > 20    #include "main.h" > 21    #include "interps.h" > 22 > 23    int > (top-gdb) > 24    main (int argc, char **argv) > 25    { > 26      struct captured_main_args args; > 27 > 28      memset (&args, 0, sizeof args); > 29      args.argc = argc; > 30      args.argv = argv; > 31      args.interpreter_p = INTERP_CONSOLE; > 32      return gdb_main (&args); > 33    } > (top-gdb) > warning: previous list command has already reached the end of the > file. Listing default location again > 14       GNU General Public License for more details. > 15 > 16       You should have received a copy of the GNU General Public > License > 17       along with this program.  If not, see > .  */ > 18 > 19    #include "defs.h" > 20    #include "main.h" > 21    #include "interps.h" > 22 > 23    int > (top-gdb) > > The "stuck" behavior/bug here aside, I would like to suggest a simpler > solution: > list the last N lines of the file when attempting to list past the > last line. > > Something like (completely uncoded/artificially created): > > (top-gdb) > 1481        gdb_printf (stream, _("\n\ > 1482    Report bugs to %ps.\n\ > 1483    "), styled_string (file_name_style.style (), REPORT_BUGS_TO)); > 1484      if (stream == gdb_stdout) > 1485        gdb_printf (stream, _("\n\ > 1486    You can ask GDB-related questions on the GDB users mailing > list\n\ > 1487    (gdb@sourceware.org) or on GDB's IRC channel (#gdb on > Libera.Chat).\n")); > 1488    } > (top-gdb) > Line number 1489 out of range; ../../src/gdb/main.c has 1488 lines. > 1481        gdb_printf (stream, _("\n\ > 1482    Report bugs to %ps.\n\ > 1483    "), styled_string (file_name_style.style (), REPORT_BUGS_TO)); > 1484      if (stream == gdb_stdout) > 1485        gdb_printf (stream, _("\n\ > 1486    You can ask GDB-related questions on the GDB users mailing > list\n\ > 1487    (gdb@sourceware.org) or on GDB's IRC channel (#gdb on > Libera.Chat).\n")); > 1488    } > [and keep repeating this behavior until a location is explicitly given > by the > user] > > That keeps the user focused at the end of the file. [AFAIK, we have no > convenient/easy way to jump to the end of a file.] > > WDYT? I feel like repeating the last lines would not be as useful. The use-case I had in mind was someone who is parked close to the end of a file and by using list twice or three times ended up there (probably listing to the end of a function) and couldn't see it anymore. If the user exploring the source file, it is likely more useful to straight up say that last list command already reached the end of file. That said, I think we could also add an argument to list at the end of a file, but idk what to call it. > > Keith > > PS. OOC do you know if changing this list-past-end-of-file behavior > alters the way MI > currently behaves? > huh, it doesn't! I thought it used the same base code, but apparently not. I'll update the MI version to keep the behaviors synced. -- Cheers, Bruno