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

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

[Michael Snyder]

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

Call error if target_read_memory fails.

OK?


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

2011-03-04  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 -u -p -r1.324 i386-tdep.c
--- i386-tdep.c	8 Feb 2011 14:01:47 -0000	1.324
+++ i386-tdep.c	4 Mar 2011 21:28:41 -0000
@@ -850,7 +850,10 @@ 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))
+    error (_("Couldn't read memory at pc (%s)"), 
+	   paddress (gdbarch, pc));
+
   if (op == 0x66)
     {
       data16 = 1;
@@ -895,7 +898,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 +920,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))
+    error (_("Couldn't read memory at %s"), paddress (gdbarch, pc));
 
   if (op != 0x58)		/* popl %eax */
     return pc;
 
-  target_read_memory (pc + 1, buf, 4);
+  if (target_read_memory (pc + 1, buf, 4))
+    error (_("Couldn't read memory at %s"), paddress (gdbarch, pc));
+
   if (memcmp (buf, proto1, 3) != 0 && memcmp (buf, proto2, 4) != 0)
     return pc;
 
@@ -944,7 +951,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 +967,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))
+    error (_("Couldn't read memory at %s"), paddress (gdbarch, pc));
 
   if (op == 0x68 || op == 0x6a)
     {
@@ -1116,12 +1124,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))
+    error (_("Couldn't read memory at %s"), paddress (gdbarch, pc));
 
   for (insn = skip_insns; insn->len > 0; insn++)
     {
@@ -1134,7 +1144,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))
+	    error (_("Couldn't read memory at %s"),
+		   paddress (gdbarch, pc + 1));
 	  for (i = 1; i < insn->len; i++)
 	    {
 	      if ((buf[i - 1] & insn->mask[i]) != insn->insn[i])
@@ -1207,12 +1219,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))
+    error (_("Couldn't read memory at %s"), paddress (gdbarch, pc));
 
   while (check) 
     {
@@ -1221,7 +1234,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))
+	    error (_("Couldn't read memory at %s"), paddress (gdbarch, pc));
 	  check = 1;
 	}
       /* Ignore no-op instruction `mov %edi, %edi'.
@@ -1237,11 +1251,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))
+	    error (_("Couldn't read memory at %s"),
+		   paddress (gdbarch, pc + 1));
 	  if (op == 0xff)
 	    {
 	      pc += 2;
-	      target_read_memory (pc, &op, 1);
+	      if (target_read_memory (pc, &op, 1))
+		error (_("Couldn't read memory at %s"),
+		       paddress (gdbarch, pc));
 	      check = 1;
 	    }
 	}
@@ -1267,7 +1285,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 +1310,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 +1322,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))
+	error (_("Couldn't read memory at %s"), paddress (gdbarch, pc));
 
       /* Check for `movl %esp, %ebp' -- can be written in two ways.  */
       switch (op)
@@ -1338,7 +1359,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))
+	error (_("Couldn't read memory at %s"), paddress (gdbarch, pc));
       if (op == 0x83)
 	{
 	  /* `subl' with 8-bit immediate.  */
@@ -1383,7 +1405,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 +1417,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))
+	error (_("Couldn't read memory at %s"), paddress (gdbarch, pc));
       if (op < 0x50 || op > 0x57)
 	break;
 
@@ -1439,13 +1463,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 +1511,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))
+	error (_("Couldn't read memory at %s"),
+	       paddress (gdbarch, pc + i));
       if (pic_pat[i] != op)
 	break;
     }
@@ -1495,7 +1521,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))
+	error (_("Couldn't read memory at %s"),
+	       paddress (gdbarch, pc + delta));
 
       if (op == 0x89)		/* movl %ebx, x(%ebp) */
 	{
@@ -1508,7 +1536,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))
+	    error (_("Couldn't read memory at %s"),
+		   paddress (gdbarch, pc + delta));
 	}
 
       /* addl y,%ebx */
