This is the mail archive of the
gdb-patches@sources.redhat.com
mailing list for the GDB project.
Re: [RFC] Add support for Morpho ms1 processor
- From: Daniel Jacobowitz <drow at false dot org>
- To: Kevin Buettner <kevinb at redhat dot com>
- Cc: gdb-patches at sources dot redhat dot com
- Date: Fri, 3 Jun 2005 20:40:40 -0400
- Subject: Re: [RFC] Add support for Morpho ms1 processor
- References: <20050603172038.449a41b9@ironwood.lan>
On Fri, Jun 03, 2005 at 05:20:38PM -0700, Kevin Buettner wrote:
> The patch below adds support for the Morpho Technologies ms1 processor
> to GDB. This code was written by Michael Snyder. (Though I and
> perhaps others have tweaked it here and there...)
>
> It's not quite ready to commit; there are still some bits at the top
> level, and in bfd/, opcodes/, and include/ that need to go in first.
> Aldy Hernandez is submitting those portions.
>
> However, it occurs to me that there may be a few things in this GDB
> portion that ought to be adjusted before it goes in, so I'm posting it
> now for comments.
>
> So... comments?
This isn't exhaustive, I skipped over a lot of pieces.
> + E_LAST_REG, /* NOTE: must be LAST
> + (following all "real" regs). */
> +
> + E_NUM_REGS = E_LAST_REG, /* number of real registers */
Something named E_LAST_REG shouldn't have a number greater than that of
any register...
> + /* No function symbol, or no line symbol.
> + Use prologue scanning method. FIXME use dwarf2 cfi! */
Um, since this target DOES use dwarf2 cfi, you shouldn't be adding
fixmes about it.
In general you probably oughtn't add any FIXMEs in a new port.
> +static struct gdbarch *
> +ms1_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
> +{
> + struct gdbarch *gdbarch;
> + static void ms1_frame_unwind_init (struct gdbarch *);
I don't even see a reason for this to be a separate function. If there
is a reason, it certainly shouldn't have a block-local prototype.
> + /* The MAC register for the MS1-16-003 is 8 bits larger
> + than it is for the other machine variants. */
> + switch (info.bfd_arch_info->mach) {
> + default:
> + case bfd_mach_ms1:
> + break;
> + }
That doesn't do anything. I don't know if you're submitting half of a
port, or if it used to do something for this port... but it doesn't
now. And it's misformatted to boot :-)
> + /*
> + * Target builtin data types.
> + */
Ditto on formatting; lots of this one.
> + set_gdbarch_short_bit (gdbarch, 2 * TARGET_CHAR_BIT);
> + set_gdbarch_int_bit (gdbarch, 4 * TARGET_CHAR_BIT);
> + set_gdbarch_long_bit (gdbarch, 4 * TARGET_CHAR_BIT);
> + set_gdbarch_long_long_bit (gdbarch, 8 * TARGET_CHAR_BIT);
> + set_gdbarch_float_bit (gdbarch, 4 * TARGET_CHAR_BIT);
> + set_gdbarch_double_bit (gdbarch, 8 * TARGET_CHAR_BIT);
> + set_gdbarch_long_double_bit (gdbarch, 8 * TARGET_CHAR_BIT);
> + set_gdbarch_ptr_bit (gdbarch, 4 * TARGET_CHAR_BIT);
Ditto.
> +/* nota bene:
> +
> +trad_frame_alloc_saved_regs (struct frame_info *);
> +trad_frame_prev_register
> +
> +*/
Hmm....
Also a meta-note: this is the third, or maybe fourth, port that Red Hat
has submitted recently which has a lot of copy-paste bits. There is
common code to do some of these things, but when I ask people to use
it, I get complaints that the common code doesn't work right. Copying
the same version that you like better into every new port is _not_ a
winning maintenance technique.
In general if there is something in your tdep file that contains
nothing specific to your target, it's in the wrong place. I'm
talking about you, find_last_line_symbol.
And please go over the comment formatting.
--
Daniel Jacobowitz
CodeSourcery, LLC