From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 31953 invoked by alias); 17 May 2005 13:32:16 -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 30417 invoked from network); 17 May 2005 13:31:38 -0000 Received: from unknown (HELO nevyn.them.org) (66.93.172.17) by sourceware.org with SMTP; 17 May 2005 13:31:38 -0000 Received: from drow by nevyn.them.org with local (Exim 4.50) id 1DY2Ab-0000wl-SZ for gdb-patches@sources.redhat.com; Tue, 17 May 2005 09:31:37 -0400 Date: Tue, 17 May 2005 13:52:00 -0000 From: Daniel Jacobowitz To: gdb-patches@sources.redhat.com Subject: Re: [RFA] Resurrect v850 Message-ID: <20050517133137.GA3543@nevyn.them.org> Mail-Followup-To: gdb-patches@sources.redhat.com References: <20050513114016.GN2805@calimero.vinschen.de> <20050515174426.GA1193@nevyn.them.org> <20050517132348.GF18174@calimero.vinschen.de> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20050517132348.GF18174@calimero.vinschen.de> User-Agent: Mutt/1.5.8i X-SW-Source: 2005-05/txt/msg00418.txt.bz2 On Tue, May 17, 2005 at 03:23:48PM +0200, Corinna Vinschen wrote: > Hi Daniel, > > On May 15 13:44, Daniel Jacobowitz wrote: > > On Fri, May 13, 2005 at 01:40:16PM +0200, Corinna Vinschen wrote: > > > Hi, > > > > > > the below patch resurrects v850. No deprecated mechanisms are used. > > > As promised, this target now uses trad_frames ;-) > > > > > > Ok to apply? > > > > Some comments... > > While reworking the code according to your comments, I came across three > problems: > > > > +enum { > > > + E_R0_REGNUM, > > > + E_R1_REGNUM, > > > + E_R2_REGNUM, E_SAVE1_START_REGNUM = E_R2_REGNUM, E_SAVE1_END_REGNUM = E_R2_REGNUM, > > > + E_R3_REGNUM, E_SP_REGNUM = E_R3_REGNUM, > > > > Several of the lines in this list are too long. I do see how you were > > trying to organize it, but if you put one to a line the lines with > > equality operators will still stand out. > > Is a layout like this: > > E_R1_REGNUM, > E_R2_REGNUM, E_SAVE1_START_REGNUM = E_R2_REGNUM, > E_SAVE1_END_REGNUM = E_R2_REGNUM, > E_R3_REGNUM, E_SP_REGNUM = E_R3_REGNUM, > [...] > > ok? I was originally going to suggest that - but no, it isn't. gdb_indent.sh will promptly undo this the next time someone needs to correct indentation in the file. That's the problem with machine-enforced standards... > > > The comments in this function are seriously un-encouraging. Is this > > excess space necessary or not? If it is, why? > > I don't know. The ABI is defined this way and inspecting a lot of code > I didn't see any reason for this behaviour. Nevertheless, gcc emits > code which allocates these 16 byte for no apparent reason. I removed > useless comments and the remaining comment now read like this: > > /* The offset onto the stack at which we will start copying parameters > (after the registers are used up) begins at 16 rather than at zero. > That's how the ABI is defined, though there's no indication that these > 16 bytes are used for anything, not even for saving incoming > argument registers. */ > > Is that ok? Yes, definitely. > > > > +static void > > > +v850_frame_prev_register (struct frame_info *next_frame, void **this_cache, > > > + int regnum, int *optimizedp, > > > + enum lval_type *lvalp, CORE_ADDR *addrp, > > > + int *realnump, void *valuep) > > > +{ > > > + struct v850_frame_cache *cache = v850_frame_cache (next_frame, this_cache); > > > + > > > + gdb_assert (regnum >= 0); > > > + > > > + /* The PC of the previous frame is stored in the PR register of > > > + the current frame. Frob regnum so that we pull the value from > > > + the correct place. */ > > > + if (regnum == E_PC_REGNUM) > > > + regnum = E_LP_REGNUM; > > > + > > > + trad_frame_get_prev_register (next_frame, cache->saved_regs, regnum, > > > + optimizedp, lvalp, addrp, realnump, valuep); > > > +} > > > > You can do the frobbing when you record the saved registers - at the > > end, copy the saved location. > > Sorry, I don't understand this. Can you show me some (pseudo) code what > you mean? I believe that you can do "cache->saved_regs[E_PC_REGNUM] = cache->saved_regs[E_LP_REGNUM]", right by the call to trad_frame_addr_p. Does that work? -- Daniel Jacobowitz CodeSourcery, LLC