Re: [PATCH v3] gdb: Change "list ." command's error when no debuginfo is available

[Tom de Vries <tdevries@suse.de>]

On 5/8/24 19:13, Guinevere Larsen wrote:
> On 5/8/24 11:25, Andrew Burgess wrote:
>> Guinevere Larsen <blarsen@redhat.com> writes:
>>
>>> From: Simon Marchi <simark@simark.ca>
>>>
>>> Currently, when a user tries to list the current location, there are 2
>>> different error messages that can happen, either:
>>>
>>>      (gdb) list .
>>>      No symbol table is loaded.  Use the "file" command.
>>> or
>>>      (gdb) list .
>>>      No debug information available to print source lines.
>>>
>>> The difference here is if gdb can find any symtabs at all or not, which
>>> is not something too important for end-users - and isn't informative at
>>> all. This commit changes it so that the error always says that there
>>> isn't debug information available, with these two variants:
>>>
>>>      (gdb) list .
>>>      Insufficient debug info for showing source lines at current PC 
>>> (0x55555555511d).
>>> or
>>>      (gdb) list .
>>>      Insufficient debug info for showing source lines at default 
>>> location.
>>>
>>> The difference now is if the inferior has started already, which is
>>> controlled by the user and may be useful.
>>>
>>> Unfortunately, it isn't as easy to differentiate if the symtab found for
>>> other list parameters is correct, so other invocations, such as "list +"
>>> still retain their original error message.
>>>
>>> Co-Authored-By: Simon Marchi <simark@simark.ca>
>>> Reviewed-By: Eli Zaretskii <eliz@gnu.org>
>>> ---
>>> Changes for v3:
>>>    Changed error message to use Eli's suggestion
>>>    Added Eli's RB tag since he approved documentation changes
>>>
>>> Changes for v2:
>>>    Added NEWS entry, should have done that from the start oops.
>>>    Added test. This test aims to roughly recreate a situation where the
>>>      current function is in a spot with no debuginfo, and being called
>>>      from somewhere that has debuginfo.
>>>
>>> ---
>>>   gdb/NEWS                                      |  6 ++
>>>   gdb/cli/cli-cmds.c                            | 47 +++++++++----
>>>   .../gdb.base/list-dot-nodebug-extra.c         | 24 +++++++
>>>   gdb/testsuite/gdb.base/list-dot-nodebug.c     | 33 +++++++++
>>>   gdb/testsuite/gdb.base/list-dot-nodebug.exp   | 67 +++++++++++++++++++
>>>   5 files changed, 163 insertions(+), 14 deletions(-)
>>>   create mode 100644 gdb/testsuite/gdb.base/list-dot-nodebug-extra.c
>>>   create mode 100644 gdb/testsuite/gdb.base/list-dot-nodebug.c
>>>   create mode 100644 gdb/testsuite/gdb.base/list-dot-nodebug.exp
>>>
>>> diff --git a/gdb/NEWS b/gdb/NEWS
>>> index feb3a37393a..99909414796 100644
>>> --- a/gdb/NEWS
>>> +++ b/gdb/NEWS
>>> @@ -36,6 +36,12 @@ set unwindonsignal on|off
>>>   show unwindonsignal
>>>     These commands are now aliases for the new set/show 
>>> unwind-on-signal.
>>> +list .
>>> +  When using the command "list ." in a location that has no debug 
>>> information
>>> +  or no file loaded, GDB now says that there is no debug information 
>>> to print
>>> +  lines.  This makes it more obvious that there is no information, 
>>> as opposed
>>> +  to implying there is no inferior loaded.
>>> +
>>>   * New commands
>>>   info missing-debug-handler
>>> diff --git a/gdb/cli/cli-cmds.c b/gdb/cli/cli-cmds.c
>>> index 3afe2178199..ee3318d7910 100644
>>> --- a/gdb/cli/cli-cmds.c
>>> +++ b/gdb/cli/cli-cmds.c
>>> @@ -1235,37 +1235,39 @@ list_command (const char *arg, int from_tty)
>>>     /* Pull in the current default source line if necessary.  */
>>>     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 && (arg == nullptr || 
>>> arg[0] != '.'))
>>>       {
>>> -      list_around_line (arg, cursal);
>>> +      set_default_source_symtab_and_line ();
>>> +      list_around_line (arg, get_current_source_symtab_and_line ());
>>>       }
>>>         /* "l" and "l +" lists the next few lines, unless we're 
>>> listing past
>>>        the end of the file.  */
>>>         else if (arg == nullptr || arg[0] == '+')
>>>       {
>>> +      set_default_source_symtab_and_line ();
>>> +      const symtab_and_line cursal = 
>>> get_current_source_symtab_and_line ();
>>>         if (last_symtab_line (cursal.symtab) >= cursal.line)
>>>           print_source_lines (cursal.symtab,
>>>                   source_lines_range (cursal.line), 0);
>>>         else
>>> -        {
>>> -          error (_("End of the file was already reached, use \"list 
>>> .\" to"
>>> -               " list the current location again"));
>>> -        }
>>> +        error (_("End of the file was already reached, use \"list 
>>> .\" to"
>>> +             " list the current location again"));
>>>       }
>>>         /* "l -" lists previous ten lines, the ones before the ten just
>>>        listed.  */
>>>         else if (arg[0] == '-')
>>>       {
>>> +      set_default_source_symtab_and_line ();
>>> +      const symtab_and_line cursal = 
>>> get_current_source_symtab_and_line ();
>>> +
>>>         if (get_first_line_listed () == 1)
>>>           error (_("Already at the start of %s."),
>>>              symtab_to_filename_for_display (cursal.symtab));
>>> +
>>>         source_lines_range range (get_first_line_listed (),
>>>                       source_lines_range::BACKWARD);
>>>         print_source_lines (cursal.symtab, range, 0);
>>> @@ -1274,25 +1276,42 @@ list_command (const char *arg, int from_tty)
>>>         /* "list ." lists the default location again.  */
>>>         else if (arg[0] == '.')
>>>       {
>>> +      std::optional<const symtab_and_line> cursal;
>>>         if (target_has_stack ())
>>>           {
>>>             /* 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);
>>> +          cursal.emplace (find_pc_line (curr_pc, 0));
>>> +
>>> +          if (cursal->symtab == nullptr)
>>> +        error
>>> +          (_("Insufficient debug info for showing source lines at "
>>> +             "current PC (%s)."), paddress (get_frame_arch (frame),
>>> +                            curr_pc));
>>>           }
>>>         else
>>>           {
>>>             /* The inferior is not running, so reset the current source
>>>            location to the default (usually the main function).  */
>>>             clear_current_source_symtab_and_line ();
>>> -          set_default_source_symtab_and_line ();
>>> -          cursal = get_current_source_symtab_and_line ();
>>> +          try
>>> +        {
>>> +          set_default_source_symtab_and_line ();
>>> +        }
>>> +          catch (const gdb_exception &e)
>>> +        {
>>> +          error (_("Insufficient debug info for showing source "
>>> +               "lines at default location"));
>>> +        }
>>> +          cursal.emplace (get_current_source_symtab_and_line ());
>>> +
>>> +          gdb_assert (cursal->symtab != nullptr);
>>>           }
>>> -      if (cursal.symtab == nullptr)
>>> -        error (_("No debug information available to print source 
>>> lines."));
>>> -      list_around_line (arg, cursal);
>>> +
>>> +      list_around_line (arg, *cursal);
>>> +
>>>         /* Set the repeat args so just pressing "enter" after using 
>>> "list ."
>>>            will print the following lines instead of the same lines 
>>> again. */
>>>         if (from_tty)
>>> diff --git a/gdb/testsuite/gdb.base/list-dot-nodebug-extra.c 
>>> b/gdb/testsuite/gdb.base/list-dot-nodebug-extra.c
>>> new file mode 100644
>>> index 00000000000..c3d2416e70d
>>> --- /dev/null
>>> +++ b/gdb/testsuite/gdb.base/list-dot-nodebug-extra.c
>>> @@ -0,0 +1,24 @@
>>> +/* This testcase is part of GDB, the GNU debugger.
>>> +
>>> +   Copyright 2024 Free Software Foundation, Inc.
>>> +
>>> +   This program is free software; you can redistribute it and/or modify
>>> +   it under the terms of the GNU General Public License as published by
>>> +   the Free Software Foundation; either version 3 of the License, or
>>> +   (at your option) any later version.
>>> +
>>> +   This program is distributed in the hope that it will be useful,
>>> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>> +   GNU General Public License for more details.
>>> +
>>> +   You should have received a copy of the GNU General Public License
>>> +   along with this program.  If not, see 
>>> <http://www.gnu.org/licenses/>.  */
>>> +
>>> +extern void bar(int *);
>> Space missing after 'bar' here.
>>
>>> +
>>> +void
>>> +foo (int *x)
>>> +{
>>> +  bar (x);
>>> +}
>>> diff --git a/gdb/testsuite/gdb.base/list-dot-nodebug.c 
>>> b/gdb/testsuite/gdb.base/list-dot-nodebug.c
>>> new file mode 100644
>>> index 00000000000..b37c3561c41
>>> --- /dev/null
>>> +++ b/gdb/testsuite/gdb.base/list-dot-nodebug.c
>>> @@ -0,0 +1,33 @@
>>> +/* This testcase is part of GDB, the GNU debugger.
>>> +
>>> +   Copyright 2024 Free Software Foundation, Inc.
>>> +
>>> +   This program is free software; you can redistribute it and/or modify
>>> +   it under the terms of the GNU General Public License as published by
>>> +   the Free Software Foundation; either version 3 of the License, or
>>> +   (at your option) any later version.
>>> +
>>> +   This program is distributed in the hope that it will be useful,
>>> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>> +   GNU General Public License for more details.
>>> +
>>> +   You should have received a copy of the GNU General Public License
>>> +   along with this program.  If not, see 
>>> <http://www.gnu.org/licenses/>.  */
>>> +
>>> +extern void foo (int *x);
>>> +
>>> +int x;
>>> +
>>> +void
>>> +bar (int *p)
>>> +{
>>> +  *p++;
>>> +}
>>> +
>>> +int
>>> +main ()
>>> +{
>>> +  foo (&x);
>>> +  return 0;
>>> +}
>>> diff --git a/gdb/testsuite/gdb.base/list-dot-nodebug.exp 
>>> b/gdb/testsuite/gdb.base/list-dot-nodebug.exp
>>> new file mode 100644
>>> index 00000000000..7c4144da8ab
>>> --- /dev/null
>>> +++ b/gdb/testsuite/gdb.base/list-dot-nodebug.exp
>>> @@ -0,0 +1,67 @@
>>> +# Copyright 2005-2024 Free Software Foundation, Inc.
>>> +
>>> +# This program is free software; you can redistribute it and/or modify
>>> +# it under the terms of the GNU General Public License as published by
>>> +# the Free Software Foundation; either version 3 of the License, or
>>> +# (at your option) any later version.
>>> +#
>>> +# This program is distributed in the hope that it will be useful,
>>> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>> +# GNU General Public License for more details.
>>> +#
>>> +# You should have received a copy of the GNU General Public License
>>> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
>>> +
>>> +# This test is here to confirm that the command "list ." will print the
>>> +# same message if GDB detects no debug information at all, or 
>>> detects some
>>> +# but nothing for the current objfile.
>>> +
>>> +require !use_gdb_stub
>>> +
>>> +set linkflags [list additional_flags="-static"]
>>> +
>>> +if { ![gdb_can_simple_compile static-libc \
>>> +       {
>>> +           void main (void) { return 0; }
>>> +       } \
>>> +       executable $linkflags] } {
>>> +    untested "Can't statically link"
>>> +    return -1
>>> +}
>>> +
>>> +standard_testfile .c -extra.c
>>> +set objmainfile [standard_output_file ${testfile}-main.o]
>>> +set objextrafile [standard_output_file ${testfile}-extra.o]
>>> +
>>> +if {[gdb_compile "$srcdir/$subdir/$srcfile" $objmainfile object 
>>> {nodebug}] != "" } {
>>> +    untested "couldn't compile main file into object"
>>> +    return -1
>>> +}
>> The gdb_compile line seems redundant, you recompile the test binary
>> within the foreach loop.  With this gone the objmainfile and
>> objextrafile variables seem redundant too.
>>
>>> +
>>> +foreach_with_prefix debug {"none" "some"} {
>>> +
>>> +    set flags "nodebug"
>>> +    if {$debug == "some"} {
>>> +    set flags "debug"
>>> +    }
>>> +
>>> +    if {[prepare_for_testing_full "failed to prepare" \
>>> +        [list ${testfile}-${debug} $linkflags \
>>> +        $srcfile [list nodebug] \
>>> +        $srcfile2 [list $debug]]]} {
>>> +    return -1
>>> +    }
>>> +
>>> +    gdb_test "list ." \
>>> +    "No debug information available to print source lines.*" \
>> I would replace this pattern with:
>>
>>     "^Insufficient debug info for showing source lines at default 
>> location" \
>>
>> this extends the text you used in your earlier reply, but avoids using
>> '.*'.  I think it's better to avoid '.*' unless it's essential.  With
>> this patter we're saying we expect that line, and that line only.
>> Absolutely no other output.
>>
>>> +    "print before start"
>>> +
>>> +    if { ![runto bar] } {
>>> +    return -1
>>> +    }
>>> +
>>> +    gdb_test "list ." \
>>> +    "No debug information available to print source lines at current 
>>> PC.*" \
>> And here:
>>
>>     "^Insufficient debug info for showing source lines at current PC 
>> \\($::hex\\)\\." \
>>
>> With these testsuite fixes, and the other things you changed in your
>> earlier reply:
>>
>> Approved-By: Andrew Burgess <aburgess@redhat.com>
> Thanks, pushed!
> 
 >

I run into fails in the new test-case:
...
FAIL: gdb.base/list-dot-nodebug.exp: debug=none: print before start
FAIL: gdb.base/list-dot-nodebug.exp: debug=some: print before start
...

Filed here ( https://sourceware.org/bugzilla/show_bug.cgi?id=31721 ).

Thanks,
- Tom