diff mbox series

[v3,1/1] gdb: abort start command if there is no symbol for 'main_name ()'

Message ID 20241211084158.368943-2-stephan.rohr@intel.com
State New
Headers
Series gdb: Fix start command for Windows |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 success Build passed
linaro-tcwg-bot/tcwg_gdb_build--master-arm success Build passed
linaro-tcwg-bot/tcwg_gdb_check--master-arm success Test passed
linaro-tcwg-bot/tcwg_gdb_check--master-aarch64 success Test passed

Commit Message

Rohr, Stephan Dec. 11, 2024, 8:41 a.m. UTC 
  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 a symbol for `main' is available 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

Reviewed-By: Guinevere Larsen <guinevere@redhat.com>
---
 gdb/infcmd.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

Comments

Luis Machado Dec. 11, 2024, 9:24 a.m. UTC | #1 
On 12/11/24 08:41, 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 a symbol for `main' is available 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
> 
> Reviewed-By: Guinevere Larsen <guinevere@redhat.com>
> ---
>  gdb/infcmd.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/gdb/infcmd.c b/gdb/infcmd.c
> index 5c0e3f51162..3c9d6aff9a4 100644
> --- a/gdb/infcmd.c
> +++ b/gdb/infcmd.c
> @@ -516,10 +516,12 @@ 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.  */
> +  struct symbol *sym
> +    = lookup_symbol_search_name (main_name (), nullptr,
> +				 SEARCH_FUNCTION_DOMAIN).symbol;
> +
> +  if (sym == nullptr)
>      error (_("No symbol table loaded.  Use the \"file\" command."));
>  
>    /* Run the program until reaching the main procedure...  */

Tom Tromey (cc-ed) could confirm this, but this may regress Ada
debugging for the case where we don't have debug info available. My
understanding is that minimal symbols come from the ELF file, and
lookup_symbol_search_name with those parameters may not dive into
minimal symbols and so won't find what it is looking for.

A test for this is trying to start a Ada program with debug info and
then trying to start one without debug info ("strip -g" the binary).

The latter may not work anymore.
Rohr, Stephan Dec. 13, 2024, 7:39 a.m. UTC | #2 
Hi Luis,

thanks for the hint, the patch indeed causes a regression if symbols are stripped.

What about changing to:

  struct symbol *sym
    = lookup_symbol_search_name (main_name (), nullptr,
				 SEARCH_FUNCTION_DOMAIN).symbol;

  if (sym == nullptr && !have_minimal_symbols (current_program_space))
    error (_("No symbol table loaded.  Use the \"file\" command."));

This fixes the Windows issue I originally observed.  Also, Ada doesn't regress
anymore.

Thanks
Stephan

