Re: [RFC] TI msp430 architecture support

[Kevin Buettner <kevinb at redhat dot com>]

On Fri, 17 May 2013 10:29:11 +0400
Joel Brobecker <brobecker@adacore.com> wrote:

> > gdb/ChangeLog:
> > 
> > 	* Makefile.in (ALL_TARGET_OBS): Add msp430-tdep.o.
> > 	(ALLDEPFILES): Add msp430-tdep.c.
> > 	* configure.tgt (msp430*-*-elf): New target.
> > 	* msp430-tdep.c: New file.
> 
> Overall, this looks pretty good to me. My comments below, which
> are almost exclusively related to coding style. Feel free to fix
> and commit...

Thanks for the review, Joel.

I fixed the formatting issues that you noticed and then, for
good measure, I ran the file through gdb_indent.sh.  I should've
run gdb_indent.sh over msp430-tdep.c prior to submitting it.

I've also added the comments that you requested.

I'll address some of your other comments below...

> > +/* Implement the "register_name" gdbarch method.  */
> > +
> > +static const char *
> > +msp430_register_name (struct gdbarch *gdbarch, int regnr)
> > +{
> > +  static const char *const reg_names[] =
> > +  {
> > +    /* Raw registers.  */
> > +    "",
> > +    "",
> > +    "",
> > +    "",
> > +    "",
> > +    "",
> > +    "",
> > +    "",
> > +    "",
> > +    "",
> > +    "",
> > +    "",
> > +    "",
> > +    "",
> > +    "",
> > +    "",
> > +    /* Pseudo registers.  */
> > +    "pc",
> > +    "sp",
> > +    "sr",
> > +    "cg",
> > +    "r4",
> > +    "r5",
> > +    "r6",
> > +    "r7",
> > +    "r8",
> > +    "r9",
> > +    "r10",
> > +    "r11",
> > +    "r12",
> > +    "r13",
> > +    "r14",
> > +    "r15"
> > +  };
> > +
> > +  return reg_names[regnr];
> > +}
> 
> This one surprised me a little. My understanding is that you expect
> this function to never be called with raw register numbers? If that's
> the case, how about adding an assert? and only create an array with
> the pseudo registers?

It's important that either NULL or "" is returned for either
unimplemented register numbers or numbers that we don't want
to show up in "info registers" and other register output.  
mi_cmd_data_list_register_names() requires this behavior, for
example.

> That's a infinitely small detail, but considering the size of
> the strings, I personally would have joined the register names
> into 1 or 2 lines. But that's a matter of personal preference,
> so do feel free to keep it the way it is.

I agree with this. The function looks like this now:

static const char *
msp430_register_name (struct gdbarch *gdbarch, int regnr)
{
  static const char *const reg_names[] =
  {
    /* Raw registers.  */
    "", "", "", "", "", "", "", "",
    "", "", "", "", "", "", "", "",
    /* Pseudo registers.  */
    "pc", "sp", "sr", "cg", "r4", "r5", "r6", "r7",
    "r8", "r9", "r10", "r11", "r12", "r13", "r14", "r15"
  };

  return reg_names[regnr];
}

> > +/* Implement the "pseudo_register_read" gdbarch method.  */
> > +
> > +static enum register_status
> > +msp430_pseudo_register_read (struct gdbarch *gdbarch,
> > +                             struct regcache *regcache,
> > +                             int regnum, gdb_byte *buffer)
> > +{
> > +  enum register_status status = REG_UNKNOWN;
> > +
> > +  if (MSP430_NUM_REGS <= regnum && regnum < MSP430_NUM_TOTAL_REGS)
> > +    {
> > +      ULONGEST val;
> > +      enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
> > +      int regsize = register_size (gdbarch, regnum);
> > +      int raw_regnum = regnum - MSP430_NUM_REGS;
> > +
> > +      status = regcache_raw_read_unsigned (regcache, raw_regnum, &val);
> > +      if (status == REG_VALID)
> > +	store_unsigned_integer (buffer, regsize, byte_order, val);
> > +
> > +    }
> > +  else
> > +    gdb_assert_not_reached ("invalid pseudo register number");
> > +
> > +  return status;
> > +}
> 
> Interesting that you chose to use the "else gdb_assert_not_reached"
> idiom, instead of the usual gdb_assert at the start of the function.
> I like the fact that you do get a slightly more meaningful message
> that way. How about adding the register number as well? There are
> other locations where this can be applied as well, if you like the
> suggestion.

I copied the gdb_assert_not_reached from another port.  I can't really
claim credit for thinking of using it here.

I do like your idea of using a more meaningful message using the
register number, but this would require changing gdb_assert_not_reached()
so that it'll handle a printf type argument list.  Alternately, we
could constuct a suitable string to pass to gdb_assert_not_reached,
but this seems a bit excessive for handling a case that we don't
expect to ever happen.  (When things like this do happen, I place
a breakpoint in the assert code and then go up the necessary number
of frames to see the values of the variables which triggered it.)

> > +  /* Record where all the registers were saved.  */
> > +  pv_area_scan (stack, check_for_saved, (void *) result);
> 
> Do you really need the cast, here?

Nope.  I removed it.  (I'm pretty sure that I copied it from somewhere
else too.)

> > +static int
> > +msp430_dwarf2_reg_to_regnum (struct gdbarch *gdbarch, int reg)
> > +{
> > +  if (reg < MSP430_NUM_REGS)
> > +    return reg + MSP430_NUM_REGS;
> > +  else
> > +    internal_error (__FILE__, __LINE__,
> > +                    _("Undefined dwarf2 register mapping of reg %d"),
> > +		    reg);
> 
> I wouldn't raise an internal_error in this case, because the invalid
> register number could come from outside sources. I'd follow what we do
> for x86_64 and return -1 instead, after having emitted a warning. See
> for instance amd64-tdep.c:amd64_dwarf_reg_to_regnum.
> 
> Our error handing isn't great regardless, and probably we should start
> thinking about what the function is expected to do in case of invalid
> register number. But that's for another patch...

I agree.  The function in question now looks like this:

static int
msp430_dwarf2_reg_to_regnum (struct gdbarch *gdbarch, int reg)
{
  if (reg < MSP430_NUM_REGS)
    return reg + MSP430_NUM_REGS;
  else
    {
      warning (_("Unmapped DWARF Register #%d encountered."), reg);
      return -1;
    }
}

Thanks again for your review.

I've committed msp430-tdep.c and related changes to the configury.  I'll write
up a NEWS entry as requested by Pedro.

Kevin