[RFC] Enhance backtrace for microsoft system DLL calls

[RFC] Enhance backtrace for microsoft system DLL calls

[Pierre Muller]

  I had troubles when trying to backtrace
when the debugge was stopped 
inside the windows DLL's.

  After some investigation, I 
found out that many exported routines
from the Microsoft operating system
start with a no-op assembler instruction
'mov %edi,%edi'.
  The presence of this instruction
leads to a complete failure of the 
function prologue analysis,
leading in turn to a bad backtrace.

  I am a small script that would look for all
the exported functions that start with 'mov %edi, %edi'
and this is what I got:
ntdll.dll: 532 functions out of 1311 start with that pattern
kernel32.dll: 721 functions out of 947.
gdi32.dll: 439 functions out of 609. 
shell32.dll: 268 functions out of 309.
user32.dll: 531 functions out of 732.
(on a Windows XP operating system).

   The patch simply discard a leading 'mov %edi,%edi'
instruction in the i386_analyze_frame_setup function.

  The main question is whether this patch is acceptable
for gdb as it is in a i386 common file, while 
it most probably only applies to MS operating system.

  The problem is that I found no other location where 
this could be done, but maybe someone in the list
has a better overview and a good idea where to put that.

  Tested on cygwin with no regressions.
If you put a breakpoint on ZwSuspendThread (in ntdll.dll)
you will get this with current CVS head:

Breakpoint 4, 0x7c90e84f in ntdll!ZwSuspendThread ()
   from /cygdrive/c/WINDOWS/system32/ntdll.dll
(top-gdb) bt
#0  0x7c90e84f in ntdll!ZwSuspendThread ()
   from /cygdrive/c/WINDOWS/system32/ntdll.dll
#1  0x7c839744 in SuspendThread ()
   from /cygdrive/c/WINDOWS/system32/kernel32.dll
#2  0x00000680 in ?? ()
#3  0x0022c550 in ?? ()
#4  0x0022c568 in ?? ()
During symbol reading, struct/union type gets multiply defined: struct
language_
defn.
#5  0x0046b34a in thread_rec (id=1664, get_context=4374409)
    at ../../purecvs/gdb/win32-nat.c:265
Backtrace stopped: frame did not save the PC

With the attached patch, you get that:
Breakpoint 4, 0x7c90e84f in ntdll!ZwSuspendThread ()
   from /cygdrive/c/WINDOWS/system32/ntdll.dll
(top-gdb) bt
#0  0x7c90e84f in ntdll!ZwSuspendThread ()
   from /cygdrive/c/WINDOWS/system32/ntdll.dll
#1  0x7c839744 in SuspendThread ()
   from /cygdrive/c/WINDOWS/system32/kernel32.dll
During symbol reading, struct/union type gets multiply defined: struct
language_
defn.
#2  0x0046b34a in thread_rec (id=3820, get_context=1)
    at ../../purecvs/gdb/win32-nat.c:265
#3  0x0046b815 in win32_fetch_inferior_registers (regcache=0x1063d9a8, r=8)
    at ../../purecvs/gdb/win32-nat.c:422



Pierre Muller

ChangeLog entry:

