Re: [RFA/ARM] Framificate the ARM port [3/3]

[Andrew Cagney <ac131313 at redhat dot com>]

So much code gets deleted, Ya!


Here are some ``GDB speak'' comments - help clarify use of `prev', 
`this', and `next'.  Some of them (e.g., s/our/THIS/ are probably not 
GNU PC).

> ===================================================================
> --- gdb.orig/arm-tdep.c	2003-06-30 18:28:56.000000000 -0400
> +++ gdb/arm-tdep.c	2003-06-30 18:28:57.000000000 -0400
> @@ -34,6 +34,9 @@
>  #include "value.h"
>  #include "arch-utils.h"
>  #include "osabi.h"
> +#include "frame-unwind.h"
> +#include "frame-base.h"
> +#include "trad-frame.h"
>  
>  #include "arm-tdep.h"
>  #include "gdb/sim-arm.h"
> @@ -155,21 +158,31 @@ static void convert_from_extended (const
>  static void convert_to_extended (const struct floatformat *, void *,
>  				 const void *);
>  
> -/* Define other aspects of the stack frame.  We keep the offsets of
> -   all saved registers, 'cause we need 'em a lot!  We also keep the
> -   current size of the stack frame, and the offset of the frame
> -   pointer from the stack pointer (for frameless functions, and when
> -   we're still in the prologue of a function with a frame).  */
> -
> -#define arm_get_cache(fi) ((struct arm_prologue_cache *) get_frame_extra_info (fi))
> -
>  struct arm_prologue_cache
>  {
> -  CORE_ADDR unwound_sp, unwound_pc;
> +  /* The stack pointer at the time this frame was created; i.e. our caller's
> +     stack pointer when we were called.  We use this to identify the frame.  */
> +  CORE_ADDR unwound_sp;

I'd call it ``prev_sp'', and ``It is used used to identify THIS frame''.

> +  /* The current PC in this frame; i.e. our callee's resume address, if we are
> +     not the innermost frame.  This is not constant throughout the lifetime
> +     of this frame, so we don't use it to identify the frame, just to find
> +     the function.  */
> +  CORE_ADDR unwound_pc;

Hmm, ``unwound_sp'' belongs to the PREV frame but ``unwound_pc'' belongs 
to THIS frame :-(

Suggest instead using a function local ``this_pc'' variable and not even 
cacheing the THIS frame's PC value.  frame_pc_unwind(next_frame) is 
effecient (it already caches the PC) and in the past bad kama has 
resulted from redundant caching of pc values (cf deprecated update frame 
pc hack).

> +  /* The frame base for this frame is just unwound_sp + frame offset - frame
> +     size.  FRAMESIZE is the size of the stack frame, and FRAMEOFFSET
> +     if the initial offset from the stack pointer (our stack pointer, not
> +     UNWOUND_SP) to the frame base.  */
> +
>    int framesize;
>    int frameoffset;

Um, which SP?  THIS or PREV?

> +  /* The register used to hold the frame pointer for this frame.  */
>    int framereg;
> -  CORE_ADDR saved_regs[1];
> +
> +  /* Saved register offsets.  */
> +  struct trad_frame_saved_reg *saved_regs;
>  };
>  
>  /* Addresses for calling Thumb functions have the bit 0 set.

> +  /* Check that we're not going round in circles with the same frame
> +     ID (but avoid applying the test to sentinel frames which do go
> +     round in circles).  */

This shouldn't be needed.   Hmm, ah, I never enabled (from frame.c):

  /* Check that this and the next frame are different.  If they are
     not, there is most likely a stack cycle.  As with the inner-than
     test, avoid the inner-most and sentinel frames.  */
  /* FIXME: cagney/2003-03-17: Can't yet enable this this check. The
     frame_id_eq() method doesn't yet use function addresses when
     comparing frame IDs.  */
  if (0
      && this_frame->level > 0
      && frame_id_eq (get_frame_id (this_frame),
                      get_frame_id (this_frame->next)))
    error ("This frame identical to next frame (corrupt stack?)");

> +  if (frame_relative_level (next_frame) >= 0
> +      && get_frame_type (next_frame) == NORMAL_FRAME
> +      && frame_id_eq (get_frame_id (next_frame), id))
> +    return;


> +  if (*this_cache == NULL)
> +    *this_cache = arm_make_prologue_cache (next_frame);
> +  cache = *this_cache;
> +
> +  /* If we are asked to unwind the PC, then we need to return the LR instead.
> +     The saved value of PC points into this frame's prologue, not the
> +     next frame's resume location.  */
> +  if (prev_regnum == ARM_PC_REGNUM)
> +    prev_regnum = ARM_LR_REGNUM;

See above, eliminating unwound_pc would get rid of the confusion.

> +  /* If someone asks for the stack pointer, then they want unwound_sp,
> +     which was our stack pointer at the time of the call.  SP is not
> +     generally saved to the stack.  */

(I can't stand the GNU style of using first and second person in code, 
it's way too ambigious :-()  I'd just state:

``The PREV frame's stack pointer was previously computed and saved in 
cache->prev_sp.  The SP is not generally saved to the stack.''

> +  if (prev_regnum == ARM_SP_REGNUM)
> +    {
> +      *lvalp = not_lval;
> +      if (valuep)
> +	store_unsigned_integer (valuep, 4, cache->unwound_sp);
> +      return;
> +    }
>  


> +static const struct frame_unwind *
> +arm_sigtramp_unwind_p (CORE_ADDR pc)
> +{
> +  /* Note: If an ARM PC_IN_SIGTRAMP method ever needs to compare
> +     against the name of the function, the code below will have to be
> +     changed to first fetch the name of the function and then pass
> +     this name to PC_IN_SIGTRAMP.  */
>  
> -  memcpy (get_frame_extra_info (fi), cache,  (sizeof (struct arm_prologue_cache)
> -					      + ((NUM_REGS + NUM_PSEUDO_REGS - 1)
> -						 * sizeof (CORE_ADDR))));
> -  memcpy (get_frame_saved_regs (fi), cache->saved_regs,
> -	  (NUM_REGS + NUM_PSEUDO_REGS - 1) * sizeof (CORE_ADDR));
> -}
> +  if (SIGCONTEXT_REGISTER_ADDRESS_P () && PC_IN_SIGTRAMP (pc, (char *) 0))
> +    return &arm_sigtramp_unwind;

Can PC_IN_SIGTRAMP be eliminated here?

> +/* Assuming NEXT_FRAME->prev is a dummy, return the frame ID of that
> +   dummy frame.  The frame ID's base needs to match the TOS value
> +   saved by save_dummy_frame_tos(), and the PC match the dummy frame's
> +   breakpoint.  */

... tos() and returned by arm_push_dummy_call.


> +/* Given THIS_FRAME, find the frame's resume PC (which will be part of the
> +   frame ID for THIS_FRAME's caller's frame).  */

Given THIS_FRAME, find the PREV frame's resume PC (which will be used to 
find the PREV frame's function and that in turn used to construct the 
PREV frame's ID).

>  static CORE_ADDR
> -arm_read_fp (void)
> +arm_unwind_pc (struct gdbarch *gdbarch, struct frame_info *this_frame)
>  {
> -  if (read_register (ARM_PS_REGNUM) & 0x20)	/* Bit 5 is Thumb state bit */
> -    return read_register (THUMB_FP_REGNUM);	/* R7 if Thumb */
> -  else
> -    return read_register (ARM_FP_REGNUM);	/* R11 if ARM */
> +  CORE_ADDR pc;
> +  pc = frame_unwind_register_unsigned (this_frame, ARM_PC_REGNUM);
> +  return IS_THUMB_ADDR (pc) ? UNMAKE_THUMB_ADDR (pc) : pc;
>  }

Nice,
Andrew