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

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

[Roland Schwingel]

Hi Joel,

Thanks for you reply and suggestions. I will prepare a new patch
tomorrow when back at the office with all your suggestions.

Joel Brobecker <brobecker@adacore.com> wrote on 14.03.2012 17:13:03:
 > In addition to Tom's answer, I have a few minor comments. The real
 > review will have to come from our Windows Maintainer for your patch
 > to be approved...
 >
 > > 2012-03-14  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.
 >
 > (missing period at the end of the first line).
OK

There is a script called gdb_indent.sh in gdb's root dir. I applied
it on amd64-windows-nat.c ahead of getting out my patch thinking
that this would be the correct way. Some misunderstanding as it
produced most of your indention concerns.

My patch is based upon the implementation for windows 32 bit in
i386-tdep.c (function  i386_pe_skip_trampoline_code()). I made
my patch as close as possible to the implementation there thinking
that would make it easier to be accepted. If you take a look there
you will see home similar my patch is.

Your suggestions are newer, so I will take them into account.

 > > +            read_memory (pc + indirect, addr, 8);
 > > +            pos = (gdb_byte *) &destination;
 > > +            pos[0] = addr[6];
 > > +            pos[1] = addr[7];
 > > +            pos[2] = addr[0];
 > > +            pos[3] = addr[1];
 > > +            pos[4] = addr[2];
 > > +            pos[5] = addr[3];
 > > +            pos[6] = addr[4];
 > > +            pos[7] = addr[5];
 > > +
 > > +            return destination;
 >
 > Yeah, Tom's suggestion is a better suggestion. I think you are going
 > to have endianness issues this way.  You could use
 > read_memory_typed_address as well, but it's a little more involved,
 > and I don't think it's necessary here.  But otherwise, to me, the
 > latter is the function to be used for reading addresses from inferior
 > memory.
Before doing it "my" way I already played around with both
read_memory_typed_address and read_memory_unsigned_integer
but did not get the correct CORE_ADDR. The bytes were always
in the wrong order believing that these are endianess issues. Will
reinvestigate that when back at the office. Anyhow my approach
appears to be working, too. I succesfully single stepped thru
many dlls using my patch on win64 which was not possible
before.

 > I am sorry if it feels like it's a lot of little rules. It is. But
 > it should be easy to learn them and it allows us to have a consistent
 > style for our code.
Sure. No problem!

Thanks for taking a look at my patch,

Roland

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

[Joel Brobecker]

> There is a script called gdb_indent.sh in gdb's root dir. I applied
> it on amd64-windows-nat.c ahead of getting out my patch thinking
> that this would be the correct way. Some misunderstanding as it
> produced most of your indention concerns.

That is very odd, the script should indeed indent the sources
in a way that follows the GNU Coding Standards. Perhaps we are
missing some parameters in the call to that script that override
certain defaults that may no longer match our standards. But FWIW,
we have rarely used that script in the past few years. I think
that this is because GNU indent, the tool that it uses underneath,
isn't very smart and often produces a layout that's worse than
what a human would produce.

I should also say that we have a lot of violations to the GCS
spread around the code, and it's very common for someone to
copy/paste them, or introduce new ones on their own. I know
I do, occasionally. I have observed that we have been trying harder
in the recent past to be stricter and more aware of them. It's
a bit of a pain both for the contributor and the reviewer, but
it's easy to fix, and I think it's for the best in the end.

> Before doing it "my" way I already played around with both
> read_memory_typed_address and read_memory_unsigned_integer
> but did not get the correct CORE_ADDR. The bytes were always
> in the wrong order believing that these are endianess issues. Will
> reinvestigate that when back at the office. Anyhow my approach
> appears to be working, too. I succesfully single stepped thru
> many dlls using my patch on win64 which was not possible before.

That's odd, particulary if you tested with a native GDB? Perhaps
the address is stored in a different way, but that would also
be very surprising. Good luck with the hunting!

-- 
Joel

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

[Stan Shebs]

