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

[Daniel Jacobowitz <drow at mvista dot com>]

On Tue, Jul 01, 2003 at 11:32:48AM -0400, Andrew Cagney wrote:
> 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).

Thanks for the comments.

> >===================================================================
> >--- 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''.

Hmm, OK.

> 
> >+  /* 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).

Makes sense.  I will need to cache a PC to fix some corner cases with
signal trampolines, but I'm not fixing that bug right now.

> >+  /* 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?

Like the comment says, THIS, not PREV.  I seem to be more comfortable
with different terminology than you use...  Wording updated.

> >+  /* 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:

I find it a convenient way to write comments that don't have enough
passive voice to make an English major cry.

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

  /* SP is generally not saved to the stack, but this frame is
     identified by NEXT_FRAME's stack pointer at the time of the call.
     The value was already reconstructed into PREV_SP.  */

> >+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?

Not trivially, arm/tm-linux.h defines IN_SIGTRAMP... it could be done
separately.

> >+/* 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.

Copied this comment verbatim from three other implementations.  You
might want to update those.

> >+/* 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).

Eh?  Uch, I suppose.  "resume PC" is a bit ambiguous.

The reason that I like my notation better is that I don't need to think
about what "this frame's caller" means.  But every time I see NEXT or
PREV I have to take five seconds to work it out.  Caller is
unambiguous.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer