[patch i386, 0/2] skip insns generated by -fstack-protector

[patch i386, 0/2] skip insns generated by -fstack-protector

[Yao Qi]

GDB doesn't handle insns for stack protector on i386.  These two patches 
are drafted to address this.

Patch 1 is about fixing GDB analyze i386 prologue for insns and/add, 
which are part of i386 prologue, but GDB can't handle.

Patch 2 is about handling i386 stack protector insns during prologue 
analysis.  Without patch 1, patch 2 doesn't work in some cases.

Regression tested on i686-pc-linux-gnu with -fstack-protector.  These 
failures are fixed,

-FAIL: gdb.mi/gdb792.exp: create var for class A
-FAIL: gdb.mi/gdb792.exp: list children of class A
-FAIL: gdb.mi/gdb792.exp: list children of A.public
-FAIL: gdb.mi/gdb792.exp: list children of A.private
-FAIL: gdb.mi/gdb792.exp: list children of A.protected
-FAIL: gdb.mi/gdb792.exp: list children of A.protected.b
-FAIL: gdb.mi/gdb792.exp: list children of A.protected.b.public
-FAIL: gdb.mi/gdb792.exp: list children of A.protected.b.private
-FAIL: gdb.mi/gdb792.exp: create var for class C which has baseclass A
-FAIL: gdb.mi/gdb792.exp: list children of class C

-- 
Yao Qi

Re: [patch i386, 0/2] skip insns generated by -fstack-protector

[Yao Qi]

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

On 12/24/2010 04:21 PM, Yao Qi wrote:
> Patch 1 is about fixing GDB analyze i386 prologue for insns and/add,
> which are part of i386 prologue, but GDB can't handle.

Here is a prologue generated by GCC, instructions on [1] can't be 
handled by GDB so far.  This patch is to handle them in prologue parsing.

      push   %ebp
      mov    %esp,%ebp

      and    $0xfffffff0,%esp  // <---- [1]
      add    $0xffffff80,%esp  // <---- [1]

      mov    %gs:0x14,%eax
      mov    %eax,0x7c(%esp)
      xor    %eax,%eax

      lea    0x54(%esp),%eax

Note that `and' instruction, for alignment, is not a must in prologue. 
My knowledge on i386 prologue is very limited and GCC i386 prologue 
generate is too complicated to understand for me, so I am not pretty 
sure on this patch.  I send it out, and your comments are appreciated.

-- 
Yao Qi

[-- Attachment #2: i386_prologue_parse_1224.patch --]
[-- Type: text/x-patch, Size: 3011 bytes --]

gdb/

	* i386-tdep.c (i386_analyze_frame_setup): Handle and/add
	sequence in prologue.

diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c
index 4016a70..8c6f896 100644
--- a/gdb/i386-tdep.c
+++ b/gdb/i386-tdep.c
@@ -1263,6 +1263,7 @@ i386_analyze_frame_setup (struct gdbarch *gdbarch,
   struct i386_insn *insn;
   gdb_byte op;
   int skip = 0;
+  int found_and_insn = 0;
 
   if (limit <= pc)
     return limit;
@@ -1332,24 +1333,71 @@ i386_analyze_frame_setup (struct gdbarch *gdbarch,
       if (limit <= pc)
 	return limit;
 
-      /* Check for stack adjustment 
+      /* GCC may generate `and' instruction in front of stack adjustment for
+	 stack alignment.  Check for stack alignment, and skip it if any,
+
+         	and $0xfffffff0,%esp
+
+	 The change of ESP caused by this instruction is computed a little bit
+	 different here, because  we can't get value of offset from
+	 instruction itself.  We simulate the execution of this in struction
+	 to calcuate the delta change of ESP.  */
+      target_read_memory (pc, &op, 1);
+      /* `and' with signed 8-bit immediate.  */
+      if (op == 0x83)
+	{
+	  gdb_byte op1 = read_memory_unsigned_integer (pc + 1, 1, byte_order);
+
+	  if (op1 == 0xe4)
+	    {
+	      gdb_byte oprand
+		= read_memory_unsigned_integer (pc + 2, 1, byte_order);
+	      CORE_ADDR esp = cache->saved_regs[I386_ESP_REGNUM];
+
+	      found_and_insn = 1;
+	      /* Compute the delta change of esp .  */
+	      cache->locals = (esp & 0xff) - (esp & 0xff & oprand);
+	    }
+	}
+
+      /* Check for stack adjustment,
 
 	    subl $XXX, %esp
 
+	or
+
+	    add $0xffffff80, %esp
+
 	 NOTE: You can't subtract a 16-bit immediate from a 32-bit
 	 reg, so we don't have to worry about a data16 prefix.  */
