This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [RFA] i386-tdep.c, check target_read_memory for error.
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.
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];