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

[Simon Marchi <simark at simark dot ca>]

Hi John,

I did a first pass review and noted a few minor nits.  I didn't get too deep in the
frame_id/unwind code because I'm not too familiar with that.

You should include the new file in the ALL_TARGET_OBS makefile target, so that
it's included in a --enable-targets=all build.

On 2018-08-23 1:35 p.m., John Darrington wrote:
> gdb/
>     * configure.tgt: Add configuration for s12z.
>     * s12z-tdep.c:  New file.
>     * NEWS: Mention new target.
> ---
>  gdb/NEWS          |   4 +
>  gdb/configure.tgt |   6 +
>  gdb/s12z-tdep.c   | 402 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 412 insertions(+)
>  create mode 100644 gdb/s12z-tdep.c
> 
> diff --git a/gdb/NEWS b/gdb/NEWS
> index a7a3674375..c46056525a 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -3,6 +3,10 @@
>  
>  *** Changes since GDB 8.2
>  
> +* New targets
> +
> + NXP S12Z		s12z-*-elf
> +
>  * GDB and GDBserver now support IPv6 connections.  IPv6 addresses
>    can be passed using the '[ADDRESS]:PORT' notation, or the regular
>    'ADDRESS:PORT' method.
> diff --git a/gdb/configure.tgt b/gdb/configure.tgt
> index 5e3bd5de71..72de1a1e4a 100644
> --- a/gdb/configure.tgt
> +++ b/gdb/configure.tgt
> @@ -643,6 +643,12 @@ spu*-*-*)
>  	build_gdbserver=yes
>  	;;
>  
> +s12z-*-*)
> +	# Target: Freescale S12z
> +	gdb_target_obs="s12z-tdep.o"
> +	build_gdbserver=yes

I don't think you should include build_gdbserver.  It is only valid
if you have a gdbserver port, for when building natively on that
architecture.

> +	;;
> +
>  tic6x-*-*linux)
>  	# Target: GNU/Linux TI C6x
>  	gdb_target_obs="tic6x-tdep.o tic6x-linux-tdep.o solib-dsbt.o \
> diff --git a/gdb/s12z-tdep.c b/gdb/s12z-tdep.c
> new file mode 100644
> index 0000000000..5169025e6b
> --- /dev/null
> +++ b/gdb/s12z-tdep.c
> @@ -0,0 +1,402 @@
> +/* Target-dependent code for the S12Z, for the GDB.
> +   Copyright (C) 2018 Free Software Foundation, Inc.
> +
> +   This file is part of GDB.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +/* Much of this file is shamelessly copied from or1k-tdep.c and others */

You don't have to include this, it's perfectly encouraged to share code between ports :).

> +
> +#include "defs.h"
> +
> +#include "arch-utils.h"
> +#include "block.h"
> +#include "cli/cli-decode.h"
> +#include "common-defs.h"
> +#include "dis-asm.h"
> +#include "dwarf2-frame.h"
> +#include "elf-bfd.h"
> +#include "floatformat.h"
> +#include "frame-base.h"
> +#include "frame.h"
> +#include "frame-unwind.h"
> +#include "gdbcmd.h"
> +#include "gdbcore.h"
> +#include "gdbtypes.h"
> +#include "infcall.h"
> +#include "inferior.h"
> +#include "language.h"
> +#include "objfiles.h"
> +#include "observable.h"
> +#include "opcode/s12z.h"
> +#include "osabi.h"
> +#include "regcache.h"
> +#include "reggroups.h"
> +#include "remote.h"
> +#include "symcat.h"
> +#include "symfile.h"
> +#include "symtab.h"
> +#include "target-descriptions.h"
> +#include "target.h"
> +#include "trad-frame.h"
> +#include "user-regs.h"
> +#include "valprint.h"
> +#include "value.h"

Are all these includes necessary?  Can you try to reduce to what you actually use?

> +#include <stdio.h>

This should not be necessary.

> +
> +#define N_PHYSICAL_REGISTERS (S12Z_N_REGISTERS - 2)
> +
> +static const int reg_perm[N_PHYSICAL_REGISTERS] =
> +  {
> +   REG_D0,
> +   REG_D1,
> +   REG_D2,
> +   REG_D3,
> +   REG_D4,
> +   REG_D5,
> +   REG_D6,
> +   REG_D7,
> +   REG_X,
> +   REG_Y,
> +   REG_S,
> +   REG_P,
> +   REG_CCW
> +  };
> +
> +static const char *
> +s12z_register_name (struct gdbarch *gdbarch, int regnum)
> +{
> +  return registers[reg_perm[regnum]].name;

Which "registers" variable does this refer to?

> +}
> +
> +static CORE_ADDR
> +s12z_skip_prologue (struct gdbarch *gdbarch,
> +		    CORE_ADDR       pc)