-      target_read_memory (pc, &op, 1);
+
+      target_read_memory (pc + (found_and_insn ? 3 : 0), &op, 1);
       if (op == 0x83)
 	{
+	  gdb_byte op1
+	    = read_memory_unsigned_integer (pc + (found_and_insn ? 4 : 1),
+					    1, byte_order);
 	  /* `subl' with 8-bit immediate.  */
-	  if (read_memory_unsigned_integer (pc + 1, 1, byte_order) != 0xec)
-	    /* Some instruction starting with 0x83 other than `subl'.  */
-	    return pc;
-
-	  /* `subl' with signed 8-bit immediate (though it wouldn't
-	     make sense to be negative).  */
-	  cache->locals = read_memory_integer (pc + 2, 1, byte_order);
-	  return pc + 3;
+	  if (op1 == 0xec)
+	    {
+	      /* `subl' with signed 8-bit immediate (though it wouldn't
+		 make sense to be negative).  */
+	      cache->locals
+		= read_memory_integer (pc + (found_and_insn ? 5 : 2),
+				       1, byte_order);
+	      return pc + 3;
+	    }
+	  /* `add' with a 8-bit immediate.  */
+	  else if (op1 == 0xc4)
+	    {
+	      cache->locals
+		+= -1 * read_memory_integer (pc + (found_and_insn ? 5 : 2),
+					     1, byte_order);
+	      return pc + (found_and_insn ? 6 : 3);
+	    }
+	  /* Some instruction starting with 0x83 other than `subl' or
+	     `add'.  */
+	  return pc;
 	}
       else if (op == 0x81)
 	{

Re: [patch i386, 2/2] skip insns generated by -fstack-protector

[Yao Qi]

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

On 12/24/2010 04:21 PM, Yao Qi wrote:
> Patch 2 is about handling i386 stack protector insns during prologue
> analysis.  Without patch 1, patch 2 doesn't work in some cases.

Here is a prologue generated by GCC, instructions on [1] are for stack 
protector.

      push   %ebp
      mov    %esp,%ebp

      and    $0xfffffff0,%esp
      add    $0xffffff80,%esp

      mov    %gs:0x14,%eax   // <---- [1]
      mov    %eax,0x7c(%esp) // <---- [1]
      xor    %eax,%eax       // <---- [1]

Compared with instructions for arm stack protector, i386's counterpart 
is relatively simpler.  This patch is to handle them in prologue 
parsing.  Comments are welcome.

-- 
Yao Qi

[-- Attachment #2: i386_skip_stack_protector_1224.patch --]
[-- Type: text/x-patch, Size: 1884 bytes --]

gdb/

	* i386-tdep.c (i386_skip_stack_protector) New.
	(i386_analyze_prologue): Chain i386_skip_stack_protector.
	
diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c
index 8c6f896..ee40603 100644
--- a/gdb/i386-tdep.c
+++ b/gdb/i386-tdep.c
@@ -1455,6 +1455,35 @@ i386_analyze_register_saves (CORE_ADDR pc, CORE_ADDR current_pc,
   return pc;
 }
 
+/* Check whether PC points at code that for stack protector, which
+   is usually a sequence of three instructions,
+
+   mov    %gs:0x14,%eax
+   mov    %eax,0x7c(%esp)
+   xor    %eax,%eax
+
+   If so, returns the address of the first instruction after the
+   stack protector code or CURRENT_PC, whichever is smaller.
+   Otherwise, return PC.  */
+
+static CORE_ADDR
+i386_skip_stack_protector (CORE_ADDR pc, CORE_ADDR current_pc)
+{
+  gdb_byte buf[12];
+  if (target_read_memory (pc, buf, sizeof buf))
+    return pc;
+
+  /* Instruction `mov %gs:0x14,%eax' can be regarded as `fingerprint' of a
+   sequence of code for stack protector, since it is unique and can't be
+   found elsewhere.  */
+  if (/* mov %gs:0x14,%eax.  */
+      buf[0] != 0x65 && buf[1] != 0xa1 && buf[2] != 14
+      && buf[10] != 0x31 /* xor    %eax,%eax.  */)
+    return pc;
+
+  return min (pc + 12, current_pc);
+}
+
 /* Do a full analysis of the prologue at PC and update CACHE
    accordingly.  Bail out early if CURRENT_PC is reached.  Return the
    address where the analysis stopped.
@@ -1493,7 +1522,9 @@ i386_analyze_prologue (struct gdbarch *gdbarch,
   pc = i386_skip_probe (pc);
   pc = i386_analyze_stack_align (pc, current_pc, cache);
   pc = i386_analyze_frame_setup (gdbarch, pc, current_pc, cache);
-  return i386_analyze_register_saves (pc, current_pc, cache);
+  pc = i386_analyze_register_saves (pc, current_pc, cache);
+  pc = i386_skip_stack_protector (pc, current_pc);
+  return pc;
 }
 
 /* Return PC of first real instruction.  */

Re: [patch i386, 0/2] skip insns generated by -fstack-protector

[Andreas Schwab]

Yao Qi <yao@codesourcery.com> writes:

> @@ -1332,24 +1333,71 @@ i386_analyze_frame_setup (struct gdbarch *gdbarch,
>        if (limit <= pc)
>  	return limit;
>  
> -      /* Check for stack adjustment 
> +      /* GCC may generate `and' instruction in front of stack adjustment for
> +	 stack alignment.  Check for stack alignment, and skip it if any,
> +
> +         	and $0xfffffff0,%esp
> +
> +	 The change of ESP caused by this instruction is computed a little bit
> +	 different here, because  we can't get value of offset from
> +	 instruction itself.  We simulate the execution of this in struction

s/in struction/instruction/

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [patch i386, 0/2] skip insns generated by -fstack-protector

[Yao Qi]

On 12/24/2010 05:15 PM, Andreas Schwab wrote:
>> +	 The change of ESP caused by this instruction is computed a little bit
>> >  +	 different here, because  we can't get value of offset from
>> >  +	 instruction itself.  We simulate the execution of this in struction
> s/in struction/instruction/

Oops.  I'll fix it along with other reviewers' comments together.

-- 
Yao Qi

[PING patch i386, 0/2] skip insns generated by -fstack-protector

[Yao Qi]

On 12/24/2010 04:21 PM, Yao Qi wrote:
> Patch 1 is about fixing GDB analyze i386 prologue for insns and/add,
> which are part of i386 prologue, but GDB can't handle.
> 

gdb/

	* i386-tdep.c (i386_analyze_frame_setup): Handle and/add
	sequence in prologue.

http://sourceware.org/ml/gdb-patches/2010-12/msg00452.html

> Patch 2 is about handling i386 stack protector insns during prologue
> analysis.  Without patch 1, patch 2 doesn't work in some cases.

gdb/

	* i386-tdep.c (i386_skip_stack_protector) New.
	(i386_analyze_prologue): Chain i386_skip_stack_protector.

http://sourceware.org/ml/gdb-patches/2010-12/msg00453.html

-- 
Yao (齐尧)

Re: [PING patch i386, 0/2] skip insns generated by -fstack-protector

[Yao Qi]

On 01/04/2011 09:22 AM, Yao Qi wrote:
> On 12/24/2010 04:21 PM, Yao Qi wrote:
>> Patch 1 is about fixing GDB analyze i386 prologue for insns and/add,
>> which are part of i386 prologue, but GDB can't handle.
>>
>
> gdb/
>
> 	* i386-tdep.c (i386_analyze_frame_setup): Handle and/add
> 	sequence in prologue.
>
> http://sourceware.org/ml/gdb-patches/2010-12/msg00452.html
>
>> Patch 2 is about handling i386 stack protector insns during prologue
>> analysis.  Without patch 1, patch 2 doesn't work in some cases.
>
> gdb/
>
> 	* i386-tdep.c (i386_skip_stack_protector) New.
> 	(i386_analyze_prologue): Chain i386_skip_stack_protector.
>
> http://sourceware.org/ml/gdb-patches/2010-12/msg00453.html
>

Ping?

-- 
Yao Qi