From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 19623 invoked by alias); 10 Feb 2014 12:37:35 -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 19612 invoked by uid 89); 10 Feb 2014 12:37:35 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.0 required=5.0 tests=AWL,BAYES_00,RP_MATCHES_RCVD,SPF_PASS,UNPARSEABLE_RELAY autolearn=ham version=3.3.2 X-HELO: aserp1040.oracle.com Received: from aserp1040.oracle.com (HELO aserp1040.oracle.com) (141.146.126.69) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-SHA encrypted) ESMTPS; Mon, 10 Feb 2014 12:37:34 +0000 Received: from acsinet21.oracle.com (acsinet21.oracle.com [141.146.126.237]) by aserp1040.oracle.com (Sentrion-MTA-4.3.2/Sentrion-MTA-4.3.2) with ESMTP id s1ACbQrh031986 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Mon, 10 Feb 2014 12:37:26 GMT Received: from userz7022.oracle.com (userz7022.oracle.com [156.151.31.86]) by acsinet21.oracle.com (8.14.4+Sun/8.14.4) with ESMTP id s1ACbPX4003701 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Mon, 10 Feb 2014 12:37:25 GMT Received: from abhmp0007.oracle.com (abhmp0007.oracle.com [141.146.116.13]) by userz7022.oracle.com (8.14.5+Sun/8.14.4) with ESMTP id s1ACbOta005428; Mon, 10 Feb 2014 12:37:24 GMT Received: from termi.oracle.com (/10.175.45.215) by default (Oracle Beehive Gateway v4.0) with ESMTP ; Mon, 10 Feb 2014 04:37:24 -0800 From: jose.marchesi@oracle.com (Jose E. Marchesi) To: Joel Brobecker Cc: Pedro Alves , gdb-patches@sourceware.org Subject: Re: [PATCH] add gdbarch_in_function_epilogue_p hook for sparc64 References: <87mwm9b8pr.fsf@oracle.com> <529E2ADD.6020409@redhat.com> <8738m9j5de.fsf@oracle.com> <529F1CE0.2060000@redhat.com> <87y540iz29.fsf@oracle.com> <87ppnau7ho.fsf@oracle.com> <87ppn0uxid.fsf@oracle.com> <20140208030146.GK5485@adacore.com> Date: Mon, 10 Feb 2014 12:37:00 -0000 In-Reply-To: <20140208030146.GK5485@adacore.com> (Joel Brobecker's message of "Sat, 8 Feb 2014 07:01:46 +0400") Message-ID: <87fvnrgmyw.fsf@oracle.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3.50 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-IsSubscribed: yes X-SW-Source: 2014-02/txt/msg00296.txt.bz2 Hi Joel. Thanks for the review! > 2013-10-16 Jose E. Marchesi > > * sparc-tdep.c (sparc_in_function_epilogue_p): New function. > (X_RETTURN): New macro. > * sparc-tdep.h: sparc_in_function_epilogue_p prototype. > > * sparc64-tdep.c (sparc64_init_abi): Hook > sparc_in_function_epilogue_p. Regarding testing this function on sparc32: Can you tell us which testcase in our testsuite this patch fixes? Although I am allowed to run the testsuite, I can still run individual testcases by hand (if not too complex, of course). Otherwise, would you have a small reproducer I could use to test on sparc32? As mentioned in the original submission, the failing test was gdb.base/watch-cond.exp. Comments about the patch below. > +/* Macros to identify some instructions. */ > +#define X_RETTURN(i) ((X_OP (i) == 0x2) && (X_OP3 (i) == 0x39)) Can your comment say a little more precisely what instruction it identifies? I think the parens around the equality operators are superfluous and should be removed. Fixed in the amended patch below. > +/* Return true if we are in a function's epilogue, i.e. after an > + instruction that destroyed a function's stack frame. */ > + > +int > +sparc_in_function_epilogue_p (struct gdbarch *gdbarch, CORE_ADDR pc) > +{ The general principle for function meant to be used as gdbarch callbacks is just to say: /* Implement "in_function_epilogue_p". */ That way, if we ever change the function's prototype, we don't have to update the documentation everywhere. This callback should only be documented in gdbarch.sh (and repeated in gdbarch.h, which is generated from gdbarch.sh). Fixed in the amended patch below. In your case, since there are 32bit and 64bit versions, you can add something like, for instance: This implementation works on both sparc32 and sparc64. That fact is implicitly true for all functions defined in sparc-tdep.c with prefix sparc_. > + /* This function must return true if we are one instruction after an > + instruction that destroyed the stack frame of the current > + function. The SPARC instructions used to restore the callers > + stack frame are RESTORE and RETURN/RETT. > + > + Of these RETURN/RETT is a branch instruction and thus we return > + true if we are in its delay slot. > + > + RESTORE is almost always found in the delay slot of a branch > + instruction that transfers control to the caller, such as JMPL. > + Thus the next instruction is in the caller frame and we don't > + need to do anything about it. */ > + > + unsigned int insn = sparc_fetch_instruction (pc - 4); > + return X_RETTURN (insn); Small quirk of the GDB Coding Style: We require an empty line between local variable declarations and the statements after. Also, I notice there are trailing spaces. Fixed in the amended patch below. > set_gdbarch_skip_prologue (gdbarch, sparc64_skip_prologue); > > + /* Detect whether PC is in function epilogue. */ > + set_gdbarch_in_function_epilogue_p (gdbarch, sparc_in_function_epilogue_p); > + I would normally not comment on this, but since I've made other comments, I'll ask that the comment be revmoved, and that you avoid the empty line between the call to set_gdbarch_skip_prologue just above and the call to set_gdbarch_in_function_epilogue_p that you are adding. Fixed in the amended patch below. 2013-10-16 Jose E. Marchesi * sparc-tdep.c (sparc_in_function_epilogue_p): New function. (X_RETTURN): New macro. * sparc-tdep.h: sparc_in_function_epilogue_p prototype. * sparc64-tdep.c (sparc64_init_abi): Hook sparc_in_function_epilogue_p. diff --git a/gdb/sparc-tdep.c b/gdb/sparc-tdep.c index 38b345b..311a156 100644 --- a/gdb/sparc-tdep.c +++ b/gdb/sparc-tdep.c @@ -88,6 +88,9 @@ struct regset; #define X_DISP19(i) ((((i) & 0x7ffff) ^ 0x40000) - 0x40000) #define X_DISP10(i) ((((((i) >> 11) && 0x300) | (((i) >> 5) & 0xff)) ^ 0x200) - 0x200) #define X_SIMM13(i) ((((i) & 0x1fff) ^ 0x1000) - 0x1000) +/* Macros to identify some instructions. */ +/* RETURN (RETT in V8) */ +#define X_RETTURN(i) ((X_OP (i) == 0x2) && (X_OP3 (i) == 0x39)) /* Fetch the instruction at PC. Instructions are always big-endian even if the processor operates in little-endian mode. */ @@ -452,6 +455,29 @@ sparc32_pseudo_register_write (struct gdbarch *gdbarch, regcache_raw_write (regcache, regnum + 1, buf + 4); } +/* Implement "in_function_epilogue_p". */ + +int +sparc_in_function_epilogue_p (struct gdbarch *gdbarch, CORE_ADDR pc) +{ + /* This function must return true if we are one instruction after an + instruction that destroyed the stack frame of the current + function. The SPARC instructions used to restore the callers + stack frame are RESTORE and RETURN/RETT. + + Of these RETURN/RETT is a branch instruction and thus we return + true if we are in its delay slot. + + RESTORE is almost always found in the delay slot of a branch + instruction that transfers control to the caller, such as JMPL. + Thus the next instruction is in the caller frame and we don't + need to do anything about it. */ + + unsigned int insn = sparc_fetch_instruction (pc - 4); + + return X_RETTURN (insn); +} + static CORE_ADDR sparc32_frame_align (struct gdbarch *gdbarch, CORE_ADDR address) diff --git a/gdb/sparc-tdep.h b/gdb/sparc-tdep.h index b83d711..a065ebe 100644 --- a/gdb/sparc-tdep.h +++ b/gdb/sparc-tdep.h @@ -193,6 +193,9 @@ extern struct sparc_frame_cache * extern struct sparc_frame_cache * sparc32_frame_cache (struct frame_info *this_frame, void **this_cache); +extern int + sparc_in_function_epilogue_p (struct gdbarch *gdbarch, CORE_ADDR pc); + extern int sparc_software_single_step (struct frame_info *frame); diff --git a/gdb/sparc64-tdep.c b/gdb/sparc64-tdep.c index 52958df..9e4db3a 100644 --- a/gdb/sparc64-tdep.c +++ b/gdb/sparc64-tdep.c @@ -1196,6 +1196,7 @@ sparc64_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch) (gdbarch, default_stabs_argument_has_addr); set_gdbarch_skip_prologue (gdbarch, sparc64_skip_prologue); + set_gdbarch_in_function_epilogue_p (gdbarch, sparc_in_function_epilogue_p); /* Hook in the DWARF CFI frame unwinder. */ dwarf2_frame_set_init_reg (gdbarch, sparc64_dwarf2_frame_init_reg);