On 3/14/12 2:03 PM, Joel Brobecker wrote:
>> There is a script called gdb_indent.sh in gdb's root dir. I applied
>> it on amd64-windows-nat.c ahead of getting out my patch thinking
>> that this would be the correct way. Some misunderstanding as it
>> produced most of your indention concerns.
> That is very odd, the script should indeed indent the sources
> in a way that follows the GNU Coding Standards. Perhaps we are
> missing some parameters in the call to that script that override
> certain defaults that may no longer match our standards. But FWIW,
> we have rarely used that script in the past few years. I think
> that this is because GNU indent, the tool that it uses underneath,
> isn't very smart and often produces a layout that's worse than
> what a human would produce.
>

I've never been able to get indent to produce exactly the same results 
as Emacs' indent, even though in theory they are using the same 
algorithm with the same parameters.

Probably the best recommendation for contributors is to use Emacs 
indent-region on their new code fragments, and to leave everything else 
in the file alone.

Stan

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

[Tom Tromey]

>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:

Joel> That is very odd, the script should indeed indent the sources
Joel> in a way that follows the GNU Coding Standards. Perhaps we are
Joel> missing some parameters in the call to that script that override
Joel> certain defaults that may no longer match our standards.

When I looked at this for GCC, some years ago now, GNU indent did not
have all the needed options to really mimic GNU style as actually used.

I would like it if we could use some indentation tool.
This would remove a lot of drudgery from patch review.

But, this would require someone doing an investigation into the existing
tools to see if they can do the job.  Maybe GNU indent is better now
(though I never heard a response to my various bug reports); or maybe
uncrustify (http://uncrustify.sourceforge.net/) is good enough.

Right now, unless someone volunteers to do the above, I think we should
remove gdb_indent.sh as being actively confusing.


Here's a starting point for uncrustify:

http://debbugs.gnu.org/db/63/6318.html

This bug has a GNU-ish config file attached.

Tom

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

[Joel Brobecker]

> Right now, unless someone volunteers to do the above, I think we should
> remove gdb_indent.sh as being actively confusing.

+1

-- 
Joel

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

[Joel Brobecker]

Hi Roland,

Thanks for the patch.

In addition to Tom's answer, I have a few minor comments. The real
review will have to come from our Windows Maintainer for your patch
to be approved...

> 2012-03-14  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.

(missing period at the end of the first line).

> +/* Stuff for WIN64 PE style DLL's but is pretty generic really.  */

It's great that you thought about adding a description comment to
document the new function, thank you! I just wish the description
was a little more precise. In fact, what this function does should
already be described in gdbarch.h.  For gdbarch callback/"methods",
we usually simply say:

/* Implement the "skip_trampoline_code" gdbarch method.  */

No need to repeat the hook documentation at each instance.

> +    struct gdbarch     *gdbarch = get_frame_arch (frame);
> +    enum bfd_endian     byte_order = gdbarch_byte_order (gdbarch);

Formatting nit: We do not align parameters in GDB code.

Also, the indentation is wrong. We use 2 spaces, not 4, so the whole
function needs to be re-indented.

> +   /* check for jmp *<offset>(%rip) */

Also, the GNU Coding Style which GDB follows requires us to start
sentences with a capital letter, and to end them with a period.
(and 2 spaces after periods).  I would format the comment above as:

        /* Check for "jmp *<offset>(%rip)".  */

> +    if (pc && read_memory_unsigned_integer (pc, 2, byte_order) == 0x25ff)
> +      {
> +	  unsigned long indirect =
> +	    read_memory_unsigned_integer (pc + 2, 4, byte_order);

Can use use ULONGEST here, instead of "unsigned long"?

> +		      CORE_ADDR           destination;
> +		      gdb_byte           *pos, addr[8];

Same as above, please, no extra spacing to align the parameter names.

> +		      read_memory (pc + indirect, addr, 8);
> +		      pos = (gdb_byte *) &destination;
> +		      pos[0] = addr[6];
> +		      pos[1] = addr[7];
> +		      pos[2] = addr[0];
> +		      pos[3] = addr[1];
> +		      pos[4] = addr[2];
> +		      pos[5] = addr[3];
> +		      pos[6] = addr[4];
> +		      pos[7] = addr[5];
> +
> +		      return destination;

Yeah, Tom's suggestion is a better suggestion. I think you are going
to have endianness issues this way.  You could use
read_memory_typed_address as well, but it's a little more involved,
and I don't think it's necessary here.  But otherwise, to me, the
latter is the function to be used for reading addresses from inferior
memory.

I am sorry if it feels like it's a lot of little rules. It is. But
it should be easy to learn them and it allows us to have a consistent
style for our code.

Thanks,
-- 
Joel

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

[Tom Tromey]

>>>>> "Roland" == Roland Schwingel <roland@onevision.com> writes:

Roland> - The jump destination is on 64bit windows of course 8 byte
Roland> long. I could not find a function that transforms this into a
Roland> CORE_ADDR like read_memory_unsigned_integer() it is doing in the
Roland> 32bit case.  So I did the transformation on my own. While this
Roland> is high performant it might not be the "official" gdb way. If
Roland> someone can give me a hint on how to the transformation the
Roland> "official" way I will adjust my patch - if wished.

Just use read_memory_unsigned_integer.
I think it is guaranteed that CORE_ADDR will not be larger than
ULONGEST.

I can't really comment on the rest of your patch.

I think it is big enough that it requires a copyright assignment.
If you don't have one, contact me off-list to get started.

Tom

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

[Roland Schwingel]

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

Hi...

When single stepping a 64bit windows application gdb at present does not 
step into
functions residing in a dll. This is due to the fact that handling of 
dll trampoline code
for win64 is missing.

I added a new function to amd64-windows-tdep.c to handle this similar to 
the existing function (i386_pe_skip_trampoline_code()). With some 
differences:

- On 32bit windows dll trampoline code is expressed as jmp *(dest) while on
   64bit windows this is expressed as jmp *<offset>(%rip). Took care of 
this.
- The jump destination is on 64bit windows of course 8 byte long. I 
could not
   find a function that transforms this into a CORE_ADDR like 
read_memory_unsigned_integer()
   it is doing in the 32bit case.  So I did the transformation on my 
own. While this is
   high performant it might not be the "official" gdb way. If someone 
can give me a
   hint on how to the transformation the "official" way I will adjust my 
patch - if wished.

Now single stepping into dll code works.

ChangeLog:

2012-03-14  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: 2086 bytes --]

--- amd64-windows-tdep.c_orig	2012-03-02 01:06:12.000000000 +0100
+++ amd64-windows-tdep.c	2012-03-14 13:31:39.815727600 +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,59 @@
   return pc;
 }
 
