gdb: continue start command if 'main' can be resolved

  From: "Rohr, Stephan" <stephan.rohr@intel.com>

GDB aborts the 'start' command if the minimal symbols cannot be
resolved.  On Windows, GDB reads the minimal symbols from the COFF
header of the PE file.  The symbol table is deprecated and the
number of symbols in the COFF header may be zero [1].  For example,
the LLVM clang compiler for Windows MSVC can be instructed to generate
DWARF:

  clang++ -g -O0 -gdwarf -fuse-ld=lld test.cpp -o test_clang

The corresponding COFF file header shows:

  FILE HEADER VALUES
        8664 machine (x64)
           E number of sections
    66E889EC time date stamp Mon Sep 16 21:41:32 2024
       FB400 file pointer to symbol table
           0 number of symbols
          F0 size of optional header
          22 characteristics

GDB is not able to read the minimal symbols; the `start' command fails
with an error:

  (gdb) start
  No symbol table loaded.  Use the "file" command.

Manually inserting a breakpoint in main works fine:

  (gdb) tbreak main
  Temporary breakpoint 1 at 0x14000100c: file test.cpp, line 6.
  (gdb) run
  Starting program: C:\test-clang

  Temporary breakpoint 1, main () at test.cpp:6
  6         std::cout << "Hello World.\n";

Fix this by testing if `main' can be resolved instead of checking the
minimal symbol table:

  (gdb) start
  Temporary breakpoint 1 at 0x14000100c: file test.cpp, line 6.
  Starting program: C:\test-clang

  Temporary breakpoint 1, main () at test.cpp:6
  6         std::cout << "Hello World.\n";
  (gdb)

[1]: https://learn.microsoft.com/en-us/windows/win32/debug/pe-format
---
 gdb/infcmd.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)
  

On 12/4/24 10:16 AM, Stephan Rohr wrote:
> From: "Rohr, Stephan" <stephan.rohr@intel.com>
>
> GDB aborts the 'start' command if the minimal symbols cannot be
> resolved.  On Windows, GDB reads the minimal symbols from the COFF
> header of the PE file.  The symbol table is deprecated and the
> number of symbols in the COFF header may be zero [1].  For example,
> the LLVM clang compiler for Windows MSVC can be instructed to generate
> DWARF:
>
>    clang++ -g -O0 -gdwarf -fuse-ld=lld test.cpp -o test_clang
>
> The corresponding COFF file header shows:
>
>    FILE HEADER VALUES
>          8664 machine (x64)
>             E number of sections
>      66E889EC time date stamp Mon Sep 16 21:41:32 2024
>         FB400 file pointer to symbol table
>             0 number of symbols
>            F0 size of optional header
>            22 characteristics
>
> GDB is not able to read the minimal symbols; the `start' command fails
> with an error:
>
>    (gdb) start
>    No symbol table loaded.  Use the "file" command.
>
> Manually inserting a breakpoint in main works fine:
>
>    (gdb) tbreak main
>    Temporary breakpoint 1 at 0x14000100c: file test.cpp, line 6.
>    (gdb) run
>    Starting program: C:\test-clang
>
>    Temporary breakpoint 1, main () at test.cpp:6
>    6         std::cout << "Hello World.\n";
>
> Fix this by testing if `main' can be resolved instead of checking the
> minimal symbol table:
>
>    (gdb) start
>    Temporary breakpoint 1 at 0x14000100c: file test.cpp, line 6.
>    Starting program: C:\test-clang
>
>    Temporary breakpoint 1, main () at test.cpp:6
>    6         std::cout << "Hello World.\n";
>    (gdb)
>
> [1]: https://learn.microsoft.com/en-us/windows/win32/debug/pe-format
> ---
>   gdb/infcmd.c | 9 +++++----
>   1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/gdb/infcmd.c b/gdb/infcmd.c
> index 5c0e3f51162..39b7e50b582 100644
> --- a/gdb/infcmd.c
> +++ b/gdb/infcmd.c
> @@ -516,10 +516,11 @@ run_command (const char *args, int from_tty)
>   static void
>   start_command (const char *args, int from_tty)
>   {
> -  /* Some languages such as Ada need to search inside the program
> -     minimal symbols for the location where to put the temporary
> -     breakpoint before starting.  */
> -  if (!have_minimal_symbols (current_program_space))
> +  /* Abort the start command if `main` cannot be resolved, e.g., the
> +     minimal / partial symbols are not available.  Some languages such
> +     as Ada need to search inside the program minimal symbols for the
> +     location where to put the temporary breakpoint before starting.  */
> +  if (main_name () == nullptr)
>       error (_("No symbol table loaded.  Use the \"file\" command."));
>   
>     /* Run the program until reaching the main procedure...  */

I agree that minimal symbols shouldn't be required (obviously by your 
example), but I don't think knowing the name of the main function is enough.

