From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 26773 invoked by alias); 1 Jul 2003 15:32:56 -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 26743 invoked from network); 1 Jul 2003 15:32:50 -0000 Received: from unknown (HELO localhost.redhat.com) (24.157.166.107) by sources.redhat.com with SMTP; 1 Jul 2003 15:32:50 -0000 Received: from redhat.com (localhost [127.0.0.1]) by localhost.redhat.com (Postfix) with ESMTP id 868F52B5F; Tue, 1 Jul 2003 11:32:48 -0400 (EDT) Message-ID: <3F01A9A0.90005@redhat.com> Date: Tue, 01 Jul 2003 15:32:00 -0000 From: Andrew Cagney User-Agent: Mozilla/5.0 (X11; U; NetBSD macppc; en-US; rv:1.0.2) Gecko/20030223 X-Accept-Language: en-us, en MIME-Version: 1.0 To: Daniel Jacobowitz Cc: gdb-patches@sources.redhat.com, rearnsha@arm.com Subject: Re: [RFA/ARM] Framificate the ARM port [3/3] References: <20030630225509.GA30844@nevyn.them.org> Content-Type: text/plain; charset=us-ascii; format=flowed Content-Transfer-Encoding: 7bit X-SW-Source: 2003-07/txt/msg00012.txt.bz2 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