2007-12-10  Pierre Muller  <muller@ics.u-strasbg.fr>

	* i386-tdep.c (i386_analyze_frame_setup): Ignore `mov %edi,%edi'
instruction
	used at entry of some operating system calls.

	


Index: gdb/i386-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/i386-tdep.c,v
retrieving revision 1.246
diff -u -p -r1.246 i386-tdep.c
--- gdb/i386-tdep.c     6 Dec 2007 16:32:59 -0000       1.246
+++ gdb/i386-tdep.c     10 Dec 2007 16:22:21 -0000
@@ -650,6 +650,17 @@ i386_analyze_frame_setup (CORE_ADDR pc,

   read_memory_nobpt (pc, &op, 1);

+  if (op == 0x8b) /* Ignore no-op instruction `mov %edi, %edi' */
+    {
+      read_memory_nobpt (pc + 1, &op, 1);
+      if (op == 0xff)
+       {
+         pc += 2;
+         read_memory_nobpt (pc, &op, 1);
+       }
+      else
+       op = 0x8b;
+    }
   if (op == 0x55)              /* pushl %ebp */
     {
       /* Take into account that we've executed the `pushl %ebp' that

Re: [RFC] Enhance backtrace for microsoft system DLL calls

[Pedro Alves]

Hi Pierre,

Pierre Muller wrote:
>   I had troubles when trying to backtrace
> when the debugge was stopped
> inside the windows DLL's.
>
>   After some investigation, I
> found out that many exported routines
> from the Microsoft operating system
> start with a no-op assembler instruction
> 'mov %edi,%edi'.

That's placed there for hot patching, together with 5 bytes of slack before the
function (the idea is to be able to replace that 2 byte op with a jump to
5 bytes back, and patch the 5 bytes with a jump into anywhere in the
32-bit address space.)

Something like:

    nop                            |
    nop                            |
    nop                            |  hot patching
    nop                            |  support
    nop                            |
    mov    %edi,%edi               |   <-- function start
-----------------------------------+
    push   %ebp                    |
    mov    %esp,%ebp               |  frame setup
    sub    $0x18,%esp              |  locals, ...

Could you add a comment explaining that as well?

>   The main question is whether this patch is acceptable
> for gdb as it is in a i386 common file, while
> it most probably only applies to MS operating system.
>
>   The problem is that I found no other location where
> this could be done, but maybe someone in the list
> has a better overview and a good idea where to put that.
>

You can put a flag in i386's gdbarch_tdep (look in
i386-tdep.h, and i386-cygwin-tdep.c).

-- 
Pedro Alves

Re: [RFC] Enhance backtrace for microsoft system DLL calls

[Daniel Jacobowitz]

On Mon, Dec 10, 2007 at 05:31:35PM +0000, Pedro Alves wrote:
> That's placed there for hot patching, together with 5 bytes of slack before the
> function (the idea is to be able to replace that 2 byte op with a jump to
> 5 bytes back, and patch the 5 bytes with a jump into anywhere in the
> 32-bit address space.)

Interesting.

> You can put a flag in i386's gdbarch_tdep (look in
> i386-tdep.h, and i386-cygwin-tdep.c).

I don't think we need to.  Hot patching this way may be MS-specific,
but a two byte NOP is a two byte NOP no matter where you find it.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFC] Enhance backtrace for microsoft system DLL calls

[Pedro Alves]

> @@ -650,6 +650,17 @@ i386_analyze_frame_setup (CORE_ADDR pc,
>
>    read_memory_nobpt (pc, &op, 1);
>
> +  if (op == 0x8b) /* Ignore no-op instruction `mov %edi, %edi' */
> +    {
> +      read_memory_nobpt (pc + 1, &op, 1);
> +      if (op == 0xff)
> +       {
> +         pc += 2;
> +         read_memory_nobpt (pc, &op, 1);
> +       }
> +      else
> +       op = 0x8b;
> +    }
>    if (op == 0x55)              /* pushl %ebp */
>      {
>        /* Take into account that we've executed the `pushl %ebp' that
>
>
>

Oh, I forgot to ask on the previous mail  -- Is there a reason you don't
read both bytes in one go?

 /* small hot patching description here.  */
 gdb_byte hot_patch[2] = { 0x8b, 0xff };
 read_memory_nobpt (pc, buf, 2);
 if (memcmp (hot_patch, buf) == 0)
   pc += 2;

 read_memory_nobpt (pc, &op, 1);
 if (op == 0x55)              /* pushl %ebp */

 ... and since this isn't really frame setup code, it
could be moved into a separate function called from
i386_analyze_prologue, probably even before
i386_follow_jump, as this is put really at the
start of the function </end nit>

-- 
Pedro Alves

Re: [RFC] Enhance backtrace for microsoft system DLL calls

[Mark Kettenis]

> From: "Pierre Muller" <muller@ics.u-strasbg.fr>
> Date: Mon, 10 Dec 2007 17:33:05 +0100
> 
>   The main question is whether this patch is acceptable
> for gdb as it is in a i386 common file, while 
> it most probably only applies to MS operating system.

I have no problem with adding this to the generic i386, but I'd prefer
to put this code in a seperate function called
i386_skip_nops(CORE_ADDR pc); And call that function from
i386_analyze_prologue(), instead of adding this code to
i386_analyze_frame_setup().

> Pierre Muller
> 
> ChangeLog entry:
> 
> 2007-12-10  Pierre Muller  <muller@ics.u-strasbg.fr>
> 
> 	* i386-tdep.c (i386_analyze_frame_setup): Ignore `mov %edi,%edi'
> instruction
> 	used at entry of some operating system calls.
> 
> 	
> 
> 
> Index: gdb/i386-tdep.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/i386-tdep.c,v
> retrieving revision 1.246
> diff -u -p -r1.246 i386-tdep.c
> --- gdb/i386-tdep.c     6 Dec 2007 16:32:59 -0000       1.246
> +++ gdb/i386-tdep.c     10 Dec 2007 16:22:21 -0000
> @@ -650,6 +650,17 @@ i386_analyze_frame_setup (CORE_ADDR pc,
> 
>    read_memory_nobpt (pc, &op, 1);
> 
> +  if (op == 0x8b) /* Ignore no-op instruction `mov %edi, %edi' */
> +    {
> +      read_memory_nobpt (pc + 1, &op, 1);
> +      if (op == 0xff)
> +       {
> +         pc += 2;
> +         read_memory_nobpt (pc, &op, 1);
> +       }
> +      else
> +       op = 0x8b;
> +    }
>    if (op == 0x55)              /* pushl %ebp */
>      {
>        /* Take into account that we've executed the `pushl %ebp' that
> 
> 
> 

RE: [RFC] Enhance backtrace for microsoft system DLL calls

[Pierre Muller]

> -----Original Message-----
> From: Mark Kettenis [mailto:mark.kettenis@xs4all.nl]
> Sent: Monday, December 10, 2007 7:54 PM
> To: muller@ics.u-strasbg.fr
> Cc: gdb-patches@sourceware.org
> Subject: Re: [RFC] Enhance backtrace for microsoft system DLL calls
> 
> > From: "Pierre Muller" <muller@ics.u-strasbg.fr>
> > Date: Mon, 10 Dec 2007 17:33:05 +0100
> >
> >   The main question is whether this patch is acceptable
> > for gdb as it is in a i386 common file, while
> > it most probably only applies to MS operating system.
> 
> I have no problem with adding this to the generic i386, but I'd prefer
> to put this code in a seperate function called
> i386_skip_nops(CORE_ADDR pc); 
  I have no objections to that proposal
and can change my patch accordingly,
but the question is then:
  Should I check all possible no-op codes?
or only the one we know is used in one case i.e. 'mov %edi, %edi'

  There are many other nops:
'nop' instruction itself of course
but also all 'mov %reg,%reg' are no-ops,
even if preceded by size modifiers of 1 or 2 byte regs.

  Testing all these might be a bit silly, no?
 
  Thus, I would only test 'mov %edi,%edi'
adding Pedro's comments about its use in microsoft's operating system dll's.
  But if you think it is worthwhile to 
test other instructions, I would like to 
know it before updating the patch.



Pierre Muller

[RFC-v2] Enhance backtrace for microsoft system DLL calls

[Pierre Muller]

  I wrote a i386_skip_noop function.

  It currently only tests for 'nop' and 'mov %edi,%edi'
instructions, but the way it is written, other
instructions should be easy to add.
  I also tried to explain the reason of the presence 
of the 'mov %edi,%edi' instruction in the win32 system DLL prologue,
as explained by Pedro. 
  

  Tested on cygwin target, no regressions found.
The patch allows to get the backtrace of the main thread of gdb 
to come up to the  functions that called the systems DLL.
If I use ./gdb ./gdb with 'set new-console on'
and use Ctrl-C on the debuggee gdb window.
Without the patch, the backtrace only shows
3 levels in ntdll.dll and kernel32.dll

Questions:
 1) Is the 'nop' test useful or should it be removed?

 2) Should we add other possible no-ops?
  As said in my previous email, the number of
possible no-ops is big, and it is probably not wise to test all of 
them.
 
3) this call is used for all i386 targets, but it
is probably useless for all operating systems but Microsoft Windows,
so should it be called only for that OS, and if yes, how should
we code this?

4) Any suggestions to make the comment clearer will be 
most appreciated.

Pierre Muller



ChangeLog entry:

2008-01-14  Pierre Muller  <muller@ics.u-strasbg.fr>

	* i386-tdep.c (i386_skip_noop): New function.
	(i386_analyze_prologue): Call i386_skip_noop function.


Index: gdb/i386-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/i386-tdep.c,v
retrieving revision 1.248
diff -u -p -r1.248 i386-tdep.c
--- gdb/i386-tdep.c	11 Jan 2008 13:20:00 -0000	1.248
+++ gdb/i386-tdep.c	14 Jan 2008 08:11:27 -0000
@@ -632,6 +632,51 @@ struct i386_insn i386_frame_setup_skip_i
   { 0 }
 };
 
+
+/* Check whether PC points to a no-op instruction.  */
+static CORE_ADDR
+i386_skip_noop (CORE_ADDR pc)
+{
+  gdb_byte op;
+  int check = 1;
+
+  read_memory_nobpt (pc, &op, 1);
+
+  while (check) 
+    {
+      check = 0;
+      /* Ignore `nop' instruction.  */
+      if (op == 0x90) 
+	{
+	  pc += 1;
+	  read_memory_nobpt (pc, &op, 1);
+	  check = 1;
+	}
+      /* Ignore no-op instruction `mov %edi, %edi'.
+	 Microsoft system dlls often start with
+	 a `mov %edi,%edi' instruction.
+	 The 5 bytes before the function start are
+	 filled with `nop' instructions.
+	 This pattern can be used for hot-patching:
+	 The `mov %edi, %edi' instruction can be replaced by a
+	 near jump to the location of the 5 `nop' instructions
+	 which can be replaced by a 32-bit jump to anywhere
+	 in the 32-bit address space.  */
+
+      else if (op == 0x8b)
+	{
+	  read_memory_nobpt (pc + 1, &op, 1);
+	  if (op == 0xff)
+	    {
+	      pc += 2;
+	      read_memory_nobpt (pc, &op, 1);
+	      check = 1;
+	    }
+	}
+    }
+  return pc; 
+}
+
 /* Check whether PC points at a code that sets up a new stack frame.
    If so, it updates CACHE and returns the address of the first
    instruction after the sequence that sets up the frame or LIMIT,
@@ -817,6 +862,7 @@ static CORE_ADDR
 i386_analyze_prologue (CORE_ADDR pc, CORE_ADDR current_pc,
 		       struct i386_frame_cache *cache)
 {
+  pc = i386_skip_noop (pc);
   pc = i386_follow_jump (pc);
   pc = i386_analyze_struct_return (pc, current_pc, cache);
   pc = i386_skip_probe (pc);

Re: [RFC-v2] Enhance backtrace for microsoft system DLL calls

[Pedro Alves]

Pierre Muller wrote:
>   I wrote a i386_skip_noop function.

>   Tested on cygwin target, no regressions found.
> The patch allows to get the backtrace of the main thread of gdb 
> to come up to the  functions that called the systems DLL.
> If I use ./gdb ./gdb with 'set new-console on'
> and use Ctrl-C on the debuggee gdb window.
> Without the patch, the backtrace only shows
> 3 levels in ntdll.dll and kernel32.dll
> 

[ I forgot to say before:
   This is great, thanks for doing this! ]

> Questions:
>  1) Is the 'nop' test useful or should it be removed?
> 
>  2) Should we add other possible no-ops?

It is my opinion that it isn't needed, and the function that
detects the mov %edi,%edi should be called i386_skip_msft_hotpatch,
or i386_skip_hotpatch.  The nop is a 2 byte op for a reason,
plus, I don't see the point of detecting a lot of patterns if
we know they're never emitted.  Plus, if we ever need to augment
this hot-patching support to another different form, of detect the
5 bytes slack before, this is the natural place to do it.
Plus not detecting for a 1 byte nop, you can read 2 bytes at once.
(ok, that is going extreme :-) )

> 3) this call is used for all i386 targets, but it
> is probably useless for all operating systems but Microsoft Windows,
> so should it be called only for that OS, and if yes, how should
> we code this?
> 

The way to do it would be to put a flag in i386's gdbarch_tdep,
but Daniel didn't think we need to keep this MSFT specific.
(adding a flag would at least prevent a needless memory read
  on non Windows platforms)

 > 4) Any suggestions to make the comment clearer will be
 > most appreciated.

Grabbing my own comment from upthread, how about:

/* Some Microsoft's system dll functions start with a
    `mov %edi,%edi' instruction, which is effectively a two byte `nop'.
    This instruction is used for hot patching support, together with 5
    bytes of slack before the function.  Later, when hot-patching, the 2
    byte op can be replaced with a relative jump to 5 bytes back.  The 5
    bytes slack is large enough to hold a jump into anywhere in
    the 32-bit address space.  */

If you find it interesting, you can add:

/* A two byte nop is used to be sure that no thread is executing
    the instruction at byte 1 of the function, so the patching can be
    performed atomically.  */

-- 
Pedro Alves

Re: [RFC-v2] Enhance backtrace for microsoft system DLL calls

[Mark Kettenis]

> From: "Pierre Muller" <muller@ics.u-strasbg.fr>
> Date: Mon, 14 Jan 2008 11:15:49 +0100

Hi Pierre,

I've been down with a nasty flu, so I didn't get around to properly
reviewing your diff until today.

>   I wrote a i386_skip_noop function.

Thanks.

>   It currently only tests for 'nop' and 'mov %edi,%edi'
> instructions, but the way it is written, other
> instructions should be easy to add.

>   I also tried to explain the reason of the presence 
> of the 'mov %edi,%edi' instruction in the win32 system DLL prologue,
> as explained by Pedro. 

The comment is badly formatted and I must say it is not very clear I
must say.  Would you mind if I rewrote it after you committed the
diff?

>   Tested on cygwin target, no regressions found.
> The patch allows to get the backtrace of the main thread of gdb 
> to come up to the  functions that called the systems DLL.
> If I use ./gdb ./gdb with 'set new-console on'
> and use Ctrl-C on the debuggee gdb window.
> Without the patch, the backtrace only shows
> 3 levels in ntdll.dll and kernel32.dll
> 
> Questions:
>  1) Is the 'nop' test useful or should it be removed?

What do you mean by this?  Are you saying you added the
single-instruction nop even though you've never seen it in those
DLL's?

>  2) Should we add other possible no-ops?

When we encounter them in the real world.  Starting with what's in
your diff is fine.

>   As said in my previous email, the number of
> possible no-ops is big, and it is probably not wise to test all of 
> them.
>  
> 3) this call is used for all i386 targets, but it
> is probably useless for all operating systems but Microsoft Windows,
> so should it be called only for that OS, and if yes, how should
> we code this?

