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

Joel Brobecker
2012-11-15T17:43:23Z

> Additionally I have also attached the bfd hunk which moves the globals into
> externs in the opcodes file. I will submit that to binutils once my gdb port
> gets a go ahead.

Both bfd and opcode bits needs to be approved before the GDB part
can go ahead.

> 2012-10-26 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.

Below are my comments. They are mostly minor, so we should be pretty
close to approval.

The opcode and bfd bits are maintained by the binutils project,
however, and will need to be approved there, and then checked in,
before this patch can proceed.

> +/* Target-dependent code for GNU/Linux on the Sitel CR16 processors.
> +
> + Copyright 2012 Free Software Foundation, Inc.

Small nit: Can you add the "(C)" after Copyright, please?

[in cr16-linux-tdep.c]
> +#include "solib-svr4.h"

Is this include really needed. This would be a little surprising
considering that solib-svr4.o is not part of the gdb_target_obs.

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

Would you mind adding an assertion that REGNR is within acceptable
bounds, please? This will trigger an internal error instead of a
out-of-bound buffer access...

You could also possibly add an assertion that the number of elements
in your array is equal to CR16_LINUX_NUM_REGS, if that makes sense.

One other possible suggestion is to move the array outside of the
function, and to use a static assertion. The advantage, I believe,
is that the assertion will be computed by the compiler, and fail
at build time, rather than run time, if something every goes hinky.

> --- ./gdb_src.orig/gdb/cr16-tdep.c 1970-01-01 05:30:00.000000000 +0530
> +++ ./gdb_src/gdb/cr16-tdep.c 2012-10-25 10:44:15.000000000 +0530
> +static const char *
> +cr16_register_name (struct gdbarch *gdbarch, int regnr)
> +{
> + static const char *const reg_names[] =
> + {
[...]
> + };
> +
> + return reg_names[regnr];

Same as above.

> +}
> +
> +
> +/* Implement the "register_type" gdbarch method. */
> +
> +static struct type *
> +cr16_register_type (struct gdbarch *gdbarch, int reg_nr)
> +{
> + switch (reg_nr)
> + {
> + case CR16_PC_REGNUM: /* Note:PC in CR16 is of 24 bits. */
Can you add a space after the colon?

> + case CR16_FP_REGNUM: /*Frame Pointer reg. */
> + case CR16_SP_REGNUM: /*Stack Pointer reg. */

Can you add a space after "/*", please? Although, FP an SP are so
common, the comment is hardly useful - but not harmful.

> + case SIM_CR16_ISP_REGNUM:
> + case SIM_CR16_USP_REGNUM:
> + case SIM_CR16_INTBASE_REGNUM:
> + return builtin_type (gdbarch)->builtin_int32;
> + break;
> +
> + case SIM_CR16_PSR_REGNUM:
> + case SIM_CR16_CFG_REGNUM:
> + return builtin_type (gdbarch)->builtin_uint32;
> + break;

Why not merge all blocks that return builtin_int32? Same for any
other registers that return the same type (maybe less of an obvious
suggestion). This are just suggestions, so please do not feel
obigated to follow them.

> + allWords =
> + ((ULONGLONG) words[0] << 32) + ((unsigned long) words[1] << 16) +
> + words[2];

Can you please void using the mixed-cap style for variables? For
instance, use all_words, not allWords.

Also, when breaking lines at a binary operator, you should break
before the operator, not after.

I think I understand the reason for the casts/promotion, but can we
cast to the same type for both pieces?

For long lines like these, the GNU Coding Standards require us to
enclose the rhs expression into parentheses. The parentheses are
superfluous, but help code formatters (or your editor's formatter).
Thus:

all_words = (((ULONGLONG) words[0] << 32)
+ ((ULONGLONG) words[1] << 16)
+ words[2])

> + else
> + {
> + break; /* Terminate the prologue scan. */
> + }

Can you remove the extra braces? They are unnecessary, and GDB's style
is to not use them in that case. Note that they would be used if
you had to add a a comment. Thus...

else
break; /* Terminate. */

... but also ...

else
{
/* Terminate the loop because ... */
break;
}

> +static struct value *
> +cr16_frame_prev_register (struct frame_info *this_frame,
> + void **this_prologue_cache, int regnum)
> +{
> + struct cr16_prologue *p =
> + cr16_analyze_frame_prologue (this_frame, this_prologue_cache);
> + CORE_ADDR frame_base = cr16_frame_base (this_frame, this_prologue_cache);
> + int reg_size = register_size (get_frame_arch (this_frame), regnum);
> + ULONGEST ra_prev;

It looks like ra_prev could be declared in the only else if block where
it is actually used. And reg_size appears to be unused.

> + /* If prologue analysis says we saved this register somewhere,
> + return a description of the stack slot holding it. */
> + else if (p->reg_offset[regnum] != 1)
> + {
> + return frame_unwind_got_memory (this_frame, regnum,
> + frame_base + p->reg_offset[regnum]);
> + }
> +
> + /* Otherwise, presume we haven't changed the value of this
> + register, and get it from the next frame. */
> + else
> + {
> + return frame_unwind_got_register (this_frame, regnum, regnum);
> + }

Can you also remove the unnecessary curly braces above, please?

> + CORE_ADDR pc;
> +

Trailing spaces here...

> +static const gdb_byte *
> +cr16_breakpoint_from_pc (struct gdbarch *gdbarch, CORE_ADDR * pcptr,
> + int *lenptr)
[...]
> + if (tdep == NULL || tdep->breakpoint == NULL)
> + {
> + return breakpoint_elf;
> + }

Unnecessary braces...

> + /* Passing NULL values in the following two functions
> + for the time being, to fix later. */
> + set_gdbarch_print_insn (gdbarch, print_insn_cr16);
> + set_gdbarch_unwind_pc (gdbarch, cr16_unwind_pc);
> + set_gdbarch_unwind_sp (gdbarch, cr16_unwind_sp);

I don't understand the comment...

> + /* Methods for saving / extracting a dummy frame's ID.
> + The ID's stack address must match the SP value returned by
> + PUSH_DUMMY_CALL, and saved by generic_save_dummy_frame_tos. */

The generic_save_dummy_frame_tos function was deleted in 2004...
I suspect you copy/pasted this code from somewhere else.
Fortunately, I think that's easily fixed, as I don't see
the real benefit of the second sentence. The first one can be
used as visual cue that the frame ID code starts there, but
the second can just be deleted entirely.

> --- ./gdb_src.orig/gdb/configure.tgt 2012-08-02 01:18:44.000000000 +0530
> +++ ./gdb_src/gdb/configure.tgt 2012-10-23 15:09:24.000000000 +0530
> --- ./gdb_src.orig/gdb/cr16-linux-tdep.c 1970-01-01 05:30:00.000000000 +0530
> +++ ./gdb_src/gdb/cr16-linux-tdep.c 2012-10-23 15:22:34.000000000 +0530
> --- ./gdb_src.orig/gdb/cr16-tdep.c 1970-01-01 05:30:00.000000000 +0530
> +++ ./gdb_src/gdb/cr16-tdep.c 2012-10-25 10:44:15.000000000 +0530
> --- ./gdb_src.orig/gdb/cr16-tdep.h 1970-01-01 05:30:00.000000000 +0530
> +++ ./gdb_src/gdb/cr16-tdep.h 2012-10-23 15:22:53.000000000 +0530

? Looks like you have all these files twice in the patch you sent?
I'll assume a pilot error an not review the second instances.

--
Joel