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

[Bruno Larsen via Gdb-patches <gdb-patches@sourceware.org>]

On 21/06/2023 18:07, Keith Seitz wrote:
> 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").]
oof, I think my keyboard or USB hub is on its way out o7
>
> An example before(?) / after would help frame the discussion (see below).
Sure!
>
>> 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".
That's fair. I have some more suggestions for the warning message below.
>
> 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.]

This is the difficult part of this change proposal: If the last command 
for GDB was "list", we can be reasonably sure that the user would prefer 
the error message, and if the last command was something else 
(especially info locals, backtrace or some other thing that generates a 
lot of output), we can guess that they want to be back at the current 
location. Unfortunately, there is no way (to my knowledge) to get that 
info, so I went with always assuming the user wants to be back at where 
they were.

Also, about the "stuck" behavior, and the confusion about "default 
position" i guess: Since you haven't started the inferior yet, the 
default position is lines around the main function. You're not listing 
gdb/main.c anymore, you're now listing gdb/gdb.c, which only has 33 
lines. The fact that GDB isn't actually listing the function itself, 
ending at the "int" line, is old behavior already

I guess the warning message could be changed to indicate where is being 
listed, saying something like "previous list command has already reached 
the end of file -- inferior not running, listing main again" or 
"previous list (...) -- User examining inferior at line X" (The awkward 
wording of "user examining" is there because we'll list lines around the 
selected frame, so if the user moved up some frames we'll list around 
that instead which is the current behavior for "list" already).

Or I could change it so if the inferior is not running, the command 
errors out, and if it is running, the new behavior is in place. I think 
its better to error out than repeating the last few lines if we can 
assume that the user is just examining the source code (which in this 
case would be when the inferior hasn't been started).

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

I feel like repeating the last lines would not be as useful. The 
use-case I had in mind was someone who is parked close to the end of a 
file and by using list twice or three times ended up there (probably 
listing to the end of a function) and couldn't see it anymore. If the 
user exploring the source file, it is likely more useful to straight up 
say that last list command already reached the end of file.

That said, I think we could also add an argument to list at the end of a 
file, but idk what to call it.

>
> Keith
>
> PS. OOC do you know if changing this list-past-end-of-file behavior 
> alters the way MI
> currently behaves?
>
huh, it doesn't! I thought it used the same base code, but apparently 
not. I'll update the MI version to keep the behaviors synced.

-- 
Cheers,
Bruno