From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 29442 invoked by alias); 15 May 2005 17:44:41 -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 29243 invoked from network); 15 May 2005 17:44:29 -0000 Received: from unknown (HELO nevyn.them.org) (66.93.172.17) by sourceware.org with SMTP; 15 May 2005 17:44:29 -0000 Received: from drow by nevyn.them.org with local (Exim 4.50) id 1DXNAA-0000VN-Az for gdb-patches@sources.redhat.com; Sun, 15 May 2005 13:44:26 -0400 Date: Sun, 15 May 2005 18:20:00 -0000 From: Daniel Jacobowitz To: gdb-patches@sources.redhat.com Subject: Re: [RFA] Resurrect v850 Message-ID: <20050515174426.GA1193@nevyn.them.org> Mail-Followup-To: gdb-patches@sources.redhat.com References: <20050513114016.GN2805@calimero.vinschen.de> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20050513114016.GN2805@calimero.vinschen.de> User-Agent: Mutt/1.5.8i X-SW-Source: 2005-05/txt/msg00378.txt.bz2 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... > +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. > +static char *v850_generic_reg_names[] = This and the two below should be const. > +struct > + { > + char **regnames; > + int mach; > + } > +v850_processor_type_table[] = > +{ > + { > + v850_generic_reg_names, bfd_mach_v850 > + } > + , Please compress these; all you need is: { v850_generic_reg_names, bfd_mach_v850 }, > +/* Function: v850_register_name > + Returns the name of the v850/v850e register N. */ Two spaces after all the periods in comments (throughout the file). You don't really need the first line either; it's right above the function already. > +static const char * > +v850_register_name (int regnum) > +{ > + if (regnum < 0 || regnum >= E_NUM_REGS) > + internal_error (__FILE__, __LINE__, > + "v850_register_name: illegal register number %d", > + regnum); Please don't add new untranslated messages. Of course, the i18n changes could have been structured so that the arguments to these functions were translated by default, without all the _() markers. Another thing that got lost in the rush to add the markup. > + /* According to ABI: > + * return TYPE_LENGTH (type) > 8); > + */ Please use full sentences in comments when possible (all through this file). Also please don't use the leading * on every line; that's not how GDB does it. > +#ifdef DEBUG > + printf_filtered ("\tSaved register r%d, offset %d", reg, pifsr->offset); > +#endif Please either delete the #ifdef DEBUG bits, or make them a runtime option under "set debug", depending on how useful they are. > +/* Helper function for v850_scan_prologue to handle pushm/pushl instructions. > + FIXME: the SR bit of the register list is not supported; must check > + that the compiler does not ever generate this bit. */ Did you check that? :-) > + else if ((insn & 0xffe0) == ((E_SP_REGNUM << 11) | 0x0240)) /* add ,sp */ Long lines again. > + /* First, just for safety, make sure stack is aligned */ > + sp = v850_frame_align (gdbarch, sp); The common code already does 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. > + I don't really know why, that's just the way it seems to work. */ > + stack_offset = 16; > + > + /* Now make space on the stack for the args. */ > + for (argnum = 0; argnum < nargs; argnum++) > + len += ((TYPE_LENGTH (value_type (args[argnum])) + 3) & ~3); > + sp -= len + stack_offset; /* possibly over-allocating, but it works... */ > + /* (you might think we could allocate 16 bytes */ > + /* less, but the ABI seems to use it all! ) */ The comments in this function are seriously un-encouraging. Is this excess space necessary or not? If it is, why? At a guess, the extra space is used by the called function to save the incoming argument registers if necessary; MIPS o32 does something similar. > + /* Now load as many as possible of the first arguments into > + registers, and push the rest onto the stack. There are 16 bytes > + in four registers available. Loop thru args from first to last. */ > + for (argnum = 0; argnum < nargs; argnum++) > + { > + int len; > + char *val; > + char valbuf[v850_reg_size]; We just spent weeks discussing the proper type for data buffers, and char isn't it. Please use gdb_byte. > +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. > +static struct gdbarch * > +v850_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches) > +{ > + struct gdbarch *gdbarch; > + int i; > + > + /* find a candidate among the list of pre-declared architectures. */ > + arches = gdbarch_list_lookup_by_info (arches, &info); > + if (arches != NULL) > + return (arches->gdbarch); This isn't right, see below... > + > + /* Change the register names based on the current machine type. */ > + if (info.bfd_arch_info->arch != bfd_arch_v850) > + return 0; > + > + gdbarch = gdbarch_alloc (&info, 0); > + > + for (i = 0; v850_processor_type_table[i].regnames != NULL; i++) > + { > + if (v850_processor_type_table[i].mach == info.bfd_arch_info->mach) > + { > + v850_register_names = v850_processor_type_table[i].regnames; > + break; > + } > + } ... here. You're setting a global variable in the process of choosing a gdbarch. That global variable should be in gdbarch_tdep, along with the criteria used to set it, and you should refuse to reuse an architecture from the arches list that doesn't have the right architecture. There are plenty of examples of this. -- Daniel Jacobowitz CodeSourcery, LLC