[RFA/commit/Win64] Remove new extra leading underscore in symbol name

[RFA/commit/Win64] Remove new extra leading underscore in symbol name

[Joel Brobecker]

For any Ada program, the debugger no longer finds the right routine as
the main subprogram when using the "start" command.  For instance:

    % gdb simple_main
    (gdb) start
    [...]
    Temporary breakpoint 1, main (argc=1, argv=(system.address) 0x232470, [...]
                            ^^^^

The expected behavior is to stop in the Ada main program:

    (gdb) start
    [...]
    Temporary breakpoint 1, simple_main () at simple_main.adb:4
    4           simple.test_simple;

The problem was introduced, I believe, by a patch in bfd causing
the names in the symbol table to now have a leading underscore:

    Adjust calling convention of x64 windows to non-leading underscore
    http://www.sourceware.org/ml/binutils/2010-04/msg00354.html

In the case of the "start" command, the search for the name of the
main subprogram requires GDB to find the __gnat_ada_main_program_name
symbol.  But since the patch above got applied, GDB now sees that
(minimal) symbol as ___gnat_ada_main_program_name (extra leading
underscore).

Other consequences of that change of behavior:

  - Ada tasking support is broken, since it relies on other known
    symbol names;

  - minimal symbol and symbols from debug info has non-matching names;
    I would think that some parts of GDB's code make the assumption
    that these names match, at least for C, but I might be unnecessarily
    pessimistic.

This patch changes the COFF reader such that, for COFF/PE64, we discard
the leading underscore.

2010-06-17  Joel Brobecker  <brobecker@adacore.com>

        gdb/
        * coffread.c (getsymname): Skip the leading underscore on pe64.

Tested on x64-windows with AdaCore's testsuite (we do a MinGW build).
I'd like to commit a fix before 7.2.

Kai's input would be greatly appreciated, as he probably knows Windows
a thousand times better than I do.

---
 gdb/coffread.c |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/gdb/coffread.c b/gdb/coffread.c
index 52417b2..4ab0640 100644
--- a/gdb/coffread.c
+++ b/gdb/coffread.c
@@ -1256,6 +1256,9 @@ getsymname (struct internal_syment *symbol_entry)
 {
   static char buffer[SYMNMLEN + 1];
   char *result;
+  const char *target = bfd_get_target (symfile_bfd);
+  const int is_pe64 = (strcmp (target, "pe-x86-64") == 0
+                       || strcmp (target, "pei-x86-64") == 0);
 
   if (symbol_entry->_n._n_n._n_zeroes == 0)
     {
@@ -1269,6 +1272,17 @@ getsymname (struct internal_syment *symbol_entry)
       buffer[SYMNMLEN] = '\0';
       result = buffer;
     }
+
+  /* On x64-windows, an extra leading underscore is usually added to
+     the symbol name.  However, the name provided in the associated
+     debug information does not include that underscore.  So remove it
+     now, to make the two names match.
+
+     This is also useful for code that searches the minimal symbols
+     for a specific symbol whose name should be target-independent.  */
+  if (is_pe64 && result[0] == '_')
+    result++;
+
   return result;
 }
 
-- 
1.7.1

Re: [RFA/commit/Win64] Remove new extra leading underscore in symbol name

[Tom Tromey]

>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:

Joel> 2010-06-17  Joel Brobecker  <brobecker@adacore.com>
Joel>         gdb/
Joel>         * coffread.c (getsymname): Skip the leading underscore on pe64.

I don't know anything about this target, but the binutils patch includes
a --enable-leading-mingw64-underscores option...

Joel> +  const char *target = bfd_get_target (symfile_bfd);
Joel> +  const int is_pe64 = (strcmp (target, "pe-x86-64") == 0
Joel> +                       || strcmp (target, "pei-x86-64") == 0);

...so maybe instead of looking at the target name, it would be better to
use bfd_get_target_info here?

Tom

Re: [RFA/commit/Win64] Remove new extra leading underscore in symbol name

[Joel Brobecker]

> Joel> 2010-06-17  Joel Brobecker  <brobecker@adacore.com>
> Joel>         gdb/
> Joel>         * coffread.c (getsymname): Skip the leading underscore on pe64.
> 
> I don't know anything about this target, but the binutils patch includes
> a --enable-leading-mingw64-underscores option...
> 
> Joel> +  const char *target = bfd_get_target (symfile_bfd);
> Joel> +  const int is_pe64 = (strcmp (target, "pe-x86-64") == 0
> Joel> +                       || strcmp (target, "pei-x86-64") == 0);
> 
> ...so maybe instead of looking at the target name, it would be better to
> use bfd_get_target_info here?

I'm a little nervous at changing the check to a non target-specific
one without multi-platform testing.  Unfortunately, I haven't had
a chance to work on this yet, and I'd really like to have that for 7.2.

Would it be OK for me to commit this change as is while I instrument
what you suggest on all other platforms we have that use COFF? (I'm
expecting to be able to make that change today)

-- 
Joel

Re: [RFA/commit/Win64] Remove new extra leading underscore in symbol name

[Tom Tromey]

>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:

Joel> Would it be OK for me to commit this change as is while I instrument
Joel> what you suggest on all other platforms we have that use COFF? (I'm
Joel> expecting to be able to make that change today)

If you're asking me -- I don't really have an opinion.
I'm really not sure what is correct here, but I trust your judgment.  

Tom

Re: [RFA/commit/Win64] Remove new extra leading underscore in symbol name

[Pedro Alves]

On Thursday 24 June 2010 19:23:17, Joel Brobecker wrote:
> > Joel> 2010-06-17  Joel Brobecker  <brobecker@adacore.com>
> > Joel>         gdb/
> > Joel>         * coffread.c (getsymname): Skip the leading underscore on pe64.
> > 
> > I don't know anything about this target, but the binutils patch includes
> > a --enable-leading-mingw64-underscores option...
> > 
> > Joel> +  const char *target = bfd_get_target (symfile_bfd);
> > Joel> +  const int is_pe64 = (strcmp (target, "pe-x86-64") == 0
> > Joel> +                       || strcmp (target, "pei-x86-64") == 0);
> > 
> > ...so maybe instead of looking at the target name, it would be better to
> > use bfd_get_target_info here?
> 
> I'm a little nervous at changing the check to a non target-specific
> one without multi-platform testing.  Unfortunately, I haven't had
> a chance to work on this yet, and I'd really like to have that for 7.2.
> 
> Would it be OK for me to commit this change as is while I instrument
> what you suggest on all other platforms we have that use COFF? (I'm
> expecting to be able to make that change today)

I'm confused on your change.  It sounds like you're using a debugger
that postdates the change to default to not output underscores on
win64, with a compiler that still outputs the underscores.  What
happens when you update your compiler?  I expect your patch to break
binaries produced by a compiler that also doesn't output
underscores anymore on c symbols, as mingw64's.  Isn't that so?

I don't think either a bfd_get_target or bfd_get_target_info check
will always get you a right answer, since those essentially are
returning hardcoded answers in bfd, not how the binaries were
built.  Am I wrong?

-- 
Pedro Alves

Re: [RFA/commit/Win64] Remove new extra leading underscore in symbol name

[Joel Brobecker]

> I'm confused on your change.  It sounds like you're using a debugger
> that postdates the change to default to not output underscores on
> win64, with a compiler that still outputs the underscores.  What
> happens when you update your compiler?  I expect your patch to break
> binaries produced by a compiler that also doesn't output
> underscores anymore on c symbols, as mingw64's.  Isn't that so?

You might be right - I haven't tried to build an updated compiler since
building one on MinGW is a big problem for someone like me who almost
never works on Windows and is just not setup for it.

> I don't think either a bfd_get_target or bfd_get_target_info check
> will always get you a right answer, since those essentially are
> returning hardcoded answers in bfd, not how the binaries were
> built.  Am I wrong?

No, you're probably right. I was slowly realizing this while I was
updating the comment I wrote in the previous patch. The problem is:
what's the right way to detect how the binary was built? Right now,
the bfd change is a major incompatibility nightmare since minimal
symbols and symbols no longer have the same name.  GDB needs to be able
to support both (IMO).

-- 
Joel

Re: [RFA/commit/Win64] Remove new extra leading underscore in symbol name

[Pedro Alves]

On Thursday 24 June 2010 20:00:21, Joel Brobecker wrote:
> No, you're probably right. I was slowly realizing this while I was
> updating the comment I wrote in the previous patch. The problem is:
> what's the right way to detect how the binary was built? 

I don't think there's a "right way".  At least, I can't think of
one.  Maybe there's some heuristic way, like looking for some well
known symbol in the implementation namespace for two or
three underscores, say.  Always likely to fail, for several reasons,
one of them that you still misfix asm defined underscored symbols.
And you'd probably want to consider handling the reverse -- missing
underscores, in gdb/bfd's perspective.  Very fragile, if you ask me.

> Right now,
> the bfd change is a major incompatibility nightmare since minimal
> symbols and symbols no longer have the same name.  

It was an ABI change.  Incompatibilities are sort of expected by
design, when all the tools don't agree on the ABI.  :-)

> GDB needs to be able to support both (IMO).

Does the --enable-leading-mingw64-underscores switch affect
bfd as well, and fix this?  While supporting both sounds ideal,
in practice, I'd think it to be enough for vendors to support
gdb builds that match the ABI of their compiler.  Or two builds,
if they care, until the old ABI is phased out.

-- 
Pedro Alves

Re: [RFA/commit/Win64] Remove new extra leading underscore in symbol name

[Kai Tietz]

2010/6/24 Pedro Alves <pedro@codesourcery.com>:
> On Thursday 24 June 2010 20:00:21, Joel Brobecker wrote:
>> No, you're probably right. I was slowly realizing this while I was
>> updating the comment I wrote in the previous patch. The problem is:
>> what's the right way to detect how the binary was built?
>
> I don't think there's a "right way".  At least, I can't think of
> one.  Maybe there's some heuristic way, like looking for some well
> known symbol in the implementation namespace for two or
> three underscores, say.  Always likely to fail, for several reasons,
> one of them that you still misfix asm defined underscored symbols.
> And you'd probably want to consider handling the reverse -- missing
> underscores, in gdb/bfd's perspective.  Very fragile, if you ask me.

Right, to ask bfd for toolchain's target default underscoring is for
sure not bad, but doesn't address the issue complete. Probing of
symbols could be a way, but it is indeed fragile.

>> Right now,
>> the bfd change is a major incompatibility nightmare since minimal
>> symbols and symbols no longer have the same name.
>
> It was an ABI change.  Incompatibilities are sort of expected by
> design, when all the tools don't agree on the ABI.  :-)

:)

