diff mbox series

[v4,2/4] gdb/cli: add '.' as an argument for 'list' command

Message ID 20230713102411.2279542-3-blarsen@redhat.com
State New
Headers
Series Small changes to "list" command |

Commit Message

Guinevere Larsen July 13, 2023, 10:24 a.m. UTC 
  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(-)

Comments

Eli Zaretskii July 13, 2023, 11:05 a.m. UTC | #1 
> Cc: Bruno Larsen <blarsen@redhat.com>
> Date: Thu, 13 Jul 2023 12:24:09 +0200
> From: Bruno Larsen via Gdb-patches <gdb-patches@sourceware.org>
> 
> 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(-)

The documentation parts are okay, thanks.

Reviewed-By: Eli Zaretskii <eliz@gnu.org>
Tom Tromey July 13, 2023, 4:53 p.m. UTC | #2 
>>>>> "Bruno" == Bruno Larsen via Gdb-patches <gdb-patches@sourceware.org> writes:

Bruno> Because this necessitated having the inferior running and the test was
Bruno> (seemingly unnecessarily) using printf in a non-essential way and it
Bruno> would make the resulting log harder to read for no benefit, it was
Bruno> replaced by a differen t statement.

Extra space in there.  Normally I wouldn't point it out but I had one
other small nit.

Bruno> +	  try
Bruno> +	    {
Bruno> +	      /* Find the current line by getting the PC of the currently
Bruno> +		 selected frame, and finding the line associated to it.  */
Bruno> +	      frame_info_ptr frame = get_selected_frame (nullptr);
Bruno> +	      CORE_ADDR curr_pc = get_frame_pc (frame);
Bruno> +	      cursal = find_pc_line (curr_pc, 0);
Bruno> +	    }
Bruno> +	  catch (const gdb_exception &e)
Bruno> +	    {
Bruno> +		  /* If there was an exception above, it means the inferior
Bruno> +		     is not running, so reset the current source location to
Bruno> +		     the default.  */
Bruno> +		  clear_current_source_symtab_and_line ();
Bruno> +		  set_default_source_symtab_and_line ();
Bruno> +		  cursal = get_current_source_symtab_and_line ();

The indentation in the 'catch' part looks off.

This is ok with these nits fixed, no need to re-send.

Approved-By: Tom Tromey <tom@tromey.com>

Tom
Pedro Alves July 14, 2023, 5:50 p.m. UTC | #3 
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.

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".

> +
>  * 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.

> +      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.


> +	      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.

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.

> +		  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?

> +    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?

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 ();
>  }
Guinevere Larsen July 17, 2023, 8:21 a.m. UTC | #4 
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
Andrew Burgess July 17, 2023, 8:44 a.m. UTC | #5 
Bruno Larsen via Gdb-patches <gdb-patches@sourceware.org> writes:

> 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?

It's fine to change the section of the NEWS file for the next release,
that is, the part currently under the 'Changes since GDB 13' heading.

We shouldn't change anything under a 'Change in GDB xxx' heading, as
that describes content that has already been released.

Thanks,
Andrew
Pedro Alves July 17, 2023, 2:14 p.m. UTC | #6 
On 2023-07-17 09:21, Bruno Larsen wrote:
> 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... 

No worries, I had seen that.  That's what I meant by being late to the party.  :-)

> I'll fix up the things you pointed in a moment and send a new patch, I just have a few questions.

Thanks.

>>
>> BTW, I think the feature is cool!
>>
>> On 2023-07-13 11:24, Bruno Larsen via Gdb-patches wrote:

>>> 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.

Makes sense.  So this is really just a shortcut for:

  (gdb) frame
  (gdb) list

>>> +      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

Actually, target_has_stack would be even more appropriate.
diff mbox series

Patch

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.
+
 * 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.  */
+      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);
+	      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.  */
+		  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\
 \"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.
 @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.
+    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
+    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"
+}
+
 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 ();
 }