Remove extra spaces, and this can fit on a single line.

> +{
> +  CORE_ADDR start_pc = 0;
> +
> +  if (find_pc_partial_function (pc, NULL, &start_pc, NULL))
> +    {
> +      CORE_ADDR prologue_end = skip_prologue_using_sal (gdbarch, pc);
> +
> +      if (0 != prologue_end)
> +	{
> +	  struct symtab_and_line prologue_sal = find_pc_line (start_pc, 0);
> +	  struct compunit_symtab *compunit
> +	    = SYMTAB_COMPUNIT (prologue_sal.symtab);
> +	  const char *debug_format = COMPUNIT_DEBUGFORMAT (compunit);
> +
> +	  if ((NULL != debug_format)
> +	      && (strlen ("dwarf") <= strlen (debug_format))
> +	      && (0 == strncasecmp ("dwarf", debug_format, strlen ("dwarf"))))
> +	    return (prologue_end > pc) ? prologue_end : pc;
> +	}
> +    }
> +
> +  fprintf_unfiltered (gdb_stdlog, "%s:%d %s FAILED TO FIND END OF PROLOGUE PC = %08x\n", __FILE__, __LINE__,
> +                      __FUNCTION__, (unsigned int) pc);

Maybe use the warning() function?  Also it's probably not necessary to use all
caps.  To print a CORE_ADDR, use:

  "%s", paddress (gdbarch, pc);

Also, user-visible messages should use _() for i18n.

> +
> +  return 0;
> +}
> +
> +static CORE_ADDR
> +s12z_unwind_pc (struct gdbarch *gdbarch, struct frame_info *next_frame)
> +{
> +  return frame_unwind_register_unsigned (next_frame, REG_P);
> +}
> +
> +/* Implement the unwind_sp gdbarch method.  */

This comment is good, can you put a similar for other gdbarch callbacks?

> +
> +static CORE_ADDR
> +s12z_unwind_sp (struct gdbarch *gdbarch, struct frame_info *next_frame)
> +{
> +  return frame_unwind_register_unsigned (next_frame, REG_S);
> +}
> +
> +
> +

Remove the extra lines.

> +static struct type *
> +s12z_register_type (struct gdbarch *gdbarch, int reg_nr)
> +{
> +  switch (registers[reg_perm[reg_nr]].bytes)
> +    {
> +    case 1:
> +      return builtin_type (gdbarch)->builtin_uint8;
> +    case 2:
> +      return builtin_type (gdbarch)->builtin_uint16;
> +    case 3:
> +      return builtin_type (gdbarch)->builtin_uint24;
> +    case 4:
> +      return builtin_type (gdbarch)->builtin_uint32;
> +    default:
> +      return builtin_type (gdbarch)->builtin_uint32;
> +    }
> +  return builtin_type (gdbarch)->builtin_int0;
> +}
> +
> +
> +static int
> +s12z_dwarf_reg_to_regnum (struct gdbarch *gdbarch, int num)
> +{
> +  switch (num)
> +    {
> +    case 15:      return REG_S;
> +    case 7:       return REG_X;
> +    case 8:       return REG_Y;
> +    case 42:      return REG_D0;
> +    case 43:      return REG_D1;
> +    case 44:      return REG_D2;
> +    case 45:      return REG_D3;
> +    case 46:      return REG_D4;
> +    case 47:      return REG_D5;
> +    case 48:      return REG_D6;
> +    case 49:      return REG_D7;
> +    }
> +  return -1;
> +}
> +
> +
> +/* Support functions for frame handling.  */
> +
> +/* Initialize a prologue cache */
> +
> +volatile int frame_size = 0;

Does this really need to be global?  And volatile?