If you look at how main_name works, it calls find_main_name if the name 
isn't cached yet, and find_main_name has a default fallback of guessing 
"main" with an unknown language if needed. In other words, even if we 
can't find that data in the inferior, we'll still guess something. I 
bring this up because if there is no minimal symbols and no debug info, 
I think GDB wouldn't be able to set a breakpoint because main can't be 
translated in any way, and so "start" would end up being silently 
converted to "run" and confuse the user. If this assumption is wrong, 
feel free to ignore the rest of the message.

I think the central issue for whether "start" works should be whether we 
can set the temporary breakpoint or not, rather than whether we can 
guess where to set it. I'm not sure if setting the tbreak would be 
possible here in the start_command due to all the preparatory work in 
run_command_1 before setting the bp (especially due to the commend in 
infcmd.c:411), but I think it would be ideal if it is possible. If that 
doesn't work, I would imagine run_command_1 should check if 
tbreak_command succeeds and return early if it fails. The issues with 
this second strategy is that the inferior is stopped and info is cleared 
before we're sure that we can run the full command, which is pretty 
annoying for the user, but I'it might be better than having to set a 
breakpoint twice (in case someone is using a slow connection to a 
gdbserver, for instance).
  

Hi Guinevere,

thanks for your feedback.

I agree on your concerns, 'main_name ()' might not be the best solution for this issue.

I also think that moving the whole breakpoint setup into 'start_command' is not
feasible.  I modified ' run_command_1' to report an error if the (temporary) breakpoint
cannot be inserted, this also fixes the issue.  I had to do some modifications:

  * Make 'break_command_1' externally visible.
  * Use 'break_command_1' to insert the temporary breakpoint instead of 'tbreak_command'.

I wonder if making 'break_command_1' externally visible could be an acceptable solution?

Thanks
Stephan

