From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 28706 invoked by alias); 11 Jul 2009 18:16:02 -0000 Received: (qmail 28697 invoked by uid 22791); 11 Jul 2009 18:16:01 -0000 X-SWARE-Spam-Status: No, hits=-2.4 required=5.0 tests=AWL,BAYES_00 X-Spam-Check-By: sourceware.org Received: from smtp-outbound-2.vmware.com (HELO smtp-outbound-2.vmware.com) (65.115.85.73) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Sat, 11 Jul 2009 18:15:48 +0000 Received: from mailhost4.vmware.com (mailhost4.vmware.com [10.16.67.124]) by smtp-outbound-2.vmware.com (Postfix) with ESMTP id A5241460E4; Sat, 11 Jul 2009 11:15:46 -0700 (PDT) Received: from [10.20.94.141] (msnyder-server.eng.vmware.com [10.20.94.141]) by mailhost4.vmware.com (Postfix) with ESMTP id 9F817C9C39; Sat, 11 Jul 2009 11:15:46 -0700 (PDT) Message-ID: <4A58D5D7.9070504@vmware.com> Date: Sat, 11 Jul 2009 20:19:00 -0000 From: Michael Snyder User-Agent: Thunderbird 1.5.0.12 (X11/20080411) MIME-Version: 1.0 To: Michael Snyder CC: Mark Kettenis , "gdb-patches@sourceware.org" , "drow@false.org" , "teawater@gmail.com" 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> In-Reply-To: <4A511332.2000607@vmware.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit X-IsSubscribed: yes 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/msg00337.txt.bz2 Mark, Daniel, is this OK now? 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); >