Don't set breakpoints on import stubs on Windows amd64
Commit Message
Hi Jon,
On 03/06/2015 12:58 PM, Jon TURNEY wrote:
> On Windows i386, setting a breakpoint on a symbol imported from a shared library
> (after that library is loaded) sets the breakpoint only in the shared library.
>
> On Windows amd64, setting a breakpoint on a symbol imported from a shared
> library (after that library is loaded) sets the breakpoint on both the import
> stub, and in the shared library.
>
> This appears to be due to the minimal symbol for the import stub not being
> correctly given the type mst_solib_trampoline on Windows amd64, unlike Windows
> i386.
>
> This looks like an error in commit 303c5. It seems from this email [1] that
> tests were only run on i386 at the time it was commited.
>
> [1] https://sourceware.org/ml/gdb-patches/2013-07/msg00100.html
>
> Note that as currently written, this is skipping over the character after the
> "__imp_" (amd64) or "_imp_" (i386) prefix, assuming that it is '_'.
>
> This patch removes that behaviour on amd64, and no characters are skipped over,
> so the name of the import stub to mark as a trampoline is correctly constructed
> from the import name (e.g the existence of '__imp_foo' implies that 'foo' in the
> same object should be marked as a trampoline, rather than the probably
> non-existent 'oo').
Yeah, x86-64 Windows/mingw does not underscore C symbols, while x86 does.
>
> I'm not totally sure that asssumption that there will always be an '_' to skip
> over on i386 is correct. Perhaps if the imported symbol is not C ABI (i.e. it's
> an assembly routine), it doesn't have an '_' there?
Yeah, I think that's possible. Maybe even easy to verify? Either by writing
a real asm file and build a dll from that, or maybe even with
int foo () asm("foo");. With that, and given things -Wl,--no-leading-underscore
/ -Wl,--leading-underscore too, not sure what we could do other than
looking up the symbols with and without the underscore.
>
> Tested on x86_64-pc-cygwin
>
> - FAIL: gdb.base/solib-symbol.exp: foo in libmd
> + PASS: gdb.base/solib-symbol.exp: foo in libmd
>
> Unfortunately, several other tests are also affected. Some which failed because
ITYM, "Some which failed now pass" here right?
> setting a breakpoint on an imported symbol said "(2 locations)" now pass. Some
> which passed now fail because this issue was masking other problems.
>
> No change on i686-pc-cygwin.
>
> ChangeLog/gdb:
>
> 2015-03-04 Jon TURNEY <jon.turney@dronecode.org.uk>
>
> * coffread.c (coff_symfile_read): Fix construction of the name of
> an import stub symbol from import symbol for amd64.
>
> Signed-off-by: Jon TURNEY <jon.turney@dronecode.org.uk>
> ---
> gdb/coffread.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/gdb/coffread.c b/gdb/coffread.c
> index 366d828..21e7a77 100644
> --- a/gdb/coffread.c
> +++ b/gdb/coffread.c
> @@ -675,7 +675,7 @@ coff_symfile_read (struct objfile *objfile, int symfile_flags)
> && (strncmp (name, "__imp_", 6) == 0
> || strncmp (name, "_imp_", 5) == 0))
> {
> - const char *name1 = (name[1] == '_' ? &name[7] : &name[6]);
> + const char *name1 = &name[6];
> struct bound_minimal_symbol found;
As is, that would make it look suspiciously like a bug to a casual
reader. It'd be very good to have a comment here.
But maybe if we rewrite this in terms of bfd_get_symbol_leading_char,
it ends up being "self commenting". Something like the hunk below.
Thanks,
Pedro Alves
Comments
On 18/03/2015 23:26, Pedro Alves wrote:
>> diff --git a/gdb/coffread.c b/gdb/coffread.c
>> index 366d828..21e7a77 100644
>> --- a/gdb/coffread.c
>> +++ b/gdb/coffread.c
>> @@ -675,7 +675,7 @@ coff_symfile_read (struct objfile *objfile, int symfile_flags)
>> && (strncmp (name, "__imp_", 6) == 0
>> || strncmp (name, "_imp_", 5) == 0))
>> {
>> - const char *name1 = (name[1] == '_' ? &name[7] : &name[6]);
>> + const char *name1 = &name[6];
>> struct bound_minimal_symbol found;
>
> As is, that would make it look suspiciously like a bug to a casual
> reader. It'd be very good to have a comment here.
> But maybe if we rewrite this in terms of bfd_get_symbol_leading_char,
> it ends up being "self commenting". Something like the hunk below.
Yes, that is better, and seems to work fine.
@@ -671,20 +671,31 @@ coff_symfile_read (struct objfile *objfile, int symfile_flags)
or "_imp_", get rid of the prefix, and search the minimal
symbol in OBJFILE. Note that 'maintenance print msymbols'
shows that type of these "_imp_XXXX" symbols is mst_data. */
- if (MSYMBOL_TYPE (msym) == mst_data
- && (startswith (name, "__imp_")
- || startswith (name, "_imp_")))
+ if (MSYMBOL_TYPE (msym) == mst_data)
{
- const char *name1 = (name[1] == '_' ? &name[7] : &name[6]);
- struct bound_minimal_symbol found;
-
- found = lookup_minimal_symbol (name1, NULL, objfile);
- /* If found, there are symbols named "_imp_foo" and "foo"
- respectively in OBJFILE. Set the type of symbol "foo"
- as 'mst_solib_trampoline'. */
- if (found.minsym != NULL
- && MSYMBOL_TYPE (found.minsym) == mst_text)
- MSYMBOL_TYPE (found.minsym) = mst_solib_trampoline;
+ const char *name1 = NULL;
+
+ if (startswith (name, "_imp_"))
+ name1 = name + 5;
+ else if (startswith (name, "__imp_"))
+ name1 = name + 6;
+ if (name1 != NULL)
+ {
+ int lead = bfd_get_symbol_leading_char (objfile->obfd);
+ struct bound_minimal_symbol found;
+
+ if (lead != '\0' && *name1 == lead)
+ name1 += 1;
+
+ found = lookup_minimal_symbol (name1, NULL, objfile);
+
+ /* If found, there are symbols named "_imp_foo" and "foo"
+ respectively in OBJFILE. Set the type of symbol "foo"
+ as 'mst_solib_trampoline'. */
+ if (found.minsym != NULL
+ && MSYMBOL_TYPE (found.minsym) == mst_text)
+ MSYMBOL_TYPE (found.minsym) = mst_solib_trampoline;
+ }
}
}
}