From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 9398 invoked by alias); 13 Jan 2006 21:05:58 -0000 Received: (qmail 9390 invoked by uid 22791); 13 Jan 2006 21:05:57 -0000 X-Spam-Check-By: sourceware.org Received: from sibelius.xs4all.nl (HELO sibelius.xs4all.nl) (82.92.89.47) by sourceware.org (qpsmtpd/0.31) with ESMTP; Fri, 13 Jan 2006 21:05:56 +0000 Received: from elgar.sibelius.xs4all.nl (root@elgar.sibelius.xs4all.nl [192.168.0.2]) by sibelius.xs4all.nl (8.13.4/8.13.4) with ESMTP id k0DL5b4p005714; Fri, 13 Jan 2006 22:05:38 +0100 (CET) Received: from elgar.sibelius.xs4all.nl (kettenis@localhost.sibelius.xs4all.nl [127.0.0.1]) by elgar.sibelius.xs4all.nl (8.13.4/8.13.3) with ESMTP id k0DL5bNE000372; Fri, 13 Jan 2006 22:05:37 +0100 (CET) Received: (from kettenis@localhost) by elgar.sibelius.xs4all.nl (8.13.4/8.13.4/Submit) id k0DL5ZcF011764; Fri, 13 Jan 2006 22:05:35 +0100 (CET) Date: Fri, 13 Jan 2006 21:05:00 -0000 Message-Id: <200601132105.k0DL5ZcF011764@elgar.sibelius.xs4all.nl> From: Mark Kettenis To: pgilliam@us.ibm.com CC: gdb-patches@sourceware.org, kevinb@redhat.com In-reply-to: <200601121623.15363.pgilliam@us.ibm.com> (message from Paul Gilliam on Thu, 12 Jan 2006 16:23:15 -0800) Subject: Re: [PATCH] add 'rs6000_in_function_epilogue_p()' (Revised) References: <200511301225.56802.pgilliam@us.ibm.com> <200601111013.24433.pgilliam@us.ibm.com> <200601111511.27699.pgilliam@us.ibm.com> <200601121623.15363.pgilliam@us.ibm.com> Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2006-01/txt/msg00149.txt.bz2 > From: Paul Gilliam > Date: Thu, 12 Jan 2006 16:23:15 -0800 [ Bleah, I'd really wish people stopped sending MIME mail, especially with that stupid quoted-printable encoding. I hate editing out all those gratuitous equal signs. ] > > I kind of lost track of this. Here is a link to the revised > > patch. It should address Mark's concerns, Unfortunately it doesn't. > 2006-01-11 Paul Gilliam > > * ppc-tdep.h: Add a define for the hard limit used when scanning an > epilogue. > * rs6000-tdep.c: Add new subroutine, 'rs6000_in_function_epilogue_p()' > and put it into the architecture vector. It's probably your stupid mailer that converts tabs into spaces or something, but please make sure the indentation of your Changelog entry is ok. Actually the whole entry is wrong. Please *do* read the GNU coding standards. Your changelog should probably read like this: 2006-01-11 Paul Gilliam * ppc-tdep.h (PPC_MAX_EPILOGUE_INSTRUCTIONS): New define. * rs6000-tdep.c (insn_changes_sp_or_jumps) (rs6000_in_function_epilogue_p): New functions. (rs6000_gdbarch_init): Set in_function_epilogue_p. Also see a few comments in the code below. > Index: rs6000-tdep.c > =================================================================== > RCS file: /cvs/src/src/gdb/rs6000-tdep.c,v > retrieving revision 1.248 > diff -a -u -r1.248 rs6000-tdep.c > --- rs6000-tdep.c 1 Nov 2005 19:32:36 -0000 1.248 > +++ rs6000-tdep.c 11 Jan 2006 23:03:43 -0000 > @@ -502,6 +502,114 @@ > return pc; > } > > +static int > +insn_changes_sp_or_jumps (unsigned long insn) > +{ > + int opcode = (insn >> 26) & 0x03f; > + int sd = (insn >> 21) & 0x01f; > + int a = (insn >> 16) & 0x01f; > + /* b = (insn >> 11) & 0x01f */ > + int subcode = (insn >> 1) & 0x3ff; > + /* rc = insn & 0x001 */ Please remove these comments. > +/* Return true if we are in the function's epilogue, i.e. after the > + instruction that destroyed the function's stack frame. > + > + 1) scan forward from the point of execution: > + a) If you find an instruction that modifies the stack pointer > + or transfers control (except a return), execution is not in > + an epilogue, return. > + b) Stop scanning if you find a return instruction or reach the > + end of the function or reach the hard limit for the size of > + an epilogue. > + 2) scan backward from the point of execution: > + a) If you find an instruction that modifies the stack pointer, > + execution *is* in an epilogue, return. > + b) Stop scanning if you reach an instruction that transfers > + control or the beginning of the function or reach the hard > + limit for the size of an epilogue. */ > + > +static int > +rs6000_in_function_epilogue_p (struct gdbarch *gdbarch, CORE_ADDR pc) > +{ > + bfd_byte insn_buf[PPC_INSN_SIZE]; > + CORE_ADDR scan_pc, func_start, func_end, epilogue_start, epilogue_end; > + unsigned long insn; > + struct frame_info *fr; Could you name this variable frame instead of the cryptic fr? > + /* Find the search limits based on function boundaries and hard limit. */ > + > + if (!find_pc_partial_function (pc, NULL, &func_start, &func_end)) > + return 0; > + > + epilogue_start = pc - PPC_MAX_EPILOGUE_INSTRUCTIONS * PPC_INSN_SIZE; > + if (epilogue_start < func_start) epilogue_start = func_start; > + > + epilogue_end = pc + PPC_MAX_EPILOGUE_INSTRUCTIONS * PPC_INSN_SIZE; > + if (epilogue_end > func_end) epilogue_end = func_end; > + > + /* Get the current frame. This may be cheap, since we might have > + just called it in watchpoint_check, before calling > + gdbarch_in_function_epilogue_p. */ I don't think this comment is actually adding any useful information, so I'd prefer it if you dropped it. > + fr = get_current_frame (); > + > + /* Scan forward until next 'blr'. */ > + > + for (scan_pc = pc; scan_pc < epilogue_end; scan_pc += PPC_INSN_SIZE) > + { > + if (!safe_frame_unwind_memory (fr, scan_pc, insn_buf, PPC_INSN_SIZE)) > + return 0; > + insn = extract_signed_integer (insn_buf, PPC_INSN_SIZE); > + if (insn == 0x4e800020) > + break; > + if (insn_changes_sp_or_jumps (insn)) > + return 0; > + } > + > + /* Scan backward until adjustment to stack pointer (R1). */ > + > + for (scan_pc = pc - PPC_INSN_SIZE; > + scan_pc >= epilogue_start; > + scan_pc -= PPC_INSN_SIZE) > + { > + if (!safe_frame_unwind_memory (fr, scan_pc, insn_buf, PPC_INSN_SIZE)) > + return 0; > + insn = extract_signed_integer (insn_buf, PPC_INSN_SIZE); > + if (insn_changes_sp_or_jumps (insn)) > + return 1; > + } > + > + return 0; > +} > + > > /* Fill in fi->saved_regs */ With those changes, this is ok. Mark