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

Tom Tromey
2018-09-07T22:03:56Z

>>>>> "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