From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 2145 invoked by alias); 4 Jun 2005 00:40:47 -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 2130 invoked by uid 22791); 4 Jun 2005 00:40:42 -0000 Received: from nevyn.them.org (HELO nevyn.them.org) (66.93.172.17) by sourceware.org (qpsmtpd/0.30-dev) with ESMTP; Sat, 04 Jun 2005 00:40:42 +0000 Received: from drow by nevyn.them.org with local (Exim 4.50) id 1DeMiO-0002vj-4Y; Fri, 03 Jun 2005 20:40:40 -0400 Date: Sat, 04 Jun 2005 00:40:00 -0000 From: Daniel Jacobowitz To: Kevin Buettner Cc: gdb-patches@sources.redhat.com Subject: Re: [RFC] Add support for Morpho ms1 processor Message-ID: <20050604004040.GA11023@nevyn.them.org> Mail-Followup-To: Kevin Buettner , gdb-patches@sources.redhat.com References: <20050603172038.449a41b9@ironwood.lan> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20050603172038.449a41b9@ironwood.lan> User-Agent: Mutt/1.5.8i X-SW-Source: 2005-06/txt/msg00041.txt.bz2 On Fri, Jun 03, 2005 at 05:20:38PM -0700, Kevin Buettner wrote: > The patch below adds support for the Morpho Technologies ms1 processor > to GDB. This code was written by Michael Snyder. (Though I and > perhaps others have tweaked it here and there...) > > It's not quite ready to commit; there are still some bits at the top > level, and in bfd/, opcodes/, and include/ that need to go in first. > Aldy Hernandez is submitting those portions. > > However, it occurs to me that there may be a few things in this GDB > portion that ought to be adjusted before it goes in, so I'm posting it > now for comments. > > So... comments? This isn't exhaustive, I skipped over a lot of pieces. > + E_LAST_REG, /* NOTE: must be LAST > + (following all "real" regs). */ > + > + E_NUM_REGS = E_LAST_REG, /* number of real registers */ Something named E_LAST_REG shouldn't have a number greater than that of any register... > + /* No function symbol, or no line symbol. > + Use prologue scanning method. FIXME use dwarf2 cfi! */ Um, since this target DOES use dwarf2 cfi, you shouldn't be adding fixmes about it. In general you probably oughtn't add any FIXMEs in a new port. > +static struct gdbarch * > +ms1_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches) > +{ > + struct gdbarch *gdbarch; > + static void ms1_frame_unwind_init (struct gdbarch *); I don't even see a reason for this to be a separate function. If there is a reason, it certainly shouldn't have a block-local prototype. > + /* The MAC register for the MS1-16-003 is 8 bits larger > + than it is for the other machine variants. */ > + switch (info.bfd_arch_info->mach) { > + default: > + case bfd_mach_ms1: > + break; > + } That doesn't do anything. I don't know if you're submitting half of a port, or if it used to do something for this port... but it doesn't now. And it's misformatted to boot :-) > + /* > + * Target builtin data types. > + */ Ditto on formatting; lots of this one. > + set_gdbarch_short_bit (gdbarch, 2 * TARGET_CHAR_BIT); > + set_gdbarch_int_bit (gdbarch, 4 * TARGET_CHAR_BIT); > + set_gdbarch_long_bit (gdbarch, 4 * TARGET_CHAR_BIT); > + set_gdbarch_long_long_bit (gdbarch, 8 * TARGET_CHAR_BIT); > + set_gdbarch_float_bit (gdbarch, 4 * TARGET_CHAR_BIT); > + set_gdbarch_double_bit (gdbarch, 8 * TARGET_CHAR_BIT); > + set_gdbarch_long_double_bit (gdbarch, 8 * TARGET_CHAR_BIT); > + set_gdbarch_ptr_bit (gdbarch, 4 * TARGET_CHAR_BIT); Ditto. > +/* nota bene: > + > +trad_frame_alloc_saved_regs (struct frame_info *); > +trad_frame_prev_register > + > +*/ Hmm.... Also a meta-note: this is the third, or maybe fourth, port that Red Hat has submitted recently which has a lot of copy-paste bits. There is common code to do some of these things, but when I ask people to use it, I get complaints that the common code doesn't work right. Copying the same version that you like better into every new port is _not_ a winning maintenance technique. In general if there is something in your tdep file that contains nothing specific to your target, it's in the wrong place. I'm talking about you, find_last_line_symbol. And please go over the comment formatting. -- Daniel Jacobowitz CodeSourcery, LLC