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 528643858C5E for ; Mon, 17 Jul 2023 08:21:15 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 528643858C5E 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=1689582074; 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=QKy0crted5qlRwNdKHTdN9QeTZONus2KYnLz8TPFgSw=; b=BVXwGkQLA/nOMo/UnUBeLTCmce5/I4AxAS7ZWgRZmelJpZCUb8wyG7CQI3xL/hmpd9NFWs hszKCQ/S6JRxvWA86JZuw7FbsBwAqcbCiM4Brpc3Y8CYnaSgrWDGaCshRWZBhGALmNHf0F /HvTil28moxGwIIxiq8fMQFyp5BAf/4= Received: from mail-ej1-f72.google.com (mail-ej1-f72.google.com [209.85.218.72]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-412-wIYKLUMVNfyfNTNBV8IX4w-1; Mon, 17 Jul 2023 04:21:13 -0400 X-MC-Unique: wIYKLUMVNfyfNTNBV8IX4w-1 Received: by mail-ej1-f72.google.com with SMTP id a640c23a62f3a-997d069a914so15452366b.1 for ; Mon, 17 Jul 2023 01:21:13 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1689582072; x=1692174072; 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=QKy0crted5qlRwNdKHTdN9QeTZONus2KYnLz8TPFgSw=; b=KysTRBYfsKFo6SkNnO2Z93HhfNw1ljgaycy/jtjVvpyATB0U7pCLv52QUClwvrsd8v AqGYamDiUt/Mjvh8t9+O7f47agL+qO2Yz2eXsrbGvREKmjFtIClKtjcw6IAep33/D7n7 5yn6LWHJiBGpa97QRfL71Kh+ReVCKCuo92y5dq8s3Ktp3gd0gqzwaUT0qDuFDuuwVxpm fITzNeyCYd8W3qQJoZehTfN14z6eYTBLwCuetkAF3syA02e8jfV3lZgq9JSflQa/QWg6 JyCqEGvYW7u+ML3ydOrcFpZX7LtyRI1KMy1ITRDjEvuDaaucA41mICtPez/QGm0Rh4kY LXLw== X-Gm-Message-State: ABy/qLZ7v8dQLCZYVydp2h+0a2bRECYmysUHK0CuVvbMkSa7Nis0A4f3 vxrTYD58cP3JXN67eXhp0RJFEJPK8oJlDZEj91zUz7E7vyv+Ad7gqEUKr8Y1Y0fo7ORE2mduEjb EOaOYfbuUgi418PEclJ+1bB1Y7u5jlQ== X-Received: by 2002:a17:906:77d3:b0:992:a85d:278b with SMTP id m19-20020a17090677d300b00992a85d278bmr10040231ejn.59.1689582072082; Mon, 17 Jul 2023 01:21:12 -0700 (PDT) X-Google-Smtp-Source: APBJJlE+cHQHjTXnDsAGfzhjBZk2TAwQb9PW5xwxeodffVWx4RMfYXQ9FRVfmxCq0254rysruagYBQ== X-Received: by 2002:a17:906:77d3:b0:992:a85d:278b with SMTP id m19-20020a17090677d300b00992a85d278bmr10040214ejn.59.1689582071566; Mon, 17 Jul 2023 01:21:11 -0700 (PDT) Received: from [10.43.2.40] (nat-pool-brq-t.redhat.com. [213.175.37.10]) by smtp.gmail.com with ESMTPSA id c7-20020a17090654c700b009882e53a42csm8813424ejp.81.2023.07.17.01.21.11 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 17 Jul 2023 01:21:11 -0700 (PDT) Message-ID: <430fb2d9-ae07-6551-3426-adb5334c127d@redhat.com> Date: Mon, 17 Jul 2023 10:21:10 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.12.0 Subject: Re: [PATCH v4 2/4] gdb/cli: add '.' as an argument for 'list' command To: Pedro Alves , gdb-patches@sourceware.org References: <20230713102411.2279542-1-blarsen@redhat.com> <20230713102411.2279542-3-blarsen@redhat.com> <835af40f-edae-ba48-f121-a1cdd1f1147e@palves.net> From: Bruno Larsen In-Reply-To: <835af40f-edae-ba48-f121-a1cdd1f1147e@palves.net> 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: 7bit X-Spam-Status: No, score=-12.0 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H4,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 14/07/2023 19:50, Pedro Alves wrote: > Hi, > > Sorry for coming late to the party here, but while trying to catch up on the > list I spotted a few things that I think should be fixed. See below. I forgot to send the email, but I've already pushed this patch... I'll fix up the things you pointed in a moment and send a new patch, I just have a few questions. > > BTW, I think the feature is cool! > > On 2023-07-13 11:24, Bruno Larsen via Gdb-patches wrote: >> Currently, after the user has used the list command once, there is no >> self-contained way to ask GDB to print the location where the inferior is >> stopped. The current best options require either using a separate >> command to scope out where the inferior is stopped, or using "list *$pc" >> requiring knowledge of GDB standard registers. This commit adds a way >> to do that using '.' as a new argument for the 'list' command. If the >> inferior isn't running, the command prints around the main function. >> >> Because this necessitated having the inferior running and the test was >> (seemingly unnecessarily) using printf in a non-essential way and it >> would make the resulting log harder to read for no benefit, it was >> replaced by a differen t statement. >> --- >> gdb/NEWS | 4 ++++ >> gdb/cli/cli-cmds.c | 31 ++++++++++++++++++++++++-- >> gdb/doc/gdb.texinfo | 5 +++++ >> gdb/testsuite/gdb.base/list.exp | 39 +++++++++++++++++++++++++++++++++ >> gdb/testsuite/gdb.base/list1.c | 2 +- >> 5 files changed, 78 insertions(+), 3 deletions(-) >> >> diff --git a/gdb/NEWS b/gdb/NEWS >> index b924834d3d7..eef440a5242 100644 >> --- a/gdb/NEWS >> +++ b/gdb/NEWS >> @@ -84,6 +84,10 @@ >> is 64k. To print longer strings you should increase >> 'max-value-size'. >> >> +* 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. > It would be more accurate to say that it's where the current thread is stopped. > > Say you run to breakpoint hit in thread 1, and then switch to thread 2, and then do > "list .". That will show you the current frame of thread 2, while "where the > inferior is stopped" could very well be interpreted as "thread 1". The wording does need changing, I agree. I think using the wording that already exists in the documentation is the best way to go: "prints around the last solitary line printed as part of displaying a stack frame". The wording I originally used (and the improvement you suggested) could make it sound like I would always print the lowermost frame, when that is not the point I wanted. What I wanted was to re-do what the first usage of "list" does without needing to know the lines. My big question: Can I change the NEWS entry, or is that some taboo that's best avoided? > >> + >> * New commands >> >> maintenance print record-instruction [ N ] >> diff --git a/gdb/cli/cli-cmds.c b/gdb/cli/cli-cmds.c >> index 00977bc2ee3..1c459afdc97 100644 >> --- a/gdb/cli/cli-cmds.c >> +++ b/gdb/cli/cli-cmds.c >> @@ -1234,14 +1234,14 @@ list_command (const char *arg, int from_tty) >> const char *p; >> >> /* Pull in the current default source line if necessary. */ >> - if (arg == NULL || ((arg[0] == '+' || arg[0] == '-') && arg[1] == '\0')) >> + if (arg == NULL || ((arg[0] == '+' || arg[0] == '-' || arg[0] == '.') && arg[1] == '\0')) >> { >> set_default_source_symtab_and_line (); >> symtab_and_line cursal = get_current_source_symtab_and_line (); >> >> /* If this is the first "list" since we've set the current >> source line, center the listing around that line. */ >> - if (get_first_line_listed () == 0) >> + if (get_first_line_listed () == 0 && (arg == nullptr || arg[0] != '.')) >> { >> list_around_line (arg, cursal); >> } >> @@ -1263,6 +1263,32 @@ list_command (const char *arg, int from_tty) >> print_source_lines (cursal.symtab, range, 0); >> } >> >> + /* "l ." lists the default location again. */ > Spelling out "list ." would be better for grepping, IMHO. I followed the convention of the code around it. All used only 'l' instead of "list", but I can change that easily enough. > >> + else if (arg[0] == '.') >> + { >> + try >> + { >> + /* Find the current line by getting the PC of the currently >> + selected frame, and finding the line associated to it. */ >> + frame_info_ptr frame = get_selected_frame (nullptr); > So this is actually using the selected frame, not the current frame. So even > the "where the thread is stopped" description would be incorrect. If you really > wanted to use frame #0 (where the thread stopped), then this should use get_current_frame. Yeah... that's the confusion I mentioned earlier... I want the selected frame, since its what "list" does when you first call it after entering a frame. > > >> + CORE_ADDR curr_pc = get_frame_pc (frame); >> + cursal = find_pc_line (curr_pc, 0); >> + } >> + catch (const gdb_exception &e) >> + { >> + /* If there was an exception above, it means the inferior >> + is not running, so reset the current source location to >> + the default. */ > Aww. Please don't use a try/catch for this... You can just check target_has_execution. Oh, I thought target_has_execution would query if the target can execute the inferior, rather than looking if the inferior is being executed. Will change > > Also, if an exception would be good for this (which it isn't), please don't catch > gdb_exception -- it catches QUIT/Ctrl-C as well, which is most often wrong. You would > want gdb_exception_error normally. Also news to me! I probably won't use it, but its good to know for when I need it :) >> + 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 ." >> + will print the following lines instead of the same lines again. */ >> + arg++; >> + } >> + >> return; >> } >> >> @@ -2770,6 +2796,7 @@ 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\ > arond -> around > >> \"list -\" lists the ten lines before a previous ten-line listing.\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\ >> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo >> index b10c06ae91f..7619efe3de9 100644 >> --- a/gdb/doc/gdb.texinfo >> +++ b/gdb/doc/gdb.texinfo >> @@ -9148,6 +9148,11 @@ Stack}), this prints lines centered around that line. >> >> @item list - >> 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. > This should be clarified as well. > >> @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 18ecd13ac0f..ed178a1dd95 100644 >> --- a/gdb/testsuite/gdb.base/list.exp >> +++ b/gdb/testsuite/gdb.base/list.exp >> @@ -400,6 +400,42 @@ proc test_list_invalid_args {} { >> "second use of \"list +INVALID\"" >> } >> >> +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. > I don't understand this comment. Why would it be unable to do so? doh, this is a comment form previous iterations. Back when I thought "list ." should error, we were entering the "never before listed" if clause, instead of finding "list ." which was a problem. Now that the behavior is the same, there isn't a reason to restart the inferior. > >> + clean_restart >> + gdb_file_cmd ${binfile} >> + >> + # Ensure that we are printing 10 lines >> + if {![set_listsize 10]} { >> + return >> + } >> + >> + # 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" >> + >> + if {![runto_main]} { >> + warning "couldn't start inferior" >> + return >> + } >> + >> + # Walk forward some lines > Missing period. > >> + 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" >> + >> + # 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" > I don't understand -- these 4 tests match ".*", so how how they ensuring what > they claim they test? Looks like WIP? you are correct, it is WIP... taking time off while developing this patch was clearly a bad choice hahaha -- Cheers, Bruno > > Pedro Alves > >> +} >> + >> clean_restart >> gdb_file_cmd ${binfile} >> >> @@ -519,4 +555,7 @@ test_list "list -" 10 2 "7-8" "5-6" >> # the current line. >> test_list "list -" 10 1 "7" "6" >> >> +# Test printing the location where the inferior is stopped. >> +test_list_current_location >> + >> remote_exec build "rm -f list0.h" >> diff --git a/gdb/testsuite/gdb.base/list1.c b/gdb/testsuite/gdb.base/list1.c >> index d694495c3fb..9297f958f46 100644 >> --- a/gdb/testsuite/gdb.base/list1.c >> +++ b/gdb/testsuite/gdb.base/list1.c >> @@ -7,7 +7,7 @@ void bar (int x) >> - >> - */ >> { >> - printf ("%d\n", x); >> + x++; >> >> long_line (); >> }