Re: [PATCH v3] Add dll trampoline code handling for windows 64bit

Re: [PATCH v3] Add dll trampoline code handling for windows 64bit

[Roland Schwingel]

Hi Joel,

gdb-patches-owner@sourceware.org wrote on 29.03.2012 20:21:02:
 > > 2012-03-16  Roland Schwingel <roland.schwingel@onevision.com>
 > >
 > >         * amd64-windows-tdep.c: #include "frame.h".
 > >         (amd64_windows_skip_trampoline_code): New function.
 > >         (amd64_windows_init_abi): Add trampoline registration.
 >
 > No one reviewed þhe patch as far as I can tell, I am sorry about that.
 > As a general guideline, it's OK to ping us every week or two...
Thanks for taking the time and looking at it.

 > I think the code looks pretty good, and you can commit, with a couple
 > of very minor almost-nitpicky little comments (a apologize for asking
 > for your forgiveness)...
No problem.

 > Hmmm, I actually cannot find any copyright assignment for you on
 > file. Do you have one? Or is this done on behalf of your employer
 > who might have one? Please let me know... We can give you write-
 > after-approval priviledges as soon as we have confirmed that you
 > do have an assignment on file.
Tom Tromey sent me the necessary informations on march 15th. One day 
later I sent out the mail to fsf-records@gnu.org but yet have not 
received *any* response. I asked Tom some days ago and he told me to 
wait for some more days before I send in a ping. I already got a CVS 
account on sourceware and I am just awaiting my okays ...

 > > +      /* Get address of function pointer at end of pc.  */
 > > +      CORE_ADDR indirect_addr = pc + offset + 6;
 >
 > I think it would be useful to explain where the magic constant "6"
 > comes from... I'd almost write the expression "pc + 6 + offset".
PC relative instructions always count starting from the end of the 
instruction itself. pc in this situation points to the beginning of the 
instruction. The instruction itself is (with operands) 6 bytes long. If 
you like to I will change the order of addition on commit.

 > > +      struct minimal_symbol *indsym =
 > > +        indirect_addr ? lookup_minimal_symbol_by_pc 
(indirect_addr) : 0;
 > > +      const char *symname = indsym ? SYMBOL_LINKAGE_NAME (indsym) : 0;
 >
 > I'd rather you used NULL instead of 0, even if we're pretty much
 > guaranteed that NULL will always be zero. That's really nit-picky,
 > but it seems clearer that way, at least for me.
Sure I can do that (on commit). I did it the same way as the 32bit code 
in i386_pe_skip_trampoline_code() does hoping it would be easier for the 
approver if he sees familiar code ...

The other 2 formatting I will also correct on commit. But if you want I 
am doing a 4th version of my patch with your suggested changes and send 
it in. Elsewise I will sit and wait until my assignment stuff gets done 
and commit it later on and avoid unneccessary noise.

Roland

Re: [PATCH v3] Add dll trampoline code handling for windows 64bit

[Pedro Alves]

On 03/30/2012 07:46 AM, Roland Schwingel wrote:

> The other 2 formatting I will also correct on commit. But if you want I am doing

> a 4th version of my patch with your suggested changes and send it in. Elsewise I will
> sit and wait until my assignment stuff gets done and commit it later on and avoid unneccessary noise.

Please send an updated patch, so we have in the archives the exact patch as what is
checked in, and in case some other maintainer wants to take a look, best have him look at
the refreshed patch.   In fact, if you had sent it already in that email, there'd have been
no extra noise, right?  ;-)

-- 
Pedro Alves

Re: [PATCH v3] Add dll trampoline code handling for windows 64bit

[Joel Brobecker]

Hi Roland,

> 2012-03-16  Roland Schwingel <roland.schwingel@onevision.com>
> 
>         * amd64-windows-tdep.c: #include "frame.h".
>         (amd64_windows_skip_trampoline_code): New function.
>         (amd64_windows_init_abi): Add trampoline registration.

No one reviewed þhe patch as far as I can tell, I am sorry about that.
As a general guideline, it's OK to ping us every week or two...

I think the code looks pretty good, and you can commit, with a couple
of very minor almost-nitpicky little comments (a apologize for asking
for your forgiveness)...

Hmmm, I actually cannot find any copyright assignment for you on
file. Do you have one? Or is this done on behalf of your employer
who might have one? Please let me know... We can give you write-
after-approval priviledges as soon as we have confirmed that you
do have an assignment on file.

