From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 27933 invoked by alias); 27 Nov 2014 13:53:59 -0000 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 Received: (qmail 27921 invoked by uid 89); 27 Nov 2014 13:53:57 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.7 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.2 X-HELO: relay1.mentorg.com Received: from relay1.mentorg.com (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 27 Nov 2014 13:53:54 +0000 Received: from svr-orw-fem-04.mgc.mentorg.com ([147.34.97.41]) by relay1.mentorg.com with esmtp id 1XtzW2-0003id-B7 from Yao_Qi@mentor.com ; Thu, 27 Nov 2014 05:53:50 -0800 Received: from qiyao.dyndns.org (147.34.91.1) by svr-orw-fem-04.mgc.mentorg.com (147.34.97.41) with Microsoft SMTP Server id 14.3.181.6; Thu, 27 Nov 2014 05:53:49 -0800 Message-ID: <54772CFC.1080801@codesourcery.com> Date: Thu, 27 Nov 2014 13:53:00 -0000 From: Yao Qi User-Agent: Mozilla/5.0 (X11; Linux i686; rv:31.0) Gecko/20100101 Thunderbird/31.1.1 MIME-Version: 1.0 To: Martin Galvan , CC: , , , Subject: Re: [PATCH] Rename in_function_epilogue_p to stack_frame_destroyed_p. References: <1416942734-8289-1-git-send-email-martin.galvan@tallertechnologies.com> In-Reply-To: <1416942734-8289-1-git-send-email-martin.galvan@tallertechnologies.com> Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit X-IsSubscribed: yes X-SW-Source: 2014-11/txt/msg00689.txt.bz2 On 11/26/2014 03:12 AM, Martin Galvan wrote: > We concluded that gdbarch_in_function_epilogue_p is misnamed since it returns true if the given PC is one instruction after the one that destroyed the stack (which isn't necessarily inside an epilogue), therefore it should be renamed to stack_frame_destroyed_p. > > I also took the liberty of renaming the arch-specific implementations to *_stack_frame_destroyed_p as well for consistency. > Hi, Martin, thanks for doing this. > gdb/: > 2014-11-25 Martin Galvan > > * Replace in_function_epilogue_p to stack_frame_destroyed_p throughout. The format of changelog entry isn't correct. Please take a look at https://sourceware.org/gdb/wiki/ContributionChecklist All changed files should be listed in the changelog entry. Do you have FSF copyright assignment? If you don't, please see "5. FSF copyright Assignment" in contribution checklist above too. > > diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c > index e69da01..9907a81 100644 > --- a/gdb/amd64-tdep.c > +++ b/gdb/amd64-tdep.c > @@ -2707,12 +2707,15 @@ static const struct frame_base amd64_frame_base = > > /* Normal frames, but in a function epilogue. */ > > -/* The epilogue is defined here as the 'ret' instruction, which will > +/* Returns true if we're at a point where the stack frame has already been > + destroyed (such as in a function's epilogue). Such comment should be moved to gdbarch.sh:stack_frame_destroyed_p, and we only "Implement the stack_frame_destroyed_p gdbarch method." This also applies to some other places in this patch. > + > + 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 > -amd64_in_function_epilogue_p (struct gdbarch *gdbarch, CORE_ADDR pc) > +amd64_stack_frame_destroyed_p (struct gdbarch *gdbarch, CORE_ADDR pc) > { > gdb_byte insn; > struct compunit_symtab *cust; > @@ -2736,7 +2739,7 @@ amd64_epilogue_frame_sniffer (const struct frame_unwind *self, > void **this_prologue_cache) > { > if (frame_relative_level (this_frame) == 0) > - return amd64_in_function_epilogue_p (get_frame_arch (this_frame), > + return amd64_stack_frame_destroyed_p (get_frame_arch (this_frame), > get_frame_pc (this_frame)); > else > return 0; > diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c > index 1002870..46043ab 100644 > --- a/gdb/arm-tdep.c > +++ b/gdb/arm-tdep.c > @@ -3231,11 +3231,11 @@ arm_dwarf2_frame_init_reg (struct gdbarch *gdbarch, int regnum, > } > } > > -/* Return true if we are in the function's epilogue, i.e. after the > - instruction that destroyed the function's stack frame. */ > +/* Returns true if we're at a point where the stack frame has already been > + destroyed (such as in a function's epilogue). */ > We can write the comment in this way, /* Implement the stack_frame_destroyed_p gdbarch method for thumb mode. */ > static int > -thumb_in_function_epilogue_p (struct gdbarch *gdbarch, CORE_ADDR pc) > +thumb_stack_frame_destroyed_p (struct gdbarch *gdbarch, CORE_ADDR pc) > { > enum bfd_endian byte_order_for_code = gdbarch_byte_order_for_code (gdbarch); > unsigned int insn, insn2; > @@ -3342,11 +3342,11 @@ thumb_in_function_epilogue_p (struct gdbarch *gdbarch, CORE_ADDR pc) > return found_stack_adjust; > } > > -/* Return true if we are in the function's epilogue, i.e. after the > - instruction that destroyed the function's stack frame. */ > +/* Returns true if we're at a point where the stack frame has already been > + destroyed (such as in a function's epilogue). */ /* Implement the stack_frame_destroyed_p gdbarch method. */ > > static int > -arm_in_function_epilogue_p (struct gdbarch *gdbarch, CORE_ADDR pc) > +arm_stack_frame_destroyed_p (struct gdbarch *gdbarch, CORE_ADDR pc) > { > enum bfd_endian byte_order_for_code = gdbarch_byte_order_for_code (gdbarch); > unsigned int insn; > @@ -3354,7 +3354,7 @@ arm_in_function_epilogue_p (struct gdbarch *gdbarch, CORE_ADDR pc) > CORE_ADDR func_start, func_end; > > if (arm_pc_is_thumb (gdbarch, pc)) > - return thumb_in_function_epilogue_p (gdbarch, pc); > + return thumb_stack_frame_destroyed_p (gdbarch, pc); > > if (!find_pc_partial_function (pc, NULL, &func_start, &func_end)) > return 0; > @@ -10380,8 +10380,9 @@ arm_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches) > /* Advance PC across function entry code. */ > set_gdbarch_skip_prologue (gdbarch, arm_skip_prologue); > > - /* Detect whether PC is in function epilogue. */ > - set_gdbarch_in_function_epilogue_p (gdbarch, arm_in_function_epilogue_p); > + /* Detect whether PC is at a point where the stack frame has been invalidated, > + such as in a function's epilogue. */ > + set_gdbarch_stack_frame_destroyed_p (gdbarch, arm_stack_frame_destroyed_p); > > /* Skip trampolines. */ > set_gdbarch_skip_trampoline_code (gdbarch, arm_skip_stub); > diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c > index 574d06c..5f9d8da 100644 > --- a/gdb/breakpoint.c > +++ b/gdb/breakpoint.c > @@ -1827,10 +1827,11 @@ update_watchpoint (struct watchpoint *b, int reparse) > struct gdbarch *frame_arch = get_frame_arch (fi); > CORE_ADDR frame_pc = get_frame_pc (fi); > > - /* If we're in a function epilogue, unwinding may not work > + /* If we're at a point where the stack frame has been invalidated > + (such as in a function's epilogue), unwinding may not work I'd like to say something like " If frame has been invalidated (such as FRAME_PC is in function's epilogue), unwinding may not work" > diff --git a/gdb/hppa-tdep.c b/gdb/hppa-tdep.c > index 627f31a..3a53bc5 100644 > --- a/gdb/hppa-tdep.c > +++ b/gdb/hppa-tdep.c > @@ -529,13 +529,16 @@ find_unwind_entry (CORE_ADDR pc) > return NULL; > } > > -/* The epilogue is defined here as the area either on the `bv' instruction > +/* Returns true if we're at a point where the stack frame has already been > + destroyed (such as in a function's epilogue). > + > + The epilogue is defined here as the area either on the `bv' instruction > itself or an instruction which destroys the function's stack frame. > > We do not assume that the epilogue is at the end of a function as we can > also have return sequences in the middle of a function. */ > static int > -hppa_in_function_epilogue_p (struct gdbarch *gdbarch, CORE_ADDR pc) > +hppa_stack_frame_destroyed_p (struct gdbarch *gdbarch, CORE_ADDR pc) > { > enum bfd_endian byte_order = gdbarch_byte_order (gdbarch); > unsigned long status; > @@ -3043,8 +3046,8 @@ hppa_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches) > /* The following gdbarch vector elements do not depend on the address > size, or in any other gdbarch element previously set. */ > set_gdbarch_skip_prologue (gdbarch, hppa_skip_prologue); > - set_gdbarch_in_function_epilogue_p (gdbarch, > - hppa_in_function_epilogue_p); > + set_gdbarch_stack_frame_destroyed_p (gdbarch, > + stack_frame_destroyed_p); s/stack_frame_destroyed_p/hppa_stack_frame_destroyed_p, otherwise this causes a compilation error. Please configure gdb with "--enable-targets=all --enable-64-bit-bfd", to make sure your patch doesn't break the build for some other targets. -- Yao (齐尧)