[PATCH 0/3] Small changes to "list" command

[PATCH 0/3] Small changes to "list" command

[Bruno Larsen]

I decided to tackle PR cli/30497, and while doing so, Andrew mentioned
that it would also be nice if we could explicitly ask GDB to print the
current location, so I also decided to add that into a series. The first
patch is just some groundwork preparation to make the rest smooth.

Bruno Larsen (3):
  gdb/cli: Factor out code to list lines for the first time
  gdb/cli: Improve UX when using list with no args
  gdb/cli: add '.' as an argument for 'list' command

 gdb/NEWS                        |  8 +++
 gdb/cli/cli-cmds.c              | 94 +++++++++++++++++++++++++++------
 gdb/source.c                    | 18 +++++++
 gdb/source.h                    |  4 ++
 gdb/testsuite/gdb.base/list.exp | 46 ++++++++++++++--
 gdb/testsuite/gdb.base/list1.c  |  2 +-
 6 files changed, 151 insertions(+), 21 deletions(-)

-- 
2.40.1

[PATCH 1/3] gdb/cli: Factor out code to list lines for the first time

[Bruno Larsen]

A future patch will add more situations that caulcates "lines around a
certain point" to be printed using print_source_lines, and the logic
could be re-used. As a preparation for those commits, this one factors
out that part of the logic of the list command into its own function.
No functional changes are expected
---
 gdb/cli/cli-cmds.c | 34 +++++++++++++++++++++-------------
 1 file changed, 21 insertions(+), 13 deletions(-)

diff --git a/gdb/cli/cli-cmds.c b/gdb/cli/cli-cmds.c
index 638c138e7cb..b0b9c08c2ec 100644
--- a/gdb/cli/cli-cmds.c
+++ b/gdb/cli/cli-cmds.c
@@ -1200,6 +1200,26 @@ pipe_command_completer (struct cmd_list_element *ignore,
      we don't know how to complete.  */
 }
 
+/* Helper for the list_command function. Resets the location to be printed
+   to the line where the inferior is stopped, then prints the lines.  */
+static void
+list_around_line (const char *arg, symtab_and_line cursal)
+{
+  int first;
+
+  first = std::max (cursal.line - get_lines_to_list () / 2, 1);
+
+  /* A small special case --- if listing backwards, and we
+     should list only one line, list the preceding line,
+     instead of the exact line we've just shown after e.g.,
+     stopping for a breakpoint.  */
+  if (arg != NULL && arg[0] == '-'
+      && get_lines_to_list () == 1 && first > 1)
+    first -= 1;
+
+  print_source_lines (cursal.symtab, source_lines_range (first), 0);
+}
+
 static void
 list_command (const char *arg, int from_tty)
 {
@@ -1221,19 +1241,7 @@ list_command (const char *arg, int from_tty)
 	 source line, center the listing around that line.  */
       if (get_first_line_listed () == 0)
 	{
-	  int first;
-
-	  first = std::max (cursal.line - get_lines_to_list () / 2, 1);
-
-	  /* A small special case --- if listing backwards, and we
-	     should list only one line, list the preceding line,
-	     instead of the exact line we've just shown after e.g.,
-	     stopping for a breakpoint.  */
-	  if (arg != NULL && arg[0] == '-'
-	      && get_lines_to_list () == 1 && first > 1)
-	    first -= 1;
-
-	  print_source_lines (cursal.symtab, source_lines_range (first), 0);
+	  list_around_line (arg, cursal);
 	}
 
       /* "l" or "l +" lists next ten lines.  */
-- 
2.40.1

[PATCH 2/3] gdb/cli: Improve UX when using list with no args

[Bruno Larsen]

When using "list" with no arguments, GDB will first print the lines
around where the inferior is stopped, then print the next N lines until
reaching the end of file, at which point it wanrs the user "Line X out
of range, file Y only has X-1 lines.".  This is usually desireable, but
if the user can no longer see the original line, they may have forgotten
the current line or that a list command was used at all, making GDB's
error message look cryptic. It was reported in bugzilla as PR cli/30497.

This commit improves the user experince by changing the behavior of
"list" slightly when a user passes no arguments.  If the line that would
be printed is past the end of the file, GDB will now warn that the
previous list command has reached the end of file, and the current line
wil be listed again.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30497
---
 gdb/NEWS                        |  5 +++++
 gdb/cli/cli-cmds.c              | 36 +++++++++++++++++++++++++++++++--
 gdb/source.c                    | 18 +++++++++++++++++
 gdb/source.h                    |  4 ++++
 gdb/testsuite/gdb.base/list.exp |  8 ++++----
 5 files changed, 65 insertions(+), 6 deletions(-)

diff --git a/gdb/NEWS b/gdb/NEWS
index fd42864c692..48147f4f916 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -78,6 +78,11 @@
   functionality is also available for dprintf when dprintf-style is
   'gdb'.
 
+* Using the 'list' command with no arguments in a situation where the
+  command would attempt to list past the end of the file, instead of
+  erroring out, it will now warn the user that the end of file has been
+  reached and the default location.
+
 * New commands
 
 maintenance print record-instruction [ N ]
diff --git a/gdb/cli/cli-cmds.c b/gdb/cli/cli-cmds.c
index b0b9c08c2ec..5973aebfad3 100644
--- a/gdb/cli/cli-cmds.c
+++ b/gdb/cli/cli-cmds.c
@@ -1244,8 +1244,40 @@ list_command (const char *arg, int from_tty)
 	  list_around_line (arg, cursal);
 	}
 