I've heard of a couple of code generation tools that do something
similar as Microsoft and insert nop instructions at the start of a
function to be patched up later.  So other targets could benefit from
the same code.  And calling this function unconditionally keeps the
code simple.

> 4) Any suggestions to make the comment clearer will be 
> most appreciated.

See above.

> ChangeLog entry:
> 
> 2008-01-14  Pierre Muller  <muller@ics.u-strasbg.fr>
> 
> 	* i386-tdep.c (i386_skip_noop): New function.
> 	(i386_analyze_prologue): Call i386_skip_noop function.
> 
> 
> Index: gdb/i386-tdep.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/i386-tdep.c,v
> retrieving revision 1.248
> diff -u -p -r1.248 i386-tdep.c
> --- gdb/i386-tdep.c	11 Jan 2008 13:20:00 -0000	1.248
> +++ gdb/i386-tdep.c	14 Jan 2008 08:11:27 -0000
> @@ -632,6 +632,51 @@ struct i386_insn i386_frame_setup_skip_i
>    { 0 }
>  };
>  
> +
> +/* Check whether PC points to a no-op instruction.  */
> +static CORE_ADDR
> +i386_skip_noop (CORE_ADDR pc)
> +{
> +  gdb_byte op;
> +  int check = 1;
> +
> +  read_memory_nobpt (pc, &op, 1);
> +
> +  while (check) 
> +    {
> +      check = 0;
> +      /* Ignore `nop' instruction.  */
> +      if (op == 0x90) 
> +	{
> +	  pc += 1;
> +	  read_memory_nobpt (pc, &op, 1);
> +	  check = 1;
> +	}
> +      /* Ignore no-op instruction `mov %edi, %edi'.
> +	 Microsoft system dlls often start with
> +	 a `mov %edi,%edi' instruction.
> +	 The 5 bytes before the function start are
> +	 filled with `nop' instructions.
> +	 This pattern can be used for hot-patching:
> +	 The `mov %edi, %edi' instruction can be replaced by a
> +	 near jump to the location of the 5 `nop' instructions
> +	 which can be replaced by a 32-bit jump to anywhere
> +	 in the 32-bit address space.  */
> +
> +      else if (op == 0x8b)
> +	{
> +	  read_memory_nobpt (pc + 1, &op, 1);
> +	  if (op == 0xff)
> +	    {
> +	      pc += 2;
> +	      read_memory_nobpt (pc, &op, 1);
> +	      check = 1;
> +	    }
> +	}
> +    }
> +  return pc; 
> +}
> +
>  /* Check whether PC points at a code that sets up a new stack frame.
>     If so, it updates CACHE and returns the address of the first
>     instruction after the sequence that sets up the frame or LIMIT,
> @@ -817,6 +862,7 @@ static CORE_ADDR
>  i386_analyze_prologue (CORE_ADDR pc, CORE_ADDR current_pc,
>  		       struct i386_frame_cache *cache)
>  {
> +  pc = i386_skip_noop (pc);
>    pc = i386_follow_jump (pc);
>    pc = i386_analyze_struct_return (pc, current_pc, cache);
>    pc = i386_skip_probe (pc);
> 
> 
> 
> 
> 

[RFA] i386-tdep.c: Add i386_skip_noop function

[Pierre Muller]

  I tried to conciliate Pedro's and Mark's remarks
in a new patch.
  As this patch only tries to catch no-op
instruction added on purpose at the start
of a function to allow hot-patching, 
I removed the 'check' variable of my previous patch
that allowed to check several different patterns
as in the optics of 'on purpose no-op', 
looking for several patterns is a non-sense.
  As I also directly read 2 bytes, I added 
a check that returns pc in case the call to read_memory_nobpt
fails.

ChangeLog entry:
2008-01-25  Pierre Muller  <muller@ics.u-strasbg.fr>

	* i386-tdep.c (i386_skip_noop): New function.
	(i386_analyze_prologue): Call i386_skip_noop function.


Index: gdb/i386-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/i386-tdep.c,v
retrieving revision 1.248
diff -u -p -r1.248 i386-tdep.c
--- gdb/i386-tdep.c	11 Jan 2008 13:20:00 -0000	1.248
+++ gdb/i386-tdep.c	25 Jan 2008 10:29:09 -0000
@@ -632,6 +632,41 @@ struct i386_insn i386_frame_setup_skip_i
   { 0 }
 };
 
+
+/* Check whether PC points to a no-op instruction.  */
+static CORE_ADDR
+i386_skip_noop (CORE_ADDR pc)
+{
+  gdb_byte op[2];
+
+  if (read_memory_nobpt (pc, (gdb_byte *) &op, 2) != 0)
+    return pc;
+/* Some Microsoft's system dll functions start with a
+   `mov %edi,%edi' instruction, which is effectively a two byte `nop'.
+   This instruction is used for hot patching support, together with 5
+   bytes of slack before the function.  Later, when hot-patching, the 2
+   byte op can be replaced with a relative jump to 5 bytes back.  The 5
+   bytes slack is large enough to hold a jump into anywhere in
+   the 32-bit address space.  
+   A two byte nop is used to be sure that no thread is executing
+   the instruction at byte 1 of the function, so the patching can be
+   performed atomically.  */
+
+/* 0x8b,0xff matches `mov %edi,%edi' */
+  if (op[0] == 0x8b && op[1] == 0xff)
+    {
+      return pc + 2;
+    }
+/* Here other patterns can be added if found.  */
+/* Quoted from Mark Kettenis:
+   "I've heard of a couple of code generation tools that do something
similar
+   as Microsoft and insert nop instructions at the start of a function to
be
+   patched up later.  So other targets could benefit from the same code.  
+   And calling this function unconditionally keeps the code simple."  */
+
+  return pc; 
+}
+
 /* Check whether PC points at a code that sets up a new stack frame.
    If so, it updates CACHE and returns the address of the first
    instruction after the sequence that sets up the frame or LIMIT,
@@ -817,6 +852,7 @@ static CORE_ADDR
 i386_analyze_prologue (CORE_ADDR pc, CORE_ADDR current_pc,
 		       struct i386_frame_cache *cache)
 {
+  pc = i386_skip_noop (pc);
   pc = i386_follow_jump (pc);
   pc = i386_analyze_struct_return (pc, current_pc, cache);
   pc = i386_skip_probe (pc);

Re: [RFA] i386-tdep.c: Add i386_skip_noop function

[Joel Brobecker]

Hi Pierre,

This is not a formal review of your code - Mark is our de-facto
maintainer so unless he asks for some help, I prefer to defer to him.
But I thought I'd put a "Patch Champion" hat on, and make some tiny
comments.

> +/* Some Microsoft's system dll functions start with a

I'm not a native English speaker (originally I'm French, as I suspect
you are :), but the above sounds a little funny to me. I suggest either:

  - Some of Microsoft's system dll functions ...
  - Some functions in Microsoft's system dlls ...

Also, you inserted a line-break a bit early IMO. It's not consistent
with the line-length of the rest of the comment. But that's really
very very minor - you might have thought that you wanted `mov %edi,%edi'
and the word "instruction" on the same line, which is also a good
argument.

> +   `mov %edi,%edi' instruction, which is effectively a two byte `nop'.
                                                          ^^^^^^^^
                                            I suggest "2-byte", see below.
> +   This instruction is used for hot patching support, together with 5
> +   bytes of slack before the function.

It would be nicer, IMO, if "5" and "bytes" were on the same line.
It's easier to read.

> Later, when hot-patching, the 2

"2-byte" (no space, a dash).

> +   byte op can be replaced with a relative jump to 5 bytes back.  The 5
                                                   ^^
              Is the "to" correct, here? To me, I think it should be
              "a relative jump 5 bytes back".
> +   A two byte nop is used to be sure that no thread is executing
        ^^^^^^^^
I suggest you remain consistent and use "2-byte" everywhere.

> +   the instruction at byte 1 of the function, so the patching can be
> +   performed atomically.  */
> +
> +/* 0x8b,0xff matches `mov %edi,%edi' */
> +  if (op[0] == 0x8b && op[1] == 0xff)

