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