> -----Original Message-----
> From: Guinevere Larsen <guinevere@redhat.com>
> Sent: Wednesday, 4 December 2024 14:44
> To: Rohr, Stephan <stephan.rohr@intel.com>; gdb-patches@sourceware.org
> Subject: Re: [PATCH] gdb: continue start command if 'main' can be resolved
> 
> On 12/4/24 10:16 AM, Stephan Rohr wrote:
> > From: "Rohr, Stephan" <stephan.rohr@intel.com>
> >
> > GDB aborts the 'start' command if the minimal symbols cannot be
> > resolved.  On Windows, GDB reads the minimal symbols from the COFF
> > header of the PE file.  The symbol table is deprecated and the
> > number of symbols in the COFF header may be zero [1].  For example,
> > the LLVM clang compiler for Windows MSVC can be instructed to generate
> > DWARF:
> >
> >    clang++ -g -O0 -gdwarf -fuse-ld=lld test.cpp -o test_clang
> >
> > The corresponding COFF file header shows:
> >
> >    FILE HEADER VALUES
> >          8664 machine (x64)
> >             E number of sections
> >      66E889EC time date stamp Mon Sep 16 21:41:32 2024
> >         FB400 file pointer to symbol table
> >             0 number of symbols
> >            F0 size of optional header
> >            22 characteristics
> >
> > GDB is not able to read the minimal symbols; the `start' command fails
> > with an error:
> >
> >    (gdb) start
> >    No symbol table loaded.  Use the "file" command.
> >
> > Manually inserting a breakpoint in main works fine:
> >
> >    (gdb) tbreak main
> >    Temporary breakpoint 1 at 0x14000100c: file test.cpp, line 6.
> >    (gdb) run
> >    Starting program: C:\test-clang
> >
> >    Temporary breakpoint 1, main () at test.cpp:6
> >    6         std::cout << "Hello World.\n";
> >
> > Fix this by testing if `main' can be resolved instead of checking the
> > minimal symbol table:
> >
> >    (gdb) start
> >    Temporary breakpoint 1 at 0x14000100c: file test.cpp, line 6.
> >    Starting program: C:\test-clang
> >
> >    Temporary breakpoint 1, main () at test.cpp:6
> >    6         std::cout << "Hello World.\n";
> >    (gdb)
> >
> > [1]: https://learn.microsoft.com/en-us/windows/win32/debug/pe-format
> > ---
> >   gdb/infcmd.c | 9 +++++----
> >   1 file changed, 5 insertions(+), 4 deletions(-)
> >
> > diff --git a/gdb/infcmd.c b/gdb/infcmd.c
> > index 5c0e3f51162..39b7e50b582 100644
> > --- a/gdb/infcmd.c
> > +++ b/gdb/infcmd.c
> > @@ -516,10 +516,11 @@ run_command (const char *args, int from_tty)
> >   static void
> >   start_command (const char *args, int from_tty)
> >   {
> > -  /* Some languages such as Ada need to search inside the program
> > -     minimal symbols for the location where to put the temporary
> > -     breakpoint before starting.  */
> > -  if (!have_minimal_symbols (current_program_space))
> > +  /* Abort the start command if `main` cannot be resolved, e.g., the
> > +     minimal / partial symbols are not available.  Some languages such
> > +     as Ada need to search inside the program minimal symbols for the
> > +     location where to put the temporary breakpoint before starting.  */
> > +  if (main_name () == nullptr)
> >       error (_("No symbol table loaded.  Use the \"file\" command."));
> >
> >     /* Run the program until reaching the main procedure...  */
> 
> I agree that minimal symbols shouldn't be required (obviously by your
> example), but I don't think knowing the name of the main function is enough.
> 
> If you look at how main_name works, it calls find_main_name if the name
> isn't cached yet, and find_main_name has a default fallback of guessing
> "main" with an unknown language if needed. In other words, even if we
> can't find that data in the inferior, we'll still guess something. I
> bring this up because if there is no minimal symbols and no debug info,
> I think GDB wouldn't be able to set a breakpoint because main can't be
> translated in any way, and so "start" would end up being silently
> converted to "run" and confuse the user. If this assumption is wrong,
> feel free to ignore the rest of the message.
> 
> I think the central issue for whether "start" works should be whether we
> can set the temporary breakpoint or not, rather than whether we can
> guess where to set it. I'm not sure if setting the tbreak would be
> possible here in the start_command due to all the preparatory work in
> run_command_1 before setting the bp (especially due to the commend in
> infcmd.c:411), but I think it would be ideal if it is possible. If that
> doesn't work, I would imagine run_command_1 should check if
> tbreak_command succeeds and return early if it fails. The issues with
> this second strategy is that the inferior is stopped and info is cleared
> before we're sure that we can run the full command, which is pretty
> annoying for the user, but I'it might be better than having to set a
> breakpoint twice (in case someone is using a slow connection to a
> gdbserver, for instance).
> 
> --
> Cheers,
> Guinevere Larsen
> She/Her/Hers

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Sean Fennelly, Jeffrey Schneiderman, Tiffany Doon Silva
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928
  

On 12/6/24 7:08 AM, Rohr, Stephan wrote:
> Hi Guinevere,
>
> thanks for your feedback.
>
> I agree on your concerns, 'main_name ()' might not be the best solution for this issue.
>
> I also think that moving the whole breakpoint setup into 'start_command' is not
> feasible.  I modified ' run_command_1' to report an error if the (temporary) breakpoint
> cannot be inserted, this also fixes the issue.  I had to do some modifications:
>
>    * Make 'break_command_1' externally visible.
>    * Use 'break_command_1' to insert the temporary breakpoint instead of 'tbreak_command'.
>
> I wonder if making 'break_command_1' externally visible could be an acceptable solution?

