[PATCH 1/3] gdb: Added builtin types for 24 bit integers.

[PATCH 1/3] gdb: Added builtin types for 24 bit integers.

[John Darrington]

* gdb/gdbtypes.h (struct builtin_type): New members builtin_int24
  and builtin_uint24;
* gdb/gdbtypes.c: Initialize them.
* gdb/doc/gdb.texinfo: Add them.
---
 gdb/doc/gdb.texinfo | 2 ++
 gdb/gdbtypes.c      | 4 ++++
 gdb/gdbtypes.h      | 2 ++
 3 files changed, 8 insertions(+)

diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 5068c0ac81..e4ecd57a9e 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -42379,6 +42379,7 @@ Boolean type, occupying a single bit.
 
 @item int8
 @itemx int16
+@itemx int24
 @itemx int32
 @itemx int64
 @itemx int128
@@ -42386,6 +42387,7 @@ Signed integer types holding the specified number of bits.
 
 @item uint8
 @itemx uint16
+@itemx uint24
 @itemx uint32
 @itemx uint64
 @itemx uint128
diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
index 65b1211ec4..05bf7b1134 100644
--- a/gdb/gdbtypes.c
+++ b/gdb/gdbtypes.c
@@ -5402,6 +5402,10 @@ gdbtypes_post_init (struct gdbarch *gdbarch)
     = arch_integer_type (gdbarch, 16, 0, "int16_t");
   builtin_type->builtin_uint16
     = arch_integer_type (gdbarch, 16, 1, "uint16_t");
+  builtin_type->builtin_int24
+    = arch_integer_type (gdbarch, 24, 0, "int24_t");
+  builtin_type->builtin_uint24
+    = arch_integer_type (gdbarch, 24, 1, "uint24_t");
   builtin_type->builtin_int32
     = arch_integer_type (gdbarch, 32, 0, "int32_t");
   builtin_type->builtin_uint32
diff --git a/gdb/gdbtypes.h b/gdb/gdbtypes.h
index 14059ab3dc..eb7c365b71 100644
--- a/gdb/gdbtypes.h
+++ b/gdb/gdbtypes.h
@@ -1611,6 +1611,8 @@ struct builtin_type
   struct type *builtin_uint8;
   struct type *builtin_int16;
   struct type *builtin_uint16;
+  struct type *builtin_int24;
+  struct type *builtin_uint24;
   struct type *builtin_int32;
   struct type *builtin_uint32;
   struct type *builtin_int64;
-- 
2.11.0

[PATCH 3/3] GDB: New target s12z

[John Darrington]

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
+	;;
+
 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 */
