Re: [PATCH] Add support for Tilera TILE-Gx processor (part 1/3: gdb)

[Joel Brobecker <brobecker at adacore dot com>]

> This is part 1 of 3, containing basic support for the TILE-Gx
> processor.  Patch is attached.  It contains:




> 
>     gdb/
>     * tilegx-linux-tdep.c: New file
>     * tilegx-tdep.c: New file
>     * configure.host: Add tilegx target.
>     * configure.tgt: Add tilegx target.
>     * Makefile.in: Add tilegx dependencies.
>     gdb/regformats/
>     * reg-tilegx.dat: New file.
> 
>     gdb/config/tilegx/
>     * nm-linux.h: New file.

There is only one ChangeLog for all these files, IIRC (gdb/Changelog).
So it should be one single entry, and the filenames should be relative
to the gdb/ directory (Eg: `* regformat/reg-tilegx.dat, or
config/tilegx/nm-linux.h).


Can you be more specific about the changes make to Makefile.in?
In particular, say which variables were updated, and which rules
were added? Here is an example:

        * Makefile.in (ALL_64_TARGET_OBS): Add ia64-hpux-tdep.o.
        (HFILES_NO_SRCDIR): Add ia64-hpux-tdep.h.
        (ALLDEPFILES): Add ia64-hpux-nat.c ia64-hpux-tdep.c.

You'll also need to add your config/tilegx/nm-linux.h to HFILES_NO_SRCDIR.

> +#ifndef NM_TILELINUX_H
> +#define NM_TILELINUX_H
> +
> +#undef HAVE_LINK_H

Why do you need to do this?

> +tilegx-*-linux*)
> +        # Target: TILE-Gx
> +        gdb_target_obs="tilegx-tdep.o tilegx-linux-tdep.o solib-svr4.o \
> +			symfile-mem.o glibc-tdep.o linux-tdep.o"
> +        build_gdbserver=yes
> +        ;;

You need to remove "build_gdbserver=yes" from this patch.  Otherwise,
if we apply this patch but not the gdbserver bits, we'll get a build
error.

> +{
> +  /* Stub.  */
> +}
> +
> +
> +static const struct tramp_frame tilegx_linux_rt_sigframe =

One empty line too many (nitpick, no biggie if you like it better
this way.

> +#if 0
> +/* Target-dependent code for GNU/Linux SPARC.
> +
> +   Copyright (C) 2003-2005, 2007-2012 Free Software Foundation, Inc.

I'd really not have any commented-out code creeping unless there is
a very good reason, and we know it's going to be temporary.  In this
case, it even looks like it's still there by mistake???

> +/* TILE-Gx has 56 general purpose registers (R0 - R52, TP, SP, LR),
> + * plus 8 special general purpose registers (network and ZERO),
> + * plus 1 magic register (PC).
> + *
> + * TP (aka R53) is the thread specific data pointer.
> + * SP (aka R54) is the stack pointer.
> + * LR (aka R55) is the link register.
> + */

The comment style does not follow the GNU Coding Style (GCS). Can
you remove the extra `*' at the start of each line (except the first
one, of course)?

> +struct tilegx_frame_cache
> +{
> +  /* Base address.  */
> +  CORE_ADDR base;
> +  CORE_ADDR start_pc;
> +  struct trad_frame_saved_reg *saved_regs;
> +};

Can you quickly document each field, please?

> +enum reverse_state {
> +  REVERSE_STATE_REGISTER,
> +  REVERSE_STATE_VALUE, 
> +  REVERSE_STATE_UNKNOWN
> +};

Trailing space at the end of REVERSE_STATE_VALUE.  Can you document
this enum, please, as well as ...

> +struct tilegx_reverse_regs
> +{
> +  LONGEST value;
> +  enum reverse_state state;
> +};

... this struct?

> +static const struct tilegx_reverse_regs
> +template_reverse_regs[E_NUM_PHYS_REGS] =

Same here.

> +/* This is the implementation of gdbarch method register_name.  */

This is a bit of a nitpick, but MarkK recently requested that
we try to be consistent when documeting the gdbarch method
implementations. So, can you update the variable comments and say

/* Implement the "register_name" gdbarch method.  */

?

> +static int
> +tilegx_dwarf2_reg_to_regnum (struct gdbarch *gdbarch, int num)

Please document this function.