> +
> +static struct trad_frame_cache *
> +s12z_frame_cache (struct frame_info *this_frame, void **prologue_cache)
> +{
> +  struct trad_frame_cache *info;
> +
> +  CORE_ADDR this_sp;
> +  CORE_ADDR this_sp_for_id;
> +
> +  CORE_ADDR start_addr;
> +  CORE_ADDR end_addr;
> +
> +  /* Nothing to do if we already have this info.  */
> +  if (NULL != *prologue_cache)
> +    return (struct trad_frame_cache *) *prologue_cache;
> +
> +  /* Get a new prologue cache and populate it with default values.  */
> +  info = trad_frame_cache_zalloc (this_frame);
> +  *prologue_cache = info;
> +
> +  /* Find the start address of this function (which is a normal frame, even
> +     if the next frame is the sentinel frame) and the end of its prologue.  */
> +  CORE_ADDR this_pc = get_frame_pc (this_frame);
> +  struct gdbarch *gdbarch = get_frame_arch (this_frame);
> +  find_pc_partial_function (this_pc, NULL, &start_addr, NULL);

Check the return value?

> +
> +  /* Get the stack pointer if we have one (if there's no process executing
> +     yet we won't have a frame.  */
> +  this_sp = (NULL == this_frame) ? 0 :
> +    get_frame_register_unsigned (this_frame, REG_S);
> +
> +  /* Return early if GDB couldn't find the function.  */
> +  if (start_addr == 0)
> +    {
> +      fprintf_unfiltered (gdb_stdlog, "  couldn't find function\n");

Use warning (_()) ?

> +
> +      /* JPB: 28-Apr-11.  This is a temporary patch, to get round GDB
> +	 crashing right at the beginning.  Build the frame ID as best we
> +	 can.  */
> +      trad_frame_set_id (info, frame_id_build (this_sp, this_pc));
> +
> +      return info;
> +    }
> +
> +  /* The default frame base of this frame (for ID purposes only - frame
> +     base is an overloaded term) is its stack pointer.  For now we use the
> +     value of the SP register in this frame.  However if the PC is in the
> +     prologue of this frame, before the SP has been set up, then the value
> +     will actually be that of the prev frame, and we'll need to adjust it
> +     later.  */
> +  trad_frame_set_this_base (info, this_sp);
> +  this_sp_for_id = this_sp;
> +
> +  /* The default is to find the PC of the previous frame in the link
> +     register of this frame.  This may be changed if we find the link
> +     register was saved on the stack.  */
> +  //  trad_frame_set_reg_realreg (info, S12Z_NPC_REGNUM, S12Z_LR_REGNUM);

Why is this commented?  Either it should be used or it should not be there.

> +
> +  /* We should only examine code that is in the prologue.  This is all code
> +     up to (but not including) end_addr.  We should only populate the cache
> +     while the address is up to (but not including) the PC or end_addr,
> +     whichever is first.  */
> +  end_addr = s12z_skip_prologue (gdbarch, start_addr);
> +
> +  /* All the following analysis only occurs if we are in the prologue and
> +     have executed the code.  Check we have a sane prologue size, and if
> +     zero we are frameless and can give up here.  */
> +  if (end_addr < start_addr)
> +    error (_("end addr %s is less than start addr %s"),
> +	   paddress (gdbarch, end_addr), paddress (gdbarch, start_addr));
> +
> +  if (end_addr == start_addr)
> +    {
> +      frame_size = 0;
> +    }

Remove braces.

> +  else
> +    {
> +      CORE_ADDR addr = start_addr; /* Where we have got to */
> +
> +      gdb_byte byte;
> +
> +      if (0 != target_read_code (addr, &byte, 1))
> +        memory_error (TARGET_XFER_E_IO, addr);

I think if you call read_code, it throws memory_error for you on failure.

> +
> +      int simm;
> +      if (byte == 0x1a)   /* LEA S, (S, xxxx) */
> +        {
> +          if (0 != target_read_code (addr + 1, &byte, 1))
> +            memory_error (TARGET_XFER_E_IO, addr);

Same.

> +
> +          simm = (signed char) byte;
> +	  frame_size = -simm;
> +	  addr += 2;
> +
> +	  /* If the PC has not actually got to this point, then the frame
> +	     base will be wrong, and we adjust it.
> +
> +	     If we are past this point, then we need to populate the stack
> +	     accordingly.  */
> +	  if (this_pc <= addr)
> +	    {
> +	      /* Only do if executing.  */
> +	      if (0 != this_sp)
> +		{
> +		  this_sp_for_id = this_sp + frame_size;
> +		  trad_frame_set_this_base (info, this_sp_for_id);
> +		}
> +	    }
> +	  else
> +	    {
> +	      /* We are past this point, so the stack pointer of the prev
> +	         frame is frame_size greater than the stack pointer of this
> +	         frame.  */
> +	      trad_frame_set_reg_value (info, REG_S,
> +					this_sp + frame_size + 3);
> +	    }
> +        }
> +
> +      /* From now on we are only populating the cache, so we stop once we
> +	 get to either the end OR the current PC.  */
> +      //      end_addr = (this_pc < end_addr) ? this_pc : end_addr;
> +
> +
> +      trad_frame_set_reg_addr (info, REG_P, this_sp + frame_size);
> +    }
> +
> +  /* Build the frame ID */
> +  trad_frame_set_id (info, frame_id_build (this_sp_for_id, start_addr));
> +
> +  return info;
> +}
> +
> +/* Implement the this_id function for the stub unwinder.  */
> +
> +static void
> +s12z_frame_this_id (struct frame_info *this_frame,
> +		    void **prologue_cache, struct frame_id *this_id)
> +{
> +  struct trad_frame_cache *info = s12z_frame_cache (this_frame,
> +						    prologue_cache);
> +
> +  trad_frame_get_id (info, this_id);
> +}
> +
> +/* Implement the prev_register function for the stub unwinder.  */
> +
> +static struct value *
> +s12z_frame_prev_register (struct frame_info *this_frame,
> +			  void **prologue_cache, int regnum)
> +{
> +  struct trad_frame_cache *info = s12z_frame_cache (this_frame,
> +						    prologue_cache);
> +
> +  return trad_frame_get_register (info, this_frame, regnum);
> +}
> +
> +/* Data structures for the normal prologue-analysis-based unwinder.  */
> +
> +static const struct frame_unwind s12z_frame_unwind = {
> +  NORMAL_FRAME,
> +  default_frame_unwind_stop_reason,
> +  s12z_frame_this_id,
> +  s12z_frame_prev_register,
> +  NULL,
> +  default_frame_sniffer,
> +  NULL,
> +};
> +
> +
> +constexpr gdb_byte s12z_break_insn[] = {0x00};
> +
> +typedef BP_MANIPULATION (s12z_break_insn) s12z_breakpoint;
> +
> +struct gdbarch_tdep
> +{
> +};
> +
> +static struct gdbarch *
> +s12z_gdbarch_init (struct gdbarch_info info,
> +                    struct gdbarch_list *arches)

