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

[Keith Seitz <keiths@redhat.com>]

Hi,

Apologies, I'm late to the review here, so if I'm stepping on other's
toes, please ignore me!

On 6/21/23 03:45, Bruno Larsen via Gdb-patches wrote:
> 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.

I've run into this myself over the years. While I've adapted to it, you
(and the original bz reporter) have a very valid point. This is simply
not very user-friendly. So thank you for posting something to address this.

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

[If this is to be your commit message, please note there are several
typos in these two paragraphs ("desireable", "experince").]

An example before(?) / after would help frame the discussion (see below).

> 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"));

[Warning: You may want to skip this nit-picking and jump to "HOWEVER" below!]

Nit: Grep'ping through the code, I see that some warnings are implemented
as full sentences and some as incomplete sentences/phrases. This is both.
I'm not a fan. I would prefer full sentences or "previous list command has
already reached the end of the file -- listing default location again".

On that front, I'm not sure I like the whole "Listing default location
again" phrase. Do users typically know what a "default" location is?

I don't see this term specifically defined in either the manual or the
proposed patches? [I see there is a definition in NEWS which seems
insufficient.]

HOWEVER, if I may offer up an example around which to discuss things...

Current behavior, using "list main.c:0" on gdb's sources, and simply hitting
[Enter] until the behavior of interest appears:

(top-gdb)
1481	    gdb_printf (stream, _("\n\
1482	Report bugs to %ps.\n\
1483	"), styled_string (file_name_style.style (), REPORT_BUGS_TO));
1484	  if (stream == gdb_stdout)
1485	    gdb_printf (stream, _("\n\
1486	You can ask GDB-related questions on the GDB users mailing list\n\
1487	(gdb@sourceware.org) or on GDB's IRC channel (#gdb on Libera.Chat).\n"));
1488	}
(top-gdb)

Line number 1489 out of range; ../../src/gdb/main.c has 1488 lines.
(top-gdb)

The proposal here changes this to:

(top-gdb)
1481	    gdb_printf (stream, _("\n\
1482	Report bugs to %ps.\n\
1483	"), styled_string (file_name_style.style (), REPORT_BUGS_TO));
1484	  if (stream == gdb_stdout)
1485	    gdb_printf (stream, _("\n\
1486	You can ask GDB-related questions on the GDB users mailing list\n\
1487	(gdb@sourceware.org) or on GDB's IRC channel (#gdb on Libera.Chat).\n"));
1488	}
(top-gdb)
warning: previous list command has already reached the end of the file. Listing default location again
14	   GNU General Public License for more details.
15	
16	   You should have received a copy of the GNU General Public License
17	   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
18	
19	#include "defs.h"
20	#include "main.h"
21	#include "interps.h"
22	
23	int
(top-gdb)

TBH, I was surprised by this. If a user was looking at the end of a source
file, would they really want to suddenly jump to the top? [They could always
"list 0" to easily accomplish that.]

Also note that as implemented in these patches, hitting [Enter] AGAIN gets "stuck"
in a weird way [this scenario needs tests]:

(top-gdb)
1481	    gdb_printf (stream, _("\n\
1482	Report bugs to %ps.\n\
1483	"), styled_string (file_name_style.style (), REPORT_BUGS_TO));
1484	  if (stream == gdb_stdout)
1485	    gdb_printf (stream, _("\n\
1486	You can ask GDB-related questions on the GDB users mailing list\n\
1487	(gdb@sourceware.org) or on GDB's IRC channel (#gdb on Libera.Chat).\n"));
1488	}
(top-gdb)
warning: previous list command has already reached the end of the file. Listing default location again
14	   GNU General Public License for more details.
15	
16	   You should have received a copy of the GNU General Public License
17	   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
18	
19	#include "defs.h"
20	#include "main.h"
21	#include "interps.h"
22	
23	int
(top-gdb)
24	main (int argc, char **argv)
25	{
26	  struct captured_main_args args;
27	
28	  memset (&args, 0, sizeof args);
29	  args.argc = argc;
30	  args.argv = argv;
31	  args.interpreter_p = INTERP_CONSOLE;
32	  return gdb_main (&args);
33	}
(top-gdb)
warning: previous list command has already reached the end of the file. Listing default location again
14	   GNU General Public License for more details.
15	
16	   You should have received a copy of the GNU General Public License
17	   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
18	
19	#include "defs.h"
20	#include "main.h"
21	#include "interps.h"
22	
23	int
(top-gdb)

The "stuck" behavior/bug here aside, I would like to suggest a simpler solution:
list the last N lines of the file when attempting to list past the last line.

Something like (completely uncoded/artificially created):

(top-gdb)
1481	    gdb_printf (stream, _("\n\
1482	Report bugs to %ps.\n\
1483	"), styled_string (file_name_style.style (), REPORT_BUGS_TO));
1484	  if (stream == gdb_stdout)
1485	    gdb_printf (stream, _("\n\
1486	You can ask GDB-related questions on the GDB users mailing list\n\
1487	(gdb@sourceware.org) or on GDB's IRC channel (#gdb on Libera.Chat).\n"));
1488	}
(top-gdb)
Line number 1489 out of range; ../../src/gdb/main.c has 1488 lines.
1481	    gdb_printf (stream, _("\n\
1482	Report bugs to %ps.\n\
1483	"), styled_string (file_name_style.style (), REPORT_BUGS_TO));
1484	  if (stream == gdb_stdout)
1485	    gdb_printf (stream, _("\n\
1486	You can ask GDB-related questions on the GDB users mailing list\n\
1487	(gdb@sourceware.org) or on GDB's IRC channel (#gdb on Libera.Chat).\n"));
1488	}
[and keep repeating this behavior until a location is explicitly given by the
user]

That keeps the user focused at the end of the file. [AFAIK, we have no
convenient/easy way to jump to the end of a file.]

WDYT?

Keith

PS. OOC do you know if changing this list-past-end-of-file behavior alters the way MI
currently behaves?