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

[Mark Kettenis <mark dot kettenis at xs4all dot nl>]

> 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);
> 
> 
> 
> 
>