+/* Stuff for WIN64 PE style DLL's but is pretty generic really.  */
+
+static CORE_ADDR
+amd64_windows_skip_trampoline_code (struct frame_info *frame, CORE_ADDR pc)
+{
+    struct gdbarch     *gdbarch = get_frame_arch (frame);
+    enum bfd_endian     byte_order = gdbarch_byte_order (gdbarch);
+
+   /* check for jmp *<offset>(%rip) */
+    if (pc && read_memory_unsigned_integer (pc, 2, byte_order) == 0x25ff)
+      {
+	  unsigned long indirect =
+	    read_memory_unsigned_integer (pc + 2, 4, byte_order);
+	  struct minimal_symbol *indsym =
+	    indirect ? lookup_minimal_symbol_by_pc (pc + indirect) : 0;
+	  const char *symname =
+	    indsym ? SYMBOL_LINKAGE_NAME (indsym) : 0;
+
+	  if (symname)
+	    {
+		if (strncmp (symname, "__imp_", 6) == 0
+		    || strncmp (symname, "_imp_", 5) == 0)
+		  {
+		      CORE_ADDR           destination;
+		      gdb_byte           *pos, addr[8];
+
+		      read_memory (pc + indirect, addr, 8);
+		      pos = (gdb_byte *) &destination;
+		      pos[0] = addr[6];
+		      pos[1] = addr[7];
+		      pos[2] = addr[0];
+		      pos[3] = addr[1];
+		      pos[4] = addr[2];
+		      pos[5] = addr[3];
+		      pos[6] = addr[4];
+		      pos[7] = addr[5];
+
+		      return destination;
+		  }
+	    }
+      }
+    return 0;			/* Not a trampoline.  */
+}
 
 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.  */