[PATCH 1/4] gdb: Add builtin types for 24 bit integers.

[PATCH 1/4] gdb: Add builtin types for 24 bit integers.

[John Darrington]

Add int24 and uint24.  These are used by the upcoming S12Z target, but will be
needed for any arch which features 24 bit registers.

* gdb/gdbtypes.h (struct builtin_type): New members builtin_int24
  and builtin_uint24;
* gdb/gdbtypes.c: Initialize them.
* gdb/doc/gdb.texinfo (Predefined Target Types): Mention types int24 and uint24.
---
 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/4] GDB: Add support for 24 bit addresses

[John Darrington]

* 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 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

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));
-- 
2.11.0

S12Z [new patchset]

[John Darrington]

Here is a revised patch set including earlier suggestions.

I still don't see the test failures that were mentioned, so
either this version has fixed them or they don't manifest themselves
on my environment.

[PATCH 4/4] GDB: New target s12z

[John Darrington]

gdb/
    * configure.tgt: Add configuration for s12z.
    * s12z-tdep.c:  New file.
    * NEWS: Mention new target.
---
 gdb/Makefile.in   |   1 +
 gdb/NEWS          |   4 +
 gdb/configure.tgt |   5 +
 gdb/s12z-tdep.c   | 446 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 456 insertions(+)
 create mode 100644 gdb/s12z-tdep.c

diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index 118c3c8062..f448d1ee19 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -758,6 +758,7 @@ ALL_TARGET_OBS = \
 	rs6000-lynx178-tdep.o \
 	rs6000-tdep.o \
 	rx-tdep.o \
+	s12z-tdep.o \
 	s390-linux-tdep.o \
 	s390-tdep.o \
 	score-tdep.o \
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..5573413db4 100644
--- a/gdb/configure.tgt
+++ b/gdb/configure.tgt
@@ -643,6 +643,11 @@ spu*-*-*)
 	build_gdbserver=yes
 	;;
 
+s12z-*-*)
+	# Target: Freescale S12z
+	gdb_target_obs="s12z-tdep.o"
+	;;
+
 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..1df33c0a7c
