This is the mail archive of the
mailing list for the GDB project.
Re: [PATCH] Don't set breakpoints on import stubs on Windows amd64
- From: Pedro Alves <palves at redhat dot com>
- To: Jon TURNEY <jon dot turney at dronecode dot org dot uk>, gdb-patches at sourceware dot org
- Date: Wed, 18 Mar 2015 23:26:54 +0000
- Subject: Re: [PATCH] Don't set breakpoints on import stubs on Windows amd64
- Authentication-results: sourceware.org; auth=none
- References: <1425646709-5216-1-git-send-email-jon dot turney at dronecode dot org dot uk>
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
> This looks like an error in commit 303c5. It seems from this email  that
> tests were only run on i386 at the time it was commited.
>  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.
> 2015-03-04 Jon TURNEY <email@example.com>
> * 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 <firstname.lastname@example.org>
> 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 == '_' ? &name : &name);
> + const char *name1 = &name;
> 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.
diff --git a/gdb/coffread.c b/gdb/coffread.c
index 28f7b18..3b5a968 100644
@@ -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 == '_' ? &name : &name);
- 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;