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