I don't quite understand why you needed to use break_command_1 to get 
this information, since I don't see any extra information you'd get from 
that, but maybe I'm just missing something, this is my first time 
looking at breakpoint code... with that said, have you considered making 
the "*break_command" functions return bool instead?

Since the last thing that break_command_1 does is a call to 
create_breakpoint, which already returns true for a successful insertion 
and false otherwise (using ints), you could just change break_command_1 
to return bool, and then each of the *break_commands could return the 
result to break_command_1 without needing to export the original function.

This looks to me like it would allow run_command_1 to detect and report 
the problem of the breakpoint creation without needing to change 
visibilities.
  

> -----Original Message-----
> From: Guinevere Larsen <guinevere@redhat.com>
> Sent: Friday, 6 December 2024 12:51
> To: Rohr, Stephan <stephan.rohr@intel.com>; gdb-patches@sourceware.org
> Subject: Re: [PATCH] gdb: continue start command if 'main' can be resolved
> 
> On 12/6/24 7:08 AM, Rohr, Stephan wrote:
> > Hi Guinevere,
> >
> > thanks for your feedback.
> >
> > I agree on your concerns, 'main_name ()' might not be the best solution for
> this issue.
> >
> > I also think that moving the whole breakpoint setup into 'start_command' is
> not
> > feasible.  I modified ' run_command_1' to report an error if the (temporary)
> breakpoint
> > cannot be inserted, this also fixes the issue.  I had to do some modifications:
> >
> >    * Make 'break_command_1' externally visible.
> >    * Use 'break_command_1' to insert the temporary breakpoint instead of
> 'tbreak_command'.
> >
> > I wonder if making 'break_command_1' externally visible could be an
> acceptable solution?
> 
> I don't quite understand why you needed to use break_command_1 to get
> this information, since I don't see any extra information you'd get from
> that, but maybe I'm just missing something, this is my first time
> looking at breakpoint code... with that said, have you considered making
> the "*break_command" functions return bool instead?
> 
> Since the last thing that break_command_1 does is a call to
> create_breakpoint, which already returns true for a successful insertion
> and false otherwise (using ints), you could just change break_command_1
> to return bool, and then each of the *break_commands could return the
> result to break_command_1 without needing to export the original function.
> 
> This looks to me like it would allow run_command_1 to detect and report
> the problem of the breakpoint creation without needing to change
> visibilities.
> 

I tried that direction at first, too.  This would require additional changes in the way
CLI commands are added.  The current implementation only accepts commands
with a void return type:

  /* The "simple" signature of command callbacks, which doesn't include a
       cmd_list_element parameter.  */
  
  typedef void cmd_simple_func_ftype (const char *args, int from_tty);

See 'command.h:388' for details.  I don't think changing this would be a good
idea.   I tried one more approach:

  * Check if the 'main' symbol can be found inside 'start_command', e.g., with
     ' lookup_symbol_search_name ()'.
  * If that fails, abort with the same error message as of today.

This also doesn't leave us with a stopped inferior, i.e., when adding the check to
'tbreak_command ()'. 

Would this be a better solution?

Thanks
Stephan

