[v2] gdb/cli: fixes to newly added "list ." command

  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(-)
  

> Cc: pedro@palves.net,
> 	Bruno Larsen <blarsen@redhat.com>
> Date: Fri, 21 Jul 2023 12:26:56 +0200
> From: Bruno Larsen via Gdb-patches <gdb-patches@sourceware.org>
> 
> 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

Thanks.

> --- 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
                                                   ^^
Two spaces there, please.

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

Reviewed-By: Eli Zaretskii <eliz@gnu.org>
  

Ping!
  

On 04/08/2023 10:37, Bruno Larsen wrote:
> Ping!
>
  

Bruno Larsen via Gdb-patches <gdb-patches@sourceware.org> 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