Re: [PATCH 4/4] GDB: New target s12z

[Tom Tromey <tom at tromey dot com>]

>>>>> "John" == John Darrington <john@darrington.wattle.id.au> writes:

Thank you for the patch.  It seems generally good.  I have a few nits,
but nothing serious.

John> diff --git a/gdb/Makefile.in b/gdb/Makefile.in
John> index 118c3c8062..f448d1ee19 100644
John> --- a/gdb/Makefile.in
John> +++ b/gdb/Makefile.in
John> @@ -758,6 +758,7 @@ ALL_TARGET_OBS = \
John>  	rs6000-lynx178-tdep.o \
John>  	rs6000-tdep.o \
John>  	rx-tdep.o \
John> +	s12z-tdep.o \

This change should be mentioned in the ChangeLog.

John> +
John> +#define N_PHYSICAL_REGISTERS (S12Z_N_REGISTERS - 2)
John> +
John> +static const int reg_perm[N_PHYSICAL_REGISTERS] =
John> +  {

New functions, comments, and macros should have an explanatory comment.
Sometimes this can be just a reference to some generic thing, like
implementations of gdbarch methods can refer to gdbarch.h.  There are a
number of cases of this in the file.

John> +  /*  registers is declared in opcodes/s12z.h */

GNU comment style is to start with a capital letter and end with a
period and two spaces.

John> +      if (0 != prologue_end)
John> +	{
John> +	  struct symtab_and_line prologue_sal = find_pc_line (start_pc, 0);
John> +	  struct compunit_symtab *compunit
John> +	    = SYMTAB_COMPUNIT (prologue_sal.symtab);
John> +	  const char *debug_format = COMPUNIT_DEBUGFORMAT (compunit);
John> +
John> +	  if ((NULL != debug_format)
John> +	      && (strlen ("dwarf") <= strlen (debug_format))
John> +	      && (0 == strncasecmp ("dwarf", debug_format, strlen ("dwarf"))))
John> +	    return (prologue_end > pc) ? prologue_end : pc;
John> +	}

Is this stuff useful?

I would think that because s12z_frame_unwind is added after the DWARF
unwinders that this would perhaps just be dead code.  I do not know for
sure.

I see this in or1k-tdep.c. gdb is kind of lenient about some targets,
but it seems like there should be a better way.

John> +      /* JPB: 28-Apr-11.  This is a temporary patch, to get round GDB
John> +	 crashing right at the beginning.  Build the frame ID as best we
John> +	 can.  */
John> +      trad_frame_set_id (info, frame_id_build (this_sp, this_pc));

Could you explain this more?  I wonder if there is something we could
change.

John> +struct gdbarch_tdep
John> +{
John> +};

I wonder if it could simply be eliminated.  Not super important to me.

John> +  int i;
John> +  for (i = 0; i < stop_1 - len; ++i)

We've been preferring "for (int i = ...".

John> +  const int numregs = gdbarch_num_regs (gdbarch)
John> +		      + gdbarch_num_pseudo_regs (gdbarch);

Parens around the right hand side when line-breaking is the GNU style.

John> +      int reg;
John> +      for (reg = 0; reg < numregs; reg++)

for (int reg ...

Tom