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

Keith Seitz via Gdb-patches
2023-06-21T16:09:09Z

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?