From patchwork Wed Mar 18 23:26:54 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Pedro Alves X-Patchwork-Id: 5692 Received: (qmail 116909 invoked by alias); 18 Mar 2015 23:27:00 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 116899 invoked by uid 89); 18 Mar 2015 23:26:59 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.2 required=5.0 tests=AWL, BAYES_00, KAM_STOCKGEN, SPF_HELO_PASS, T_RP_MATCHES_RCVD autolearn=no version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Wed, 18 Mar 2015 23:26:58 +0000 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (Postfix) with ESMTPS id 6E8A08E782; Wed, 18 Mar 2015 23:26:57 +0000 (UTC) Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t2INQsdH032315; Wed, 18 Mar 2015 19:26:55 -0400 Message-ID: <550A09BE.7020601@redhat.com> Date: Wed, 18 Mar 2015 23:26:54 +0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.5.0 MIME-Version: 1.0 To: Jon TURNEY , gdb-patches@sourceware.org Subject: Re: [PATCH] Don't set breakpoints on import stubs on Windows amd64 References: <1425646709-5216-1-git-send-email-jon.turney@dronecode.org.uk> In-Reply-To: <1425646709-5216-1-git-send-email-jon.turney@dronecode.org.uk> 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 > > * 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 > --- > 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 diff --git a/gdb/coffread.c b/gdb/coffread.c index 28f7b18..3b5a968 100644 --- a/gdb/coffread.c +++ b/gdb/coffread.c @@ -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; + } } } }