Re: [RFC] Add support for Morpho ms1 processor

[Daniel Jacobowitz <drow@false.org>]

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