--- /dev/null
+++ b/gdb/s12z-tdep.c
@@ -0,0 +1,446 @@
+/* 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 "dwarf2-frame.h"
+#include "errors.h"
+#include "frame-unwind.h"
+#include "gdbcore.h"
+#include "inferior.h"
+#include "opcode/s12z.h"
+#include "trad-frame.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)
+{
+  /*  registers is declared in opcodes/s12z.h */
+  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;
+	}
+    }
+
+  warning (_("%s Failed to find end of prologue PC = %08x\n"),
+                      __FUNCTION__, (unsigned int) pc);
+
+  return 0;
+}
+
+/* Implement the unwind_pc gdbarch method.  */
+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 */
+
+static struct trad_frame_cache *
+s12z_frame_cache (struct frame_info *this_frame, void **prologue_cache)
+{
+  int frame_size = 0;
+  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)
+    {
+      warning (_("  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;
+
+  /* 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);
+	    }
+        }
+
+      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 const char ccw_bits[] =
+  {
+   'C',  /* Carry */
+   'V',  /* Two's Complement Overflow */
+   'Z',  /* Zero */
+   'N',  /* Negative */
+   'I',  /* Interrupt */
+   '-',
+   'X',  /* Non-Maskable Interrupt */
+   'S',  /* STOP Disable */
+   '0',  /*
+   '0',   *  Interrupt priority level
+   '0',   */
+   '-',
+   '-',
+   '-',
+   '-',
+   'U'   /* User/Supervisor State */
+  };
+
+/*
+   Print a human readable representation of the CCW register.
+   For example: "u----000SX-Inzvc" corresponds to the value
+   0xD0
+*/
+static void
+s12z_print_ccw_info (struct gdbarch *gdbarch,
+                     struct ui_file *file,
+                     struct frame_info *frame,
+                     int reg)
+{
+  struct value *v = value_of_register (reg, frame);
+  const char *name = gdbarch_register_name (gdbarch, reg);
+  uint32_t ccw = value_as_long (v);
+  fputs_filtered (name, file);
+  size_t len = strlen (name);
+  const int stop_1 = 15;
+  const int stop_2 = 17;
+  int i;
+  for (i = 0; i < stop_1 - len; ++i)
+    fputc_filtered (' ', file);
+  fprintf_filtered (file, "0x%04x", ccw);
+  for (i = 0; i < stop_2 - len; ++i)
+    fputc_filtered (' ', file);
+  int b;
+  for (b = 15; b >= 0; --b)
+    {
+      if (ccw & (0x1u << b))
+        {
+          if (ccw_bits[b] == 0)
+            fputc_filtered ('1', file);
+          else
+            fputc_filtered (ccw_bits[b], file);
+        }
+      else
+        fputc_filtered (tolower (ccw_bits[b]), file);
+    }
+  fputc_filtered ('\n', file);
+}
+
+static void
+s12z_print_registers_info (struct gdbarch *gdbarch,
+			    struct ui_file *file,
+			    struct frame_info *frame,
+			    int regnum, int print_all)
+{
+  const int numregs = gdbarch_num_regs (gdbarch)
+		      + gdbarch_num_pseudo_regs (gdbarch);
+
+  if (regnum == -1)
+    {
+      int reg;
+      for (reg = 0; reg < numregs; reg++)
+        {
+          if (REG_CCW == reg_perm[reg])
+            {
+              s12z_print_ccw_info (gdbarch, file, frame, reg);
+              continue;
+            }
+          default_print_registers_info (gdbarch, file, frame, reg, print_all);
+        }
+    }
+  else if (REG_CCW == reg_perm[regnum])
+    s12z_print_ccw_info (gdbarch, file, frame, regnum);
+  else
+    default_print_registers_info (gdbarch, file, frame, regnum, print_all);
+}
+
+static struct gdbarch *
+s12z_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
+{
+  struct gdbarch_tdep *tdep = XNEW (struct gdbarch_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_print_registers_info (gdbarch, s12z_print_registers_info);
+
+  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, N_PHYSICAL_REGISTERS);
+  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

[PATCH 2/4] Add a dwarf unit type to represent 24 bit values.

[John Darrington]

	* include/dwarf2.h (enum dwarf_unit_type) [DE_EH_PE_udata3]: New member.
---
 include/dwarf2.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/dwarf2.h b/include/dwarf2.h
index cf0039a92a..0fe88a3a7e 100644
--- a/include/dwarf2.h
+++ b/include/dwarf2.h
@@ -474,6 +474,7 @@ 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_sleb128	0x09
 #define DW_EH_PE_sdata2		0x0A
 #define DW_EH_PE_sdata4		0x0B
-- 
2.11.0

Re: [PATCH 2/4] Add a dwarf unit type to represent 24 bit values.

[Tom Tromey]

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

John> 	* include/dwarf2.h (enum dwarf_unit_type) [DE_EH_PE_udata3]: New member.
John> ---
John>  include/dwarf2.h | 1 +
John>  1 file changed, 1 insertion(+)

I'm afraid I didn't look at the earlier discussion of this.

dwarf2.h is canonically maintained in the gcc repository.  So, any
change here has to be sent to gcc-patches and be committed there first.
Then the patch can be applied to gdb -- this counts as obvious (IMO).

Tom

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

[Tom Tromey]

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

Re: [PATCH 1/4] gdb: Add builtin types for 24 bit integers.

[Tom Tromey]

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

John> Add int24 and uint24.  These are used by the upcoming S12Z target, but will be
John> needed for any arch which features 24 bit registers.

John> * gdb/gdbtypes.h (struct builtin_type): New members builtin_int24
John>   and builtin_uint24;
John> * gdb/gdbtypes.c: Initialize them.
John> * gdb/doc/gdb.texinfo (Predefined Target Types): Mention types int24 and uint24.

The non-documentation parts are ok.

Tom

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

[Tom Tromey]

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

John> * gdb/dwarf2-frame.c (encoding_for_size): Deal with case 3.
John>                 (read_encoded_value): Deal with case DW_EH_PE_udata3

This is ok; pending the dwarf2.h patch of course.

John>      default:
John> -      internal_error (__FILE__, __LINE__, _("Unsupported address size"));
John> +      internal_error (__FILE__, __LINE__, _("Unsupported address size %d"), size);

Thanks for doing that.

Tom

Re: S12Z [new patchset]

[Simon Marchi]

On 2018-08-29 03:18 PM, John Darrington wrote:
> Here is a revised patch set including earlier suggestions.
> 
> I still don't see the test failures that were mentioned, so
> either this version has fixed them or they don't manifest themselves
> on my environment.
> 

I still see it.  Are you sure you have an --enable-targets=all build?  What
configure line do you use?

Simon

Re: S12Z [new patchset]

[John Darrington]

On Fri, Sep 07, 2018 at 11:44:53PM +0100, Simon Marchi wrote:
     On 2018-08-29 03:18 PM, John Darrington wrote:
     > Here is a revised patch set including earlier suggestions.
     > 
     > I still don't see the test failures that were mentioned, so
     > either this version has fixed them or they don't manifest themselves
     > on my environment.
     > 
     
     I still see it.  Are you sure you have an --enable-targets=all build?  What
     configure line do you use?
     

I used:

/home/john/binutils-gdb/configure --target=s12z --enable-targets=all CFLAGS="-g -O0" CXXFLAGS="-O0 -g" --prefix=$(mktemp -d -p /Scratch/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/4] gdb: Add builtin types for 24 bit integers.

[John Darrington]

On Fri, Sep 07, 2018 at 04:03:38PM -0600, Tom Tromey wrote:
     >>>>> "John" == John Darrington <john@darrington.wattle.id.au> writes:
     
     John> Add int24 and uint24.  These are used by the upcoming S12Z target, but will be
     John> needed for any arch which features 24 bit registers.
     
     John> * gdb/gdbtypes.h (struct builtin_type): New members builtin_int24
     John>   and builtin_uint24;
     John> * gdb/gdbtypes.c: Initialize them.
     John> * gdb/doc/gdb.texinfo (Predefined Target Types): Mention types int24 and uint24.
     
     The non-documentation parts are ok.
     
... and what's wrong with the documentation part ?

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 4/4] GDB: New target s12z

[John Darrington]

On Fri, Sep 07, 2018 at 04:03:17PM -0600, Tom Tromey wrote:
     >>>>> "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 suspect not.  In fact, I'm very dubious about the whole
skip_prologue_using_sal function.  It seems to not work very well. 
     
     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'm afraid I can't.  This was code I took over from the or1k target.

Re: [PATCH 1/4] gdb: Add builtin types for 24 bit integers.

[Tom Tromey]

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

Tom>      The non-documentation parts are ok.
     
John> ... and what's wrong with the documentation part ?

Eli reviews those.

Tom

Re: S12Z [new patchset]

[John Darrington]

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

On Sat, Sep 08, 2018 at 06:18:01AM +0200, John Darrington wrote:
          
          I still see it.  Are you sure you have an --enable-targets=all build?  What
          configure line do you use?
          
     
     I used:
     
     /home/john/binutils-gdb/configure --target=s12z --enable-targets=all CFLAGS="-g -O0" CXXFLAGS="-O0 -g" --prefix=$(mktemp -d -p /Scratch/john)
     

Now I do see it.

Can you apply the attached patch, and see if it fixes it for you?

J'

[-- Attachment #2: 0001-Add-ARCH_s12z-to-the-ARCH_all-case.patch --]
[-- Type: text/x-diff, Size: 662 bytes --]

From df16439ce7de10b6f1c72c6974f544cdf806e9da Mon Sep 17 00:00:00 2001
From: John Darrington <john@darrington.wattle.id.au>
Date: Sat, 8 Sep 2018 06:59:09 +0200
Subject: [PATCH] Add ARCH_s12z to the ARCH_all case.

* opcodes/disassemble.c: Define ARCH_s12z when ARCH_all is defined.
---
 opcodes/disassemble.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/opcodes/disassemble.c b/opcodes/disassemble.c
index c5941826f0..ce83423b6d 100644
--- a/opcodes/disassemble.c
+++ b/opcodes/disassemble.c
@@ -77,6 +77,7 @@
 #define ARCH_rs6000
 #define ARCH_rl78
 #define ARCH_rx
+#define ARCH_s12z
 #define ARCH_s390
 #define ARCH_score
 #define ARCH_sh
-- 
2.11.0

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

[Tom Tromey]

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

Tom>      Is this stuff useful?
     
John> I suspect not.  In fact, I'm very dubious about the whole
John> skip_prologue_using_sal function.  It seems to not work very well. 
     
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));
     
Tom>      Could you explain this more?

John> I'm afraid I can't.  This was code I took over from the or1k target.

Ok; in that case I think you should drop these parts from the patch.

Tom

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

[John Darrington]

On Sat, Sep 08, 2018 at 07:16:04AM -0600, Tom Tromey wrote:
     >>>>> "John" == John Darrington <john@darrington.wattle.id.au> writes:
     
     Tom>      Is this stuff useful?
          
     John> I suspect not.  In fact, I'm very dubious about the whole
     John> skip_prologue_using_sal function.  It seems to not work very well. 
          
     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));
          
     Tom>      Could you explain this more?
     
     John> I'm afraid I can't.  This was code I took over from the or1k target.
     
     Ok; in that case I think you should drop these parts from the patch.
     

