From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 21216 invoked by alias); 7 Jul 2003 13:47:46 -0000 Mailing-List: contact gdb-patches-help@sources.redhat.com; run by ezmlm Precedence: bulk List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sources.redhat.com Received: (qmail 21192 invoked from network); 7 Jul 2003 13:47:45 -0000 Received: from unknown (HELO crack.them.org) (146.82.138.56) by sources.redhat.com with SMTP; 7 Jul 2003 13:47:45 -0000 Received: from dsl093-172-017.pit1.dsl.speakeasy.net ([66.93.172.17] helo=nevyn.them.org ident=mail) by crack.them.org with asmtp (Exim 3.12 #1 (Debian)) id 19ZWMB-00060e-00; Mon, 07 Jul 2003 08:48:40 -0500 Received: from drow by nevyn.them.org with local (Exim 3.36 #1 (Debian)) id 19ZWKq-0006XX-00; Mon, 07 Jul 2003 09:47:16 -0400 Date: Mon, 07 Jul 2003 13:47:00 -0000 From: Daniel Jacobowitz To: Andrew Cagney Cc: gdb-patches@sources.redhat.com, rearnsha@arm.com Subject: Re: [RFA/ARM] Framificate the ARM port [3/3] Message-ID: <20030707134715.GA24634@nevyn.them.org> Mail-Followup-To: Andrew Cagney , gdb-patches@sources.redhat.com, rearnsha@arm.com References: <20030630225509.GA30844@nevyn.them.org> <3F01A9A0.90005@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <3F01A9A0.90005@redhat.com> User-Agent: Mutt/1.5.1i X-SW-Source: 2003-07/txt/msg00113.txt.bz2 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