> -----Original Message-----
> From: Luis Machado <luis.machado@arm.com>
> Sent: Wednesday, 11 December 2024 10:25
> To: Rohr, Stephan <stephan.rohr@intel.com>; gdb-patches@sourceware.org
> Cc: guinevere@redhat.com; eliz@gnu.org; Tom Tromey <tom@tromey.com>
> Subject: Re: [PATCH v3 1/1] gdb: abort start command if there is no symbol for
> 'main_name ()'
> 
> On 12/11/24 08:41, 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 a symbol for `main' is available 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
> >
> > Reviewed-By: Guinevere Larsen <guinevere@redhat.com>
> > ---
> >  gdb/infcmd.c | 10 ++++++----
> >  1 file changed, 6 insertions(+), 4 deletions(-)
> >
> > diff --git a/gdb/infcmd.c b/gdb/infcmd.c
> > index 5c0e3f51162..3c9d6aff9a4 100644
> > --- a/gdb/infcmd.c
> > +++ b/gdb/infcmd.c
> > @@ -516,10 +516,12 @@ 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.  */
> > +  struct symbol *sym
> > +    = lookup_symbol_search_name (main_name (), nullptr,
> > +				 SEARCH_FUNCTION_DOMAIN).symbol;
> > +
> > +  if (sym == nullptr)
> >      error (_("No symbol table loaded.  Use the \"file\" command."));
> >
> >    /* Run the program until reaching the main procedure...  */
> 
> Tom Tromey (cc-ed) could confirm this, but this may regress Ada
> debugging for the case where we don't have debug info available. My
> understanding is that minimal symbols come from the ELF file, and
> lookup_symbol_search_name with those parameters may not dive into
> minimal symbols and so won't find what it is looking for.
> 
> A test for this is trying to start a Ada program with debug info and
> then trying to start one without debug info ("strip -g" the binary).
> 
> The latter may not work anymore.
> 

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
Tom Tromey Dec. 13, 2024, 2:51 p.m. UTC | #3 
>>>>> "Stephan" == Stephan Rohr <stephan.rohr@intel.com> writes:

Thanks for the patch.

Stephan> -  /* Some languages such as Ada need to search inside the program
Stephan> -     minimal symbols for the location where to put the temporary
Stephan> -     breakpoint before starting.  */
Stephan> -  if (!have_minimal_symbols (current_program_space))
Stephan> +  /* Abort the start command if 'main' cannot be resolved.  */
Stephan> +  struct symbol *sym
Stephan> +    = lookup_symbol_search_name (main_name (), nullptr,
Stephan> +				 SEARCH_FUNCTION_DOMAIN).symbol;
Stephan> +
Stephan> +  if (sym == nullptr)
Stephan>      error (_("No symbol table loaded.  Use the \"file\" command."));

I agree that the check currently in the code isn't really correct, but I
think this change means that 'start' will now fail if the inferior
doesn't have debug symbols.

I wonder what happens if you simply remove the check entirely.  If we
want to preserve the text about using "file", this could maybe be done
by wrapping the call to tbreak_command in a try/catch.

Tom
Luis Machado Dec. 16, 2024, 4:33 p.m. UTC | #4 
On 12/13/24 07:39, Rohr, Stephan wrote:
> Hi Luis,
> 
> thanks for the hint, the patch indeed causes a regression if symbols are stripped.

No worries.

> 
> What about changing to:
> 
>   struct symbol *sym
>     = lookup_symbol_search_name (main_name (), nullptr,
> 				 SEARCH_FUNCTION_DOMAIN).symbol;
> 
>   if (sym == nullptr && !have_minimal_symbols (current_program_space))
>     error (_("No symbol table loaded.  Use the \"file\" command."));
> 
> This fixes the Windows issue I originally observed.  Also, Ada doesn't regress
> anymore.

I think it might work, though it skips the minimal symbols check whenever we return
a non-null sym. I'll defer to Tromey to confirm this is enough for Ada to work
correctly.

> 
> Thanks
> Stephan
> 
>> -----Original Message-----
>> From: Luis Machado <luis.machado@arm.com>
>> Sent: Wednesday, 11 December 2024 10:25
>> To: Rohr, Stephan <stephan.rohr@intel.com>; gdb-patches@sourceware.org
>> Cc: guinevere@redhat.com; eliz@gnu.org; Tom Tromey <tom@tromey.com>
>> Subject: Re: [PATCH v3 1/1] gdb: abort start command if there is no symbol for
>> 'main_name ()'
>>
>> On 12/11/24 08:41, 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 a symbol for `main' is available 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
>>>
>>> Reviewed-By: Guinevere Larsen <guinevere@redhat.com>
>>> ---
>>>  gdb/infcmd.c | 10 ++++++----
>>>  1 file changed, 6 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/gdb/infcmd.c b/gdb/infcmd.c
>>> index 5c0e3f51162..3c9d6aff9a4 100644
>>> --- a/gdb/infcmd.c
>>> +++ b/gdb/infcmd.c
>>> @@ -516,10 +516,12 @@ 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.  */
>>> +  struct symbol *sym
>>> +    = lookup_symbol_search_name (main_name (), nullptr,
>>> +				 SEARCH_FUNCTION_DOMAIN).symbol;
>>> +
>>> +  if (sym == nullptr)
>>>      error (_("No symbol table loaded.  Use the \"file\" command."));
>>>
>>>    /* Run the program until reaching the main procedure...  */
>>
>> Tom Tromey (cc-ed) could confirm this, but this may regress Ada
>> debugging for the case where we don't have debug info available. My
>> understanding is that minimal symbols come from the ELF file, and
>> lookup_symbol_search_name with those parameters may not dive into
>> minimal symbols and so won't find what it is looking for.
>>
>> A test for this is trying to start a Ada program with debug info and
>> then trying to start one without debug info ("strip -g" the binary).
>>
>> The latter may not work anymore.
>>
> 
> 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
Rohr, Stephan Jan. 10, 2025, 9:50 a.m. UTC | #5 
Hi Tom,

