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

Kevin Buettner
2012-01-30T22:44:05Z

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