> +      /* Get address of function pointer at end of pc.  */
> +      CORE_ADDR indirect_addr = pc + offset + 6;

I think it would be useful to explain where the magic constant "6"
comes from... I'd almost write the expression "pc + 6 + offset".

> +      struct minimal_symbol *indsym =
> +        indirect_addr ? lookup_minimal_symbol_by_pc (indirect_addr) : 0;
> +      const char *symname = indsym ? SYMBOL_LINKAGE_NAME (indsym) : 0;

I'd rather you used NULL instead of 0, even if we're pretty much
guaranteed that NULL will always be zero. That's really nit-picky,
but it seems clearer that way, at least for me.

> +            destination = 

Trailing space there?

> +  /* register trampoline handling code.  */

Sentences need to start with a capital letter...

-- 
Joel

[PATCH v3] Add dll trampoline code handling for windows 64bit

[Roland Schwingel]

[-- Attachment #1: Type: text/plain, Size: 908 bytes --]

Hi,

I hope this is the last round on this...

 > Kai Tietz read this patch -- he says "hello" and asked me to respond.
 >
 > Roland> +      struct minimal_symbol *indsym =
 > Roland> +        indirect ? lookup_minimal_symbol_by_pc (pc + 
indirect) : 0;
 >
 > Kai says that PC-relative expressions are relative to the end of the
 > instruction.  So, 'pc + indirect' is incorrect here and also in the
 > read_memory call.
Oh man! I couldn't see the forest for the trees.
While my previous approach was in the end correctly working this is
the cleaner solution. That way read_memory_unsigned_integer()
can be used now, delivering correct results.

ChangeLog:

2012-03-16  Roland Schwingel <roland.schwingel@onevision.com>

         * amd64-windows-tdep.c: #include "frame.h".
         (amd64_windows_skip_trampoline_code): New function.
         (amd64_windows_init_abi): Add trampoline registration.

Roland


[-- Attachment #2: amd64-windows-tdep.c.patch --]
[-- Type: text/plain, Size: 1999 bytes --]

--- amd64-windows-tdep.c_orig	2012-03-02 01:06:12.000000000 +0100
+++ amd64-windows-tdep.c	2012-03-16 10:35:22.863279100 +0100
@@ -23,6 +23,7 @@
 #include "gdbtypes.h"
 #include "gdbcore.h"
 #include "regcache.h"
+#include "frame.h"
 
 /* The registers used to pass integer arguments during a function call.  */
 static int amd64_windows_dummy_call_integer_regs[] =
@@ -153,12 +154,49 @@
   return pc;
 }
 
+/* Check win64 DLL jmp trampolines and find jump destination.  */
+
+static CORE_ADDR
+amd64_windows_skip_trampoline_code (struct frame_info *frame, CORE_ADDR pc)
+{
+  CORE_ADDR destination = 0;
+  struct gdbarch *gdbarch = get_frame_arch (frame);
+  enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
+
+  /* Check for jmp *<offset>(%rip) (jump near, absolute indirect (/4)).  */
+  if (pc && read_memory_unsigned_integer (pc, 2, byte_order) == 0x25ff)
+    {
+      /* Get opcode offset and see if we can find a reference in our data.  */
+      ULONGEST offset =
+        read_memory_unsigned_integer (pc + 2, 4, byte_order);
+
+      /* Get address of function pointer at end of pc.  */
+      CORE_ADDR indirect_addr = pc + offset + 6;
+
+      struct minimal_symbol *indsym =
+        indirect_addr ? lookup_minimal_symbol_by_pc (indirect_addr) : 0;
+      const char *symname = indsym ? SYMBOL_LINKAGE_NAME (indsym) : 0;
+
+      if (symname)
+        {
+          if (strncmp (symname, "__imp_", 6) == 0
+              || strncmp (symname, "_imp_", 5) == 0)
+            destination = 
+                read_memory_unsigned_integer (indirect_addr, 8, byte_order);
+        }
+    }
+
+  return destination;
+}
 
 static void
 amd64_windows_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
 {
   struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
 
+  /* register trampoline handling code.  */
+  set_gdbarch_skip_trampoline_code (gdbarch, amd64_windows_skip_trampoline_code);
+
   amd64_init_abi (info, gdbarch);
 
   /* On Windows, "long"s are only 32bit.  */