-      /* "l" or "l +" lists next ten lines.  */
-      else if (arg == NULL || arg[0] == '+')
+      /* "l" lists the next few lines, unless we're listing past the end of
+	 the file.  If it would be past the end, re-print the current line.  */
+      else if (arg == nullptr)
+	{
+	  if (can_print_line (cursal.symtab, cursal.line))
+	    print_source_lines (cursal.symtab,
+				source_lines_range (cursal.line), 0);
+	  else
+	    {
+	      warning (_("previous list command has already reached the end "
+			 "of the file. Listing default location again"));
+	      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);
+	    }
+	}
+
+      /* "l +" always lists next few lines.  */
+      else if (arg[0] == '+')
 	print_source_lines (cursal.symtab,
 			    source_lines_range (cursal.line), 0);
 
diff --git a/gdb/source.c b/gdb/source.c
index 9997cccb31b..1aa08c6e080 100644
--- a/gdb/source.c
+++ b/gdb/source.c
@@ -1484,6 +1484,24 @@ print_source_lines (struct symtab *s, source_lines_range line_range,
 			   line_range.stopline (), flags);
 }
 
+/* See source.h.  */
+
+bool
+can_print_line (struct symtab *s, int line)
+{
+  const std::vector<off_t> *offset_p;
+
+  /* If we can't get the offsets, we definitely can't print the line.  */
+  if (!g_source_cache.get_line_charpos (s, &offset_p))
+    return false;
+  if (offset_p == nullptr)
+    return false;
+
+  /* Otherwise we are able to print LINE if there are at least that many
+     lines in the symtab.  */
+  return line <= offset_p->size ();
+}
+
 
 \f
 /* Print info on range of pc's in a specified line.  */
diff --git a/gdb/source.h b/gdb/source.h
index 8fbc365680d..ae6764322d0 100644
--- a/gdb/source.h
+++ b/gdb/source.h
@@ -192,6 +192,10 @@ class source_lines_range
   int m_stopline;
 };
 
