Mark Kettenis wrote: >> Date: Sun, 06 Mar 2011 11:00:12 -0800 >> From: Michael Snyder >> >> Mark Kettenis wrote: >>>> Date: Sun, 6 Mar 2011 15:15:16 +0100 >>>> From: 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. >>> 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 >> >> * 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