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 AFF10385DC02 for ; Wed, 23 Aug 2023 15:00:05 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org AFF10385DC02 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=1692802805; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=snFZD51Wf7cW2il3cEj1GmTjMrKbmmDSynhhDInZmdo=; b=XOvYKEFA68mmRiKP/cfih28O+0+QJ6R3Fs6UndB4CyluMX+CoPCWtnjp+fj8kt07vw8Wpp ZcsrXEpkuCQ2i0KXevadRrtipzmLk3cIae1EfHoJ7Ms81LH+26X4WauyiF5ytyPhnytwQt r1hb9wdtza8I+ahaIWexhz82AZYknmY= Received: from mail-ed1-f71.google.com (mail-ed1-f71.google.com [209.85.208.71]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-230-uvwbx6eQO8auTQjY3yqr6Q-1; Wed, 23 Aug 2023 11:00:04 -0400 X-MC-Unique: uvwbx6eQO8auTQjY3yqr6Q-1 Received: by mail-ed1-f71.google.com with SMTP id 4fb4d7f45d1cf-51bee352ffcso3970608a12.1 for ; Wed, 23 Aug 2023 08:00:03 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1692802802; x=1693407602; h=mime-version:message-id:date:references:in-reply-to:subject:cc:to :from:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=snFZD51Wf7cW2il3cEj1GmTjMrKbmmDSynhhDInZmdo=; b=edqn2il3CW3E3c7FEtHaQpCMTMytHp1jDYk3/sYvCuV7yj2b/AU0XiBlxyVxdsm4OL 9e2P0F3RUFIHDVUaN7kb5CGTYqnQej9RYOIWAAHaP8TLHHrfo1bhi3Ez/F++nifZiD2c XPnWofPN4d21gwKQk2VST+9tvxaBVS+aSFFA4jEHPZEyDWCmFttMCrbYXnYbmhajG/GB lx0qs5G8XVDHIO5alXZsqJ+HFPaJyJIIOmxU500nF3bMMSaS6qvc3fV8GVjdpdUO23uc /SuD/e9hcnb6K+wuHNCBnFfxr1nhPbbaT31c1mataskfGK3Vot9olOxnpKHb/iovClmP r8dg== X-Gm-Message-State: AOJu0YyfFejWJQ3C60fLGwmM4EWdeSRsnfBlaF34twiiXZnmiyqin6LQ KvISV2EeBxsMeBoMeHpWedjiM1NU6q5fBvVnUirMTOjH6CCjHr1Wmh4SHhbR7FhcmIymSgH4QVZ fsUK6seEFbG3sINrV+3C71QCBeUx6IS7sQHEd4gSPTCWi1mQzFOguktZR+h0B0HGAPIJsuZKQTL FSRNjqlQ== X-Received: by 2002:a05:6402:1610:b0:525:75fb:442a with SMTP id f16-20020a056402161000b0052575fb442amr11037077edv.36.1692802801978; Wed, 23 Aug 2023 08:00:01 -0700 (PDT) X-Google-Smtp-Source: AGHT+IE54ngID2UsTZsbzZGa3vwK4AQWsl3EoI256NtsCH1XdS/NaaeW8kwH/YbCzSruJNrBK2/DZQ== X-Received: by 2002:a05:6402:1610:b0:525:75fb:442a with SMTP id f16-20020a056402161000b0052575fb442amr11037052edv.36.1692802801510; Wed, 23 Aug 2023 08:00:01 -0700 (PDT) Received: from localhost ([31.111.84.232]) by smtp.gmail.com with ESMTPSA id r9-20020aa7da09000000b005256e0797acsm9317864eds.37.2023.08.23.08.00.00 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 23 Aug 2023 08:00:00 -0700 (PDT) From: Andrew Burgess To: Bruno Larsen via Gdb-patches , gdb-patches@sourceware.org Cc: pedro@palves.net, Bruno Larsen Subject: Re: [PATCH v2] gdb/cli: fixes to newly added "list ." command In-Reply-To: <20230721102655.3486091-2-blarsen@redhat.com> References: <20230718112140.1911522-2-blarsen@redhat.com> <20230721102655.3486091-2-blarsen@redhat.com> Date: Wed, 23 Aug 2023 16:00:00 +0100 Message-ID: <87a5uhby8f.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_H4,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: Bruno 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 v2: > * reworded documentation and help text > > --- > gdb/NEWS | 6 ++++-- > gdb/cli/cli-cmds.c | 21 ++++++++++++--------- > gdb/doc/gdb.texinfo | 6 +++--- > gdb/testsuite/gdb.base/list.exp | 19 +++++++++---------- > 4 files changed, 28 insertions(+), 24 deletions(-) > > diff --git a/gdb/NEWS b/gdb/NEWS > index ac5dc424d3f..93010178364 100644 > --- a/gdb/NEWS > +++ b/gdb/NEWS > @@ -87,8 +87,10 @@ > * The Ada 2022 Enum_Rep and Enum_Val attributes are now supported. > > * 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 default location. The default location > + is where the inferior is stopped in the current stack frame; if the While reading this patch I struggled with the definition(s) of 'default location'. Here you talk about 'where the inferior is stopped', but I'm not sure about that -- what about multi-threaded inferiors? So maybe we should say 'current thread is stopped'? But then you also pull in the idea of the 'current stack frame'. So I wonder if we can actually drop reference to inferior/thread completely. How about: The default location is the point of execution within the currently selected frame. The phrase 'point of execution' is something I pulled from the manual -- it's used in several places. > + inferior didn't start yet, the command will print around the beginning > + of the 'main' function. s/inferior didn't start/inferior hasn't started/ -- I think this reads better. > > * 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..9b047513e02 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 default. */ > 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,12 @@ 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 default location.\n\ > +The default location is around the location where the inferior is stopped\n\ > +in the current frame, or around the main function if the inferior didn't start.\n\ > +\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 cd86da50f46..de44317ec6f 100644 > --- a/gdb/doc/gdb.texinfo > +++ b/gdb/doc/gdb.texinfo > @@ -9155,9 +9155,9 @@ 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 default location. The default location is where the inferior Maybe: Print lines surrounding the default location. The default location is the point of execution within the currently selected frame. > +is stopped in the current frame or around the beginning of the 'main' > +function, if the inferior didn't start yet. > @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..520424c37c6 100644 > --- a/gdb/testsuite/gdb.base/list.exp > +++ b/gdb/testsuite/gdb.base/list.exp > @@ -402,18 +402,16 @@ 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. > + # Restart the inferior to test "list ." before the inferior is started. I think you mean 'Restart GDB to test ....', otherwise this sentence makes no sense! > clean_restart > 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 +421,18 @@ 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" Here, and below, you should wrap these long lines. Within the regexp you should consider using a '^' anchor, e.g.: gdb_test "list ." "^10\[ \t\]+foo \(.*\);.*19\[ \t\]+foo \(.*\);" \ "list current line after starting" this ensures there is no output before the lines starting '10 ....'. Obviously that suggestion applies in the other 3 locations too. > > # 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 Thanks, Andrew