+/* Check if the line LINE can be found in the symtab S, so that it can be
+   printed.  */
+extern bool can_print_line (struct symtab *s, int line);
+
 /* Variation of previous print_source_lines that takes a range instead of a
    start and end line number.  */
 extern void print_source_lines (struct symtab *s, source_lines_range r,
diff --git a/gdb/testsuite/gdb.base/list.exp b/gdb/testsuite/gdb.base/list.exp
index 18ecd13ac0f..35e099ebaff 100644
--- a/gdb/testsuite/gdb.base/list.exp
+++ b/gdb/testsuite/gdb.base/list.exp
@@ -175,8 +175,8 @@ proc_with_prefix test_list_forward {} {
 	"list 25-34"
     gdb_test "list" "35\[ \t\]+foo \\(.*\\);.*${last_line_re}" \
 	"list 35-42"
-    gdb_test "list" "Line number 44 out of range; \[^\r\n\]+ has 43 lines\." \
-	"end of file error after \"list\" command"
+    gdb_test "list" "warning: previous list command has already reached the end of the file. Listing default location again.*1\[ \t\]+#include \"list0.h\".*" \
+	"list past end of file"
 }
 
 # Test that repeating the list linenum command doesn't print the same
@@ -194,8 +194,8 @@ proc_with_prefix test_repeat_list_command {} {
 	"list 25-34"
     gdb_test " " "35\[ \t\]+foo \\(.*\\);.*${last_line_re}" \
 	"list 35-42"
-    gdb_test "list" "Line number 44 out of range; \[^\r\n\]+ has 43 lines\." \
-	"end of file error after using 'return' to repeat the list command"
+    gdb_test "list" "warning: previous list command has already reached the end of the file. Listing default location again.*1\[ \t\]+#include \"list0.h\".*" \
+	"list past end of file"
 }
 
 proc_with_prefix test_list_backwards {} {
-- 
2.40.1

Re: [PATCH 2/3] gdb/cli: Improve UX when using list with no args

[Eli Zaretskii]

> Cc: Bruno Larsen <blarsen@redhat.com>
> Date: Tue, 20 Jun 2023 14:29:12 +0200
> From: Bruno Larsen via Gdb-patches <gdb-patches@sourceware.org>
> 
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -78,6 +78,11 @@
>    functionality is also available for dprintf when dprintf-style is
>    'gdb'.
>  
> +* Using the 'list' command with no arguments in a situation where the
> +  command would attempt to list past the end of the file, instead of
> +  erroring out, it will now warn the user that the end of file has been
> +  reached and the default location.
             ^^^^^^^^^^^^^^^^^^^^^^^^
What is that "and the default location" part about?  It looks
misplaced.

Thanks.

Re: [PATCH 2/3] gdb/cli: Improve UX when using list with no args

[Bruno Larsen]

On 20/06/2023 15:43, Eli Zaretskii wrote:
>> Cc: Bruno Larsen <blarsen@redhat.com>
>> Date: Tue, 20 Jun 2023 14:29:12 +0200
>> From: Bruno Larsen via Gdb-patches <gdb-patches@sourceware.org>
>>
>> --- a/gdb/NEWS
>> +++ b/gdb/NEWS
>> @@ -78,6 +78,11 @@
>>     functionality is also available for dprintf when dprintf-style is
>>     'gdb'.
>>   
>> +* Using the 'list' command with no arguments in a situation where the
>> +  command would attempt to list past the end of the file, instead of
>> +  erroring out, it will now warn the user that the end of file has been
>> +  reached and the default location.
>               ^^^^^^^^^^^^^^^^^^^^^^^^
> What is that "and the default location" part about?  It looks
> misplaced.
>
Oops. It is incomplete. It should have been "And the default location 
will be printed", default location being either where the inferior is 
(if it has started already) or around the main function.

-- 
Cheers,
Bruno

Re: [PATCH 2/3] gdb/cli: Improve UX when using list with no args

[Eli Zaretskii]

> Date: Tue, 20 Jun 2023 15:46:20 +0200
> Cc: gdb-patches@sourceware.org
> From: Bruno Larsen <blarsen@redhat.com>
> 
> On 20/06/2023 15:43, Eli Zaretskii wrote:
> >> Cc: Bruno Larsen <blarsen@redhat.com>
> >> Date: Tue, 20 Jun 2023 14:29:12 +0200
> >> From: Bruno Larsen via Gdb-patches <gdb-patches@sourceware.org>
> >>
> >> --- a/gdb/NEWS
> >> +++ b/gdb/NEWS
> >> @@ -78,6 +78,11 @@
> >>     functionality is also available for dprintf when dprintf-style is
> >>     'gdb'.
> >>   
> >> +* Using the 'list' command with no arguments in a situation where the
> >> +  command would attempt to list past the end of the file, instead of
> >> +  erroring out, it will now warn the user that the end of file has been
> >> +  reached and the default location.
> >               ^^^^^^^^^^^^^^^^^^^^^^^^
> > What is that "and the default location" part about?  It looks
> > misplaced.
> >
> Oops. It is incomplete. It should have been "And the default location 
> will be printed", default location being either where the inferior is 
> (if it has started already) or around the main function.

If so, I think this also warrants to be mentioned in the manual.

Thanks.

Re: [PATCH 2/3] gdb/cli: Improve UX when using list with no args

[Bruno Larsen]

On 20/06/2023 15:48, Eli Zaretskii wrote:
>> Date: Tue, 20 Jun 2023 15:46:20 +0200
>> Cc: gdb-patches@sourceware.org
>> From: Bruno Larsen <blarsen@redhat.com>
>>
>> On 20/06/2023 15:43, Eli Zaretskii wrote:
>>>> Cc: Bruno Larsen <blarsen@redhat.com>
>>>> Date: Tue, 20 Jun 2023 14:29:12 +0200
>>>> From: Bruno Larsen via Gdb-patches <gdb-patches@sourceware.org>
>>>>
>>>> --- a/gdb/NEWS
>>>> +++ b/gdb/NEWS
>>>> @@ -78,6 +78,11 @@
>>>>      functionality is also available for dprintf when dprintf-style is
>>>>      'gdb'.
>>>>    
>>>> +* Using the 'list' command with no arguments in a situation where the
>>>> +  command would attempt to list past the end of the file, instead of
>>>> +  erroring out, it will now warn the user that the end of file has been
>>>> +  reached and the default location.
>>>                ^^^^^^^^^^^^^^^^^^^^^^^^
>>> What is that "and the default location" part about?  It looks
>>> misplaced.
>>>
>> Oops. It is incomplete. It should have been "And the default location
>> will be printed", default location being either where the inferior is
>> (if it has started already) or around the main function.
> If so, I think this also warrants to be mentioned in the manual.
Will do! And I'll also add the manual entry to patch 3, as you asked

-- 
Cheers,
Bruno

[PATCH 3/3] gdb/cli: add '.' as an argument for 'list' command

[Bruno Larsen]

Currently, after the user has used the list command once, there is no
way to ask GDB to print the location where the inferior is stopped.
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 throws an
error.  The test gdb.base/list.exp was updated to test this new argument.

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                        |  3 +++
 gdb/cli/cli-cmds.c              | 26 ++++++++++++++++++++--
 gdb/testsuite/gdb.base/list.exp | 38 +++++++++++++++++++++++++++++++++
 gdb/testsuite/gdb.base/list1.c  |  2 +-
 4 files changed, 66 insertions(+), 3 deletions(-)

diff --git a/gdb/NEWS b/gdb/NEWS
index 48147f4f916..debe07847d2 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -83,6 +83,9 @@
   erroring out, it will now warn the user that the end of file has been
   reached and the default location.
 
+* The 'list' command now accepts '.' as a parameter, which tells GDB to
+  print the default location.
+
 * New commands
 
 maintenance print record-instruction [ N ]
diff --git a/gdb/cli/cli-cmds.c b/gdb/cli/cli-cmds.c
index 5973aebfad3..a966142eaea 100644
--- a/gdb/cli/cli-cmds.c
+++ b/gdb/cli/cli-cmds.c
@@ -1232,14 +1232,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);
 	}
