[PATCH] Don't set breakpoints on import stubs on Windows amd64

[Pedro Alves]

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.

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;
+		}
 	    }
 	}
     }

Thanks,
Pedro Alves