Re: [RFC] Add v850e3v5 and RH850 ABI support to v850-tdep.c

[Joel Brobecker <brobecker at adacore dot com>]

Hi Kevin,

On Thu, Apr 25, 2013 at 04:16:22PM -0700, Kevin Buettner wrote:
> As stated in the subject line, this patch adds v850e3v5 and RH850 ABI
> support to v850-tdep.c.
> 
> I'll wait a few days for comments before checking it in.
> 
> Kevin
> 
> 	* v850-tdep.c (elf-bfd.h, elf/v850.h): Include.
> 	(R_149_REGNUM, E_NUM_OF_V850E2_REGS, E_SELID_1_R0_REGNUM)
> 	(E_SELID_1_R31_REGNUM, E_SELID_2_R0_REGNUM, E_SELID_2_R31_REGNUM)
> 	(E_SELID_3_R0_REGNUM, E_SELID_3_R31_REGNUM, E_SELID_4_R0_REGNUM)
> 	(E_SELID_4_R31_REGNUM, E_SELID_5_R0_REGNUM, E_SELID_5_R31_REGNUM)
> 	(E_SELID_6_R0_REGNUM, E_SELID_6_R31_REGNUM, E_SELID_7_R0_REGNUM, E_SELID_7_R31_REGNUM)
> 	(E_VR0_REGNUM, E_VR31_REGNUM, E_NUM_OF_V850E3V5_REGS): Define.
> 	(v850_abi, V850_ABI_GCC, V850_ABI_RH850): New enum and constants.
> 	(gdbarch_tdep): New struct.
> 	(v850e2_register_name): Use E_NUM_OF_V850E2_REGS instead of
> 	E_NUM_REGS.
> 	(v850e3v5_register_name): New function.
> 	(v850_register_type): v850e3v5 vector registers are 64-bits wide.
> 	(v850_use_struct_convention): Add `gdbarch' parameter.  Add new
> 	code handling the struct return conventions for the RH850 ABI.
> 	Update all callers.
> 	(v850_eight_byte_align_p): New function.
> 	(v850_push_call_dummy): Push structs by value, not by reference
> 	for the RH850 ABI.  Add support for eight byte alignment.
> 	(v850_dbtrap_breakpoint_from_pc): New function.
> 	(v850_gdbarch_init): Add ABI detection code.  Register
> 	v850e3v5_register_name for the v850e3v5 architecture.  Set the
> 	number of registers for v850e3v5.  Register
> 	v850_dbtrap_breakpoint_from_pc as appropriate.
> 	(_initialize_gdbarch_init): Registration bfd_arch_v850_rh850.

I only have comments regarding coding style...

>  /* Should call_function allocate stack space for a struct return?  */
>  static int
> -v850_use_struct_convention (struct type *type)
> +v850_use_struct_convention (struct gdbarch *gdbarch, struct type *type)

There should be an empty line between the function documentation
and its implementation. Would you mind adding it here, while you
are at it?

> +/* Return 1 if the data structure has any 8-byte fields that'll require
> +   the entire data structure to be aligned.  Otherwise, return 0.  */
> +static int
> +v850_eight_byte_align_p (struct type *type)

Empty line after function documentation. While looking at this piece,
just letting you know that we'd like lines to be 70 characters long
or shorter, if possible. The hard-limit is 80 characters... There are
several instances in your patch where the comment width is exceeding
the 70 characters, and some might be easier than others for you to
fix...

> +/* Implement software breakpoints by using the dbtrap instruction.  Older
> +   architectures had no such instruction.  For those, an unconditional branch
> +   to self instruction is used.  */
> +const static unsigned char *
> +v850_dbtrap_breakpoint_from_pc (struct gdbarch *gdbarch, CORE_ADDR *pcptr, int *lenptr)

Same as above. Also, the last line is too long.

> +{
> +  static unsigned char breakpoint[] = { 0x40, 0xf8 };
> +  *lenptr = sizeof (breakpoint);

Can you add an empty line after the variable declarations?


-- 
Joel