Re: [RFA] i386-tdep.c, check target_read_memory for error.

[Michael Snyder <msnyder@vmware.com>]

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

Mark Kettenis wrote:
>> Date: Sun, 6 Mar 2011 15:15:16 +0100
>> From: Jan Kratochvil <jan.kratochvil@redhat.com>
>>
>> On Fri, 04 Mar 2011 22:37:52 +0100, Michael Snyder wrote:
>>> Call error if target_read_memory fails.
>> [...]
>>> -  target_read_memory (pc, &op, 1);
>>> +  if (target_read_memory (pc, &op, 1))
>>> +    error (_("Couldn't read memory at pc (%s)"), 
>>> +	   paddress (gdbarch, pc));
>> There is the function `read_memory' for such purpose.
> 
> But read_memory() will throw an exception if reading fails.  That is
> not necessarily what we want here.  In fact, most of these reads
> should silently fail.  They are part of the prologue analysis code,
> which to some of extent is based on heuristics.  And one of the
> heristics here is that if we fail to read an instruction at a certain
> address, we're no longer looking at a function prologue.  Higher level
> code will try an alternative strategy or issue an error message.
> Spamming the user with more error messages isn't going to be terribly
> helpful.
> 
> But Michael is right that there is an issue here.  The code is relying
> on uninitialized stack variables not matching the specific opcodes we
> check against.  I think most of the:
> 
>     target_read_memory(pc, &op, 1);
> 
> statements, should be replaced with
> 
>     if (target_read_memory(pc, &op, 1))
>       return pc;
> 
> Cheers,
> 
> Mark

Thanks.  So changed.  Will you give it an eyeball?
Michael



[-- Attachment #2: target_read2.txt --]
[-- Type: text/plain, Size: 8062 bytes --]

2011-03-05  Michael Snyder  <msnyder@vmware.com>

	* i386-tdep.c (i386_follow_jump): Check target_read_memory for error.
	(i386_analyze_struct_return): Add gdbarch parameter.
	Check target_read_memory for error.
	(i386_skip_probe): Ditto.
	(i386_match_insn): Ditto.
	(i386_skip_noop): Ditto.
	(i386_analyze_register_saves): Ditto.
	(i386_analyze_frame_setup): Pass gdbarch to i386_match_insn.
	Check target_read_memory for error.
	(i386_analyze_prologue): Pass gdbarch to sub-functions.
	(i386_skip_prologue): Check target_read_memory for error.
	(i386_skip_main_prologue): Ditto.
	
Index: i386-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/i386-tdep.c,v
retrieving revision 1.324
diff -u -p -r1.324 i386-tdep.c
--- i386-tdep.c	8 Feb 2011 14:01:47 -0000	1.324
+++ i386-tdep.c	6 Mar 2011 18:58:31 -0000
@@ -850,7 +850,9 @@ i386_follow_jump (struct gdbarch *gdbarc
   long delta = 0;
   int data16 = 0;
 
-  target_read_memory (pc, &op, 1);
+  if (target_read_memory (pc, &op, 1))
+    return pc;
+
   if (op == 0x66)
     {
       data16 = 1;
@@ -895,7 +897,8 @@ i386_follow_jump (struct gdbarch *gdbarc
    whichever is smaller.  Otherwise, return PC.  */
 
 static CORE_ADDR
-i386_analyze_struct_return (CORE_ADDR pc, CORE_ADDR current_pc,
+i386_analyze_struct_return (struct gdbarch *gdbarch, CORE_ADDR pc,
+			    CORE_ADDR current_pc,
 			    struct i386_frame_cache *cache)
 {
   /* Functions that return a structure or union start with:
@@ -916,12 +919,15 @@ i386_analyze_struct_return (CORE_ADDR pc
   if (current_pc <= pc)
     return pc;
 
-  target_read_memory (pc, &op, 1);
+  if (target_read_memory (pc, &op, 1))
+    return pc;
 
   if (op != 0x58)		/* popl %eax */
     return pc;
 
-  target_read_memory (pc + 1, buf, 4);
+  if (target_read_memory (pc + 1, buf, 4))
+    return pc + 1;
+
   if (memcmp (buf, proto1, 3) != 0 && memcmp (buf, proto2, 4) != 0)
     return pc;
 
@@ -944,7 +950,7 @@ i386_analyze_struct_return (CORE_ADDR pc
 }
 
 static CORE_ADDR
-i386_skip_probe (CORE_ADDR pc)
+i386_skip_probe (struct gdbarch *gdbarch, CORE_ADDR pc)
 {
   /* A function may start with
 
@@ -960,7 +966,8 @@ i386_skip_probe (CORE_ADDR pc)
   gdb_byte buf[8];
   gdb_byte op;
 
-  target_read_memory (pc, &op, 1);
+  if (target_read_memory (pc, &op, 1))
+    return pc;
 
   if (op == 0x68 || op == 0x6a)
     {
@@ -1116,12 +1123,14 @@ struct i386_insn
    NULL.  */
 
 static struct i386_insn *
-i386_match_insn (CORE_ADDR pc, struct i386_insn *skip_insns)
+i386_match_insn (struct gdbarch *gdbarch, CORE_ADDR pc,
+		 struct i386_insn *skip_insns)
 {
   struct i386_insn *insn;
   gdb_byte op;
 
-  target_read_memory (pc, &op, 1);
+  if (target_read_memory (pc, &op, 1))
+    return pc;
 
   for (insn = skip_insns; insn->len > 0; insn++)
     {
@@ -1134,7 +1143,9 @@ i386_match_insn (CORE_ADDR pc, struct i3
 	  gdb_assert (insn->len > 1);
 	  gdb_assert (insn->len <= I386_MAX_MATCHED_INSN_LEN);
 
-	  target_read_memory (pc + 1, buf, insn->len - 1);
+	  if (target_read_memory (pc + 1, buf, insn->len - 1))
+	    return pc;
+
 	  for (i = 1; i < insn->len; i++)
 	    {
 	      if ((buf[i - 1] & insn->mask[i]) != insn->insn[i])
@@ -1207,12 +1218,13 @@ struct i386_insn i386_frame_setup_skip_i
 
 /* Check whether PC points to a no-op instruction.  */
 static CORE_ADDR
-i386_skip_noop (CORE_ADDR pc)
+i386_skip_noop (struct gdbarch *gdbarch, CORE_ADDR pc)
 {
   gdb_byte op;
   int check = 1;
 
-  target_read_memory (pc, &op, 1);
+  if (target_read_memory (pc, &op, 1))
+    return pc;
 
   while (check) 
     {
@@ -1221,7 +1233,8 @@ i386_skip_noop (CORE_ADDR pc)
       if (op == 0x90) 
 	{
 	  pc += 1;
-	  target_read_memory (pc, &op, 1);
+	  if (target_read_memory (pc, &op, 1))
+	    return pc;
 	  check = 1;
 	}
       /* Ignore no-op instruction `mov %edi, %edi'.