@@ -1538,7 +1568,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))
+    error (_("Couldn't read memory at %s"), paddress (gdbarch, pc));
   if (op == 0xe8)
     {
       gdb_byte buf[4];

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

[Jan Kratochvil]

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.


Thanks,
Jan

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

[Mark Kettenis]

> 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

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

[Michael Snyder]

Jan Kratochvil wrote:
> 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.

I don't understand the objection.  target_read_memory may fail and
return an error code.  Coverity reports that the return value is checked
in 78 out of 97 calling instances.  So what's wrong with checking it now?

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

[Jan Kratochvil]

On Sun, 06 Mar 2011 19:42:34 +0100, Michael Snyder wrote:
> Jan Kratochvil wrote:
> >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.
> 
> I don't understand the objection.  target_read_memory may fail and
> return an error code.  Coverity reports that the return value is checked
> in 78 out of 97 calling instances.  So what's wrong with checking it now?

I was suggesting that instead of the code
  if (target_read_memory (pc, &op, 1))
    error (_("Couldn't read memory at pc (%s)"), paddress (gdbarch, pc));

One can write a shorter code with the same effect:
  read_memory (pc, &op, 1);

I did not do a real patch review and I also did not write any notice about any
review/approval.

Mark Kettenis correctly noticed that the introduced errors (either by error or
through read_memory) in some of the cases are wrong / cause regressions.

Just if in some cases the error is appropriate (I do not say in which specific
cases, if any) I was suggesting calling `read_memory' is more suitable than
the explicit `target_read_memory'+`error' calls.


Thanks,
Jan

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

[Michael Snyder]

[-- 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];

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

[Mark Kettenis]

> Date: Sun, 06 Mar 2011 11:00:12 -0800
> From: Michael Snyder <msnyder@vmware.com>
> 
> 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
> 
> 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.

Hmm, several functions now have a gdbarch parameter that is unused.

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

I think you want to return NULL here.

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

And here too.

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

This should be a "return pc"

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

Overlooked this one?

> @@ -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;
> +

This is checking for a very specific sequence.  If we don't match the
complete sequence, we should probably return the address of the start
of the sequence, not the address of the partial match.  So this should
be "return pc".

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

Same here.

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

and here.

> +

and please don't introduce spurious whitespace here.

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

[Michael Snyder]

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

Mark Kettenis wrote:
>> Date: Sun, 06 Mar 2011 11:00:12 -0800
>> From: Michael Snyder <msnyder@vmware.com>
>>
>> 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
>>
>> 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.
> 
> Hmm, several functions now have a gdbarch parameter that is unused.
> 
>> 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
>> @@ -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;
> 
> I think you want to return NULL here.
> 
>>    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;
> 
> And here too.
> 
>> @@ -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;
> 
> This should be a "return pc"
> 
>> @@ -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));
> 
> Overlooked this one?
> 
>> @@ -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;
>> +
> 
> This is checking for a very specific sequence.  If we don't match the
> complete sequence, we should probably return the address of the start
> of the sequence, not the address of the partial match.  So this should
> be "return pc".
> 
>> @@ -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;
> 
> Same here.
> 
>> @@ -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;
> 
> and here.
> 
>> +
> 
> and please don't introduce spurious whitespace here.

Thanks for your perseverance.  New patch attached.

There's still one more "return pc + skip" in there, which you
didn't mention.  Should I take it out?

