Re: [RFA 3/5] New port: CR16: gdb port

Joel Brobecker
2012-10-04T14:48:25Z

> 2012-10-04 Kaushik Phatak <kaushik.phatak@kpitcummins.com>
>
> gdb/Changelog
> * cr16-linux-tdep.c: New file.
> * cr16-tdep.c: New file.
> * cr16-tdep.h: New file.

A good start :-). I am running out of time for today, but I noticed
a few things you can work on right away:

The biggie is that I think that there is a log of stuff in cr16-tdep
that is actually linux-specific, and that you should move to
cr16-linux-tdep.c. You'll see that it'll simplify your gdbarch_init
phase a bit (the linux one will probably build on top of the bareboard
one).

also, everything entity needs to be properly documented. This applies
to new types, new constants, global variables and functions. Therefore:

> +const gdb_byte breakpoint_uclinux[] = { 0xC7, 0x00 };

Needs a comment. For global variables and constants, I think that
the general practice is to put the comment on the line just before
the definition:

/* The breakpoint instruction to use when bla bla bla. */
const gdb_byte breakpoint_uclinux[] = { 0xC7, 0x00 };

For functions and types, however, we decided to have an empty line
between the documentation/comment and the function.

On formatting nits... (sorry - the GDB project is a little picky
about this, but for good reasons, IMO)

> +static void
> +cr16_uclinux_init_abi (struct gdbarch_info info,
> + struct gdbarch *gdbarch)

The last line isn't aligned properly.

> + /* The opcode of excp bpt is 0x00C8, however for uclinux we will use the
> + excp flg (0x00C7) to insert a breakpoint. The excp bpt requires external
> + hardware support for breakpoints to work on CR16 target. Software based
> + breakpoints are implemented in the kernel using excp flg and tested on
> + the SC14452 target. Use 0x00C7 with gdbserver/kernel and 0x00C8 for sim/ELF
> + We represent the breakpoint in little endian format since CR16 supports
> + only little endian.
> + */

The recommended line limit is 70, with a hard limit of 80 characters.
So this paragraph needs to be reformatted.

Also, the GNU Coding Style requires that 2 spaces be used after periods.
This applies for instance to the second line. Please also move the closing
"*/" to the end of the last line, with two spaces after the last period.
I will not highlight the other occurences of these formatting issues.
Can you make a pass over your code?

> +typedef unsigned short wordU;
> +extern wordU words[3];
> +extern ULONGLONG allWords;
> +extern ins currInsn;

This looks strange. Why extern, and where would they come from?
Needs a comment.

> +/* Use functions from opcodes/cr16-dis.c by making them non-static */
> +extern void make_instruction (void);
> +extern int match_opcode (void);
> +extern void get_words_at_PC (bfd_vma memaddr, struct disassemble_info *info);

Don't these functions have declarations in opcode? This is really ugly,
and potentially dangerous if the function's profile get changed at
some point in the future. Please consider adding these to a header
file in opcodes, and have both opcodes and GDB inclulde those, to
ensure consistency.

> +static const struct frame_unwind cr16_frame_unwind = {

Formatting nit: The '{' needs to be on the next line. There are probably
other instances of this issue.

> + /* We use different breakpoint instructions for ELF and uClinux.
> + See cr16-linux-tdep.c for more details.
> + */

Trailing space at the end of the second line. Please also move the closing
*/ to the second line as well.

> +extern const gdb_byte breakpoint_elf[];
> +extern const gdb_byte breakpoint_linux[];

I don't think these need to be in the .h file. I couldn't find
a reference to the breakpoint_linux entity, and I think that
breakpoint_elf can be declared static in cr16-tdep.c.

> +/* Target-dependent structure in gdbarch. */
> +/* Architecture specific data. */
> +struct gdbarch_tdep

One comment is enough :). And empty line between comment and
type definition.

> + /* The ELF header flags specify the multilib used. */
> + int elf_flags;
> +
> + const char *breakpoint; /* Breakpoint instruction. */

I'd rather you moved the last comment to just above the field
declaration, for consistency. And the type should probably use
gdb_bytes.

--
Joel