I will run some tests to see what ill efects there are (if any) without
these bits.

J'

Re: S12Z [new patchset]

[Simon Marchi]

On 2018-09-08 07:30 AM, John Darrington wrote:
> On Sat, Sep 08, 2018 at 06:18:01AM +0200, John Darrington wrote:
>           
>           I still see it.  Are you sure you have an --enable-targets=all build?  What
>           configure line do you use?
>           
>      
>      I used:
>      
>      /home/john/binutils-gdb/configure --target=s12z --enable-targets=all CFLAGS="-g -O0" CXXFLAGS="-O0 -g" --prefix=$(mktemp -d -p /Scratch/john)
>      
> 
> Now I do see it.
> 
> Can you apply the attached patch, and see if it fixes it for you?
> 
> J'
> 

Yep, that seems right (and I see the patch has already been applied).

Thanks,

Simon

Re: [PATCH 2/4] Add a dwarf unit type to represent 24 bit values.

[John Darrington]

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

After discussion with the gcc folks, it seems that there is an easier
and much simpler solution, which I'm attaching.

J'

On Fri, Sep 07, 2018 at 03:50:00PM -0600, Tom Tromey wrote:
     >>>>> "John" == John Darrington <john@darrington.wattle.id.au> writes:
     
     John> 	* include/dwarf2.h (enum dwarf_unit_type) [DE_EH_PE_udata3]: New member.
     John> ---
     John>  include/dwarf2.h | 1 +
     John>  1 file changed, 1 insertion(+)
     
     I'm afraid I didn't look at the earlier discussion of this.
     
     dwarf2.h is canonically maintained in the gcc repository.  So, any
     change here has to be sent to gcc-patches and be committed there first.
     Then the patch can be applied to gdb -- this counts as obvious (IMO).
     
     Tom

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