Michael



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

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

	* i386-tdep.c (i386_follow_jump): Check return value of
	target_read_memory.
	(i386_analyze_struct_return): Ditto.
	(i386_skip_probe): Ditto.
	(i386_match_insn): Ditto.
	(i386_skip_noop): Ditto.
	(i386_analyze_frame_setup): Ditto.
	(i386_analyze_register_saves): Ditto.
	(i386_skip_prologue): Ditto.
	(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	8 Mar 2011 18:32:01 -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;
@@ -916,12 +918,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;
+
   if (memcmp (buf, proto1, 3) != 0 && memcmp (buf, proto2, 4) != 0)
     return pc;
 
@@ -960,7 +965,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)
     {
@@ -1121,7 +1127,8 @@ i386_match_insn (CORE_ADDR pc, struct i3
   struct i386_insn *insn;
   gdb_byte op;
 
-  target_read_memory (pc, &op, 1);
+  if (target_read_memory (pc, &op, 1))
+    return NULL;
 
   for (insn = skip_insns; insn->len > 0; insn++)
     {
@@ -1134,7 +1141,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 NULL;
+
 	  for (i = 1; i < insn->len; i++)
 	    {
 	      if ((buf[i - 1] & insn->mask[i]) != insn->insn[i])
@@ -1212,7 +1221,8 @@ i386_skip_noop (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 +1231,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 +1248,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;
+
 	  if (op == 0xff)
 	    {
 	      pc += 2;
-	      target_read_memory (pc, &op, 1);
+	      if (target_read_memory (pc, &op, 1))
+		return pc;
+
 	      check = 1;
 	    }
 	}
@@ -1267,7 +1282,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))
+    return pc;
 
   if (op == 0x55)		/* pushl %ebp */
     {
@@ -1302,7 +1318,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 +1355,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.  */
@@ -1394,7 +1412,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;
 
@@ -1487,7 +1506,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;
+
       if (pic_pat[i] != op)
 	break;
     }
@@ -1495,7 +1516,8 @@ 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;
 
       if (op == 0x89)		/* movl %ebx, x(%ebp) */
 	{
@@ -1508,7 +1530,8 @@ 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;
 	}
 
       /* addl y,%ebx */
@@ -1538,7 +1561,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];

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

[Mark Kettenis]

> Date: Tue, 08 Mar 2011 10:37:55 -0800
> From: Michael Snyder <msnyder@vmware.com>
> 
> Thanks for your perseverance.  New patch attached.
> 
> There's still one more "return pc + skip" in there, which you
> didn't mention.  Should I take it out?

No, that one is right as far as I can tell.  Diff looks good to me.

> 2011-03-08  Michael Snyder  <msnyder@vmware.com>
> 
> 	* i386-tdep.c (i386_follow_jump): Check return value of
> 	target_read_memory.
> 	(i386_analyze_struct_return): Ditto.
> 	(i386_skip_probe): Ditto.
> 	(i386_match_insn): Ditto.
> 	(i386_skip_noop): Ditto.
> 	(i386_analyze_frame_setup): Ditto.
> 	(i386_analyze_register_saves): Ditto.
> 	(i386_skip_prologue): Ditto.
> 	(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	8 Mar 2011 18:32:01 -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;
> @@ -916,12 +918,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;
> +
>    if (memcmp (buf, proto1, 3) != 0 && memcmp (buf, proto2, 4) != 0)
>      return pc;
>  
> @@ -960,7 +965,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)
>      {
> @@ -1121,7 +1127,8 @@ i386_match_insn (CORE_ADDR pc, struct i3
>    struct i386_insn *insn;
>    gdb_byte op;
>  
> -  target_read_memory (pc, &op, 1);
> +  if (target_read_memory (pc, &op, 1))
> +    return NULL;
>  
>    for (insn = skip_insns; insn->len > 0; insn++)
>      {
> @@ -1134,7 +1141,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 NULL;
> +
>  	  for (i = 1; i < insn->len; i++)
>  	    {
>  	      if ((buf[i - 1] & insn->mask[i]) != insn->insn[i])
> @@ -1212,7 +1221,8 @@ i386_skip_noop (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 +1231,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 +1248,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;
> +
>  	  if (op == 0xff)
>  	    {
>  	      pc += 2;
> -	      target_read_memory (pc, &op, 1);
> +	      if (target_read_memory (pc, &op, 1))
> +		return pc;
> +
>  	      check = 1;
>  	    }
>  	}
> @@ -1267,7 +1282,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))
> +    return pc;
>  
>    if (op == 0x55)		/* pushl %ebp */
>      {
> @@ -1302,7 +1318,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 +1355,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.  */
> @@ -1394,7 +1412,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;
>  
> @@ -1487,7 +1506,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;
> +
>        if (pic_pat[i] != op)
>  	break;
>      }
> @@ -1495,7 +1516,8 @@ 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;
>  
>        if (op == 0x89)		/* movl %ebx, x(%ebp) */
>  	{
> @@ -1508,7 +1530,8 @@ 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;
>  	}
>  
>        /* addl y,%ebx */
> @@ -1538,7 +1561,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];
> 
> --------------090807040303000008010100--
> 

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

[Pedro Alves]

On Tuesday 08 March 2011 18:37:55, Michael Snyder wrote:
> @@ -1221,7 +1231,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;

I think you're meant to return PC as it was at function
start.  Note the pc += 1 above.  There are other instances
in the patch.

-- 
Pedro Alves

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

[Mark Kettenis]

> From: Pedro Alves <pedro@codesourcery.com>
> Date: Tue, 8 Mar 2011 18:58:43 +0000
> 
> On Tuesday 08 March 2011 18:37:55, Michael Snyder wrote:
> > @@ -1221,7 +1231,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;
> 
> I think you're meant to return PC as it was at function
> start.  Note the pc += 1 above.  There are other instances
> in the patch.

Those are actually fine.  Skipping nop instructions is harmless, even
if we get stuck somewhere in the middle.

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

[Pedro Alves]

On Tuesday 08 March 2011 19:25:20, Mark Kettenis wrote:
> > From: Pedro Alves <pedro@codesourcery.com>
> > Date: Tue, 8 Mar 2011 18:58:43 +0000
> > 
> > On Tuesday 08 March 2011 18:37:55, Michael Snyder wrote:
> > > @@ -1221,7 +1231,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;
> > 
> > I think you're meant to return PC as it was at function
> > start.  Note the pc += 1 above.  There are other instances
> > in the patch.
> 
> Those are actually fine.  Skipping nop instructions is harmless, even
> if we get stuck somewhere in the middle.

You're right, missed that.

BTW, I noticed a bunch of read_memory_unsigned_integer/read_memory_integer
calls in these skippers/sniffers.  These functions call read_memory, which
throws.

-- 
Pedro Alves

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

[Michael Snyder]

Pedro Alves wrote:
> On Tuesday 08 March 2011 19:25:20, Mark Kettenis wrote:
>>> From: Pedro Alves <pedro@codesourcery.com>
>>> Date: Tue, 8 Mar 2011 18:58:43 +0000
>>>
>>> On Tuesday 08 March 2011 18:37:55, Michael Snyder wrote:
>>>> @@ -1221,7 +1231,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;
>>> I think you're meant to return PC as it was at function
>>> start.  Note the pc += 1 above.  There are other instances
>>> in the patch.
>> Those are actually fine.  Skipping nop instructions is harmless, even
>> if we get stuck somewhere in the middle.
> 
> You're right, missed that.

OK, committing.

Thanks all for the reviews.