@@ -1237,11 +1250,15 @@ i386_skip_noop (CORE_ADDR pc)
 
       else if (op == 0x8b)
 	{
-	  target_read_memory (pc + 1, &op, 1);
+	  if (target_read_memory (pc + 1, &op, 1))
+	    return pc + 1;
+
 	  if (op == 0xff)
 	    {
 	      pc += 2;
-	      target_read_memory (pc, &op, 1);
+	      if (target_read_memory (pc, &op, 1))
+		return pc;
+
 	      check = 1;
 	    }
 	}
@@ -1267,7 +1284,8 @@ i386_analyze_frame_setup (struct gdbarch
   if (limit <= pc)
     return limit;
 
-  target_read_memory (pc, &op, 1);
+  if (target_read_memory (pc, &op, 1))
+    error (_("Couldn't read memory at %s"), paddress (gdbarch, pc));
 
   if (op == 0x55)		/* pushl %ebp */
     {
@@ -1291,7 +1309,8 @@ i386_analyze_frame_setup (struct gdbarch
 	 `movl %esp, %ebp' that actually sets up the frame.  */
       while (pc + skip < limit)
 	{
-	  insn = i386_match_insn (pc + skip, i386_frame_setup_skip_insns);
+	  insn = i386_match_insn (gdbarch, pc + skip,
+				  i386_frame_setup_skip_insns);
 	  if (insn == NULL)
 	    break;
 
@@ -1302,7 +1321,8 @@ i386_analyze_frame_setup (struct gdbarch
       if (limit <= pc + skip)
 	return limit;
 
-      target_read_memory (pc + skip, &op, 1);
+      if (target_read_memory (pc + skip, &op, 1))
+	return pc + skip;
 
       /* Check for `movl %esp, %ebp' -- can be written in two ways.  */
       switch (op)
@@ -1338,7 +1358,8 @@ i386_analyze_frame_setup (struct gdbarch
 
 	 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);
+      if (target_read_memory (pc, &op, 1))
+	return pc;
       if (op == 0x83)
 	{
 	  /* `subl' with 8-bit immediate.  */
@@ -1383,7 +1404,8 @@ i386_analyze_frame_setup (struct gdbarch
    smaller.  Otherwise, return PC.  */
 
 static CORE_ADDR
-i386_analyze_register_saves (CORE_ADDR pc, CORE_ADDR current_pc,
+i386_analyze_register_saves (struct gdbarch *gdbarch, CORE_ADDR pc,
+			     CORE_ADDR current_pc,
 			     struct i386_frame_cache *cache)
 {
   CORE_ADDR offset = 0;
@@ -1394,7 +1416,8 @@ i386_analyze_register_saves (CORE_ADDR p
     offset -= cache->locals;
   for (i = 0; i < 8 && pc < current_pc; i++)
     {
-      target_read_memory (pc, &op, 1);
+      if (target_read_memory (pc, &op, 1))
+	return pc;
       if (op < 0x50 || op > 0x57)
 	break;
 
@@ -1439,13 +1462,13 @@ i386_analyze_prologue (struct gdbarch *g
 		       CORE_ADDR pc, CORE_ADDR current_pc,
 		       struct i386_frame_cache *cache)
 {
-  pc = i386_skip_noop (pc);
+  pc = i386_skip_noop (gdbarch, pc);
   pc = i386_follow_jump (gdbarch, pc);
-  pc = i386_analyze_struct_return (pc, current_pc, cache);
-  pc = i386_skip_probe (pc);
+  pc = i386_analyze_struct_return (gdbarch, pc, current_pc, cache);
+  pc = i386_skip_probe (gdbarch, 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);
+  return i386_analyze_register_saves (gdbarch, pc, current_pc, cache);
 }
 
 /* Return PC of first real instruction.  */
@@ -1487,7 +1510,9 @@ i386_skip_prologue (struct gdbarch *gdba
 
   for (i = 0; i < 6; i++)
     {
-      target_read_memory (pc + i, &op, 1);
+      if (target_read_memory (pc + i, &op, 1))
+	return pc + i;
+
       if (pic_pat[i] != op)
 	break;
     }
@@ -1495,7 +1520,9 @@ i386_skip_prologue (struct gdbarch *gdba
     {
       int delta = 6;
 
-      target_read_memory (pc + delta, &op, 1);
+      if (target_read_memory (pc + delta, &op, 1))
+	return pc + delta;
+
 
       if (op == 0x89)		/* movl %ebx, x(%ebp) */
 	{
@@ -1508,7 +1535,9 @@ i386_skip_prologue (struct gdbarch *gdba
 	  else			/* Unexpected instruction.  */
 	    delta = 0;
 
-          target_read_memory (pc + delta, &op, 1);
+          if (target_read_memory (pc + delta, &op, 1))
+	    return pc + delta;
+
 	}
 
       /* addl y,%ebx */
@@ -1538,7 +1567,8 @@ i386_skip_main_prologue (struct gdbarch 
   enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
   gdb_byte op;
 
-  target_read_memory (pc, &op, 1);
+  if (target_read_memory (pc, &op, 1))
+    return pc;
   if (op == 0xe8)
     {
       gdb_byte buf[4];