[-- Attachment #2: 0001-GDB-Don-t-abort-if-the-address-size-is-3-bytes.patch --]
[-- Type: text/x-diff, Size: 702 bytes --]

From f2948209c0292d836ade4dda222c1ffe4aa9abe4 Mon Sep 17 00:00:00 2001
From: John Darrington <john@darrington.wattle.id.au>
Date: Wed, 19 Sep 2018 10:28:43 +0200
Subject: [PATCH] GDB: Don't abort if the address size is 3 bytes.

* gdb/dwarf2-frame.c (encoding_for_size): Deal with the case where size == 3.
---
 gdb/dwarf2-frame.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/gdb/dwarf2-frame.c b/gdb/dwarf2-frame.c
index 55ab081bff..dfae2aaf97 100644
--- a/gdb/dwarf2-frame.c
+++ b/gdb/dwarf2-frame.c
@@ -1527,6 +1527,7 @@ encoding_for_size (unsigned int size)
     {
     case 2:
       return DW_EH_PE_udata2;
+    case 3:
     case 4:
       return DW_EH_PE_udata4;
     case 8:
-- 
2.11.0

Re: [PATCH 2/4] Add a dwarf unit type to represent 24 bit values.

[Simon Marchi]

On 2018-09-26 13:41, John Darrington wrote:
> After discussion with the gcc folks, it seems that there is an easier
> and much simpler solution, which I'm attaching.
> 
> J'

Err can you explain how this works, or link to the gcc discussion if 
it's explained there?  I must be missing something, because as far as I 
understand, this will read 4 bytes when reading an address, when I 
suppose it's encoded with 3 bytes in the DWARF info, isn't it?

Simon

Re: [PATCH 2/4] Add a dwarf unit type to represent 24 bit values.

[John Darrington]

On Wed, Sep 26, 2018 at 10:53:34PM -0400, Simon Marchi wrote:
     On 2018-09-26 13:41, John Darrington wrote:
     > After discussion with the gcc folks, it seems that there is an easier
     > and much simpler solution, which I'm attaching.
     > 
     > J'
     
     Err can you explain how this works, or link to the gcc discussion if it's
     explained there?  I must be missing something, because as far as I
     understand, this will read 4 bytes when reading an address, when I
     suppose it's encoded with 3 bytes in the DWARF info, isn't it?
     
     Simon

The relevant discussion is here: https://gcc.gnu.org/ml/gcc-patches/2018-09/msg00712.html

The bottom line is that the dwarf address size does not need to
correspond to the machine address size (and in general it does not) - 
it just needs to be at least as long.

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 2/4] Add a dwarf unit type to represent 24 bit values.

[Simon Marchi]

On 2018-09-27 01:49, John Darrington wrote:
> On Wed, Sep 26, 2018 at 10:53:34PM -0400, Simon Marchi wrote:
>      On 2018-09-26 13:41, John Darrington wrote:
>      > After discussion with the gcc folks, it seems that there is an 
> easier
>      > and much simpler solution, which I'm attaching.
>      >
>      > J'
> 
>      Err can you explain how this works, or link to the gcc discussion 
> if it's
>      explained there?  I must be missing something, because as far as I
>      understand, this will read 4 bytes when reading an address, when I
>      suppose it's encoded with 3 bytes in the DWARF info, isn't it?
> 
>      Simon
> 
> The relevant discussion is here:
> https://gcc.gnu.org/ml/gcc-patches/2018-09/msg00712.html
> 
> The bottom line is that the dwarf address size does not need to
> correspond to the machine address size (and in general it does not) -
> it just needs to be at least as long.

After a short discussion with John on IRC, what I understand is that the 
debug info he is dealing with encodes the 24-bit addresses on 32-bits, 
with the most significant byte equal to zero.  Therefore that revised 
patch looks ok to me.  I would just ask you to add a comment explaining 
that this "case" exists for producer XYZ on S12Z, which encodes 24 bit 
addresses on 32 bits.  Maybe that in the future somebody will stumble on 
a producer that encodes 24 bit addresses on 24 bits.  Having an 
explanation for why things are the way they are will help.

Thanks,

Simon

Re: [PATCH 2/4] Add a dwarf unit type to represent 24 bit values.

[John Darrington]

On Thu, Sep 27, 2018 at 01:53:35PM -0400, Simon Marchi wrote:
     
     After a short discussion with John on IRC, what I understand is that the
     debug info he is dealing with encodes the 24-bit addresses on 32-bits,
     with the most significant byte equal to zero.  Therefore that revised
     patch looks ok to me.  I would just ask you to add a comment explaining
     that this "case" exists for producer XYZ on S12Z, which encodes 24 bit
     addresses on 32 bits.  Maybe that in the future somebody will stumble on
     a producer that encodes 24 bit addresses on 24 bits.  Having an
     explanation for why things are the way they are will help.
     

When I look at the situation in more detail a number of things are
apparent:

1.  The dwarf .frame_debug CFI information does indeed code these values
    as 32 bits.

2.  This would seem to contradict the dwarf 2.0 standard which says they are 
    "addressing-unit sized values".

3.  objdump expects these values to be 24 bits.

4.  However readelf expects them to be 32 bits.

5.  Notwithstanding the above, the produced dwarf which I have, contains
    lots of FDE entries, with lots of CFIs.  However the contents of all 
    CFIs are empty.   Consequently it's of little use.

Given the above, I think the best course of action is to do nothing, on 
this point.

So I'll do without a dwarf unwinder until there's a producer which
actually produces something useful.

J'