>> GDB needs to be able to support both (IMO).
>
> Does the --enable-leading-mingw64-underscores switch affect
> bfd as well, and fix this?  While supporting both sounds ideal,
> in practice, I'd think it to be enough for vendors to support
> gdb builds that match the ABI of their compiler.  Or two builds,
> if they care, until the old ABI is phased out.

Yes, by --enable-leading-mingw64-underscores just the default in bfd
is changed. So it could be a work-a-round for supporting binaries
having non-ABI name-decoration.

> --
> Pedro Alves
>

Kai

-- 
|  (\_/) This is Bunny. Copy and paste
| (='.'=) Bunny into your signature to help
| (")_(") him gain world domination

Re: [RFA/commit/Win64] Remove new extra leading underscore in symbol name

[Joel Brobecker]

> Yes, by --enable-leading-mingw64-underscores just the default in bfd
> is changed. So it could be a work-a-round for supporting binaries
> having non-ABI name-decoration.

I'm afraid that this is what we're going to have to do.  One thing
we need to decide on, now, is whether 7.2 should build with the old
behavior by default or not.  If there hasn't been a GCC release
including the ABI change yet, then perhaps it's better for the users
if we tweak 7.2 to expect the non-ABI name-decorations.

-- 
Joel

Re: [RFA/commit/Win64] Remove new extra leading underscore in symbol name