The indentation here looks wrongs (should use tabs for groups of 8 columns and
then complete with spaces, and there is one extra space).

> +{
> +  struct gdbarch_tdep *tdep = (struct gdbarch_tdep *) xmalloc (sizeof *tdep);

You could use XNEW (gdbarch_tdep), which has the advantage of causing a build failure
if it's unsafe to malloc a structure of this type.

> +  struct gdbarch *gdbarch = gdbarch_alloc (&info, tdep);
> +
> +  /* Target data types.  */
> +  set_gdbarch_short_bit (gdbarch, 16);
> +  set_gdbarch_int_bit (gdbarch, 16);
> +  set_gdbarch_long_bit (gdbarch, 32);
> +  set_gdbarch_long_long_bit (gdbarch, 32);
> +  set_gdbarch_ptr_bit (gdbarch, 24);
> +  set_gdbarch_addr_bit (gdbarch, 24);
> +  set_gdbarch_char_signed (gdbarch, 0);
> +
> +  set_gdbarch_ps_regnum (gdbarch, REG_CCW);
> +  set_gdbarch_pc_regnum (gdbarch, REG_P);
> +  set_gdbarch_sp_regnum (gdbarch, REG_S);
> +
> +
> +  set_gdbarch_breakpoint_kind_from_pc (gdbarch,
> +				       s12z_breakpoint::kind_from_pc);
> +  set_gdbarch_sw_breakpoint_from_kind (gdbarch,
> +				       s12z_breakpoint::bp_from_kind);
> +
> +  set_gdbarch_num_regs (gdbarch, S12Z_N_REGISTERS - 2);
> +  set_gdbarch_register_name (gdbarch, s12z_register_name);
> +  set_gdbarch_skip_prologue (gdbarch, s12z_skip_prologue);
> +  set_gdbarch_inner_than (gdbarch, core_addr_lessthan);
> +  set_gdbarch_dwarf2_reg_to_regnum (gdbarch, s12z_dwarf_reg_to_regnum);
> +
> +  set_gdbarch_register_type (gdbarch, s12z_register_type);
> +
> +  /* Functions to access frame data.  */
> +  set_gdbarch_unwind_pc (gdbarch, s12z_unwind_pc);
> +  set_gdbarch_unwind_sp (gdbarch, s12z_unwind_sp);
> +
> +  dwarf2_append_unwinders (gdbarch);
> +  frame_unwind_append_unwinder (gdbarch, &s12z_frame_unwind);
> +
> +  return gdbarch;
> +}
> +
> +void
> +_initialize_s12z_tdep (void)
> +{
> +  gdbarch_register (bfd_arch_s12z, s12z_gdbarch_init, NULL);
> +}
> 

Thanks,

Simon