Re: [RFC] Add support for Morpho ms1 processor

[Mark Kettenis <mark dot kettenis at xs4all dot nl>]

   Date: Fri, 3 Jun 2005 17:20:38 -0700
   From: Kevin Buettner <kevinb@redhat.com>

   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?

There are a few coding-style violations, mostly regarding multi-line
comment style.  There is some excessive spacing too; the GNU coding
standards seem to suggest using ^L to seperate larger chunks of code,
not multiple lines of whitespace.  I also really would like to
encourage the usage of target-specific (ms1_) prefixes for function
and variable names.  Especially naming an enum `gdb_regnum' seems like
a bad idea to me.  What's the E_-prefix trying to convey?  I seem to
remember a few other embedded processors using the same E_-prefix for
their register names.  Internationalization of strings is missing too.

Can we somehow make sure this doesn't become yet another
commit-and-abondon target?

A few more comments down in the code.

   +
   +/* Function: register_name
   +   Returns the name of the standard ms1 register N.  */

Stating the function name in the comment here doesn't make sense.  The
GNU coding standards certainly don't suggest it.

   +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 *);
   +
   ...
   +  /* 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;

This doesn't do anything...

   +
   +  set_gdbarch_register_name (gdbarch, ms1_register_name);
   +  set_gdbarch_num_regs      (gdbarch, E_NUM_REGS);
   +  set_gdbarch_num_pseudo_regs (gdbarch, E_NUM_PSEUDO_REGS);
   +  set_gdbarch_pc_regnum (gdbarch, E_PC_REGNUM);
   +  set_gdbarch_sp_regnum (gdbarch, E_SP_REGNUM);
   +  set_gdbarch_pseudo_register_read  (gdbarch, ms1_pseudo_register_read);
   +  set_gdbarch_pseudo_register_write (gdbarch, ms1_pseudo_register_write);
   +  set_gdbarch_skip_prologue (gdbarch, ms1_skip_prologue);
   +  set_gdbarch_inner_than    (gdbarch, core_addr_lessthan);
   +  set_gdbarch_breakpoint_from_pc    (gdbarch, ms1_breakpoint_from_pc);
   +  set_gdbarch_decr_pc_after_break   (gdbarch, 0);
   +  set_gdbarch_frame_args_skip (gdbarch, 0);
   +  set_gdbarch_print_insn      (gdbarch, print_insn_ms1);
   +  set_gdbarch_register_type   (gdbarch, ms1_register_type);
   +  set_gdbarch_register_reggroup_p (gdbarch, ms1_register_reggroup_p);

Don't try to line things up here; there should only be a single space.

   +   * Target builtin data types.
   +   */
   +  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);

Personally I think this TARGET_CHAR_BIT is really silly, as if we're
going to support something other than 8.  I really thing hardcoding
the numbers is much clearer.  A `short' is 16 bits, not 2 times some
arbitrary constant.

   +
   +static void
   +ms1_frame_unwind_init (struct gdbarch *gdbarch)
   +{
   +  /* This is the keystone.  You add your "sniffer" to a 
   +     sniffer list, and now you're in the game.  This registers
   +     a) the sniffer, b) the 'prev_register', and c) the 'this_id'
   +     methods.
   +  */

That last */ defenitely shouldn't sit alone on a line.

Mark