From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 27027 invoked by alias); 12 Jul 2009 11:12:36 -0000 Received: (qmail 27018 invoked by uid 22791); 12 Jul 2009 11:12:34 -0000 X-SWARE-Spam-Status: No, hits=-2.5 required=5.0 tests=AWL,BAYES_00 X-Spam-Check-By: sourceware.org Received: from sibelius.xs4all.nl (HELO sibelius.xs4all.nl) (82.92.89.47) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Sun, 12 Jul 2009 11:12:26 +0000 Received: from brahms.sibelius.xs4all.nl (kettenis@localhost [127.0.0.1]) by brahms.sibelius.xs4all.nl (8.14.3/8.14.3) with ESMTP id n6CBCIFV028763; Sun, 12 Jul 2009 13:12:18 +0200 (CEST) Received: (from kettenis@localhost) by brahms.sibelius.xs4all.nl (8.14.3/8.14.3/Submit) id n6CBCG1x032181; Sun, 12 Jul 2009 13:12:16 +0200 (CEST) Date: Sun, 12 Jul 2009 17:07:00 -0000 Message-Id: <200907121112.n6CBCG1x032181@brahms.sibelius.xs4all.nl> From: Mark Kettenis To: msnyder@vmware.com CC: msnyder@vmware.com, gdb-patches@sourceware.org, drow@false.org, teawater@gmail.com In-reply-to: <4A58D5D7.9070504@vmware.com> (message from Michael Snyder on Sat, 11 Jul 2009 11:11:35 -0700) Subject: Re: [RFA] epilogue unwinder for i386 (reverse 1/2) References: <4A4EA0F7.1040004@vmware.com> <4A4EA3B3.9030107@vmware.com> <200907051235.n65CZhDb024857@brahms.sibelius.xs4all.nl> <4A511332.2000607@vmware.com> <4A58D5D7.9070504@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: 2009-07/txt/msg00343.txt.bz2 > Date: Sat, 11 Jul 2009 11:11:35 -0700 > From: Michael Snyder > > Mark, Daniel, is this OK now? Not quite. You fullfilled my request for using lower case for the instruction mneonics, only to add more uppercase ones in the new comment :(. Oh, anc for consistency please use %reg syntax instead of REG. You know what; go ahead and commit this; I'll clean up afterwards. Mark > Michael Snyder wrote: > > Mark Kettenis wrote: > >>> Date: Fri, 03 Jul 2009 17:34:59 -0700 > >>> From: Michael Snyder > >>> > >>> Michael Snyder wrote: > >>>> This comes out of a discussion with Daniel, about how gcc > >>>> does not generate the right dwarf info to allow correct > >>>> frame unwinding in function epilogues, causing frame_unwind > >>>> to return bad results. > >>>> > >>>> It's necessary for reverse-step, which will frequently step > >>>> backward to the return instruction of a function. But it also > >>>> provides an improvement for forward debugging, in that now, > >>>> without this change, if you STEPI until you are at the return > >>>> instruction, you will get a bad backtrace. > >>>> > >>>> The infrun changes that take advantage of this patch will follow > >>>> separately. > >>>> > >>>> Michael > >>> Oops, the patch wasn't meant to have that "#if 0" in it... > >>> corrected patch below. > >> Still has the #if 0 in there. > > > > Sorry. ;-( > >> I also think you should add a comment about the specific ordering of > >> this unwinder. It has to come before the dwarf2 unwinder because GCC > >> doesn't provide proper CFI for the epilogue, right? > > > > Right. Since the others are similarly order-dependent, I will > > expand on their comments as well. > > > > > >>> + if (target_read_memory (pc, &insn, 1) != 0) > >>> + return 0; /* Can't read memory at pc. */ > >> For consistency's sake, can you drop the != 0 here? > > > > OK. > > > >>> + if (insn != 0xc3) /* RET */ > >>> + return 0; > >> Please use lowercase for instruction mnemonics. > > > > OK. Revised patch attached. > > > > > > > > > > > > ------------------------------------------------------------------------ > > > > 2009-07-03 Michael Snyder > > > > * i386-tdep.c: Add a frame unwinder for function epilogues. > > (i386_in_function_epilogue_p): New function. > > (i386_epilogue_frame_sniffer): New function. > > (i386_epilogue_frame_cache): New function. > > (i386_epilogue_frame_this_id): New function. > > (i386_epilogue_frame_unwind): New struct frame_unwind. > > (i386_gdbarch_init): Hook the new unwinder. > > > > Index: i386-tdep.c > > =================================================================== > > RCS file: /cvs/src/src/gdb/i386-tdep.c,v > > retrieving revision 1.280 > > diff -u -p -r1.280 i386-tdep.c > > --- i386-tdep.c 2 Jul 2009 17:25:54 -0000 1.280 > > +++ i386-tdep.c 5 Jul 2009 20:56:08 -0000 > > @@ -1487,6 +1487,89 @@ static const struct frame_unwind i386_fr > > NULL, > > default_frame_sniffer > > }; > > + > > +/* Normal frames, but in a function epilogue. */ > > + > > +/* The epilogue is defined here as the RET instruction, which will > > + follow any instruction such as LEAVE or POP EBP that destroys the > > + function's stack frame. */ > > + > > +static int > > +i386_in_function_epilogue_p (struct gdbarch *gdbarch, CORE_ADDR pc) > > +{ > > + gdb_byte insn; > > + > > + if (target_read_memory (pc, &insn, 1)) > > + return 0; /* Can't read memory at pc. */ > > + > > + if (insn != 0xc3) /* 'ret' instruction. */ > > + return 0; > > + > > + return 1; > > +} > > + > > +static int > > +i386_epilogue_frame_sniffer (const struct frame_unwind *self, > > + struct frame_info *this_frame, > > + void **this_prologue_cache) > > +{ > > + if (frame_relative_level (this_frame) == 0) > > + return i386_in_function_epilogue_p (get_frame_arch (this_frame), > > + get_frame_pc (this_frame)); > > + else > > + return 0; > > +} > > + > > +static struct i386_frame_cache * > > +i386_epilogue_frame_cache (struct frame_info *this_frame, void **this_cache) > > +{ > > + struct gdbarch *gdbarch = get_frame_arch (this_frame); > > + enum bfd_endian byte_order = gdbarch_byte_order (gdbarch); > > + struct i386_frame_cache *cache; > > + gdb_byte buf[4]; > > + > > + if (*this_cache) > > + return *this_cache; > > + > > + cache = i386_alloc_frame_cache (); > > + *this_cache = cache; > > + > > + /* Cache base will be ESP plus cache->sp_offset (-4). */ > > + get_frame_register (this_frame, I386_ESP_REGNUM, buf); > > + cache->base = extract_unsigned_integer (buf, 4, > > + byte_order) + cache->sp_offset; > > + > > + /* Cache pc will be the frame func. */ > > + cache->pc = get_frame_pc (this_frame); > > + > > + /* The saved ESP will be at cache->base plus 8. */ > > + cache->saved_sp = cache->base + 8; > > + > > + /* The saved EIP will be at cache->base plus 4. */ > > + cache->saved_regs[I386_EIP_REGNUM] = cache->base + 4; > > + > > + return cache; > > +} > > + > > +static void > > +i386_epilogue_frame_this_id (struct frame_info *this_frame, > > + void **this_cache, > > + struct frame_id *this_id) > > +{ > > + struct i386_frame_cache *cache = i386_epilogue_frame_cache (this_frame, > > + this_cache); > > + > > + (*this_id) = frame_id_build (cache->base + 8, cache->pc); > > +} > > + > > +static const struct frame_unwind i386_epilogue_frame_unwind = > > +{ > > + NORMAL_FRAME, > > + i386_epilogue_frame_this_id, > > + i386_frame_prev_register, > > + NULL, > > + i386_epilogue_frame_sniffer > > +}; > > > > > > /* Signal trampolines. */ > > @@ -5329,7 +5412,15 @@ i386_gdbarch_init (struct gdbarch_info i > > /* Helper for function argument information. */ > > set_gdbarch_fetch_pointer_argument (gdbarch, i386_fetch_pointer_argument); > > > > - /* Hook in the DWARF CFI frame unwinder. */ > > + /* Hook the function epilogue frame unwinder. This unwinder is > > + appended to the list first, so that it supercedes the Dwarf > > + unwinder in function epilogues (where the Dwarf unwinder > > + currently fails). */ > > + frame_unwind_append_unwinder (gdbarch, &i386_epilogue_frame_unwind); > > + > > + /* Hook in the DWARF CFI frame unwinder. This unwinder is appended > > + to the list before the prologue-based unwinders, so that Dwarf > > + CFI info will be used if it is available. */ > > dwarf2_append_unwinders (gdbarch); > > > > frame_base_set_default (gdbarch, &i386_frame_base); > > @@ -5337,6 +5428,7 @@ i386_gdbarch_init (struct gdbarch_info i > > /* Hook in ABI-specific overrides, if they have been registered. */ > > gdbarch_init_osabi (info, gdbarch); > > > > + /* Hook in the legacy prologue-based unwinders last (fallback). */ > > frame_unwind_append_unwinder (gdbarch, &i386_sigtramp_frame_unwind); > > frame_unwind_append_unwinder (gdbarch, &i386_frame_unwind); > > > >