Re: [patch/rfa:rs6000] Don't use ->prev

[Michael Snyder <msnyder at redhat dot com>]

Andrew Cagney wrote:
> 
> Hello,
> 
> A number of targets contained code that tested ``->prev'' as part of
> doing their frame analysis.  I think this ``cheating''.  The objective
> of the frame analysis code is to create the ->prev frame using
> information from the current frame, not to use the still being created
> ->prev frame :-)
> 
> Two cases - frame_chain for z8k and s390 - were, I think, simply wrong.
>   The deleted z8k comment is telling - someone was 180 degrees out! :-)

I notice that your patch does not preserve the functionality on s390 --
if the condition is false, the old code would set prev_fp, but yours
will not.

> The other two cases - get_saved_regs for SPARC and rs6000 - are more
> interesting.
> 
> The SPARC code clearly relies on there already being a ->prev frame so
> I've used get_prev_frame().  Since ->prev must exist, there is no risk
> of recursion - get_prev_frame() calling get_saved_regs() (yes, grotty).

Maybe a comment to that effect?
Otherwise the sparc bit is (belatedly) approved.  ;-)


> The rs6000 has me puzzled. I think the ->frame contains the address of
> the wrong end of the frame!  If ->frame pointed at the frame's start,
> the code below wouldn't even be needed.  Anyway, I've changed it to use
> frame_chain() (I don't see regressions on NetBSD/PPC.  The other
> possability would be to risk a (recursive) get_prev_frame() call.
> 
> Anyway, is the rs6000 ok?
> 
> Andrew
> 
>   ------------------------------------------------------------------------
> 2002-04-14  Andrew Cagney  <ac131313@redhat.com>
> 
>         * sparc-tdep.c (sparc_get_saved_register): Use get_prev_frame
>         instead of ->prev.
>         * z8k-tdep.c (z8k_frame_chain): Do not use ->prev.
>         * s390-tdep.c (s390_frame_chain): Do not use ->prev.
>         * rs6000-tdep.c (frame_get_saved_regs): Use rs6000_frame_chain()
>         instead of ->prev.
> 
> Index: rs6000-tdep.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/rs6000-tdep.c,v
> retrieving revision 1.55
> diff -u -r1.55 rs6000-tdep.c
> --- rs6000-tdep.c       12 Apr 2002 19:48:36 -0000      1.55
> +++ rs6000-tdep.c       14 Apr 2002 16:48:56 -0000
> @@ -1388,10 +1388,12 @@
>        && fdatap->cr_offset == 0
>        && fdatap->vr_offset == 0)
>      frame_addr = 0;
> -  else if (fi->prev && fi->prev->frame)
> -    frame_addr = fi->prev->frame;
>    else
> -    frame_addr = read_memory_addr (fi->frame, wordsize);
> +    /* NOTE: cagney/2002-04-14: The ->frame points to the inner-most
> +       address of the current frame.  Things might be easier if the
> +       ->frame pointed to the outer-most address of the frame.  In the
> +       mean time, the address of the prev frame is used.  */
> +    frame_addr = rs6000_frame_chain (fi);
> 
>    /* if != -1, fdatap->saved_fpr is the smallest number of saved_fpr.
>       All fpr's from saved_fpr to fp31 are saved.  */
> Index: s390-tdep.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/s390-tdep.c,v
> retrieving revision 1.43
> diff -u -r1.43 s390-tdep.c
> --- s390-tdep.c 6 Apr 2002 00:02:50 -0000       1.43
> +++ s390-tdep.c 14 Apr 2002 16:49:11 -0000
> @@ -1009,9 +1009,7 @@
>  {
>    CORE_ADDR prev_fp = 0;
> 
> -  if (thisframe->prev && thisframe->prev->frame)
> -    prev_fp = thisframe->prev->frame;
> -  else if (generic_find_dummy_frame (thisframe->pc, thisframe->frame))
> +  if (generic_find_dummy_frame (thisframe->pc, thisframe->frame))
>      return generic_read_register_dummy (thisframe->pc, thisframe->frame,
>                                          S390_SP_REGNUM);
>    else
> Index: sparc-tdep.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/sparc-tdep.c,v
> retrieving revision 1.26
> diff -u -r1.26 sparc-tdep.c
> --- sparc-tdep.c        12 Apr 2002 18:18:57 -0000      1.26
> +++ sparc-tdep.c        14 Apr 2002 16:49:23 -0000
> @@ -829,11 +829,11 @@
>             addr = frame1->frame + (regnum - G0_REGNUM) * SPARC_INTREG_SIZE
>               - (FP_REGISTER_BYTES + 8 * SPARC_INTREG_SIZE);
>           else if (regnum >= I0_REGNUM && regnum < I0_REGNUM + 8)
> -           addr = (frame1->prev->extra_info->bottom
> +           addr = (get_prev_frame (frame1)->extra_info->bottom
>                     + (regnum - I0_REGNUM) * SPARC_INTREG_SIZE
>                     + FRAME_SAVED_I0);
>           else if (regnum >= L0_REGNUM && regnum < L0_REGNUM + 8)
> -           addr = (frame1->prev->extra_info->bottom
> +           addr = (get_prev_frame (frame1)->extra_info->bottom
>                     + (regnum - L0_REGNUM) * SPARC_INTREG_SIZE
>                     + FRAME_SAVED_L0);
>           else if (regnum >= O0_REGNUM && regnum < O0_REGNUM + 8)
> @@ -875,11 +875,11 @@
>         {
>           /* Normal frame.  Local and In registers are saved on stack.  */
>           if (regnum >= I0_REGNUM && regnum < I0_REGNUM + 8)
> -           addr = (frame1->prev->extra_info->bottom
> +           addr = (get_prev_frame (frame1)->extra_info->bottom
>                     + (regnum - I0_REGNUM) * SPARC_INTREG_SIZE
>                     + FRAME_SAVED_I0);
>           else if (regnum >= L0_REGNUM && regnum < L0_REGNUM + 8)
> -           addr = (frame1->prev->extra_info->bottom
> +           addr = (get_prev_frame (frame1)->extra_info->bottom
>                     + (regnum - L0_REGNUM) * SPARC_INTREG_SIZE
>                     + FRAME_SAVED_L0);
>           else if (regnum >= O0_REGNUM && regnum < O0_REGNUM + 8)
> Index: z8k-tdep.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/z8k-tdep.c,v
> retrieving revision 1.7
> diff -u -r1.7 z8k-tdep.c
> --- z8k-tdep.c  12 Apr 2002 18:18:57 -0000      1.7
> +++ z8k-tdep.c  14 Apr 2002 16:49:25 -0000
> @@ -160,10 +160,6 @@
>  CORE_ADDR
>  z8k_frame_chain (struct frame_info *thisframe)
>  {
> -  if (thisframe->prev == 0)
> -    {
> -      /* This is the top of the stack, let's get the sp for real */
> -    }
>    if (!inside_entry_file (thisframe->pc))
>      {
>        return read_memory_pointer (thisframe->frame);