This is the mail archive of the
gdb-patches@sources.redhat.com
mailing list for the GDB project.
Re: [rfa/ppc] prologue parser tweaks
- From: Kevin Buettner <kevinb at redhat dot com>
- To: gdb-patches at sources dot redhat dot com
- Date: Thu, 18 Mar 2004 09:15:32 -0700
- Subject: Re: [rfa/ppc] prologue parser tweaks
- Organization: Red Hat
- References: <40577FA6.5080506@gnu.org>
On Tue, 16 Mar 2004 17:28:54 -0500
Andrew Cagney <cagney@gnu.org> wrote:
> * rs6000-tdep.c: Add field "func_start".
If this part goes in, it should be:
* rs6000-tdep.c (struct rs6000_framedata): Add field "func_start".
> (skip_prologue): New variable num_skip_syscall_insn, use to skip
> over first half of a GNU/Linux syscall and update "func_start".
> Record only the first LR save, use to skip over PIC code.
>
> Index: rs6000-tdep.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/rs6000-tdep.c,v
> retrieving revision 1.183
> diff -u -r1.183 rs6000-tdep.c
> --- rs6000-tdep.c 2 Mar 2004 02:20:25 -0000 1.183
> +++ rs6000-tdep.c 16 Mar 2004 22:08:24 -0000
> @@ -65,6 +65,7 @@
>
> struct rs6000_framedata
> {
> + CORE_ADDR func_start; /* true function start */
Why do you need to add this field? Do you have some other patches which
depend on it?
> int offset; /* total size of frame --- the distance
> by which we decrement sp to allocate
> the frame */
> @@ -502,6 +503,7 @@
> int minimal_toc_loaded = 0;
> int prev_insn_was_prologue_insn = 1;
> int num_skip_non_prologue_insns = 0;
> + int num_skip_syscall_insn = 0;
Could you add a brief comment indicating that this is a state variable
for the PPC64 GNU/Linux system call tests? I don't want someone
to inadvertently start using it for some other purpose. Hmm, even
better might be to name it something like ``ppc64_linux_syscall_state''.
With a name like that, it's unlikely that it'll inadvertently get used
for some other purpose.
> const struct bfd_arch_info *arch_info = gdbarch_bfd_arch_info (current_gdbarch);
> struct gdbarch_tdep *tdep = gdbarch_tdep (current_gdbarch);
>
> @@ -521,6 +523,7 @@
> lim_pc = refine_prologue_limit (pc, lim_pc);
>
> memset (fdata, 0, sizeof (struct rs6000_framedata));
> + fdata->func_start = pc;
> fdata->saved_gpr = -1;
> fdata->saved_fpr = -1;
> fdata->saved_vr = -1;
> @@ -548,6 +551,70 @@
[...]
> + if (((op & 0xffff0000) == 0x38000000 /* li r0,N */
> + && pc == fdata->func_start + 0)
It seems to me that substituting ``orig_pc'' in place of ``fdata->func_start''
will work, wont't it? E.g.:
&& pc == orig_pc + 0)
> + || (op == 0x44000002 /* sc */
> + && pc == fdata->func_start + 4
&& pc == orig_pc + 4
??
> + && num_skip_syscall_insn == 1)
> + || (op == 0x4ca30020 /* bnslr+ */
> + && pc == fdata->func_start + 8
&& pc == orig_pc + 8
??
> + && num_skip_syscall_insn == 2))
> + {
> + num_skip_syscall_insn++;
> + continue;
> + }
> + else if ((op & 0xfc000003) == 0x48000000 /* b __syscall_error */
> + && pc == fdata->func_start + 12
&& pc == orig_pc + 12
> + && num_skip_syscall_insn == 3)
> + {
> + num_skip_syscall_insn++;
> + fdata->func_start = pc;
What's this about? Where else does fdata->func_start get used?
> + continue;
> + }
I'd like to see the above test moved down a ways in the sequence of
tests. Hmm, maybe it could go right before the AltiVec tests?
> +
> + if ((op & 0xfc1fffff) == 0x7c0802a6)
> + { /* mflr Rx */
> + /* Since shared library / PIC code, which needs to get its
> + address at runtime, can appear to save more than one link
> + register vis:
> +
> + *INDENT-OFF*
> + stwu r1,-304(r1)
> + mflr r3
> + bl 0xff570d0 (blrl)
> + stw r30,296(r1)
> + mflr r30
> + stw r31,300(r1)
> + stw r3,308(r1);
> + ...
> + *INDENT-ON*
> +
> + remember just the first one, but skip over additional
> + ones. */
> + if (lr_reg < 0)
> + lr_reg = (op & 0x03e00000);
> + continue;
> + }
>
> if ((op & 0xfc1fffff) == 0x7c0802a6)
> { /* mflr Rx */
The above is okay so long as you remove the old "mflr Rx" test.