+
+#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"
+
+
+#include <stdio.h>
+
+#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;
+}
+
+static CORE_ADDR
+s12z_skip_prologue (struct gdbarch *gdbarch,
+		    CORE_ADDR       pc)
+{
+  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);
+
+  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.  */
+
+static CORE_ADDR
+s12z_unwind_sp (struct gdbarch *gdbarch, struct frame_info *next_frame)
+{
+  return frame_unwind_register_unsigned (next_frame, REG_S);
+}
+
+
+
+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;
+
+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);
+
+  /* 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");
+
+      /* 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);
+
+  /* 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;
+    }
+  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);
+
+      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);
+
+          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)
+{
+  struct gdbarch_tdep *tdep = (struct gdbarch_tdep *) xmalloc (sizeof *tdep);
+  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);
+}
-- 
2.11.0

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

[Eli Zaretskii]

> From: John Darrington <john@darrington.wattle.id.au>
> Cc: John Darrington <john@darrington.wattle.id.au>
> Date: Thu, 23 Aug 2018 19:35:26 +0200
> 
> gdb/
>     * configure.tgt: Add configuration for s12z.
>     * s12z-tdep.c:  New file.
>     * NEWS: Mention new target.

OK for the NEWS part.

Thanks.

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

[Simon Marchi]

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

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

[John Darrington]

Hi Simon,

Thanks for the review.

On Sun, Aug 26, 2018 at 01:19:29PM -0400, Simon Marchi wrote:
     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.

Pity.  I was hoping someone could help me there :{P

Many of your comments and questions stem from the code which I used as a
pattern, so I'm unsure of the answers.  But I will find out as best I
can and prepare a new patch within the next few days.

J'

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

[Simon Marchi]

On 2018-08-26 1:41 p.m., John Darrington wrote:
> Hi Simon,
> 
> Thanks for the review.
> 
> On Sun, Aug 26, 2018 at 01:19:29PM -0400, Simon Marchi wrote:
>      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.
> 
> Pity.  I was hoping someone could help me there :{P

I just haven't written enough of those to be able to spot bugs off-hand in them.

Maybe just one insight, it seems (I'm not sure) that s12z_frame_cache uses the SP value of the
current frame (the one for which we compute the id) in the frame id.

The usual thing to do (I looked at a few other arches) is to use the same value as the
canonical frame address as defined by DWARF (Section 6.4 in DWARF5.pdf), which is the
value of the stack pointer just before that frame was created.

This is of course not mandatory, but I suppose that adhering to this de facto rule could
make things clearer.

> Many of your comments and questions stem from the code which I used as a
> pattern, so I'm unsure of the answers.  But I will find out as best I
> can and prepare a new patch within the next few days.
> 
> J'

Thanks,

Simon

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

[Simon Marchi]

On 2018-08-26 2:15 p.m., Simon Marchi wrote:
> On 2018-08-26 1:41 p.m., John Darrington wrote:
>> Hi Simon,
>>
>> Thanks for the review.
>>
>> On Sun, Aug 26, 2018 at 01:19:29PM -0400, Simon Marchi wrote:
>>      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.
>>
>> Pity.  I was hoping someone could help me there :{P
> 
> I just haven't written enough of those to be able to spot bugs off-hand in them.
> 
> Maybe just one insight, it seems (I'm not sure) that s12z_frame_cache uses the SP value of the
> current frame (the one for which we compute the id) in the frame id.
> 
> The usual thing to do (I looked at a few other arches) is to use the same value as the
> canonical frame address as defined by DWARF (Section 6.4 in DWARF5.pdf), which is the
> value of the stack pointer just before that frame was created.
> 
> This is of course not mandatory, but I suppose that adhering to this de facto rule could
> make things clearer.
> 
>> Many of your comments and questions stem from the code which I used as a
>> pattern, so I'm unsure of the answers.  But I will find out as best I
>> can and prepare a new patch within the next few days.
>>
>> J'
> 
> Thanks,
> 
> Simon

I also noticed that the gdb.base/all-architectures test fails when testing s12z.  Try to run

  make check TESTS="gdb.base/all-architectures-*.exp"

in the gdb build directory, see if you get a bunch of:

  FAIL: gdb.base/all-architectures-6.exp: tests: osabi=AIX: arch=s12z: endian=auto: disassemble 0x0,+4 (GDB internal error)

Simon

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

[John Darrington]

On Sun, Aug 26, 2018 at 06:55:31PM -0400, Simon Marchi wrote:
     I also noticed that the gdb.base/all-architectures test fails when testing s12z.  Try to run
     
       make check TESTS="gdb.base/all-architectures-*.exp"
     
     in the gdb build directory, see if you get a bunch of:
     
       FAIL: gdb.base/all-architectures-6.exp: tests: osabi=AIX: arch=s12z: endian=auto: disassemble 0x0,+4 (GDB internal error)
     

No.  I get:


...

                === gdb Summary ===

 # of expected passes         24


but I'll run that check before I submit my next patch.

J'

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

[Simon Marchi]

On 2018-08-27 02:30, John Darrington wrote:
> On Sun, Aug 26, 2018 at 06:55:31PM -0400, Simon Marchi wrote:
>      I also noticed that the gdb.base/all-architectures test fails
> when testing s12z.  Try to run
> 
>        make check TESTS="gdb.base/all-architectures-*.exp"
> 
>      in the gdb build directory, see if you get a bunch of:
> 
>        FAIL: gdb.base/all-architectures-6.exp: tests: osabi=AIX:
> arch=s12z: endian=auto: disassemble 0x0,+4 (GDB internal error)
> 
> 
> No.  I get:
> 
> 
> ...
> 
>                 === gdb Summary ===
> 
>  # of expected passes         24
> 
> 
> but I'll run that check before I submit my next patch.
> 
> J'

I fails when trying unexpected combinations of arches and osabis (like 
s12z with AIX), so you'd probably have to try on an --enable-targets=all 
build, after you've added s12z-tdep.o to ALL_TARGET_OBS.

Simon

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

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

Simon> The usual thing to do (I looked at a few other arches) is to use the same value as the
Simon> canonical frame address as defined by DWARF (Section 6.4 in DWARF5.pdf), which is the
Simon> value of the stack pointer just before that frame was created.

Simon> This is of course not mandatory, but I suppose that adhering to this de facto rule could
Simon> make things clearer.

ISTR there was one obscure case where it was necessary -- I think some
architectures have a special epilogue unwinder, and it was important
that this unwinder agree with the DWARF unwinder about the CFA in some
case.  This was many years ago so as you can tell I don't really recall
the details.

Anyway, I think using the current frame's SP will fail if the function
uses alloca -- the frame id will change across a step.  The CFA is
usually used in order to make the frame id invariant.  There may be a
comment in one of the frame*.h files explaining this.

Tom

[PATCH 2/3] GDB: Add support for 24 bit addresses

[John Darrington]

* include/dwarf2.h (enum dwarf_unit_type)[DW_EH_PE_udata3]: New member.
* gdb/dwarf2-frame.c (encoding_for_size): Deal with case 3.
	    (read_encoded_value): Deal with case DW_EH_PE_udata3
---
 gdb/dwarf2-frame.c | 7 ++++++-
 include/dwarf2.h   | 5 ++++-
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/gdb/dwarf2-frame.c b/gdb/dwarf2-frame.c
index f7dc820f4d..b329e34997 100644
--- a/gdb/dwarf2-frame.c
+++ b/gdb/dwarf2-frame.c
@@ -1527,12 +1527,14 @@ encoding_for_size (unsigned int size)
     {
     case 2:
       return DW_EH_PE_udata2;
+    case 3:
+      return DW_EH_PE_udata3;
     case 4:
       return DW_EH_PE_udata4;
     case 8:
       return DW_EH_PE_udata8;
     default:
-      internal_error (__FILE__, __LINE__, _("Unsupported address size"));
+      internal_error (__FILE__, __LINE__, _("Unsupported address size %d"), size);
     }
 }
 
@@ -1605,6 +1607,9 @@ read_encoded_value (struct comp_unit *unit, gdb_byte encoding,
     case DW_EH_PE_udata2:
       *bytes_read_ptr += 2;
       return (base + bfd_get_16 (unit->abfd, (bfd_byte *) buf));
+    case DW_EH_PE_udata3:
+      *bytes_read_ptr += 3;
+      return (base + bfd_get_24 (unit->abfd, (bfd_byte *) buf));
     case DW_EH_PE_udata4:
       *bytes_read_ptr += 4;
       return (base + bfd_get_32 (unit->abfd, (bfd_byte *) buf));
diff --git a/include/dwarf2.h b/include/dwarf2.h
index cf0039a92a..05c328057b 100644
--- a/include/dwarf2.h
+++ b/include/dwarf2.h
@@ -474,11 +474,14 @@ enum dwarf_unit_type
 #define DW_EH_PE_udata2		0x02
 #define DW_EH_PE_udata4		0x03
 #define DW_EH_PE_udata8		0x04
+
+#define DW_EH_PE_udata3		0x05
+
+#define DW_EH_PE_signed		0x08
 #define DW_EH_PE_sleb128	0x09
 #define DW_EH_PE_sdata2		0x0A
 #define DW_EH_PE_sdata4		0x0B
 #define DW_EH_PE_sdata8		0x0C
-#define DW_EH_PE_signed		0x08
 
 #define DW_EH_PE_pcrel		0x10
 #define DW_EH_PE_textrel	0x20
-- 
2.11.0

Re: [PATCH 2/3] GDB: Add support for 24 bit addresses

[Simon Marchi]

(CCing gcc-patches because of the change in include/dwarf2.h)

On 2018-08-23 13:35, John Darrington wrote:
> * include/dwarf2.h (enum dwarf_unit_type)[DW_EH_PE_udata3]: New member.
> * gdb/dwarf2-frame.c (encoding_for_size): Deal with case 3.
> 	    (read_encoded_value): Deal with case DW_EH_PE_udata3
> ---
>  gdb/dwarf2-frame.c | 7 ++++++-
>  include/dwarf2.h   | 5 ++++-
>  2 files changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/gdb/dwarf2-frame.c b/gdb/dwarf2-frame.c
> index f7dc820f4d..b329e34997 100644
> --- a/gdb/dwarf2-frame.c
> +++ b/gdb/dwarf2-frame.c
> @@ -1527,12 +1527,14 @@ encoding_for_size (unsigned int size)
>      {
>      case 2:
>        return DW_EH_PE_udata2;
> +    case 3:
> +      return DW_EH_PE_udata3;
>      case 4:
>        return DW_EH_PE_udata4;
>      case 8:
>        return DW_EH_PE_udata8;
>      default:
> -      internal_error (__FILE__, __LINE__, _("Unsupported address 
> size"));
> +      internal_error (__FILE__, __LINE__, _("Unsupported address size
> %d"), size);
>      }
>  }
> 
> @@ -1605,6 +1607,9 @@ read_encoded_value (struct comp_unit *unit,
> gdb_byte encoding,
>      case DW_EH_PE_udata2:
>        *bytes_read_ptr += 2;
>        return (base + bfd_get_16 (unit->abfd, (bfd_byte *) buf));
> +    case DW_EH_PE_udata3:
> +      *bytes_read_ptr += 3;
> +      return (base + bfd_get_24 (unit->abfd, (bfd_byte *) buf));
>      case DW_EH_PE_udata4:
>        *bytes_read_ptr += 4;
>        return (base + bfd_get_32 (unit->abfd, (bfd_byte *) buf));
> diff --git a/include/dwarf2.h b/include/dwarf2.h
> index cf0039a92a..05c328057b 100644
> --- a/include/dwarf2.h
> +++ b/include/dwarf2.h
> @@ -474,11 +474,14 @@ enum dwarf_unit_type
>  #define DW_EH_PE_udata2		0x02
>  #define DW_EH_PE_udata4		0x03
>  #define DW_EH_PE_udata8		0x04
> +
> +#define DW_EH_PE_udata3		0x05
> +
> +#define DW_EH_PE_signed		0x08
>  #define DW_EH_PE_sleb128	0x09
>  #define DW_EH_PE_sdata2		0x0A
>  #define DW_EH_PE_sdata4		0x0B
>  #define DW_EH_PE_sdata8		0x0C
> -#define DW_EH_PE_signed		0x08
> 
>  #define DW_EH_PE_pcrel		0x10
>  #define DW_EH_PE_textrel	0x20

This file is owned by GCC (see the MAINTAINERS file at the top of 
binutils-gdb).  To get a modification in it, you would need to provide a 
patch to gcc, then we can import the change in binutils-gdb.

I am not too sure who is responsible for allocating these values, as 
they are not from the DWARF standard.  At the very least, there should 
be a comment to say what architecture uses this non-standard value.  Is 
there also a gcc port you are planning to upstream, that would use this 
value?

Simon

Re: [PATCH 2/3] GDB: Add support for 24 bit addresses

[John Darrington]

On Fri, Aug 24, 2018 at 04:34:11PM -0400, Simon Marchi wrote:
     (CCing gcc-patches because of the change in include/dwarf2.h)
     
     This file is owned by GCC (see the MAINTAINERS file at the top of
     binutils-gdb).  To get a modification in it, you would need to provide a
     patch to gcc, then we can import the change in binutils-gdb.
     
     I am not too sure who is responsible for allocating these values, as they
     are not from the DWARF standard.  At the very least, there should be a
     comment to say what architecture uses this non-standard value.  Is there
     also a gcc port you are planning to upstream, that would use this value?

I might think about a gcc port when Gdb and Binutils are robust for this
arch.   But I haven't started any work on that so far.

J'
     

-- 
Avoid eavesdropping.  Send strong encrypted email.
PGP Public key ID: 1024D/2DE827B3 
fingerprint = 8797 A26D 0854 2EAB 0285  A290 8A67 719C 2DE8 27B3
See http://sks-keyservers.net or any PGP keyserver for public key.

Re: [PATCH 1/3] gdb: Added builtin types for 24 bit integers.

[Eli Zaretskii]

> From: John Darrington <john@darrington.wattle.id.au>
> Cc: John Darrington <john@darrington.wattle.id.au>
> Date: Thu, 23 Aug 2018 19:35:24 +0200
> 
> * gdb/doc/gdb.texinfo: Add them.

This should mention the name of the node in which the change is being
made.  Also, since gdb/doc/ has its own ChangeLog file, please make
the description specific, because it will be physically separate from
the other log entries.

> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
> index 5068c0ac81..e4ecd57a9e 100644
> --- a/gdb/doc/gdb.texinfo
> +++ b/gdb/doc/gdb.texinfo

This part is OK.

Thanks.

Re: [PATCH 1/3] gdb: Added builtin types for 24 bit integers.

[Simon Marchi]

On 2018-08-23 13:35, John Darrington wrote:
> * gdb/gdbtypes.h (struct builtin_type): New members builtin_int24
>   and builtin_uint24;
> * gdb/gdbtypes.c: Initialize them.
> * gdb/doc/gdb.texinfo: Add them.
> ---
>  gdb/doc/gdb.texinfo | 2 ++
>  gdb/gdbtypes.c      | 4 ++++
>  gdb/gdbtypes.h      | 2 ++
>  3 files changed, 8 insertions(+)
> 
> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
> index 5068c0ac81..e4ecd57a9e 100644
> --- a/gdb/doc/gdb.texinfo
> +++ b/gdb/doc/gdb.texinfo
> @@ -42379,6 +42379,7 @@ Boolean type, occupying a single bit.
> 
>  @item int8
>  @itemx int16
> +@itemx int24
>  @itemx int32
>  @itemx int64
>  @itemx int128
> @@ -42386,6 +42387,7 @@ Signed integer types holding the specified
> number of bits.
> 
>  @item uint8
>  @itemx uint16
> +@itemx uint24
>  @itemx uint32
>  @itemx uint64
>  @itemx uint128
> diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
> index 65b1211ec4..05bf7b1134 100644
> --- a/gdb/gdbtypes.c
> +++ b/gdb/gdbtypes.c
> @@ -5402,6 +5402,10 @@ gdbtypes_post_init (struct gdbarch *gdbarch)
>      = arch_integer_type (gdbarch, 16, 0, "int16_t");
>    builtin_type->builtin_uint16
>      = arch_integer_type (gdbarch, 16, 1, "uint16_t");
> +  builtin_type->builtin_int24
> +    = arch_integer_type (gdbarch, 24, 0, "int24_t");
> +  builtin_type->builtin_uint24
> +    = arch_integer_type (gdbarch, 24, 1, "uint24_t");
>    builtin_type->builtin_int32
>      = arch_integer_type (gdbarch, 32, 0, "int32_t");
>    builtin_type->builtin_uint32
> diff --git a/gdb/gdbtypes.h b/gdb/gdbtypes.h
> index 14059ab3dc..eb7c365b71 100644
> --- a/gdb/gdbtypes.h
> +++ b/gdb/gdbtypes.h
> @@ -1611,6 +1611,8 @@ struct builtin_type
>    struct type *builtin_uint8;
>    struct type *builtin_int16;
>    struct type *builtin_uint16;
> +  struct type *builtin_int24;
> +  struct type *builtin_uint24;
>    struct type *builtin_int32;
>    struct type *builtin_uint32;
>    struct type *builtin_int64;

Hi John,

It's always useful to have a little commit message along with the patch 
to explain why this is needed.  What's your problem and how does it help 
you solve it.  Could you expand a bit on your patches?

Thanks,

Simon

Re: [PATCH 1/3] gdb: Added builtin types for 24 bit integers.

[John Darrington]

On Thu, Aug 23, 2018 at 03:41:11PM -0400, Simon Marchi wrote:

     Hi John,
     
     It's always useful to have a little commit message along with the patch
     to explain why this is needed.  What's your problem and how does it help
     you solve it.  Could you expand a bit on your patches?
     
     Thanks,
     
Hi Simon,

Sorry, I had presumed it was self evident from the final patch, which
adds support for the S12Z target - a 24 bit architecture.

It seems that up till now there has been no 24 bit targets, so the other
two patches as some necessary things to make that possible.

J'

-- 
Avoid eavesdropping.  Send strong encrypted email.
PGP Public key ID: 1024D/2DE827B3 
fingerprint = 8797 A26D 0854 2EAB 0285  A290 8A67 719C 2DE8 27B3
See http://sks-keyservers.net or any PGP keyserver for public key.

Re: [PATCH 1/3] gdb: Added builtin types for 24 bit integers.

[Simon Marchi]

On 2018-08-23 16:03, John Darrington wrote:
> On Thu, Aug 23, 2018 at 03:41:11PM -0400, Simon Marchi wrote:
> 
>      Hi John,
> 
>      It's always useful to have a little commit message along with the 
> patch
>      to explain why this is needed.  What's your problem and how does 
> it help
>      you solve it.  Could you expand a bit on your patches?
> 
>      Thanks,
> 
> Hi Simon,
> 
> Sorry, I had presumed it was self evident from the final patch, which
> adds support for the S12Z target - a 24 bit architecture.

Well this is new information to me :).

It is clear now, but somebody doing a git blame to know why 24-bit 
integer types were added would only find the patch that adds them by 
itself and wonder who uses that.  A little message like

   This patch adds 24-bit integer types, used when debugging on the S12Z 
architecture (added by a later patch in this series).

clears that up.  That might looks a bit silly, but I think it helps in 
the long run.

> It seems that up till now there has been no 24 bit targets, so the 
> other
> two patches as some necessary things to make that possible.

Thanks.  Coming back to the code of the patch, I was wondering if these 
24-bit types are useful or even relevant for any other architecture.  
Would it work if you only defined the types for s12z architectures, 
storing the reference in the gdbarch_tdep object?

Simon

Re: [PATCH 1/3] gdb: Added builtin types for 24 bit integers.

[John Darrington]

On Thu, Aug 23, 2018 at 04:35:25PM -0400, Simon Marchi wrote:

     It is clear now, but somebody doing a git blame to know why 24-bit
     integer types were added would only find the patch that adds them by
     itself and wonder who uses that.  A little message like
     
       This patch adds 24-bit integer types, used when debugging on the S12Z
     architecture (added by a later patch in this series).
     
     clears that up.  That might looks a bit silly, but I think it helps in
     the long run.

I fully agree with you.   I've worked on other projects however had a
different opinion - they insisted that the checkin comment NOT contain
any rationale for the change, instead it should just summarize what
changed.  I find that rather pointless but anyway ....
     
     > It seems that up till now there has been no 24 bit targets, so the
     > other
     > two patches as some necessary things to make that possible.
     
     Thanks.  Coming back to the code of the patch, I was wondering if these
     24-bit types are useful or even relevant for any other architecture.

There most certainly are plenty of 24 bit architectures  especially in the 
embedded world  - just apparently none that gdb currently supports :(

     Would it work if you only defined the types for s12z architectures,
     storing the reference in the gdbarch_tdep object?

My first reaction is that it probably *could* be made to work, but not 
in an elegant fashion.   Somehow I'd have to avoid that gdb ever calls the 
read_encoded_value function. 

I do concede that  adding DW_EH_PE_udata3 might be problematic since
it's not part of the dwarf standard.  An alternative might be to rework
the read_encoded_value function to not rely on the dwarf enums (all it
really cares about is the size of the target's address space.

Regards

John




-- 
Avoid eavesdropping.  Send strong encrypted email.
PGP Public key ID: 1024D/2DE827B3 
fingerprint = 8797 A26D 0854 2EAB 0285  A290 8A67 719C 2DE8 27B3
See http://sks-keyservers.net or any PGP keyserver for public key.

Re: [PATCH 1/3] gdb: Added builtin types for 24 bit integers.

[Simon Marchi]

[-- Attachment #1: Type: text/plain, Size: 2804 bytes --]

On 2018-08-24 02:11, John Darrington wrote:
> On Thu, Aug 23, 2018 at 04:35:25PM -0400, Simon Marchi wrote:
> 
>      It is clear now, but somebody doing a git blame to know why 24-bit
>      integer types were added would only find the patch that adds them 
> by
>      itself and wonder who uses that.  A little message like
> 
>        This patch adds 24-bit integer types, used when debugging on the 
> S12Z
>      architecture (added by a later patch in this series).
> 
>      clears that up.  That might looks a bit silly, but I think it 
> helps in
>      the long run.
> 
> I fully agree with you.   I've worked on other projects however had a
> different opinion - they insisted that the checkin comment NOT contain
> any rationale for the change, instead it should just summarize what
> changed.  I find that rather pointless but anyway ....

Well, if you look at our commit history, you'll see we like to be 
verbose :).

>      > It seems that up till now there has been no 24 bit targets, so 
> the
>      > other
>      > two patches as some necessary things to make that possible.
> 
>      Thanks.  Coming back to the code of the patch, I was wondering if 
> these
>      24-bit types are useful or even relevant for any other 
> architecture.
> 
> There most certainly are plenty of 24 bit architectures  especially in 
> the
> embedded world  - just apparently none that gdb currently supports :(
> 
>      Would it work if you only defined the types for s12z 
> architectures,
>      storing the reference in the gdbarch_tdep object?
> 
> My first reaction is that it probably *could* be made to work, but not
> in an elegant fashion.   Somehow I'd have to avoid that gdb ever calls 
> the
> read_encoded_value function.

I'm not sure I understand.  I was only talking about the definition of 
the int24_t and uint24_t types, not the handling of DW_EH_PE_udata3.  
 From what I read, the C99 standard mandates that the 8, 16, 32 and 64 
variants of the intX_t/uintX_t types exist.  Other types (with other 
values of X) would be extensions.  That's why I thought it would make 
sense to define that in the s12z-specific gdbarches only.  In the end I 
don't really mind, but it just looks like the "clean" way to do it and 
doesn't seem really more difficult.  Can you see if the attached diff 
(applied on top of your series) work for you?

And as far as I understand, this is disconnected from the handling of 
DW_EH_PE_udata3.

> I do concede that  adding DW_EH_PE_udata3 might be problematic since
> it's not part of the dwarf standard.  An alternative might be to rework
> the read_encoded_value function to not rely on the dwarf enums (all it
> really cares about is the size of the target's address space.

I'll take a look at that patch (2/3) separately and reply to it.

Simon

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Move-builtin_uint24_t-type-to-s12z-tdep.c.patch --]
[-- Type: text/x-diff; name=0001-Move-builtin_uint24_t-type-to-s12z-tdep.c.patch, Size: 2813 bytes --]

From e0902733a29726ba107c41980bdbd8500261c852 Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@ericsson.com>
Date: Fri, 24 Aug 2018 11:03:45 -0400
Subject: [PATCH] Move builtin_uint24_t type to s12z-tdep.c

---
 gdb/gdbtypes.c  |  4 ----
 gdb/gdbtypes.h  |  2 --
 gdb/s12z-tdep.c | 16 +++++++++++-----
 3 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
index 05bf7b1..65b1211 100644
--- a/gdb/gdbtypes.c
+++ b/gdb/gdbtypes.c
@@ -5402,10 +5402,6 @@ gdbtypes_post_init (struct gdbarch *gdbarch)
     = arch_integer_type (gdbarch, 16, 0, "int16_t");
   builtin_type->builtin_uint16
     = arch_integer_type (gdbarch, 16, 1, "uint16_t");
-  builtin_type->builtin_int24
-    = arch_integer_type (gdbarch, 24, 0, "int24_t");
-  builtin_type->builtin_uint24
-    = arch_integer_type (gdbarch, 24, 1, "uint24_t");
   builtin_type->builtin_int32
     = arch_integer_type (gdbarch, 32, 0, "int32_t");
   builtin_type->builtin_uint32
diff --git a/gdb/gdbtypes.h b/gdb/gdbtypes.h
index eb7c365..14059ab 100644
--- a/gdb/gdbtypes.h
+++ b/gdb/gdbtypes.h
@@ -1611,8 +1611,6 @@ struct builtin_type
   struct type *builtin_uint8;
   struct type *builtin_int16;
   struct type *builtin_uint16;
-  struct type *builtin_int24;
-  struct type *builtin_uint24;
   struct type *builtin_int32;
   struct type *builtin_uint32;
   struct type *builtin_int64;
diff --git a/gdb/s12z-tdep.c b/gdb/s12z-tdep.c
index 5169025..48af422 100644
--- a/gdb/s12z-tdep.c
+++ b/gdb/s12z-tdep.c
@@ -76,6 +76,11 @@ static const int reg_perm[N_PHYSICAL_REGISTERS] =
    REG_CCW
   };
 
+struct gdbarch_tdep
+{
+  type *builtin_uint24;
+};
+
 static const char *
 s12z_register_name (struct gdbarch *gdbarch, int regnum)
 {
@@ -138,7 +143,10 @@ s12z_register_type (struct gdbarch *gdbarch, int reg_nr)
     case 2:
       return builtin_type (gdbarch)->builtin_uint16;
     case 3:
-      return builtin_type (gdbarch)->builtin_uint24;
+      {
+	struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
+	return tdep->builtin_uint24;
+      }
     case 4:
       return builtin_type (gdbarch)->builtin_uint32;
     default:
@@ -347,10 +355,6 @@ 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)
@@ -358,6 +362,8 @@ s12z_gdbarch_init (struct gdbarch_info info,
   struct gdbarch_tdep *tdep = (struct gdbarch_tdep *) xmalloc (sizeof *tdep);
   struct gdbarch *gdbarch = gdbarch_alloc (&info, tdep);
 
+  tdep->builtin_uint24 = arch_integer_type (gdbarch, 24, 1, "uint24_t");
+
   /* Target data types.  */
   set_gdbarch_short_bit (gdbarch, 16);
   set_gdbarch_int_bit (gdbarch, 16);
