Re: [RFC] Add support for the Renesas rl78 architecture

[Kevin Buettner <kevinb at redhat dot com>]

Hi Yao,

Thanks for looking at my patch.

With regard to the blank lines after declarations, I've made those changes
to rl78-tdep.c.  I will post an update in a few minutes in case you'd like
to spot check this.

I'll address your other concerns below...

On Sun, 29 Jan 2012 17:48:26 +0800
Yao Qi <yao@codesourcery.com> wrote:

> On 01/26/2012 07:58 AM, Kevin Buettner wrote:
>
> > +/* Strip bits to form an instruction address.  (When fetching a
> > +   32-bit address from the stack, the high eight bits are garbage.
> > +   This function strips off those unused bits.)  */
> > +static CORE_ADDR
> > +rl78_make_instruction_address (CORE_ADDR addr)
> > +{
> > +  return addr & 0xffffff;
> > +}
> > +
> > +/* Set / clear bits necessary to make a data address.  */
> > +static CORE_ADDR
> > +rl78_make_data_address (CORE_ADDR addr)
> > +{
> > +  return (addr & 0xffff) | 0xf0000;
> > +}
> > +
> 
> Why can't we use macro instead of function here?

We could use a macro here.  When I've attempted to use macros in the
past, I was told "Macros are bad, M'kay."  The GDB internals doc even
says this.  (That's an exact quote.)  See section 16.1.4:

http://sourceware.org/gdb/current/onlinedocs/gdbint/Coding-Standards.html

I think today's compiler technology makes macro use for efficiency
reasons much less compelling than it once was.

> > +/* Implement the "breakpoint_from_pc" gdbarch method.  */
> > +const gdb_byte *
> > +rl78_breakpoint_from_pc (struct gdbarch *gdbarch, CORE_ADDR *pcptr, int *lenptr)
> > +{
> > +  static gdb_byte breakpoint[] = { 0xff };
> > +
> > +  /* The above are the bytes required for the BRK instruction.  However,
> > +     instructions can be as short as one byte.  The simulator looks for
> > +     memory writes to program areas using this pattern, however, and
> > +     implements breakpoints via a different mechanism.  */
> 
> The code is clear to me, but the comment here is confusing.
> 
> > +  *lenptr = sizeof breakpoint;
> > +  return breakpoint;
> > +}

Thank you for catching this.  I used to have a two byte breakpoint
sequence there.  We later learned of a one byte sequence that could
be used and I forgot to update the comment.

I've revised that function to appear as shown below...

/* Implement the "breakpoint_from_pc" gdbarch method.  */

const gdb_byte *
rl78_breakpoint_from_pc (struct gdbarch *gdbarch, CORE_ADDR *pcptr,
                         int *lenptr)
{
  /* The documented BRK instruction is actually a two byte sequence,
     {0x61, 0xcc}, but instructions may be as short as one byte.
     Correspondence with Renesas revealed that the one byte sequence
     0xff is used when a one byte breakpoint instruction is required.  */
  static gdb_byte breakpoint[] = { 0xff };

  *lenptr = sizeof breakpoint;
  return breakpoint;
}

> > +/* Implement the "pointer_to_address" gdbarch method.  */
> > +static CORE_ADDR
> > +rl78_pointer_to_address (struct gdbarch *gdbarch,
> > +                         struct type *type, const gdb_byte *buf)
> > +{
> > +  enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
> > +  CORE_ADDR addr
> > +    = extract_unsigned_integer (buf, TYPE_LENGTH (type), byte_order);
> > +
> > +  /* Is it a code address?  */
> > +  if (TYPE_CODE (TYPE_TARGET_TYPE (type)) == TYPE_CODE_FUNC
> > +      || TYPE_CODE (TYPE_TARGET_TYPE (type)) == TYPE_CODE_METHOD
> > +      || TYPE_CODE_SPACE (TYPE_TARGET_TYPE (type))
> > +      || TYPE_LENGTH (type) == 4
> 
> If a pointer points to an integer (it is a data address, and size is
> 4-byte), we will get the instruction address, which is not correct, IMO.

I agree with what you're saying, but I'm checking the size of the pointer
here.  (Well, that's my intention anyway.)  Code pointers can be 4 bytes
in size.  Data pointers are 2 bytes in size.

> > +      )
> 
> I think this parenthesis should be put in previous line, IMO.

I agree.  Done.

> > +static CORE_ADDR
> > +rl78_unwind_pc (struct gdbarch *arch, struct frame_info *next_frame)
> > +{
> > +  struct gdbarch_tdep *tdep = gdbarch_tdep (arch);
> 
> `tdep' is not used.

I've removed this declaration as well as the other unused tdep declaration
that you found too.

Thanks again for your detailed review.  It is much appreciated.

Kevin