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 23AF93858D32 for ; Mon, 18 Sep 2023 13:16:09 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 23AF93858D32 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=1695042968; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=ivgNsYEGSvhfGODmKfYFH34SeR3YyFXx1sBYChikiII=; b=aQC7vvDApHLhsCJD8cX3anpifs1AgnrwR2ctwHOuUyc45Xn67ds4kLuNoXDJNBkjoF/5KZ DwBDQpsXQpm+ffflQJjoWTYLGXhvoOi5oMtQsACoMkIvShMdTPP8v67QGwrApEE5niCr4i Ozf5obdo0o5J8coyCP9a5ue5VvtmdxY= Received: from mail-wr1-f72.google.com (mail-wr1-f72.google.com [209.85.221.72]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-688-GDjga2ngP8CijiQBb0nZug-1; Mon, 18 Sep 2023 09:16:05 -0400 X-MC-Unique: GDjga2ngP8CijiQBb0nZug-1 Received: by mail-wr1-f72.google.com with SMTP id ffacd0b85a97d-3172a94b274so3005932f8f.0 for ; Mon, 18 Sep 2023 06:16:05 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1695042964; x=1695647764; h=mime-version:message-id:date:references:in-reply-to:subject:to:from :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=ivgNsYEGSvhfGODmKfYFH34SeR3YyFXx1sBYChikiII=; b=hdTni4rgYL84y9ghvK9pzmSriflp/V0hwJbc3sjY2KDWqySMB6L/DN0/dmgzbI7uqz gye2K9TZsliNjz7i0O1Cp7JKy4HIRHDOh74vDkSZ1g0H5Dd0HeL54MLpcuwn6PfSqZkV WRg7o1EOIB9l6emmDOkeUVjWwYMCzTpsQWvaQnt3+7sT6Z2Ebhsux6xI3s3GdSU3+nKT Jdan2AEuzjDtB95B0aT/uBchyVZdQApquGFB7FVGlnZ7s2plFkKDmesZwy/amrnrz48t 6T2ycNRUMxb3LT54UarNulAKXlwylHIiTJ76tLcMWE/gXY6ZmCBmWL3Rt8HWtPQmwPPb HFuA== X-Gm-Message-State: AOJu0YxLlik/eBovv2WSr+lrcpF77Sq37lQh0tS57DaLYZPH6UIwfh7z 4ATy6CKGS1eGKGySTBlFpQeNpDCxHJnKnNGD0uCoEJiUKyGdzlJNIdy2FV/0fdvR6nswCoPK6NU spzzs4chz6zbrNKQAtAYdXhYt9r7E/w== X-Received: by 2002:a5d:4442:0:b0:319:8bd0:d18c with SMTP id x2-20020a5d4442000000b003198bd0d18cmr8125752wrr.52.1695042964090; Mon, 18 Sep 2023 06:16:04 -0700 (PDT) X-Google-Smtp-Source: AGHT+IFXC+6PXdBxdcs+SFzaxXceevfgqOkl0IyPTJgjLJ5LPy4+igRyPDaJQjB+h4weY4KcBZqjVg== X-Received: by 2002:a5d:4442:0:b0:319:8bd0:d18c with SMTP id x2-20020a5d4442000000b003198bd0d18cmr8125727wrr.52.1695042963576; Mon, 18 Sep 2023 06:16:03 -0700 (PDT) Received: from localhost (197.126.90.146.dyn.plus.net. [146.90.126.197]) by smtp.gmail.com with ESMTPSA id d6-20020adfef86000000b0031f82743e25sm12622286wro.67.2023.09.18.06.16.02 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 18 Sep 2023 06:16:02 -0700 (PDT) From: Andrew Burgess To: Guinevere Larsen , gdb-patches@sourceware.org Subject: Re: [PATCH v3] gdb/cli: fixes to newly added "list ." command In-Reply-To: <20230828155039.120251-2-blarsen@redhat.com> References: <20230721102655.3486091-2-blarsen@redhat.com> <20230828155039.120251-2-blarsen@redhat.com> Date: Mon, 18 Sep 2023 14:16:01 +0100 Message-ID: <87bkdzzkoe.fsf@redhat.com> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain X-Spam-Status: No, score=-11.9 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE,TXREP 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: Guinevere Larsen via Gdb-patches writes: > After the series that added this command was pushed, Pedro mentioned > that the news description could easily be misinterpreted, as well as > some code and test improvements that should be made. > > While fixing the test, I realized that code repetition wasn't > happening as it should, so I took care of that too. > --- > Changes for v3: > * Changed documentation wording again, from "defualt location" to > "point of execution", seeing as it is already in the manual. > > Since I changed docs, I removed Eli's review tag. > > Changes for v2: > * reworded documentation and help text > > --- > gdb/NEWS | 5 +++-- > gdb/cli/cli-cmds.c | 18 +++++++++--------- > gdb/doc/gdb.texinfo | 5 ++--- > gdb/testsuite/gdb.base/list.exp | 24 +++++++++++++----------- > 4 files changed, 27 insertions(+), 25 deletions(-) > > diff --git a/gdb/NEWS b/gdb/NEWS > index c4b1f7a7e3b..f34bda60a88 100644 > --- a/gdb/NEWS > +++ b/gdb/NEWS > @@ -90,8 +90,9 @@ > expression parser. > > * The 'list' command now accepts '.' as an argument, which tells GDB to > - print the location where the inferior is stopped. If the inferior hasn't > - started yet, the command will print around the main function. > + print the location around the point of execution within the current frame. > + If the inferior hasn't started yet, the command wil print around the > + beginning of the 'main' function. > > * Using the 'list' command with no arguments in a situation where the > command would attempt to list past the end of the file now warns the > diff --git a/gdb/cli/cli-cmds.c b/gdb/cli/cli-cmds.c > index 0fa24fd3df9..6bf10c98c54 100644 > --- a/gdb/cli/cli-cmds.c > +++ b/gdb/cli/cli-cmds.c > @@ -1272,10 +1272,10 @@ list_command (const char *arg, int from_tty) > print_source_lines (cursal.symtab, range, 0); > } > > - /* "l ." lists the default location again. */ > + /* "list ." lists the default location again. */ > else if (arg[0] == '.') > { > - try > + if (target_has_stack ()) > { > /* Find the current line by getting the PC of the currently > selected frame, and finding the line associated to it. */ > @@ -1283,19 +1283,19 @@ list_command (const char *arg, int from_tty) > CORE_ADDR curr_pc = get_frame_pc (frame); > cursal = find_pc_line (curr_pc, 0); > } > - catch (const gdb_exception &e) > + else > { > - /* If there was an exception above, it means the inferior > - is not running, so reset the current source location to > - the default. */ > + /* The inferior is not running, so reset the current source > + location to the point of execution. */ I'm not sure this comment makes sense -- if the inferior is NOT running then set the location to the point of execution! How about: /* The inferior is not running, so reset the current source location the default (usually the main function). */ If this, or something like this is acceptable to you then: > clear_current_source_symtab_and_line (); > set_default_source_symtab_and_line (); > cursal = get_current_source_symtab_and_line (); > } > list_around_line (arg, cursal); > - /* Advance argument so just pressing "enter" after using "list ." > + /* Set the repeat args so just pressing "enter" after using "list ." > will print the following lines instead of the same lines again. */ > - arg++; > + if (from_tty) > + set_repeat_arguments (""); > } > > return; > @@ -2805,9 +2805,9 @@ and send its output to SHELL_COMMAND.")); > = add_com ("list", class_files, list_command, _("\ > List specified function or line.\n\ > With no argument, lists ten more lines after or around previous listing.\n\ > -\"list .\" lists ten lines arond where the inferior is stopped.\n\ > \"list +\" lists the ten lines following a previous ten-line listing.\n\ > \"list -\" lists the ten lines before a previous ten-line listing.\n\ > +\"list .\" lists ten lines around the point of execution in the current frame.\n\ > One argument specifies a line, and ten lines are listed around that line.\n\ > Two arguments with comma between specify starting and ending lines to list.\n\ > Lines can be specified in these ways:\n\ > diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo > index 8be9725d1a2..784da709ea2 100644 > --- a/gdb/doc/gdb.texinfo > +++ b/gdb/doc/gdb.texinfo > @@ -9212,9 +9212,8 @@ Same as using with no arguments. > Print lines just before the lines last printed. > > @item list . > -Print the lines surrounding the location that is where the inferior > -is stopped. If the inferior is not running, print around the main > -function instead. > +Print the point of execution within the currently selected frame. > +If the inferior is not running, print around the main function instead. I think saying 'print the point of execution' might be confusing. How about retaining the mention of 'lines surrounding', something like: Print the lines surrounding the point of execution within the currently selected frame. If the inferior is not running print lines around the start of the main function instead. If you're happy with the comment change, then, at least for the code portion: Approved-By: Andrew Burgess If you Cc Eli with your final doc version then you'll usually get a quick reply. Thanks, Andrew > @end table > > @cindex @code{list}, how many lines to display > diff --git a/gdb/testsuite/gdb.base/list.exp b/gdb/testsuite/gdb.base/list.exp > index 582355996b0..306fe41c3a3 100644 > --- a/gdb/testsuite/gdb.base/list.exp > +++ b/gdb/testsuite/gdb.base/list.exp > @@ -402,18 +402,15 @@ proc test_list_invalid_args {} { > > proc test_list_current_location {} { > global binfile > - # If the first "list" command that GDB runs is "list ." GDB may be > - # unable to recognize that the inferior isn't running, so we should > - # reload the inferior to test that condition. > - clean_restart > + # Reload the inferior to test "list ." before the inferior is started. > gdb_file_cmd ${binfile} > > - # Ensure that we are printing 10 lines > + # Ensure that we are printing 10 lines. > if {![set_listsize 10]} { > return > } > > - # First guarantee that GDB prints around the main function correctly > + # First guarantee that GDB prints around the main function correctly. > gdb_test "list ." \ > "1.*\r\n2\[ \t\]+\r\n3\[ \t\]+int main \[)(\]+.*5\[ \t\]+int x;.*" \ > "list . with inferior not running" > @@ -423,17 +420,22 @@ proc test_list_current_location {} { > return > } > > - # Walk forward some lines > + # Walk forward some lines. > gdb_test "until 15" ".*15.*foo.*" > > # Test that the correct location is printed and that > # using just "list" will print the following lines. > - gdb_test "list ." ".*" "list current line after starting" > - gdb_test "list" ".*" "confirm we are printing the following lines" > + gdb_test "list ." "10\[ \t\]+foo \(.*\);.*19\[ \t\]+foo \(.*\);" \ > + "list current line after starting" > + gdb_test "list" "20\[ \t\]+foo \(.*\);.*29\[ \t\]+foo \(.*\);" \ > + "confirm we are printing the following lines" > > # Test that list . will reset to current location > - gdb_test "list ." ".*" "list around current line again" > - gdb_test " " ".*" "testing repeated invocations with GDB's auto-repeat" > + # and that an empty line lists the following lines. > + gdb_test "list ." "10\[ \t\]+foo \(.*\);.*19\[ \t\]+foo \(.*\);" \ > + "list around current line again" > + gdb_test " " "20\[ \t\]+foo \(.*\);.*29\[ \t\]+foo \(.*\);" \ > + "testing repeated invocations with GDB's auto-repeat" > } > > clean_restart > -- > 2.41.0