From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 28087 invoked by alias); 27 Nov 2014 16:30:07 -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 28069 invoked by uid 89); 27 Nov 2014 16:30:06 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.6 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_LOW,SPF_SOFTFAIL autolearn=no version=3.3.2 X-HELO: mail-lb0-f171.google.com Received: from mail-lb0-f171.google.com (HELO mail-lb0-f171.google.com) (209.85.217.171) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Thu, 27 Nov 2014 16:30:05 +0000 Received: by mail-lb0-f171.google.com with SMTP id n15so4400421lbi.2 for ; Thu, 27 Nov 2014 08:30:01 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:date :message-id:subject:from:to:cc:content-type :content-transfer-encoding; bh=671i8u0kpkhVnsW545nJuykZGpxHlCy9I5/X67nL+jw=; b=N9NsLxCk2z1U4yCni8vDXueEcA5iqcylPgj9FtQRLr5DbvChYhQn/aVn65JlOAIybU 1fbBT+bhGmkHTBZmHQzHkuQkiZRlfx+ogRef8VXIpKhluYr3If0PBhM4wcnupbupWHyT cvZic+mfeD6LDjw41D/JArCR+tKnDox8glvS1Tsf+fVGMFruXxOIC0/Z9r+wWt7h+58L 3TBlg8hedHW+/Yy7w6a3eF5kN+/QptgvDXnO4s5I65Fle6YT5JclRnTOUFe4HZ50E3wr yonQIl4qjD2e9gEzHSnfChjUi9hZ9KGiOuukpuVwZ4awCrm6ulJn3vCENTrW2nS3Lz5j dwPw== X-Gm-Message-State: ALoCoQm3EuA1r59WdAPlJ0oS1fVY/CtG30YvnzeBjcyuArjRI3fCQXmTkXw7eIIjcon9eQEnZV8X MIME-Version: 1.0 X-Received: by 10.112.172.97 with SMTP id bb1mr39506620lbc.38.1417105801202; Thu, 27 Nov 2014 08:30:01 -0800 (PST) Received: by 10.112.59.129 with HTTP; Thu, 27 Nov 2014 08:30:01 -0800 (PST) In-Reply-To: <54772CFC.1080801@codesourcery.com> References: <1416942734-8289-1-git-send-email-martin.galvan@tallertechnologies.com> <54772CFC.1080801@codesourcery.com> Date: Thu, 27 Nov 2014 16:30:00 -0000 Message-ID: Subject: Re: [PATCH] Rename in_function_epilogue_p to stack_frame_destroyed_p. From: Martin Galvan To: Yao Qi Cc: gdb-patches , Doug Evans , Ulrich Weigand , Pedro Alves , Daniel Gutson Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-SW-Source: 2014-11/txt/msg00693.txt.bz2 On Thu, Nov 27, 2014 at 10:54 AM, Yao Qi wrote: > On 11/26/2014 03:12 AM, Martin Galvan wrote: >> We concluded that gdbarch_in_function_epilogue_p is misnamed since it re= turns 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 througho= ut. > > 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. Hi Yao! Thanks a lot for the feedback. I'll get to work on changing the comments and building for the rest of the targets (sorry about that!). About the copyright assignment, I know my company started the paperwork about a year ago but I'm not sure what the current status is. I'll get back to you on that one. >> 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 = =3D >> >> /* 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 b= een >> + 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_u= nwind *self, >> void **this_prologue_cache) >> { >> if (frame_relative_level (this_frame) =3D=3D 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 *gdbar= ch, 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 b= een >> + 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 =3D gdbarch_byte_order_for_code (= gdbarch); >> unsigned int insn, insn2; >> @@ -3342,11 +3342,11 @@ thumb_in_function_epilogue_p (struct gdbarch *gd= barch, 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 b= een >> + 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 =3D gdbarch_byte_order_for_code (= gdbarch); >> unsigned int insn; >> @@ -3354,7 +3354,7 @@ arm_in_function_epilogue_p (struct gdbarch *gdbarc= h, 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, stru= ct 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 inv= alidated, >> + such as in a function's epilogue. */ >> + set_gdbarch_stack_frame_destroyed_p (gdbarch, arm_stack_frame_destroy= ed_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 rep= arse) >> struct gdbarch *frame_arch =3D get_frame_arch (fi); >> CORE_ADDR frame_pc =3D 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' instruct= ion >> +/* Returns true if we're at a point where the stack frame has already b= een >> + destroyed (such as in a function's epilogue). >> + >> + The epilogue is defined here as the area either on the `bv' instruct= ion >> 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 =3D gdbarch_byte_order (gdbarch); >> unsigned long status; >> @@ -3043,8 +3046,8 @@ hppa_gdbarch_init (struct gdbarch_info info, struc= t 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=3Dall --enable-64-bit-bfd", to make sure your patch > doesn't break > the build for some other targets. > > -- > Yao (=E9=BD=90=E5=B0=A7) --=20 Mart=C3=ADn Galv=C3=A1n Software Engineer Taller Technologies Argentina San Lorenzo 47, 3rd Floor, Office 5 C=C3=B3rdoba, Argentina Phone: 54 351 4217888 / +54 351 4218211