Re: [PATCH] add 'rs6000_in_function_epilogue_p()' (Revised, again)

Paul Gilliam
2005-12-02T23:20:06Z

On Friday 02 December 2005 11:56, Mark Kettenis wrote:
> > From: Paul Gilliam <pgilliam@us.ibm.com>
> > Date: Fri, 2 Dec 2005 11:20:22 -0800
> >
> > And here is the revised patch:
> >
> > 2005-12-02 Paul Gilliam <pgilliam@us.ibm.com>
> >
> > * rs6000-tdep.c: Add new subroutine, 'rs6000_in_function_epilogue_p()'
> > and put it into the architecture vector.
>
> Paul,
>
> There are a couple of problems with your patch. Please read read the
> GNU condig standards http://www.gnu.org/prep/standards/. Here are a
> few issues:

OK.

> * Please put a space before the ( that starts the functions arguments.

OK

> * ins_changes_sp contains unused variables. Please configure gdb
> with --enable-gdb-build-warnings=,-Werror, this'll prevent them
> from slipping through.

The unused variables where on purpose. They show the complete 'cracking'
of the types of instructions we are interested in.

But I guess a comment will work as well.

> * Please don't make your lines too long. The emacs default
> fill-column is 72, but in general we tolerate lines that are a bit
> longer. But make sure the line ends before column 79/80.

I am not sure which line you are refering to: the longest lines end in column 79.

But I'll sorten them.

> * "break" should be put on a line of its own.

OK

> * Always surround =, -=, etc. with spaces.

OK

> I think your patch should go in. But it's really Andrew Cagney's and
> Kevin Buettner's call.
>
> Mark
>

Thank you, Mark. The 'GNU Coding Standards' is a good start, but there is nothing
like to voice of experiance.

So if my revised patch below addresses your concerns, I'll check it in as per Kevin Buettner's message:
> On Friday 02 December 2005 12:32, Kevin Buettner wrote:
> > On Fri, 2 Dec 2005 11:20:22 -0800
> > Paul Gilliam <pgilliam@us.ibm.com> wrote:
> >
> > > * rs6000-tdep.c: Add new subroutine, 'rs6000_in_function_epilogue_p()'
> > > and put it into the architecture vector.
> >
> > Your patch is okay to commit after you address Mark's concerns.
> >
> > Kevin
> >
> >

-=# Paul #=-

PS: I also fixed a typo or two.

Revised Patch:

2005-12-02 Paul Gilliam <pgilliam@us.ibm.com>

* rs6000-tdep.c: Add new subroutine, 'rs6000_in_function_epilogue_p()'
and put it into the architecture vector.

Index: rs6000-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/rs6000-tdep.c,v
retrieving revision 1.248
diff -a -u -p -r1.248 rs6000-tdep.c
--- rs6000-tdep.c 1 Nov 2005 19:32:36 -0000 1.248
+++ rs6000-tdep.c 2 Dec 2005 23:08:10 -0000
@@ -502,6 +502,74 @@ rs6000_skip_prologue (CORE_ADDR pc)
return pc;
}

+static int
+insn_changes_sp (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 */
+
+ if (opcode == 31 && subcode == 444 && a == 1)
+ return 1; /* mr R1,Rn */
+ if (opcode == 14 && sd == 1)
+ return 1; /* addi R1,Rn,simm */
+ if (opcode == 58 && sd == 1)
+ return 1; /* ld R1,ds(Rn) */
+
+ return 0;
+}
+
+/* Return true if we are in the function's epilogue, i.e. after the
+ instruction that destroyed the function's stack frame. */
+
+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_addr, func_end;
+ unsigned long insn;
+ struct frame_info *fr;
+
+ /* Find the search limits. */
+ if (!find_pc_partial_function (pc, NULL, &func_addr, &func_end))
+ return 0;
+
+ /* 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. */
+
+ fr = get_current_frame ();
+
+ /* Scan forward untill next 'blr'. */
+ for (scan_pc = pc; scan_pc < func_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 (insn))
+ return 0;
+ }
+
+ /* Scan backward untill adjustment to stack pointer (R1). */
+ for (scan_pc = pc-PPC_INSN_SIZE;
+ scan_pc >= func_addr;
+ 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 (insn))
+ return 1;
+ }
+
+ return 0;
+}
+

/* Fill in fi->saved_regs */

@@ -3342,6 +3409,8 @@ rs6000_gdbarch_init (struct gdbarch_info
set_gdbarch_deprecated_extract_struct_value_address (gdbarch, rs6000_extract_struct_value_address);

set_gdbarch_skip_prologue (gdbarch, rs6000_skip_prologue);
+ set_gdbarch_in_function_epilogue_p (gdbarch, rs6000_in_function_epilogue_p);
+
set_gdbarch_inner_than (gdbarch, core_addr_lessthan);
set_gdbarch_breakpoint_from_pc (gdbarch, rs6000_breakpoint_from_pc);