Re: [rfa/ppc] prologue parser tweaks

[Kevin Buettner <kevinb@redhat.com>]

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.