> +static int
> +tilegx_type_is_scalar (struct type *t)
> +{
> +  return (TYPE_CODE(t) != TYPE_CODE_STRUCT
> +    && TYPE_CODE(t) != TYPE_CODE_UNION
> +    && TYPE_CODE(t) != TYPE_CODE_ARRAY);

The indentation is wrong.  The `&&' operators should be aligned with
the start of the `TYPE_CODE' macro.

> +/* Returns non-zero if the given struct type will be returned using
> +   a special convention, rather than the normal function return method.
> +   Used in the context of the "return" command, and target function
> +   calls from the debugger.  */ 

Trailing space at the end of the last line.

> +  /* Only scalars which fit in R0 - R9 can be returned in registers.
> +     Otherwise, they are returned via a pointer passed in R0.  */
> +  return !tilegx_type_is_scalar (type)
> +    && (TYPE_LENGTH (type) > (1 + E_R9_REGNUM - E_R0_REGNUM) * tilegx_reg_size);

Same as above.  Indentation issue.  Also, please wrap the condition
inside parenthesis.  This is not strictly necessary, but is mandated
by the GCS in order to help automatic formatters.

> +/* Find a function's return value in the appropriate registers (in
> +   regbuf), and copy it into valbuf.  */
> +
> +static void
> +tilegx_extract_return_value (struct type *type, struct regcache *regcache,
> +                             void *valbuf)
                                ^^^^^^^^^^^^
valbuf should be a gdb_byte *.

> +{
> +  int len = TYPE_LENGTH (type);
> +  int i, regnum = E_R0_REGNUM;
> +
> +  for (i = 0; i < len; i += tilegx_reg_size)
> +    regcache_raw_read (regcache, regnum++, (char *) valbuf + i);

Remove the cast.

> +/* Copy the function return value from VALBUF into the
> +   proper location for a function return. 
> +   Called only in the context of the "return" command.  */

Trailing space at the end of the second line.  Please also reformat
to better fit 70-character lines (first line too short).

> +static void 
> +tilegx_store_return_value (struct type *type, struct regcache *regcache,
> +                           const void *valbuf)
> +{
> +  if (TYPE_LENGTH (type) < tilegx_reg_size)
> +    {    

Trailing spaces on 2 of the lines above.

> +      /* Add leading zeros to the value.  */
> +      char buf[tilegx_reg_size];

Use gdb_byte instead of char.

> +      for (i = 0; i < len; i += tilegx_reg_size)
> +        regcache_raw_write (regcache, regnum++, (char *) valbuf + i);

Remove the cast/

> +static enum return_value_convention
> +tilegx_return_value (struct gdbarch *gdbarch, struct type *func_type,
> +                     struct type *type, struct regcache *regcache,
> +                     gdb_byte *readbuf, const gdb_byte *writebuf)

This function needs documentation.

> +static CORE_ADDR
> +tilegx_frame_align (struct gdbarch *gdbarch, CORE_ADDR addr)

Same here.

> +/* Setup the function arguments for GDB to call a function in the inferior.
> +   Called only in the context of a target function call from the debugger.
> +   Returns the value of the SP register after the args are pushed.  */

The lines are too lone (soft-limit is 70 characters).  But you should
just refer to the "push_dummy_call" gdbarch method, rather than repeating
the generic documentation (if the gdbarch method is not properly
documentated, please consider submitting an independent patch that
improves it there instead).

> +static CORE_ADDR
> +tilegx_push_dummy_call (struct gdbarch *gdbarch,
> +                        struct value *function,
> +                        struct regcache *regcache,
> +                        CORE_ADDR bp_addr, int nargs,
> +                        struct value **args,
> +                        CORE_ADDR sp, int struct_return,
> +                        CORE_ADDR struct_addr)
> +{
> +  enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
> +  CORE_ADDR stack_dest = sp;
> +  int argreg = E_R0_REGNUM;
> +  int i, j;
> +  int typelen, slacklen, alignlen;
> +  static const int zero_words[2] = { 0, 0 };

Can you make that a `gdb_bype two_zero_words[8] = { 0 }' instead?

> +  /* Arguments are passed in R0 - R9, and as soon as an argument will not
> +     fit completely in the remaining registers, then it, and all remaining
> +     arguments, are put on the stack.  */

Can you reformat the comment to make the lines a little shorter?

> +          /* ISSUE: Why special handling (only) for "typelen = 4x + 1"?
> +             I am unable to induce any "typelen" values except 4 and 8.  */
> +          int n = (typelen - j == 1) ? 1 : tilegx_reg_size;
> +          ULONGEST w = extract_unsigned_integer (val + j, n, byte_order);
> +          regcache_cooked_write_unsigned (regcache, argreg++, w);

Empty line after last variable declaration....

> +  /* Loop backwards through remaining arguments and push them on the stack,
> +     wordaligned.  */

Lines too long...

> +    {
> +      char *val;

gdb_byte *...

> +      typelen = TYPE_LENGTH (value_enclosing_type (args[j]));

Empty line after last variable declaration...


> +      /* Now write this data to the stack. The stack grows downwards.  */

Missing second space after period.

> +  /* Add 2 words for linkage space to the stack.  */
> +  stack_dest = stack_dest - 8;
> +  write_memory (stack_dest, (void*)zero_words, 8);

Remove the cast.

> +  struct tilegx_decoded_instruction decoded[TILEGX_MAX_INSTRUCTIONS_PER_BUNDLE];
> +  struct tilegx_reverse_regs new_reverse_frame[TILEGX_MAX_INSTRUCTIONS_PER_BUNDLE];

These two lines are too long...

> +  /* Initialize the reverse frame. This maps the CURRENT frame's registers
> +     to the outer frame's registers (the frame on the stack goes the other
> +     way).  */

Lines are two long. Missing second space after period.

> +  /* To cut down on round-trip overhead, we fetch multiple bundles at once.
> +   * These variables describe the range of memory we have prefetched.  */

Likewise.

> +          /* Figure out how many bytes to fetch. Don't span a page boundary
> +             since that might cause an unnecessary memory error.  */

Ditto.

> +          unsigned int size_on_same_page = 4096 - (next_addr & 4095);
> +          instbuf_size = sizeof instbuf;

Missing empty line after variable declaration.

> +          status = safe_frame_unwind_memory (next_frame, instbuf_start, instbuf,
> +                                            instbuf_size);

Line too long.

> +      bundle = extract_unsigned_integer (&instbuf[next_addr - instbuf_start],
> +                                         8, byte_order);

Ditto.


> +          struct tilegx_decoded_instruction *this_insn = &decoded[i];
> +          int64_t *operands = (int64_t *)this_insn->operand_values;
> +          const struct tilegx_opcode *opcode = this_insn->opcode;
> +          switch (opcode->mnemonic)

Empty line after decls.

> +            {
> +            case TILEGX_OPC_ST:
> +              if (cache && 

Trailing space.

> +                  LONGEST saved_address = reverse_frame[operands[0]].value;
> +                  unsigned saved_register = (unsigned)reverse_frame[operands[1]].value;

Line too long. Add empty line after decls.

> +                  /* realreg >= 0 and addr != -1 indicates that the value
> +                     of saved_register is in memory location saved_address.
> +                     The value of realreg is not meaningful in this case but
> +                     it must be >= 0. See trad-frame.h  */

Period at end of sentence.

> +              if (cache 
> +                  && operands[0] == E_SP_REGNUM 
> +                  && operands[1] == E_SP_REGNUM
> +                  && reverse_frame[operands[1]].state == 
> +                     REVERSE_STATE_REGISTER)

Trailing spaces...

> +                  /* This is a special case. We're fixing up the stack frame.  */

> +                  uint64_t hopefully_sp =
> +                    (unsigned)reverse_frame[operands[1]].value;
> +                  short op2_as_short = (short)operands[2];
> +                  signed char op2_as_char = (signed char)operands[2];

I thin we're supposed to put a space after the ')' in casts.  I might
have missed some earlier.  Can you fix all of them? I won't comment
on any other violations I might find later on.

> +
> +                  /* Fix up the sign-extension.  */
> +                  if (opcode->mnemonic == TILEGX_OPC_ADDI)
> +                    op2_as_short = op2_as_char;
> +                  prev_sp_value = cache->saved_regs[hopefully_sp].addr - op2_as_short;

Line too long. Same thing: I will stop commenting on all style issues
such as trailing spaces, lines that are too long, missing empty line
Safter variable declarations, etc. Can you do a pass an fix them all?
The soft limit for line length is 70 characters, and you can exceed it
if sticking to 70 would make the code ugly, of your comment harder to
read (or any other valid excuse). The hard limit is 80 characters and
should always be obeyed unless you really have a good reason.

> +                  /* We don't know anything about the values. Punt.  */
> +                  /* We don't know anything about the values. Punt.  */

Two spaces after period.  Can you also include this in your pass?

> +              /* Nothing to see here, move on.
> +               * Note that real NOP is treated as a 'real' instruction 
> +               * because someone must have intended that it be there.
> +               * It therefore terminates the prolog.
> +               */

Same request for this type of comments.

> +              /* we're really done -- this is a branch  */

Missing period at end of sentence.

> +      for (i = 0; i < num_insns; i++)
> +        {
> +          /* ISSUE: Does this properly handle "network" registers?  */
> +          if ((reverse_frame_valid & (1<<i)) && dest_regs[i] != E_ZERO_REGNUM)
> +          {
> +            reverse_frame[dest_regs[i]] = new_reverse_frame[i];
> +          }
> +        }

Unnecessary curly braces for the "if" block (single statement). For
the "for" loop, they are OK, because of the comment you added.

> +          if (reverse_frame[i].state == REVERSE_STATE_REGISTER
> +              && reverse_frame[i].value != i)
> +            {
> +              unsigned saved_register = (unsigned)reverse_frame[i].value;

Add empty line.

> +/* If the input address is in a function prologue, 
> +   returns the address of the end of the prologue;
> +   else returns the input address.
> +
> +   Note: the input address is likely to be the function start, 
> +   since this function is mainly used for advancing a breakpoint
> +   to the first line, or stepping to the first line when we have
> +   stepped into a function call.  */
> +
> +static CORE_ADDR
> +tilegx_skip_prologue (struct gdbarch *gdbarch, CORE_ADDR pc)

Please document this function using a reference to the gdbarch method
instead.

> +/* The epilogue is defined here as the area at the end of a function,
> +   either on the `ret' instruction itself or after an instruction which
> +   destroys the function's stack frame.  */
> +static int

Another little coding rule, this time specific to GDB: Empyt line
between the function documentation and the function definintion.

> +}
> +
> +
> +static const unsigned char *

You can delete the extra line, if you'd like.

> +tilegx_breakpoint_from_pc (struct gdbarch *gdbarch,
> +                           CORE_ADDR *pcptr, int *lenptr)

Please document this function.

> +  static unsigned char breakpoint[sizeof (tilegx_bundle_bits)];

gdb_byte?

> +  tilegx_bundle_bits bits = TILEGX_BPT_BUNDLE;

Where is tilegx_bundle_bits defined? I can't find it anywhere in this
patch...

> +  unsigned int i;
> +
> +  for (i = 0; i < sizeof (breakpoint); i++)
> +    {
> +      breakpoint[i] = (unsigned char)bits;
> +      bits >>= 8;
> +    }

I am wondering whether there might be an easier way. Isn't the
tilegx breakpoint instrction someting statically known?

> +static struct value*
> +tilegx_frame_prev_register (struct frame_info *this_frame,
> +                            void **this_cache,
> +                            int regnum)

Please document this function.

> +static void
> +tilegx_frame_this_id (struct frame_info *this_frame, void **this_cache,
> +                      struct frame_id *this_id)

Likewise. I'll stop commenting on this as well, can you include all
other functions in your update?

> +/* This logic must match that of struct pt_regs in "ptrace.h".  */
> +
> +static void
> +tilegx_linux_supply_regset (const struct regset *regset,
> +                            struct regcache *regcache,
> +                            int regnum, const void *regs, size_t len)

Please document what this function is supposed to do.  I'd add your
current comment inside the function itself, as an implementation
detail.

> +      int gri = (i < E_NUM_EASY_REGS) ? i : E_PC_REGNUM;
> +      if (regnum == gri || regnum == -1)

Missing empty line.

> +/* Initializer function for the tilegx gdbarch vector.
> +   Called by gdbarch.  Sets up the gdbarch vector(s) for this target.  */

You don't need to repeat what should already be documented in
the gdbarch module.

> +  /* try to find a pre-existing architecture  */

Missing period at end of sentence.

> +  set_gdbarch_regset_from_core_section (gdbarch,
> +          tilegx_regset_from_core_section);

Formatting (alignment of second parameter).

> +  /*
> +   * Frame Info
> +   */

Comment style.

> +  set_gdbarch_in_function_epilogue_p (gdbarch,
> +              tilegx_in_function_epilogue_p);

Formatting.

I recomment you go through the gdbserver patch with the same comments
in mind, before you ask Pedro to review them.  Otherwise, he might
have to make the same comments...

-- 
Joel