@@ -1293,6 +1293,27 @@ 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.  */
+	      error (_("Inferior is not running. No current location."));
+	    }
+	  list_around_line (arg, cursal);
+	}
+
       return;
     }
 
@@ -2800,6 +2821,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/testsuite/gdb.base/list.exp b/gdb/testsuite/gdb.base/list.exp
index 35e099ebaff..f853a9b814d 100644
--- a/gdb/testsuite/gdb.base/list.exp
+++ b/gdb/testsuite/gdb.base/list.exp
@@ -400,6 +400,41 @@ 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 correctly identifies that the inferior
+    # isn't running.
+    gdb_test "list ." "Inferior is not running. No current location." \
+	"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"
+}
+
 clean_restart
 gdb_file_cmd ${binfile}
 
@@ -519,4 +554,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 ();
 }
-- 
2.40.1

Re: [PATCH 3/3] gdb/cli: add '.' as an argument for 'list' command

[Eli Zaretskii]

> Cc: Bruno Larsen <blarsen@redhat.com>
> Date: Tue, 20 Jun 2023 14:29:13 +0200
> From: Bruno Larsen via Gdb-patches <gdb-patches@sourceware.org>
> 
> diff --git a/gdb/NEWS b/gdb/NEWS
> index 48147f4f916..debe07847d2 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -83,6 +83,9 @@
>    erroring out, it will now warn the user that the end of file has been
>    reached and the default location.
>  
> +* The 'list' command now accepts '.' as a parameter, which tells GDB to
> +  print the default location.        ^^^^^^^^^^^^^^

"as an argument" is better, I guess?

Otherwise, the NEWS part is OK.

Shouldn't this feature be reflected in the manual as well?