[Kai Tietz]

2010/6/24 Joel Brobecker <brobecker@adacore.com>:
>> Yes, by --enable-leading-mingw64-underscores just the default in bfd
>> is changed. So it could be a work-a-round for supporting binaries
>> having non-ABI name-decoration.
>
> I'm afraid that this is what we're going to have to do.  One thing
> we need to decide on, now, is whether 7.2 should build with the old
> behavior by default or not.  If there hasn't been a GCC release
> including the ABI change yet, then perhaps it's better for the users
> if we tweak 7.2 to expect the non-ABI name-decorations.
>
> --
> Joel
>

Well, maybe the way to go. From beginning of 4.5.1 new ABI (means that
one specified by MS for this target) will be default for gcc.

Kai

-- 
|  (\_/) This is Bunny. Copy and paste
| (='.'=) Bunny into your signature to help
| (")_(") him gain world domination

RE: [RFA/commit/Win64] Remove new extra leading underscore in symbol name

[Pierre Muller]

  Why don't you simply use
bfd_get_symbol_leading_char function 
or use EXTERNAL_NAME macro defined line 65 in coffread.C
to remove the extra underscore?

Pierre Muller

> -----Message d'origine-----
> De : gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] De la part de Joel Brobecker
> Envoyé : Friday, June 18, 2010 12:26 AM
> À : gdb-patches@sourceware.org
> Cc : ktietz70@googlemail.com; Joel Brobecker
> Objet : [RFA/commit/Win64] Remove new extra leading underscore in
> symbol name
> 
> For any Ada program, the debugger no longer finds the right routine as
> the main subprogram when using the "start" command.  For instance:
> 
>     % gdb simple_main
>     (gdb) start
>     [...]
>     Temporary breakpoint 1, main (argc=1, argv=(system.address)
> 0x232470, [...]
>                             ^^^^
> 
> The expected behavior is to stop in the Ada main program:
> 
>     (gdb) start
>     [...]
>     Temporary breakpoint 1, simple_main () at simple_main.adb:4
>     4           simple.test_simple;
> 
> The problem was introduced, I believe, by a patch in bfd causing
> the names in the symbol table to now have a leading underscore:
> 
>     Adjust calling convention of x64 windows to non-leading underscore
>     http://www.sourceware.org/ml/binutils/2010-04/msg00354.html
> 
> In the case of the "start" command, the search for the name of the
> main subprogram requires GDB to find the __gnat_ada_main_program_name
> symbol.  But since the patch above got applied, GDB now sees that
> (minimal) symbol as ___gnat_ada_main_program_name (extra leading
> underscore).
> 
> Other consequences of that change of behavior:
> 
>   - Ada tasking support is broken, since it relies on other known
>     symbol names;
> 
>   - minimal symbol and symbols from debug info has non-matching names;
>     I would think that some parts of GDB's code make the assumption
>     that these names match, at least for C, but I might be
> unnecessarily
>     pessimistic.
> 
> This patch changes the COFF reader such that, for COFF/PE64, we discard
> the leading underscore.
> 
> 2010-06-17  Joel Brobecker  <brobecker@adacore.com>
> 
>         gdb/
>         * coffread.c (getsymname): Skip the leading underscore on pe64.
> 
> Tested on x64-windows with AdaCore's testsuite (we do a MinGW build).
> I'd like to commit a fix before 7.2.
> 
> Kai's input would be greatly appreciated, as he probably knows Windows
> a thousand times better than I do.
> 
> ---
>  gdb/coffread.c |   14 ++++++++++++++
>  1 files changed, 14 insertions(+), 0 deletions(-)
> 
> diff --git a/gdb/coffread.c b/gdb/coffread.c
> index 52417b2..4ab0640 100644
> --- a/gdb/coffread.c
> +++ b/gdb/coffread.c
> @@ -1256,6 +1256,9 @@ getsymname (struct internal_syment *symbol_entry)
>  {
>    static char buffer[SYMNMLEN + 1];
>    char *result;
> +  const char *target = bfd_get_target (symfile_bfd);
> +  const int is_pe64 = (strcmp (target, "pe-x86-64") == 0
> +                       || strcmp (target, "pei-x86-64") == 0);
> 
>    if (symbol_entry->_n._n_n._n_zeroes == 0)
>      {
> @@ -1269,6 +1272,17 @@ getsymname (struct internal_syment
> *symbol_entry)
>        buffer[SYMNMLEN] = '\0';
>        result = buffer;
>      }
> +
> +  /* On x64-windows, an extra leading underscore is usually added to
> +     the symbol name.  However, the name provided in the associated
> +     debug information does not include that underscore.  So remove it
> +     now, to make the two names match.
> +
> +     This is also useful for code that searches the minimal symbols
> +     for a specific symbol whose name should be target-independent.
> */
> +  if (is_pe64 && result[0] == '_')
> +    result++;
> +
>    return result;
>  }
> 
> --
> 1.7.1