> --
> Cheers,
> Guinevere Larsen
> She/Her/Hers
> 
> >
> > Thanks
> > Stephan
> >
> >> -----Original Message-----
> >> From: Guinevere Larsen <guinevere@redhat.com>
> >> Sent: Wednesday, 4 December 2024 14:44
> >> To: Rohr, Stephan <stephan.rohr@intel.com>; gdb-
> patches@sourceware.org
> >> Subject: Re: [PATCH] gdb: continue start command if 'main' can be resolved
> >>
> >> On 12/4/24 10:16 AM, Stephan Rohr wrote:
> >>> From: "Rohr, Stephan" <stephan.rohr@intel.com>
> >>>
> >>> GDB aborts the 'start' command if the minimal symbols cannot be
> >>> resolved.  On Windows, GDB reads the minimal symbols from the COFF
> >>> header of the PE file.  The symbol table is deprecated and the
> >>> number of symbols in the COFF header may be zero [1].  For example,
> >>> the LLVM clang compiler for Windows MSVC can be instructed to generate
> >>> DWARF:
> >>>
> >>>     clang++ -g -O0 -gdwarf -fuse-ld=lld test.cpp -o test_clang
> >>>
> >>> The corresponding COFF file header shows:
> >>>
> >>>     FILE HEADER VALUES
> >>>           8664 machine (x64)
> >>>              E number of sections
> >>>       66E889EC time date stamp Mon Sep 16 21:41:32 2024
> >>>          FB400 file pointer to symbol table
> >>>              0 number of symbols
> >>>             F0 size of optional header
> >>>             22 characteristics
> >>>
> >>> GDB is not able to read the minimal symbols; the `start' command fails
> >>> with an error:
> >>>
> >>>     (gdb) start
> >>>     No symbol table loaded.  Use the "file" command.
> >>>
> >>> Manually inserting a breakpoint in main works fine:
> >>>
> >>>     (gdb) tbreak main
> >>>     Temporary breakpoint 1 at 0x14000100c: file test.cpp, line 6.
> >>>     (gdb) run
> >>>     Starting program: C:\test-clang
> >>>
> >>>     Temporary breakpoint 1, main () at test.cpp:6
> >>>     6         std::cout << "Hello World.\n";
> >>>
> >>> Fix this by testing if `main' can be resolved instead of checking the
> >>> minimal symbol table:
> >>>
> >>>     (gdb) start
> >>>     Temporary breakpoint 1 at 0x14000100c: file test.cpp, line 6.
> >>>     Starting program: C:\test-clang
> >>>
> >>>     Temporary breakpoint 1, main () at test.cpp:6
> >>>     6         std::cout << "Hello World.\n";
> >>>     (gdb)
> >>>
> >>> [1]: https://learn.microsoft.com/en-us/windows/win32/debug/pe-
> format
> >>> ---
> >>>    gdb/infcmd.c | 9 +++++----
> >>>    1 file changed, 5 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/gdb/infcmd.c b/gdb/infcmd.c
> >>> index 5c0e3f51162..39b7e50b582 100644
> >>> --- a/gdb/infcmd.c
> >>> +++ b/gdb/infcmd.c
> >>> @@ -516,10 +516,11 @@ run_command (const char *args, int from_tty)
> >>>    static void
> >>>    start_command (const char *args, int from_tty)
> >>>    {
> >>> -  /* Some languages such as Ada need to search inside the program
> >>> -     minimal symbols for the location where to put the temporary
> >>> -     breakpoint before starting.  */
> >>> -  if (!have_minimal_symbols (current_program_space))
> >>> +  /* Abort the start command if `main` cannot be resolved, e.g., the
> >>> +     minimal / partial symbols are not available.  Some languages such
> >>> +     as Ada need to search inside the program minimal symbols for the
> >>> +     location where to put the temporary breakpoint before starting.  */
> >>> +  if (main_name () == nullptr)
> >>>        error (_("No symbol table loaded.  Use the \"file\" command."));
> >>>
> >>>      /* Run the program until reaching the main procedure...  */
> >> I agree that minimal symbols shouldn't be required (obviously by your
> >> example), but I don't think knowing the name of the main function is
> enough.
> >>
> >> If you look at how main_name works, it calls find_main_name if the name
> >> isn't cached yet, and find_main_name has a default fallback of guessing
> >> "main" with an unknown language if needed. In other words, even if we
> >> can't find that data in the inferior, we'll still guess something. I
> >> bring this up because if there is no minimal symbols and no debug info,
> >> I think GDB wouldn't be able to set a breakpoint because main can't be
> >> translated in any way, and so "start" would end up being silently
> >> converted to "run" and confuse the user. If this assumption is wrong,
> >> feel free to ignore the rest of the message.
> >>
> >> I think the central issue for whether "start" works should be whether we
> >> can set the temporary breakpoint or not, rather than whether we can
> >> guess where to set it. I'm not sure if setting the tbreak would be
> >> possible here in the start_command due to all the preparatory work in
> >> run_command_1 before setting the bp (especially due to the commend in
> >> infcmd.c:411), but I think it would be ideal if it is possible. If that
> >> doesn't work, I would imagine run_command_1 should check if
> >> tbreak_command succeeds and return early if it fails. The issues with
> >> this second strategy is that the inferior is stopped and info is cleared
> >> before we're sure that we can run the full command, which is pretty
> >> annoying for the user, but I'it might be better than having to set a
> >> breakpoint twice (in case someone is using a slow connection to a
> >> gdbserver, for instance).
> >>
> >> --
> >> Cheers,
> >> Guinevere Larsen
> >> She/Her/Hers
> > Intel Deutschland GmbH
> > Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
> > Tel: +49 89 99 8853-0, www.intel.de
> > Managing Directors: Sean Fennelly, Jeffrey Schneiderman, Tiffany Doon Silva
> > Chairperson of the Supervisory Board: Nicole Lau
> > Registered Office: Munich
> > Commercial Register: Amtsgericht Muenchen HRB 186928

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Sean Fennelly, Jeffrey Schneiderman, Tiffany Doon Silva
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928
  

