From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 3409 invoked by alias); 7 Mar 2011 22:26:33 -0000 Received: (qmail 3397 invoked by uid 22791); 7 Mar 2011 22:26:32 -0000 X-SWARE-Spam-Status: No, hits=-2.0 required=5.0 tests=AWL,BAYES_00,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from sibelius.xs4all.nl (HELO glazunov.sibelius.xs4all.nl) (83.163.83.176) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Mon, 07 Mar 2011 22:26:26 +0000 Received: from glazunov.sibelius.xs4all.nl (kettenis@localhost [127.0.0.1]) by glazunov.sibelius.xs4all.nl (8.14.3/8.14.3) with ESMTP id p27MQBjP011692; Mon, 7 Mar 2011 23:26:11 +0100 (CET) Received: (from kettenis@localhost) by glazunov.sibelius.xs4all.nl (8.14.3/8.14.3/Submit) id p27MQ8Zp004631; Mon, 7 Mar 2011 23:26:08 +0100 (CET) Date: Tue, 08 Mar 2011 02:35:00 -0000 Message-Id: <201103072226.p27MQ8Zp004631@glazunov.sibelius.xs4all.nl> From: Mark Kettenis To: msnyder@vmware.com CC: jan.kratochvil@redhat.com, gdb-patches@sourceware.org In-reply-to: <4D73D9BC.6020705@vmware.com> (message from Michael Snyder on Sun, 06 Mar 2011 11:00:12 -0800) Subject: Re: [RFA] i386-tdep.c, check target_read_memory for error. References: <4D715BB0.8030506@vmware.com> <20110306141515.GA1895@host1.jankratochvil.net> <201103061455.p26Etppr028003@glazunov.sibelius.xs4all.nl> <4D73D9BC.6020705@vmware.com> Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2011-03/txt/msg00510.txt.bz2 > 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.