Re: [RFA] Resurrect v850

Daniel Jacobowitz
2005-05-15T17:44:41Z

On Fri, May 13, 2005 at 01:40:16PM +0200, Corinna Vinschen wrote:
> Hi,
>
> the below patch resurrects v850. No deprecated mechanisms are used.
> As promised, this target now uses trad_frames ;-)
>
> Ok to apply?

Some comments...

> +enum {
> + E_R0_REGNUM,
> + E_R1_REGNUM,
> + E_R2_REGNUM, E_SAVE1_START_REGNUM = E_R2_REGNUM, E_SAVE1_END_REGNUM = E_R2_REGNUM,
> + E_R3_REGNUM, E_SP_REGNUM = E_R3_REGNUM,

Several of the lines in this list are too long. I do see how you were
trying to organize it, but if you put one to a line the lines with
equality operators will still stand out.

> +static char *v850_generic_reg_names[] =

This and the two below should be const.

> +struct
> + {
> + char **regnames;
> + int mach;
> + }
> +v850_processor_type_table[] =
> +{
> + {
> + v850_generic_reg_names, bfd_mach_v850
> + }
> + ,

Please compress these; all you need is:
{ v850_generic_reg_names, bfd_mach_v850 },

> +/* Function: v850_register_name
> + Returns the name of the v850/v850e register N. */

Two spaces after all the periods in comments (throughout the file).
You don't really need the first line either; it's right above the
function already.

> +static const char *
> +v850_register_name (int regnum)
> +{
> + if (regnum < 0 || regnum >= E_NUM_REGS)
> + internal_error (__FILE__, __LINE__,
> + "v850_register_name: illegal register number %d",
> + regnum);

Please don't add new untranslated messages.

Of course, the i18n changes could have been structured so that the
arguments to these functions were translated by default, without all
the _() markers. Another thing that got lost in the rush to add the
markup.

> + /* According to ABI:
> + * return TYPE_LENGTH (type) > 8);
> + */

Please use full sentences in comments when possible (all through this
file). Also please don't use the leading * on every line; that's
not how GDB does it.

> +#ifdef DEBUG
> + printf_filtered ("\tSaved register r%d, offset %d", reg, pifsr->offset);
> +#endif

Please either delete the #ifdef DEBUG bits, or make them a runtime
option under "set debug", depending on how useful they are.

> +/* Helper function for v850_scan_prologue to handle pushm/pushl instructions.
> + FIXME: the SR bit of the register list is not supported; must check
> + that the compiler does not ever generate this bit. */

Did you check that? :-)

> + else if ((insn & 0xffe0) == ((E_SP_REGNUM << 11) | 0x0240)) /* add <imm>,sp */

Long lines again.

> + /* First, just for safety, make sure stack is aligned */
> + sp = v850_frame_align (gdbarch, sp);

The common code already does this.

> + /* The offset onto the stack at which we will start copying parameters
> + (after the registers are used up) begins at 16 rather than at zero.
> + I don't really know why, that's just the way it seems to work. */
> + stack_offset = 16;
> +
> + /* Now make space on the stack for the args. */
> + for (argnum = 0; argnum < nargs; argnum++)
> + len += ((TYPE_LENGTH (value_type (args[argnum])) + 3) & ~3);
> + sp -= len + stack_offset; /* possibly over-allocating, but it works... */
> + /* (you might think we could allocate 16 bytes */
> + /* less, but the ABI seems to use it all! ) */

The comments in this function are seriously un-encouraging. Is this
excess space necessary or not? If it is, why? At a guess, the extra
space is used by the called function to save the incoming argument
registers if necessary; MIPS o32 does something similar.

> + /* Now load as many as possible of the first arguments into
> + registers, and push the rest onto the stack. There are 16 bytes
> + in four registers available. Loop thru args from first to last. */
> + for (argnum = 0; argnum < nargs; argnum++)
> + {
> + int len;
> + char *val;
> + char valbuf[v850_reg_size];

We just spent weeks discussing the proper type for data buffers, and
char isn't it. Please use gdb_byte.

> +static void
> +v850_frame_prev_register (struct frame_info *next_frame, void **this_cache,
> + int regnum, int *optimizedp,
> + enum lval_type *lvalp, CORE_ADDR *addrp,
> + int *realnump, void *valuep)
> +{
> + struct v850_frame_cache *cache = v850_frame_cache (next_frame, this_cache);
> +
> + gdb_assert (regnum >= 0);
> +
> + /* The PC of the previous frame is stored in the PR register of
> + the current frame. Frob regnum so that we pull the value from
> + the correct place. */
> + if (regnum == E_PC_REGNUM)
> + regnum = E_LP_REGNUM;
> +
> + trad_frame_get_prev_register (next_frame, cache->saved_regs, regnum,
> + optimizedp, lvalp, addrp, realnump, valuep);
> +}

You can do the frobbing when you record the saved registers - at the
end, copy the saved location.

> +static struct gdbarch *
> +v850_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
> +{
> + struct gdbarch *gdbarch;
> + int i;
> +
> + /* find a candidate among the list of pre-declared architectures. */
> + arches = gdbarch_list_lookup_by_info (arches, &info);
> + if (arches != NULL)
> + return (arches->gdbarch);

This isn't right, see below...

> +
> + /* Change the register names based on the current machine type. */
> + if (info.bfd_arch_info->arch != bfd_arch_v850)
> + return 0;
> +
> + gdbarch = gdbarch_alloc (&info, 0);
> +
> + for (i = 0; v850_processor_type_table[i].regnames != NULL; i++)
> + {
> + if (v850_processor_type_table[i].mach == info.bfd_arch_info->mach)
> + {
> + v850_register_names = v850_processor_type_table[i].regnames;
> + break;
> + }
> + }

... here. You're setting a global variable in the process of choosing
a gdbarch. That global variable should be in gdbarch_tdep, along with
the criteria used to set it, and you should refuse to reuse an
architecture from the arches list that doesn't have the right
architecture. There are plenty of examples of this.

--
Daniel Jacobowitz
CodeSourcery, LLC