The practice in that file (and many other tdep files that I have
worked on) is to just specify the instruction. Like so:

  if (op[0] == 0x8b && op[1] == 0xff)  /* mov %edi,%edi */

> +/* Here other patterns can be added if found.  */

I think that this comment in unnecessary, but check with Mark.


> +/* Quoted from Mark Kettenis:
> +   "I've heard of a couple of code generation tools that do something
> similar
> +   as Microsoft and insert nop instructions at the start of a function to
> be
> +   patched up later.  So other targets could benefit from the same code.  
> +   And calling this function unconditionally keeps the code simple."  */

I suggest that this comment be moved up, inside/after the comment explaining
what happens in some DLL functions.  You don't need to quote him, I
think that it's better if you write something that connects better with
what you wrote. For instance:

  Mark Kettenis (or maybe just "we") have heard of a couple of code
  generation tools taht do something similer.

Otherwise, the code itself looks good to me!

-- 
Joel

[RFA] i386-tdep.c: Add i386_skip_noop function; updated

[Pierre Muller]

  Joel,
  I took your suggestions into account.

  I hope this sounds better for everyone.


ChangeLog entry:
2008-01-25  Pierre Muller  <muller@ics.u-strasbg.fr>

	* i386-tdep.c (i386_skip_noop): New function.
	(i386_analyze_prologue): Call i386_skip_noop function.