I would vote for removing the check entirely:

  * Adding a try / catch block in 'tbreak' or in 'run_command_1' is not straight
     forward.  If 'main' cannot be resolved, a 'NOT_FOUND_ERROR' is thrown.
  * A 'No symbol table is loaded.  Use the "file" command' error is printed.
  * The error is consumed in 'create_breakpoint' and the user may insert a 
     pending breakpoint.  The error only rethrown if pending breakpoints are not
     supported.

We would need to rethrow the error in 'create_breakpoint'.  This requires
additional changes and may impact other calls to 'create_breakpoint'.
I would like to avoid this.

I could not observer regressions for Ada if I entirely remove the check.

Do you have any concerns?

Thanks
Stephan


> -----Original Message-----
> From: Tom Tromey <tom@tromey.com>
> Sent: Friday, 13 December 2024 15:51
> To: Rohr, Stephan <stephan.rohr@intel.com>
> Cc: gdb-patches@sourceware.org; guinevere@redhat.com; eliz@gnu.org
> Subject: Re: [PATCH v3 1/1] gdb: abort start command if there is no symbol
> for 'main_name ()'
> 
> >>>>> "Stephan" == Stephan Rohr <stephan.rohr@intel.com> writes:
> 
> Thanks for the patch.
> 
> Stephan> -  /* Some languages such as Ada need to search inside the program
> Stephan> -     minimal symbols for the location where to put the temporary
> Stephan> -     breakpoint before starting.  */
> Stephan> -  if (!have_minimal_symbols (current_program_space))
> Stephan> +  /* Abort the start command if 'main' cannot be resolved.  */
> Stephan> +  struct symbol *sym
> Stephan> +    = lookup_symbol_search_name (main_name (), nullptr,
> Stephan> +
> SEARCH_FUNCTION_DOMAIN).symbol;
> Stephan> +
> Stephan> +  if (sym == nullptr)
> Stephan>      error (_("No symbol table loaded.  Use the \"file\" command."));
> 
> I agree that the check currently in the code isn't really correct, but I
> think this change means that 'start' will now fail if the inferior
> doesn't have debug symbols.
> 
> I wonder what happens if you simply remove the check entirely.  If we
> want to preserve the text about using "file", this could maybe be done
> by wrapping the call to tbreak_command in a try/catch.
> 
> Tom
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
Tom Tromey Jan. 14, 2025, 4:08 p.m. UTC | #6 
>>>>> Rohr, Stephan <stephan.rohr@intel.com> writes:

>   * A 'No symbol table is loaded.  Use the "file" command' error is printed.
>   * The error is consumed in 'create_breakpoint' and the user may insert a 
>      pending breakpoint.  The error only rethrown if pending breakpoints are not
>      supported.

Ugh yeah.

> We would need to rethrow the error in 'create_breakpoint'.  This requires
> additional changes and may impact other calls to 'create_breakpoint'.
> I would like to avoid this.

> I could not observer regressions for Ada if I entirely remove the check.

> Do you have any concerns?

Not really, I think.  You might as well try it IMO.

Tom
diff mbox series

Patch

diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index 5c0e3f51162..3c9d6aff9a4 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -516,10 +516,12 @@  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.  */
+  struct symbol *sym
+    = lookup_symbol_search_name (main_name (), nullptr,
+				 SEARCH_FUNCTION_DOMAIN).symbol;
+
+  if (sym == nullptr)
     error (_("No symbol table loaded.  Use the \"file\" command."));
 
   /* Run the program until reaching the main procedure...  */