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

Joel Brobecker
2012-10-22T22:41:17Z

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

This looks pretty good. It's nice to see that you're using
prologue-value...

My comments below:

> +static const char *
> +cr16_linux_register_name (struct gdbarch *gdbarch, int regnr)
> +{
> + static const char *const reg_names[] = {

Formatting: Could you move the '{' to the next line, please?

> + /* 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

Trailing space at the end of this line. Can you do a pass with your
editor, and make sure that they are all removed?

> --- ./gdb_src.orig/gdb/cr16-tdep.c 1970-01-01 05:30:00.000000000 +0530
> +++ ./gdb_src/gdb/cr16-tdep.c 2012-10-09 18:47:25.000000000 +0530
[...]
> +#include "linux-tdep.h"

Intuitively, I don't think you would need to include this file from
the non-linux tdep, no?

> +/* Data type to store instruction opcodes. */
> +typedef unsigned short wordU;
> +
> +/* Globals to store opcode related information. */
> +wordU words[3];
> +ULONGLONG allWords;
> +ins currInsn;

These types of globals are a big no-no, and I don't really understand
how they are being used. Some of them seem to never be set, others
to be set but never read... Can you adjust your code to avoid those
globals?

> +static void
> +cr16_analyze_prologue (CORE_ADDR start_pc,
> + CORE_ADDR limit_pc, struct cr16_prologue *result)
> +{
> + CORE_ADDR pc, next_pc;
> + gdb_byte buf[6];

Can you move the declaration of this buffer to the scope where
it is used, please? It's clearer that way.

> + char insn_byte1, insn_byte2;

I think you probably want to use gdb_byte, here.

> + /* If PUSH, then save RA and other regs. */
> + if (insn_byte1 == 0x01)
> + {
> + int r1, r2;
> + int r;
> + insn_byte2 = words[0];

Empty line between variable declarations and the rest of the code.
There are other locations where I saw the same issue. Can you do a pass
over those, please?

> +static struct cr16_prologue *
> +cr16_analyze_frame_prologue (struct frame_info *this_frame,
> + void **this_prologue_cache)
> +{
> + if (!*this_prologue_cache)

This is a bit of a nit, and you are welcome to keep the code as is,
but the typical usage in this case is to do:

if (*this_prologue_cache)
return *this_prologue_cache;

This allows to have the rest of the code less indented, which is
usually more easily readable.

> +static CORE_ADDR
> +cr16_unwind_pc (struct gdbarch *gdbarch, struct frame_info *this_frame)
> +{
> + ULONGEST pc;
> +
> + pc = frame_unwind_register_unsigned (this_frame, CR16_PC_REGNUM);
> + return pc;
> +}

Just curious: Why not just:

return frame_unwind_register_unsigned (this_frame, CR16_PC_REGNUM);

I have no doubt that the compiler is going to remove the local
variable, and so we'll end up with the same code. But then you have
your local variable's type being different from the return value type.

> +
> +/* Implement the "unwind_sp" gdbarch method. */
> +
> +static CORE_ADDR
> +cr16_unwind_sp (struct gdbarch *gdbarch, struct frame_info *this_frame)

Same here.

> + u =
> + extract_unsigned_integer (arg_bits, arg_size, byte_order);

Nit: I think it's fine to format the entire statement into a single
line. It looks like it just fits 80 chars. Otherwise, i'd format it
like

u = extract_unsigned_integer (arg_bits, arg_size,
byte_order);

> + /* If we don't pass the option -mint32
> + FIXME: add if else case depending on the option passed,
> + sp that we can have int size as 16 or 32 bits both. */

Is there any way of handling this fixme easily?

> +/* Implement the "register_name" gdbarch method. */
> +
> +static const char *
> +cr16_register_name (struct gdbarch *gdbarch, int regnr)
> +{
> + static const char *const reg_names[] = {

Formatting - same as above.

> + default:
> + printf
> + ("\nRegister Type not supported\nFunction : cr16_register_type\n");
> + return 0;
> + break;

You cannot do that: We assume that that this function never NULL.
Depending on the reason for not "supporting" certain registers,
you can add a gdb_assert_not_reached if you know you've covered
all registers, or else return any type, like builtin_int32.

--
Joel