Index: gdb/i386-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/i386-tdep.c,v
retrieving revision 1.248
diff -u -p -r1.248 i386-tdep.c
--- gdb/i386-tdep.c     11 Jan 2008 13:20:00 -0000      1.248
+++ gdb/i386-tdep.c     25 Jan 2008 16:35:10 -0000
@@ -632,6 +632,39 @@ struct i386_insn i386_frame_setup_skip_i
   { 0 }
 };

+
+/* Check whether PC points to a no-op instruction.  */
+static CORE_ADDR
+i386_skip_noop (CORE_ADDR pc)
+{
+  gdb_byte op[2];
+
+  if (read_memory_nobpt (pc, (gdb_byte *) &op, 2) != 0)
+    return pc;
+/* Some functions in Microsoft's system dlls start with a
+   `mov %edi,%edi' instruction, which is effectively a 2-byte `nop'.
+   This instruction is used for hot patching support, together with
+   5 bytes of slack before the function.  Later, when hot-patching, the
+   2-byte op can be replaced with a relative jump 5 bytes back.  The
+   5-byte slack is large enough to hold a jump into anywhere in
+   the 32-bit address space.
+   A 2-byte nop is used to be sure that no thread is executing
+   the instruction at byte 1 of the function, so the patching can be
+   performed atomically.  */
+
+/* We've heard of a couple of code generation tools that do something
similar
+   as Microsoft and insert nop instructions at the start of a function to
be
+   patched up later.  So other targets could benefit from the same code.
+   And calling this function unconditionally keeps the code simple.  */
+
+  if (op[0] == 0x8b && op[1] == 0xff) /* mov %edi,%edi */
+    {
+      return pc + 2;
+    }
+
+  return pc;
+}
+
 /* Check whether PC points at a code that sets up a new stack frame.
    If so, it updates CACHE and returns the address of the first
    instruction after the sequence that sets up the frame or LIMIT,
@@ -817,6 +850,7 @@ static CORE_ADDR
 i386_analyze_prologue (CORE_ADDR pc, CORE_ADDR current_pc,
                       struct i386_frame_cache *cache)
 {
+  pc = i386_skip_noop (pc);
   pc = i386_follow_jump (pc);
   pc = i386_analyze_struct_return (pc, current_pc, cache);
   pc = i386_skip_probe (pc);

Re: [RFA] i386-tdep.c: Add i386_skip_noop function; updated

[Mark Kettenis]

> From: "Pierre Muller" <muller@ics.u-strasbg.fr>
> Date: Fri, 25 Jan 2008 17:37:40 +0100
> 
>   Joel,
>   I took your suggestions into account.
> 
>   I hope this sounds better for everyone.

Sorry Pierre, but I relly liked your origional diff better (the one
that I reviewed yesterday).  Could you commit that one?  I'll clean up
the comments a bit after that.

Re: [RFA] i386-tdep.c: Add i386_skip_noop function; updated

[Joel Brobecker]

> >   Joel,
> >   I took your suggestions into account.
> > 
> >   I hope this sounds better for everyone.
> 
> Sorry Pierre, but I relly liked your origional diff better (the one
> that I reviewed yesterday).  Could you commit that one?  I'll clean up
> the comments a bit after that.

Sorry Pierre and Mark for causing extra work.

-- 
Joel

RE: [RFA] i386-tdep.c: Add i386_skip_noop function; updated

[Pierre Muller]

Of course I can.

Done,
please modify the comments at your will...

Pierre

> -----Original Message-----
> From: gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] On Behalf Of Mark Kettenis
> Sent: Friday, January 25, 2008 5:51 PM
> To: muller@ics.u-strasbg.fr
> Cc: brobecker@adacore.com; mark.kettenis@xs4all.nl; gdb-
> patches@sourceware.org
> Subject: Re: [RFA] i386-tdep.c: Add i386_skip_noop function; updated
> 
> > From: "Pierre Muller" <muller@ics.u-strasbg.fr>
> > Date: Fri, 25 Jan 2008 17:37:40 +0100
> >
> >   Joel,
> >   I took your suggestions into account.
> >
> >   I hope this sounds better for everyone.
> 
> Sorry Pierre, but I relly liked your origional diff better (the one
> that I reviewed yesterday).  Could you commit that one?  I'll clean up
> the comments a bit after that.