-- 
2.7.4

Re: [PATCH 1/3] gdb: Added builtin types for 24 bit integers.

[John Darrington]

On Fri, Aug 24, 2018 at 11:09:40AM -0400, Simon Marchi wrote:

     I'm not sure I understand.  I was only talking about the definition of
     the int24_t and uint24_t types, not the handling of DW_EH_PE_udata3.
     From what I read, the C99 standard mandates that the 8, 16, 32 and 64
     variants of the intX_t/uintX_t types exist.  Other types (with other
     values of X) would be extensions.  That's why I thought it would make
     sense to define that in the s12z-specific gdbarches only.  In the end I
     don't really mind, but it just looks like the "clean" way to do it and
     doesn't seem really more difficult.  Can you see if the attached diff
     (applied on top of your series) work for you?
     

Yes, I see no reason why that shouldn't work.  Like you say, C99 et. al.
doesn't require uint24_t but doesn't prohibit it either.  I just
thought, that it makes sense to keep all these type definitions in the
same place so as to avoid the next 24 bit arch having to copy this
definition.   But I don't have a strong opinion either way.

J'

Re: [PATCH 1/3] gdb: Added builtin types for 24 bit integers.

[Simon Marchi]

On 2018-08-24 11:29, John Darrington wrote:
> On Fri, Aug 24, 2018 at 11:09:40AM -0400, Simon Marchi wrote:
> 
>      I'm not sure I understand.  I was only talking about the 
> definition of
>      the int24_t and uint24_t types, not the handling of 
> DW_EH_PE_udata3.
>      From what I read, the C99 standard mandates that the 8, 16, 32 and 
> 64
>      variants of the intX_t/uintX_t types exist.  Other types (with 
> other
>      values of X) would be extensions.  That's why I thought it would 
> make
>      sense to define that in the s12z-specific gdbarches only.  In the 
> end I
>      don't really mind, but it just looks like the "clean" way to do it 
> and
>      doesn't seem really more difficult.  Can you see if the attached 
> diff
>      (applied on top of your series) work for you?
> 
> 
> Yes, I see no reason why that shouldn't work.  Like you say, C99 et. 
> al.
> doesn't require uint24_t but doesn't prohibit it either.  I just
> thought, that it makes sense to keep all these type definitions in the
> same place so as to avoid the next 24 bit arch having to copy this
> definition.   But I don't have a strong opinion either way.

I see, then that's fine with mean to have it in gdbarch.

Simon