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

Simon Marchi
2018-08-26T17:19:36Z

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