On 12/9/24 5:00 AM, Rohr, Stephan wrote:
>
>> -----Original Message-----
>> From: Guinevere Larsen <guinevere@redhat.com>
>> Sent: Friday, 6 December 2024 12:51
>> To: Rohr, Stephan <stephan.rohr@intel.com>; gdb-patches@sourceware.org
>> Subject: Re: [PATCH] gdb: continue start command if 'main' can be resolved
>>
>> On 12/6/24 7:08 AM, Rohr, Stephan wrote:
>>> Hi Guinevere,
>>>
>>> thanks for your feedback.
>>>
>>> I agree on your concerns, 'main_name ()' might not be the best solution for
>> this issue.
>>> I also think that moving the whole breakpoint setup into 'start_command' is
>> not
>>> feasible.  I modified ' run_command_1' to report an error if the (temporary)
>> breakpoint
>>> cannot be inserted, this also fixes the issue.  I had to do some modifications:
>>>
>>>     * Make 'break_command_1' externally visible.
>>>     * Use 'break_command_1' to insert the temporary breakpoint instead of
>> 'tbreak_command'.
>>> I wonder if making 'break_command_1' externally visible could be an
>> acceptable solution?
>>
>> I don't quite understand why you needed to use break_command_1 to get
>> this information, since I don't see any extra information you'd get from
>> that, but maybe I'm just missing something, this is my first time
>> looking at breakpoint code... with that said, have you considered making
>> the "*break_command" functions return bool instead?
>>
>> Since the last thing that break_command_1 does is a call to
>> create_breakpoint, which already returns true for a successful insertion
>> and false otherwise (using ints), you could just change break_command_1
>> to return bool, and then each of the *break_commands could return the
>> result to break_command_1 without needing to export the original function.
>>
>> This looks to me like it would allow run_command_1 to detect and report
>> the problem of the breakpoint creation without needing to change
>> visibilities.
>>
> I tried that direction at first, too.  This would require additional changes in the way
> CLI commands are added.  The current implementation only accepts commands
> with a void return type:
>
>    /* The "simple" signature of command callbacks, which doesn't include a
>         cmd_list_element parameter.  */
>    
>    typedef void cmd_simple_func_ftype (const char *args, int from_tty);
>
> See 'command.h:388' for details.  I don't think changing this would be a good
> idea.

Oh right, this makes perfect sense. Yeah, I agree that changing how 
commands are added would be a bad idea for this.

> I tried one more approach:
>
>    * Check if the 'main' symbol can be found inside 'start_command', e.g., with
>       ' lookup_symbol_search_name ()'.
>    * If that fails, abort with the same error message as of today.
>
> This also doesn't leave us with a stopped inferior, i.e., when adding the check to
> 'tbreak_command ()'.
>
> Would this be a better solution?

I really like this final version you came up with. It feels like the key 
point of the breakpoint test before we mess with the current inferior. 
+1 from me!