[PATCH 0/4] Initial support for FreeBSD/riscv

[PATCH 0/4] Initial support for FreeBSD/riscv

[John Baldwin]

This series adds support for FreeBSD/riscv targets.  Note that I've
attempted to make the port support either RV32 or RV64, but FreeBSD
currently only supports RV64.

Patch 1 tries to make it easier to write handlers for signal frame by
allowing the register map structures used with register caches to be
used with trad-frame to supply a block of registers at a starting
address.  It gets somewhat squishy when thinking about how to handle
registers whose size doesn't match the "slot" size in a register map.
I've attempted to make the trad-frame handling match the semantics
that regcache uses.  However, these semantics aren't documented
anywhere and we should perhaps document them.  Also, in this patch I
used 'void *' for the map only because it matches what the regcache
functions do.  I'm happy to make the map argument type-safe instead if
others prefer that.  Also, the comments for regcache_map_entry should
perhaps be made more generic to say it isn't specific to regcache but
is used to describe the layout of a register block.  Arguably the type
should even be renamed to something less regcache-specific
(register_map_entry?).  If we do adopt this patch I will probably make
use of it in some other FreeBSD architectures (aarch64 and arm at
least, possibly x86).

Patch 2's commit log is a bit preachy perhaps and can be toned down if
needed.  I'm curious if my thoughts about using target descriptions
are correct?  It is what x86 does to handle 32-bit vs 64-bit as well
as handling various optional register sets.

John Baldwin (4):
  Add helper functions to trad_frame to support register cache maps.
  Fall back to a default value of 0 for the MISA register.
  Add FreeBSD/riscv architecture.
  Add native target for FreeBSD/riscv.

 gdb/ChangeLog         |  30 ++++++
 gdb/Makefile.in       |   4 +
 gdb/NEWS              |   2 +
 gdb/configure.host    |   1 +
 gdb/configure.nat     |   4 +
 gdb/configure.tgt     |   5 +
 gdb/doc/ChangeLog     |   5 +
 gdb/doc/gdb.texinfo   |   6 ++
 gdb/riscv-fbsd-nat.c  | 135 +++++++++++++++++++++++++++
 gdb/riscv-fbsd-tdep.c | 206 ++++++++++++++++++++++++++++++++++++++++++
 gdb/riscv-fbsd-tdep.h |  33 +++++++
 gdb/riscv-tdep.c      |  13 ++-
 gdb/trad-frame.c      |  69 ++++++++++++++
 gdb/trad-frame.h      |   8 ++
 14 files changed, 519 insertions(+), 2 deletions(-)
 create mode 100644 gdb/riscv-fbsd-nat.c
 create mode 100644 gdb/riscv-fbsd-tdep.c
 create mode 100644 gdb/riscv-fbsd-tdep.h

-- 
2.18.0

[PATCH 2/4] Fall back to a default value of 0 for the MISA register.

[John Baldwin]

The riscv architecture supports multiple architectures with differing
register sizes.  If the MISA register is present, the specific
architecture is chosen based on its value, otherwise properties are
inferred from flags in the ELF header.  However, the code to read MISA
throws an exception if the register is not present.  This does not
trip when cross-debugging a core dump, but does trigger when
attempting to debug native processes if the native target does not
provide MISA.

MISA is a machine-mode register in RISC-V which may or may not be
available to supervisor operating systems.  Rather than requiring
targets to always provide a fake MISA value, fallback to 0 for now.
(The Linux native target currently always provides a fake value of 0
for MISA.)

Longer term, the riscv architecture should perhaps add target
descriptions for the various sub-architectures and permit targets to
set a description.  It would then only use MISA as a fallback if an
explicit description is not provided.  This will permit the proper
register set to be used when debugging a RV32 process (where U-XLEN in
SSTATUS is set to 32 bits) on an RV64 host (where XLEN in MISA
indicates 64 bits) for example by using the U-XLEN field in SSTATUS to
set the target description (RV32 vs RV64) for individual processes.

gdb/ChangeLog:

	* riscv-tdep.c (riscv_read_misa_reg): Fall back to a default value
	of zero.
---
 gdb/ChangeLog    |  5 +++++
 gdb/riscv-tdep.c | 13 +++++++++++--
 2 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index e1892f531a..9b8a5175b0 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,8 @@
+2018-09-19  John Baldwin  <jhb@FreeBSD.org>
+
+	* riscv-tdep.c (riscv_read_misa_reg): Fall back to a default value
+	of zero.
+
 2018-09-19  John Baldwin  <jhb@FreeBSD.org>
 
 	* trad-frame.c (trad_frame_set_regmap, trad_frame_set_reg_regmap):
diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c
index 254914c9c7..4b385ed5da 100644
--- a/gdb/riscv-tdep.c
+++ b/gdb/riscv-tdep.c
@@ -317,9 +317,18 @@ riscv_read_misa_reg (bool *read_p)
 	}
       CATCH (ex, RETURN_MASK_ERROR)
 	{
-	  /* Old cores might have MISA located at a different offset.  */
-	  value = get_frame_register_unsigned (frame,
+	  TRY
+	    {
+	      /* Old cores might have MISA located at a different offset.  */
+	      value
+		= get_frame_register_unsigned (frame,
 					       RISCV_CSR_LEGACY_MISA_REGNUM);
+	    }
+	  CATCH (ex, RETURN_MASK_ERROR)
+	    {
+	      value = 0;
+	    }
+	  END_CATCH
 	}
       END_CATCH
     }
-- 
2.18.0

[PATCH 3/4] Add FreeBSD/riscv architecture.

[John Baldwin]

Support for collecting and supplying general purpose and floating
point register sets is provided along with signal frame unwinding.

FreeBSD only supports RV64 currently, so while some provision is made
for RV32 in the general-purpose register set, the changes have only
been tested on RV64.

gdb/ChangeLog:

	* Makefile.in (ALL_TARGET_OBS): Add riscv-fbsd-tdep.o.
	(HFILES_NO_SRCDIR): Add riscv-fbsd-tdep.h.
	(ALLDEPFILES): Add riscv-fbsd-tdep.c.
	* NEWS: Mention new FreeBSD/riscv target.
	* configure.tgt: Add riscv*-*-freebsd*.
	* riscv-fbsd-tdep.c: New file.
	* riscv-fbsd-tdep.h: New file.
---
 gdb/ChangeLog         |  10 ++
 gdb/Makefile.in       |   3 +
 gdb/NEWS              |   1 +
 gdb/configure.tgt     |   5 +
 gdb/riscv-fbsd-tdep.c | 206 ++++++++++++++++++++++++++++++++++++++++++
 gdb/riscv-fbsd-tdep.h |  33 +++++++
 6 files changed, 258 insertions(+)
 create mode 100644 gdb/riscv-fbsd-tdep.c
 create mode 100644 gdb/riscv-fbsd-tdep.h

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 9b8a5175b0..5b4996028b 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,13 @@
+2018-09-19  John Baldwin  <jhb@FreeBSD.org>
+
+	* Makefile.in (ALL_TARGET_OBS): Add riscv-fbsd-tdep.o.
+	(HFILES_NO_SRCDIR): Add riscv-fbsd-tdep.h.
+	(ALLDEPFILES): Add riscv-fbsd-tdep.c.
+	* NEWS: Mention new FreeBSD/riscv target.
+	* configure.tgt: Add riscv*-*-freebsd*.
+	* riscv-fbsd-tdep.c: New file.
+	* riscv-fbsd-tdep.h: New file.
+
 2018-09-19  John Baldwin  <jhb@FreeBSD.org>
 
 	* riscv-tdep.c (riscv_read_misa_reg): Fall back to a default value
diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index 87eb825aa9..3e46130134 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -745,6 +745,7 @@ ALL_TARGET_OBS = \
 	ppc-sysv-tdep.o \
 	ppc64-tdep.o \
 	ravenscar-thread.o \
+	riscv-fbsd-tdep.o \
 	riscv-linux-tdep.o \
 	riscv-tdep.o \
 	rl78-tdep.o \
@@ -1339,6 +1340,7 @@ HFILES_NO_SRCDIR = \
 	remote.h \
 	remote-fileio.h \
 	remote-notif.h \
+	riscv-fbsd-tdep.h \
 	riscv-tdep.h \
 	rs6000-aix-tdep.h \
 	rs6000-tdep.h \
@@ -2307,6 +2309,7 @@ ALLDEPFILES = \
 	procfs.c \
 	ravenscar-thread.c \
 	remote-sim.c \
+	riscv-fbsd-tdep.c \
 	riscv-linux-nat.c \
 	riscv-linux-tdep.c \
 	riscv-tdep.c \
diff --git a/gdb/NEWS b/gdb/NEWS
index a1936ca1cc..a191ae2714 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -86,6 +86,7 @@ GNU/Linux/RISC-V		riscv*-*-linux*
 GNU/Linux/RISC-V		riscv*-*-linux*
 CSKY ELF			csky*-*-elf
 CSKY GNU/LINUX			csky*-*-linux
+FreeBSD/riscv			riscv*-*-freebsd*
 
 * Python API
 
diff --git a/gdb/configure.tgt b/gdb/configure.tgt
index 6d1a4df84a..3b94942732 100644
--- a/gdb/configure.tgt
+++ b/gdb/configure.tgt
@@ -528,6 +528,11 @@ s390*-*-linux*)
 	build_gdbserver=yes
 	;;
 
+riscv*-*-freebsd*)
+	# Target: FreeBSD/riscv
+	gdb_target_obs="riscv-fbsd-tdep.o riscv-tdep.o"
+	;;
+
 riscv*-*-linux*)
 	# Target: Linux/RISC-V
 	gdb_target_obs="riscv-linux-tdep.o riscv-tdep.o glibc-tdep.o \
diff --git a/gdb/riscv-fbsd-tdep.c b/gdb/riscv-fbsd-tdep.c
new file mode 100644
index 0000000000..c09ec650ca
--- /dev/null
+++ b/gdb/riscv-fbsd-tdep.c
@@ -0,0 +1,206 @@
+/* Target-dependent code for FreeBSD on RISC-V processors.
+   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/>.  */
+
+#include "defs.h"
+#include "fbsd-tdep.h"
+#include "osabi.h"
+#include "riscv-tdep.h"
+#include "riscv-fbsd-tdep.h"
+#include "solib-svr4.h"
+#include "target.h"
+#include "trad-frame.h"
+#include "tramp-frame.h"
+
+/* Register maps.  */
+
+static const struct regcache_map_entry riscv_fbsd_gregmap[] =
+  {
+    { 1, RISCV_RA_REGNUM, 0 },
+    { 1, RISCV_SP_REGNUM, 0 },
+    { 1, RISCV_GP_REGNUM, 0 },
+    { 1, RISCV_TP_REGNUM, 0 },
+    { 3, 5, 0 },		/* t0 - t2 */
+    { 4, 28, 0 },		/* t3 - t6 */
+    { 2, RISCV_FP_REGNUM, 0 },	/* s0 - s1 */
+    { 10, 18, 0 },		/* s2 - s11 */
+    { 8, RISCV_A0_REGNUM, 0 },	/* a0 - a7 */
+    { 1, RISCV_PC_REGNUM, 0 },
+    { 1, RISCV_CSR_SSTATUS_REGNUM, 0 },
+    { 0 }
+  };
+
+static const struct regcache_map_entry riscv_fbsd_fpregmap[] =
+  {
+    { 32, RISCV_FIRST_FP_REGNUM, 16 },
+    { 1, RISCV_CSR_FCSR_REGNUM, 8 },
+    { 0 }
+  };
+
+/* Supply the general-purpose registers stored in GREGS to REGCACHE.
+   This function only exists to supply the always-zero x0 in addition
+   to the registers in GREGS.  */
+
+static void
+riscv_fbsd_supply_gregset (const struct regset *regset,
+			   struct regcache *regcache, int regnum,
+			   const void *gregs, size_t len)
+{
+  regcache->supply_regset (&riscv_fbsd_gregset, regnum, gregs, len);
+  if (regnum == -1 || regnum == RISCV_ZERO_REGNUM)
+    regcache->raw_supply_zeroed (RISCV_ZERO_REGNUM);
+}
+
+/* Register set definitions.  */
+
+const struct regset riscv_fbsd_gregset =
+  {
+    riscv_fbsd_gregmap,
+    riscv_fbsd_supply_gregset, regcache_collect_regset
+  };
+
+const struct regset riscv_fbsd_fpregset =
+  {
+    riscv_fbsd_fpregmap,
+    regcache_supply_regset, regcache_collect_regset
+  };
+
+/* Implement the "regset_from_core_section" gdbarch method.  */
+
+static void
+riscv_fbsd_iterate_over_regset_sections (struct gdbarch *gdbarch,
+					 iterate_over_regset_sections_cb *cb,
+					 void *cb_data,
+					 const struct regcache *regcache)
+{
+  cb (".reg", RISCV_FBSD_NUM_GREGS * riscv_isa_xlen (gdbarch),
+      RISCV_FBSD_NUM_GREGS * riscv_isa_xlen (gdbarch),
+      &riscv_fbsd_gregset, NULL, cb_data);
+  cb (".reg2", RISCV_FBSD_SIZEOF_FPREGSET, RISCV_FBSD_SIZEOF_FPREGSET,
+      &riscv_fbsd_fpregset, NULL, cb_data);
+}
+
+/* In a signal frame, sp points to a 'struct sigframe' which is
+   defined as:
+
+   struct sigframe {
+	   siginfo_t	sf_si;
+	   ucontext_t	sf_uc;
+   };
+
+   ucontext_t is defined as:
+
+   struct __ucontext {
+	   sigset_t	uc_sigmask;
+	   mcontext_t	uc_mcontext;
+	   ...
+   };
+
+   The mcontext_t contains the general purpose register set followed
+   by the floating point register set.  The floating point register
+   set is only valid if the _MC_FP_VALID flag is set in mc_flags.  */
+
+#define RISCV_SIGFRAME_UCONTEXT_OFFSET 		80
+#define RISCV_UCONTEXT_MCONTEXT_OFFSET		16
+#define RISCV_MCONTEXT_FLAG_FP_VALID		0x1
+
+/* Implement the "init" method of struct tramp_frame.  */
+
+static void
+riscv_fbsd_sigframe_init (const struct tramp_frame *self,
+			  struct frame_info *this_frame,
+			  struct trad_frame_cache *this_cache,
+			  CORE_ADDR func)
+{
+  struct gdbarch *gdbarch = get_frame_arch (this_frame);
+  enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
+  CORE_ADDR sp = get_frame_register_unsigned (this_frame, RISCV_SP_REGNUM);
+  CORE_ADDR mcontext_addr =
+    sp
+    + RISCV_SIGFRAME_UCONTEXT_OFFSET
+    + RISCV_UCONTEXT_MCONTEXT_OFFSET;
+  gdb_byte buf[4];
+  int i;
+
+  trad_frame_set_reg_regmap (this_cache, riscv_fbsd_gregmap, mcontext_addr,
+			     RISCV_FBSD_NUM_GREGS * riscv_isa_xlen (gdbarch));
+
+  CORE_ADDR fpregs_addr
+    = mcontext_addr + RISCV_FBSD_NUM_GREGS * riscv_isa_xlen (gdbarch);
+  CORE_ADDR fp_flags_addr
+    = fpregs_addr + RISCV_FBSD_SIZEOF_FPREGSET;
+  if (target_read_memory (fp_flags_addr, buf, 4) == 0
+      && (extract_unsigned_integer (buf, 4, byte_order)
+	  & RISCV_MCONTEXT_FLAG_FP_VALID))
+    trad_frame_set_reg_regmap (this_cache, riscv_fbsd_fpregmap, fpregs_addr,
+			       RISCV_FBSD_SIZEOF_FPREGSET);
+
+  trad_frame_set_id (this_cache, frame_id_build (sp, func));
+}
+
+/* RISC-V supports 16-bit instructions ("C") as well as 32-bit
+   instructions.  The signal trampoline on FreeBSD uses a mix of
+   these, but tramp_frame assumes a fixed instruction size.  To cope,
+   claim that all instructions are 16 bits and use two "slots" for
+   32-bit instructions.  */
+
+static const struct tramp_frame riscv_fbsd_sigframe =
+{
+  SIGTRAMP_FRAME,
+  2,
+  {
+    {0x850a, ULONGEST_MAX},		/* mov  a0, sp  */
+    {0x0513, ULONGEST_MAX},		/* addi a0, a0, #SF_UC  */
+    {0x0505, ULONGEST_MAX},
+    {0x0293, ULONGEST_MAX},		/* li   t0, #SYS_sigreturn  */
+    {0x1a10, ULONGEST_MAX},
+    {0x0073, ULONGEST_MAX},		/* ecall  */
+    {0x0000, ULONGEST_MAX},
+    {TRAMP_SENTINEL_INSN, ULONGEST_MAX}
+  },
+  riscv_fbsd_sigframe_init
+};
+
+/* Implement the 'init_osabi' method of struct gdb_osabi_handler.  */
+
+static void
+riscv_fbsd_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
+{
+  struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
+
+  /* Generic FreeBSD support.  */
+  fbsd_init_abi (info, gdbarch);
+
+  set_gdbarch_software_single_step (gdbarch, riscv_software_single_step);
+
+  set_solib_svr4_fetch_link_map_offsets (gdbarch,
+					 (riscv_isa_xlen (gdbarch) == 4
+					  ? svr4_ilp32_fetch_link_map_offsets
+					  : svr4_lp64_fetch_link_map_offsets));
+
+  tramp_frame_prepend_unwinder (gdbarch, &riscv_fbsd_sigframe);
+
+  set_gdbarch_iterate_over_regset_sections
+    (gdbarch, riscv_fbsd_iterate_over_regset_sections);
+}
+
+void
+_initialize_riscv_fbsd_tdep (void)
+{
+  gdbarch_register_osabi (bfd_arch_riscv, 0, GDB_OSABI_FREEBSD,
+			  riscv_fbsd_init_abi);
+}
diff --git a/gdb/riscv-fbsd-tdep.h b/gdb/riscv-fbsd-tdep.h
new file mode 100644
index 0000000000..8b6abd565b
--- /dev/null
+++ b/gdb/riscv-fbsd-tdep.h
@@ -0,0 +1,33 @@
+/* FreeBSD/riscv target support, prototypes.
+
+   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/>.  */
+
+#include "regset.h"
+
+/* The general-purpose regset consists of 31 X registers, EPC, and
+   SSTATUS.  */
+#define RISCV_FBSD_NUM_GREGS		33
+
+/* The fp regset always consists of 32 128-bit registers, plus a
+   64-bit CSR_FCSR.  If 'Q' is not supported, only the low 64-bits of
+   each floating point register are valid.  If 'D' is not supported,
+   only the low 32-bits of each floating point register are valid.  */
+#define RISCV_FBSD_SIZEOF_FPREGSET (32 * 16 + 8)
+
+extern const struct regset riscv_fbsd_gregset;
+extern const struct regset riscv_fbsd_fpregset;
-- 
2.18.0

[PATCH 1/4] Add helper functions to trad_frame to support register cache maps.

[John Baldwin]

Currently, signal frame handlers require explicitly coded calls to
trad_frame_set_reg_addr() to describe the location of saved registers
within a signal frame.  This change permits the regcache_map_entry
arrays used with regcache::supply_regset and regcache::collect_regset
to be used to describe a block of saved registers given an initial
address for the register block.

Some systems use the same layout for registers in core dump notes,
native register sets with ptrace(), and the register contexts saved in
signal frames.  On these systems, a single register map can now be
used to describe the layout of registers in all three places.

If a register map entry's size does not match the native size of a
register, try to match the semantics used by
regcache::transfer_regset.  If a register slot is too large, assume
that the register's value is stored in the first N bytes and ignore
the remaning bytes.  If the register slot is smaller than the
register, assume the slot holds the low N bytes of the register's
value.  Read these low N bytes from the target and zero-extend them to
generate a register value.

gdb/ChangeLog:

	* trad-frame.c (trad_frame_set_regmap, trad_frame_set_reg_regmap):
	New.
	* trad-frame.h (trad_frame_set_regmap, trad_frame_set_reg_regmap):
	New.
---
 gdb/ChangeLog    |  7 +++++
 gdb/trad-frame.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++++
 gdb/trad-frame.h |  8 ++++++
 3 files changed, 84 insertions(+)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index dfdde50b26..e1892f531a 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,10 @@
+2018-09-19  John Baldwin  <jhb@FreeBSD.org>
+
+	* trad-frame.c (trad_frame_set_regmap, trad_frame_set_reg_regmap):
+	New.
+	* trad-frame.h (trad_frame_set_regmap, trad_frame_set_reg_regmap):
+	New.
+
 2018-09-19  Xavier Roirand  <roirand@adacore.com>
 
 	PR gdb/20981:
diff --git a/gdb/trad-frame.c b/gdb/trad-frame.c
index fd9f24084d..e09a333499 100644
--- a/gdb/trad-frame.c
+++ b/gdb/trad-frame.c
@@ -22,6 +22,7 @@
 #include "trad-frame.h"
 #include "regcache.h"
 #include "frame-unwind.h"
+#include "target.h"
 #include "value.h"
 
 struct trad_frame_cache
@@ -125,6 +126,64 @@ trad_frame_set_addr (struct trad_frame_saved_reg this_saved_regs[],
   this_saved_regs[regnum].addr = addr;
 }
 
+void
+trad_frame_set_regmap (struct trad_frame_saved_reg this_saved_regs[],
+		       struct gdbarch *gdbarch, const void *regmap,
+		       CORE_ADDR addr, size_t size)
+{
+  const struct regcache_map_entry *map;
+  int offs = 0, count;
+
+  for (map = (const struct regcache_map_entry *) regmap;
+       (count = map->count) != 0;
+       map++)
+    {
+      int regno = map->regno;
+      int slot_size = map->size;
+
+      if (slot_size == 0 && regno != REGCACHE_MAP_SKIP)
+	slot_size = register_size (gdbarch, regno);
+
+      if (offs + slot_size > size)
+	break;
+
+      if (regno == REGCACHE_MAP_SKIP)
+	offs += count * slot_size;
+      else
+	for (; count--; regno++, offs += slot_size)
+	  {
+	    /* Mimic the semantics of regcache::transfer_regset if a
+	       register slot's size does not match the size of a
+	       register.
+
+	       If a register slot is larger than a register, assume
+	       the register's value is stored in the first N bytes of
+	       the slot and ignore the remaining bytes.
+
+	       If the register slot is smaller than the register,
+	       assume that the slot contains the low N bytes of the
+	       register's value.  Since trad_frame assumes that
+	       registers stored by address are sized according to the
+	       register, read the low N bytes and zero-extend them to
+	       generate a register value.  */
+	    if (slot_size >= register_size (gdbarch, regno))
+	      trad_frame_set_addr (this_saved_regs, regno, addr + offs);
+	    else
+	      {
+		enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
+		gdb_byte buf[slot_size];
+
+		if (target_read_memory (addr + offs, buf, sizeof buf) == 0)
+		  {
+		    LONGEST val
+		      = extract_unsigned_integer (buf, sizeof buf, byte_order);
+		    trad_frame_set_value (this_saved_regs, regno, val);
+		  }
+	      }
+	  }
+    }
+}
+
 void
 trad_frame_set_reg_value (struct trad_frame_cache *this_trad_cache,
 			  int regnum, LONGEST val)
@@ -148,6 +207,16 @@ trad_frame_set_reg_addr (struct trad_frame_cache *this_trad_cache,
   trad_frame_set_addr (this_trad_cache->prev_regs, regnum, addr);
 }
 
+void
+trad_frame_set_reg_regmap (struct trad_frame_cache *this_trad_cache,
+			   const void *regmap, CORE_ADDR addr, size_t size)
+{
+  struct gdbarch *gdbarch = get_frame_arch (this_trad_cache->this_frame);
+
+  trad_frame_set_regmap (this_trad_cache->prev_regs, gdbarch, regmap, addr,
+			 size);
+}
+
 void
 trad_frame_set_unknown (struct trad_frame_saved_reg this_saved_regs[],
 			int regnum)
diff --git a/gdb/trad-frame.h b/gdb/trad-frame.h
index a11a2c29c9..1fc1859996 100644
--- a/gdb/trad-frame.h
+++ b/gdb/trad-frame.h
@@ -45,6 +45,9 @@ void trad_frame_set_reg_realreg (struct trad_frame_cache *this_trad_cache,
 				 int regnum, int realreg);
 void trad_frame_set_reg_addr (struct trad_frame_cache *this_trad_cache,
 			      int regnum, CORE_ADDR addr);
+void trad_frame_set_reg_regmap (struct trad_frame_cache *this_trad_cache,
+				const void *regmap, CORE_ADDR addr,
+				size_t size);
 void trad_frame_set_reg_value (struct trad_frame_cache *this_cache,
 			       int regnum, LONGEST val);
 
@@ -96,6 +99,11 @@ void trad_frame_set_realreg (struct trad_frame_saved_reg this_saved_regs[],
 void trad_frame_set_addr (struct trad_frame_saved_reg this_trad_cache[],
 			  int regnum, CORE_ADDR addr);
 
+/* Encode registers in REGMAP are at address ADDR in the trad-frame.  */
+void trad_frame_set_regmap (struct trad_frame_saved_reg this_saved_regs[],
+			    struct gdbarch *gdbarch, const void *regmap,
+			    CORE_ADDR addr, size_t size);
+
 /* Mark REGNUM as unknown.  */
 void trad_frame_set_unknown (struct trad_frame_saved_reg this_saved_regs[],
 			     int regnum);
-- 
2.18.0

[PATCH 4/4] Add native target for FreeBSD/riscv.

[John Baldwin]

gdb/ChangeLog:

	* Makefile.in (ALLDEPFILES): Add riscv-fbsd-nat.c.
	* NEWS: Mention new FreeBSD/riscv native configuration.
	* configure.host: Add riscv*-*-freebsd*.
	* configure.nat: Likewise.
	* riscv-fbsd-nat.c: New file.

gdb/doc/ChangeLog:

	* gdb.texinfo (Contributors): Add SRI International and University
	of Cambridge for FreeBSD/riscv.
---
 gdb/ChangeLog        |   8 +++
 gdb/Makefile.in      |   1 +
 gdb/NEWS             |   1 +
 gdb/configure.host   |   1 +
 gdb/configure.nat    |   4 ++
 gdb/doc/ChangeLog    |   5 ++
 gdb/doc/gdb.texinfo  |   6 ++
 gdb/riscv-fbsd-nat.c | 135 +++++++++++++++++++++++++++++++++++++++++++
 8 files changed, 161 insertions(+)
 create mode 100644 gdb/riscv-fbsd-nat.c

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 5b4996028b..75032b7a4d 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,11 @@
+2018-09-19  John Baldwin  <jhb@FreeBSD.org>
+
+	* Makefile.in (ALLDEPFILES): Add riscv-fbsd-nat.c.
+	* NEWS: Mention new FreeBSD/riscv native configuration.
+	* configure.host: Add riscv*-*-freebsd*.
+	* configure.nat: Likewise.
+	* riscv-fbsd-nat.c: New file.
+
 2018-09-19  John Baldwin  <jhb@FreeBSD.org>
 
 	* Makefile.in (ALL_TARGET_OBS): Add riscv-fbsd-tdep.o.
diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index 3e46130134..1cda285663 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -2309,6 +2309,7 @@ ALLDEPFILES = \
 	procfs.c \
 	ravenscar-thread.c \
 	remote-sim.c \
+	riscv-fbsd-nat.c \
 	riscv-fbsd-tdep.c \
 	riscv-linux-nat.c \
 	riscv-linux-tdep.c \
diff --git a/gdb/NEWS b/gdb/NEWS
index a191ae2714..4ae3adedc0 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -80,6 +80,7 @@ thread apply [all | COUNT | -COUNT] [FLAG]... COMMAND
 * New native configurations
 
 GNU/Linux/RISC-V		riscv*-*-linux*
+FreeBSD/riscv			riscv*-*-freebsd*
 
 * New targets
 
diff --git a/gdb/configure.host b/gdb/configure.host
index 23a2f16399..c87f997abc 100644
--- a/gdb/configure.host
+++ b/gdb/configure.host
@@ -149,6 +149,7 @@ powerpc64*-*-linux*)	gdb_host=ppc64-linux
 			;;
 powerpc*-*-linux*)	gdb_host=linux ;;
 
+riscv*-*-freebsd*)	gdb_host=fbsd ;;
 riscv*-*-linux*)	gdb_host=linux ;;
 
 s390*-*-linux*)		gdb_host=linux ;;
diff --git a/gdb/configure.nat b/gdb/configure.nat
index feddeaa5e0..31afcaa5c0 100644
--- a/gdb/configure.nat
+++ b/gdb/configure.nat
@@ -177,6 +177,10 @@ case ${gdb_host} in
 		# systems running FreeBSD.
 		NATDEPFILES="${NATDEPFILES} ppc-fbsd-nat.o bsd-kvm.o"
 		;;
+	    riscv*)
+		# Host: FreeBSD/riscv
+		NATDEPFILES="${NATDEPFILES} riscv-fbsd-nat.o"
+		;;
 	    sparc)
 		# Host: FreeBSD/sparc64
 		NATDEPFILES="${NATDEPFILES} sparc-nat.o sparc64-nat.o \
diff --git a/gdb/doc/ChangeLog b/gdb/doc/ChangeLog
index 6d12553a28..452d7d38ea 100644
--- a/gdb/doc/ChangeLog
+++ b/gdb/doc/ChangeLog
@@ -1,3 +1,8 @@
+2018-09-19  John Baldwin  <jhb@FreeBSD.org>
+
+	* gdb.texinfo (Contributors): Add SRI International and University
+	of Cambridge for FreeBSD/riscv.
+
 2018-09-18  John Baldwin  <jhb@FreeBSD.org>
 
 	* gdb.texinfo (info proc): Remove "running".
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index b5b6089153..b8af74e949 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -546,6 +546,12 @@ was developed by SRI International and the University of Cambridge
 Computer Laboratory under DARPA/AFRL contract FA8750-10-C-0237
 ("CTSRD"), as part of the DARPA CRASH research programme.
 
+Initial support for the FreeBSD/riscv target and native configuration
+was developed by SRI International and the University of Cambridge
+Computer Laboratory (Department of Computer Science and Technology)
+under DARPA contract HR0011-18-C-0016 ("ECATS"), as part of the DARPA
+SSITH research programme.
+
 The original port to the OpenRISC 1000 is believed to be due to
 Alessandro Forin and Per Bothner.  More recent ports have been the work
 of Jeremy Bennett, Franck Jullien, Stefan Wallentowitz and
diff --git a/gdb/riscv-fbsd-nat.c b/gdb/riscv-fbsd-nat.c
new file mode 100644
index 0000000000..ad4ea1e0ad
--- /dev/null
+++ b/gdb/riscv-fbsd-nat.c
@@ -0,0 +1,135 @@
+/* Native-dependent code for FreeBSD/riscv.
+
+   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/>.  */
+
+#include "defs.h"
+#include "target.h"
+
+#include <sys/types.h>
+#include <sys/ptrace.h>
+#include <machine/reg.h>
+
+#include "fbsd-nat.h"
+#include "riscv-tdep.h"
+#include "riscv-fbsd-tdep.h"
+#include "inf-ptrace.h"
+
+struct riscv_fbsd_nat_target final : public fbsd_nat_target
+{
+  void fetch_registers (struct regcache *, int) override;
+  void store_registers (struct regcache *, int) override;
+};
+
+static riscv_fbsd_nat_target the_riscv_fbsd_nat_target;
+
+/* Determine if PT_GETREGS fetches REGNUM.  */
+
+static bool
+getregs_supplies (struct gdbarch *gdbarch, int regnum)
+{
+  return (regnum >= RISCV_RA_REGNUM && regnum <= RISCV_PC_REGNUM);
+}
+
+/* Determine if PT_GETFPREGS fetches REGNUM.  */
+
+static bool
+getfpregs_supplies (struct gdbarch *gdbarch, int regnum)
+{
+  return ((regnum >= RISCV_FIRST_FP_REGNUM && regnum <= RISCV_LAST_FP_REGNUM)
+	  || regnum == RISCV_CSR_FCSR_REGNUM);
+}
+
+/* Fetch register REGNUM from the inferior.  If REGNUM is -1, do this
+   for all registers.  */
+
+void
+riscv_fbsd_nat_target::fetch_registers (struct regcache *regcache,
+					int regnum)
+{
+  pid_t pid = get_ptrace_pid (regcache->ptid ());
+
+  struct gdbarch *gdbarch = regcache->arch ();
+  if (regnum == -1 || regnum == RISCV_ZERO_REGNUM)
+    regcache->raw_supply_zeroed (RISCV_ZERO_REGNUM);
+  if (regnum == -1 || getregs_supplies (gdbarch, regnum))
+    {
+      struct reg regs;
+
+      if (ptrace (PT_GETREGS, pid, (PTRACE_TYPE_ARG3) &regs, 0) == -1)
+	perror_with_name (_("Couldn't get registers"));
+
+      regcache->supply_regset (&riscv_fbsd_gregset, regnum, &regs,
+			       sizeof (regs));
+    }
+
+  if (regnum == -1 || getfpregs_supplies (gdbarch, regnum))
+    {
+      struct fpreg fpregs;
+
+      if (ptrace (PT_GETFPREGS, pid, (PTRACE_TYPE_ARG3) &fpregs, 0) == -1)
+	perror_with_name (_("Couldn't get floating point status"));
+
+      regcache->supply_regset (&riscv_fbsd_fpregset, regnum, &fpregs,
+			       sizeof (fpregs));
+    }
+}
+
+/* Store register REGNUM back into the inferior.  If REGNUM is -1, do
+   this for all registers.  */
+
+void
+riscv_fbsd_nat_target::store_registers (struct regcache *regcache,
+					int regnum)
+{
+  pid_t pid = get_ptrace_pid (regcache->ptid ());
+
+  struct gdbarch *gdbarch = regcache->arch ();
+  if (regnum == -1 || getregs_supplies (gdbarch, regnum))
+    {
+      struct reg regs;
+
+      if (ptrace (PT_GETREGS, pid, (PTRACE_TYPE_ARG3) &regs, 0) == -1)
+	perror_with_name (_("Couldn't get registers"));
+
+      regcache->collect_regset (&riscv_fbsd_gregset, regnum, &regs,
+			       sizeof (regs));
+
+      if (ptrace (PT_SETREGS, pid, (PTRACE_TYPE_ARG3) &regs, 0) == -1)
+	perror_with_name (_("Couldn't write registers"));
+    }
+
+  if (regnum == -1 || getfpregs_supplies (gdbarch, regnum))
+    {
+      struct fpreg fpregs;
+
+      if (ptrace (PT_GETFPREGS, pid, (PTRACE_TYPE_ARG3) &fpregs, 0) == -1)
+	perror_with_name (_("Couldn't get floating point status"));
+
+      regcache->collect_regset (&riscv_fbsd_fpregset, regnum, &fpregs,
+				sizeof (fpregs));
+
+      if (ptrace (PT_SETFPREGS, pid, (PTRACE_TYPE_ARG3) &fpregs, 0) == -1)
+	perror_with_name (_("Couldn't write floating point status"));
+    }
+}
+
+void
+_initialize_riscv_fbsd_nat (void)
+{
+  add_inf_child_target (&the_riscv_fbsd_nat_target);
+}
-- 
2.18.0

Re: [PATCH 2/4] Fall back to a default value of 0 for the MISA register.

[Jim Wilson]

On Wed, Sep 19, 2018 at 4:20 PM John Baldwin <jhb@freebsd.org> wrote:
> MISA is a machine-mode register in RISC-V which may or may not be
> available to supervisor operating systems.  Rather than requiring
> targets to always provide a fake MISA value, fallback to 0 for now.
> (The Linux native target currently always provides a fake value of 0
> for MISA.)

The linux target provides a fake value for misa because gdb is a side
project for me, and I haven't had time to add ptrace support for it
yet.  Too many binutils and gcc problems I need to fix first.

I think providing a default misa value in riscv-tdep.c is a mistake.
One major problem is with the use of compressed breakpoints.  Consider
the case where the target has compressed instruction support, the C
library was compiled to use compressed instructions, and the user
program was compiled without compressed instruction support.  If we
can't read misa, then we check the program ELF headers, see that there
is no compressed instruction support, and start using 4-byte
breakpoints.  Storing a 4 byte breakpoint into a library compiled with
compressed instructions means that we will be clobbering instructions
and things start breaking badly.  I ran into this earlier before
fixing some bugs in the breakpoint support in riscv-tdep.c.  Glibc
provides a stub function for gdb support that happens to be exactly 2
bytes long.  Storing a 4-byte breakpoint there clobbers the first
instruction of the next function, which happens to be an important
function for program startup, which means we get an illegal
instruction error before even reaching main which is very confusing.

If we can read misa, then we don't have this problem, as we will
always know if the hardware supports compressed breakpoints, and we
can avoid this problem.  If we can't read misa, then we will have to
check the ELF headers of every shared library used by a program,
including the interpreter ld.so, to see if any of them use compressed
instructions.  We will also have to check every library loaded at
run-time via dlload.  This seems like a hassle.  Another option might
be to try to execute a 2-byte breakpoint to see if it works or not,
and then use compressed breakpoints if they work, but this also seems
like a hassle.  Reading misa seems like the easy solution, all I need
is a linux kernel patch to add ptrace support for reading it.  If we
have access to misa, then we also know whether FP registers are
present, and their size, which is also important info that we can't
easily get any other way.  misa is roughly equivalent to x86 cpuid.  I
don't know of any reason why misa wouldn't be available to the OS.

I suppose another option here might be the auxvec/hwcap info.  Maybe
info about compressed instructions and FP support can be passed from
the kernel to the user program this way, and then maybe gdb can get
the info from there, if gdb has some way to read and parse
auxvec/hwcap info passed to the user program.

Jim

Re: [PATCH 2/4] Fall back to a default value of 0 for the MISA register.

[John Baldwin]

On 9/19/18 5:09 PM, Jim Wilson wrote:
> On Wed, Sep 19, 2018 at 4:20 PM John Baldwin <jhb@freebsd.org> wrote:
>> MISA is a machine-mode register in RISC-V which may or may not be
>> available to supervisor operating systems.  Rather than requiring
>> targets to always provide a fake MISA value, fallback to 0 for now.
>> (The Linux native target currently always provides a fake value of 0
>> for MISA.)
> 
> The linux target provides a fake value for misa because gdb is a side
> project for me, and I haven't had time to add ptrace support for it
> yet.  Too many binutils and gcc problems I need to fix first.
> 
> I think providing a default misa value in riscv-tdep.c is a mistake.
> One major problem is with the use of compressed breakpoints.  Consider
> the case where the target has compressed instruction support, the C
> library was compiled to use compressed instructions, and the user
> program was compiled without compressed instruction support.  If we
> can't read misa, then we check the program ELF headers, see that there
> is no compressed instruction support, and start using 4-byte
> breakpoints.  Storing a 4 byte breakpoint into a library compiled with
> compressed instructions means that we will be clobbering instructions
> and things start breaking badly.  I ran into this earlier before
> fixing some bugs in the breakpoint support in riscv-tdep.c.  Glibc
> provides a stub function for gdb support that happens to be exactly 2
> bytes long.  Storing a 4-byte breakpoint there clobbers the first
> instruction of the next function, which happens to be an important
> function for program startup, which means we get an illegal
> instruction error before even reaching main which is very confusing.
> 
> If we can read misa, then we don't have this problem, as we will
> always know if the hardware supports compressed breakpoints, and we
> can avoid this problem.  If we can't read misa, then we will have to
> check the ELF headers of every shared library used by a program,
> including the interpreter ld.so, to see if any of them use compressed
> instructions.  We will also have to check every library loaded at
> run-time via dlload.  This seems like a hassle.  Another option might
> be to try to execute a 2-byte breakpoint to see if it works or not,
> and then use compressed breakpoints if they work, but this also seems
> like a hassle.  Reading misa seems like the easy solution, all I need
> is a linux kernel patch to add ptrace support for reading it.  If we
> have access to misa, then we also know whether FP registers are
> present, and their size, which is also important info that we can't
> easily get any other way.  misa is roughly equivalent to x86 cpuid.  I
> don't know of any reason why misa wouldn't be available to the OS.
> 
> I suppose another option here might be the auxvec/hwcap info.  Maybe
> info about compressed instructions and FP support can be passed from
> the kernel to the user program this way, and then maybe gdb can get
> the info from there, if gdb has some way to read and parse
> auxvec/hwcap info passed to the user program.

GDB certainly can examine AT_ values (it already has to read AT_BASE
to find the runtime linker and from there the list of shared libraries,
etc.).  For some things like F vs D vs Q we probably want to have different
target descriptions (this is how x86 manages x86 vs SSE vs AVX) and have
the native target's ::read_description method return a suitable description.

Whether or not 'C' is present is a bit more subtle as it isn't something
you could infer just from a register being present or not.  On the other
hand, for breakpoints, I wonder if a more straightforward approach would
be to examine the opcode of the existing instruction and use that to
decide which breakpoint to use?  That is, read the first byte of the
existing instruction and if the low 2 bits indicate a 16-bit instruction
use C.BREAK and otherwise use BREAK?

-- 
John Baldwin

                                                                            

Re: [PATCH 4/4] Add native target for FreeBSD/riscv.

[Eli Zaretskii]

> From: John Baldwin <jhb@FreeBSD.org>
> Cc: andrew.burgess@embecosm.com,	jimw@sifive.com,	palmer@sifive.com
> Date: Wed, 19 Sep 2018 16:19:50 -0700
> 
> gdb/ChangeLog:
> 
> 	* Makefile.in (ALLDEPFILES): Add riscv-fbsd-nat.c.
> 	* NEWS: Mention new FreeBSD/riscv native configuration.
> 	* configure.host: Add riscv*-*-freebsd*.
> 	* configure.nat: Likewise.
> 	* riscv-fbsd-nat.c: New file.
> 
> gdb/doc/ChangeLog:
> 
> 	* gdb.texinfo (Contributors): Add SRI International and University
> 	of Cambridge for FreeBSD/riscv.

OK for the documentation parts.

Thanks.

Re: [PATCH 2/4] Fall back to a default value of 0 for the MISA register.

[John Baldwin]

On 9/19/18 5:40 PM, John Baldwin wrote:
> Whether or not 'C' is present is a bit more subtle as it isn't something
> you could infer just from a register being present or not.  On the other
> hand, for breakpoints, I wonder if a more straightforward approach would
> be to examine the opcode of the existing instruction and use that to
> decide which breakpoint to use?  That is, read the first byte of the
> existing instruction and if the low 2 bits indicate a 16-bit instruction
> use C.BREAK and otherwise use BREAK?

This seems to work for me in my testing (I used 'stepi' to step across a
set of instructions with a mix of regular and 'C' instructions and it
worked correctly).  You can use 'set debug riscv breakpoints 1' to see what
it decides each time it sets a breakpoint.  I haven't yet tested if this
means I can remove the MISA patch, but I suspect it does as the only
place that needs MISA after this is the riscv_isa_flen() function.

Author: John Baldwin <jhb@FreeBSD.org>
Date:   Thu Sep 20 10:12:22 2018 -0700

    Use the existing instruction to determine the RISC-V breakpoint kind.
    
    RISC-V supports instructions of varying lengths.  Standard existing
    instructions in the base ISA are 4 bytes in length, but the 'C'
    extension adds support for compressed, 2 byte instructions.  RISC-V
    supports two different breakpoint instructions: EBREAK is a 4 byte
    instruction in the base ISA, and C.EBREAK is a 2 byte instruction only
    available on processors implementing the 'C' extension.  Using EBREAK
    to set breakpoints on compressed instructions causes problems as the
    second half of EBREAK will overwrite the first 2 bytes of the
    following instruction breaking other threads in the process if their
    PC is the following instruction.  Thus, breakpoints on compressed
    instructions need to use C.EBREAK instead of EBREAK.
    
    Previously, the riscv architecture checked the MISA register to
    determine if the 'C' extension was available.  If so, it used C.EBREAK
    for all breakpoints.  However, the MISA register is not necessarily
    available to supervisor mode operating systems.  While native targets
    could provide a fake MISA register value, this patch instead examines
    the existing instruction at a breakpoint target to determine which
    breakpoint instruction to use.  If the existing instruction is a
    compressed instruction, C.EBREAK is used, otherwise EBREAK is used.
    
    gdb/ChangeLog:
    
            * riscv-tdep.c (show_use_compressed_breakpoints): Remove
            'additional_info' and related logic.
            (riscv_debug_breakpoints): New variable.
            (riscv_breakpoint_kind_from_pc): Use the length of the existing
            instruction to determine the breakpoint kind.
            (_initialize_riscv_tdep): Add 'set/show debug riscv breakpoints'
            flag.  Update description of 'set/show riscv
            use-compressed-breakpoints' flag.

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 75032b7a4d..df4561d319 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,14 @@
+2018-09-20  John Baldwin  <jhb@FreeBSD.org>
+
+	* riscv-tdep.c (show_use_compressed_breakpoints): Remove
+	'additional_info' and related logic.
+	(riscv_debug_breakpoints): New variable.
+	(riscv_breakpoint_kind_from_pc): Use the length of the existing
+	instruction to determine the breakpoint kind.
+	(_initialize_riscv_tdep): Add 'set/show debug riscv breakpoints'
+	flag.  Update description of 'set/show riscv
+	use-compressed-breakpoints' flag.
+
 2018-09-19  John Baldwin  <jhb@FreeBSD.org>
 
 	* Makefile.in (ALLDEPFILES): Add riscv-fbsd-nat.c.
diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c
index 4b385ed5da..cd8dbaf9f6 100644
--- a/gdb/riscv-tdep.c
+++ b/gdb/riscv-tdep.c
@@ -210,20 +210,9 @@ show_use_compressed_breakpoints (struct ui_file *file, int from_tty,
 				 struct cmd_list_element *c,
 				 const char *value)
 {
-  const char *additional_info;
-  struct gdbarch *gdbarch = target_gdbarch ();
-
-  if (use_compressed_breakpoints == AUTO_BOOLEAN_AUTO)
-    if (riscv_has_feature (gdbarch, 'C'))
-      additional_info = _(" (currently on)");
-    else
-      additional_info = _(" (currently off)");
-  else
-    additional_info = "";
-
   fprintf_filtered (file,
 		    _("Debugger's use of compressed breakpoints is set "
-		      "to %s%s.\n"), value, additional_info);
+		      "to %s.\n"), value);
 }
 
 /* The set and show lists for 'set riscv' and 'show riscv' prefixes.  */
@@ -284,6 +273,11 @@ show_riscv_debug_variable (struct ui_file *file, int from_tty,
 		    c->name, value);
 }
 
+/* When this is set to non-zero debugging information about breakpoint
+   kinds will be printed.  */
+
+static unsigned int riscv_debug_breakpoints = 0;
+
 /* When this is set to non-zero debugging information about inferior calls
    will be printed.  */
 
@@ -426,7 +420,22 @@ riscv_breakpoint_kind_from_pc (struct gdbarch *gdbarch, CORE_ADDR *pcptr)
 {
   if (use_compressed_breakpoints == AUTO_BOOLEAN_AUTO)
     {
-      if (riscv_has_feature (gdbarch, 'C'))
+      enum bfd_endian byte_order = gdbarch_byte_order_for_code (gdbarch);
+      gdb_byte buf[1];
+      int status;
+
+      /* Read the opcode byte to determine the instruction length.  */
+      status = target_read_memory (*pcptr, buf, 1);
+      if (status)
+	memory_error (TARGET_XFER_E_IO, *pcptr);
+
+      if (riscv_debug_breakpoints)
+	fprintf_unfiltered
+	  (gdb_stdlog,
+	   "Using %s for breakpoint at %s (instruction length %d)\n",
+	   riscv_insn_length (buf[0]) == 2 ? "C.EBREAK" : "EBREAK",
+	   paddress (gdbarch, *pcptr), riscv_insn_length (buf[0]));
+      if (riscv_insn_length (buf[0]) == 2)
 	return 2;
       else
 	return 4;
@@ -2945,6 +2954,16 @@ _initialize_riscv_tdep (void)
 		  &showdebugriscvcmdlist, "show debug riscv ", 0,
 		  &showdebuglist);
 
+  add_setshow_zuinteger_cmd ("breakpoints", class_maintenance,
+			     &riscv_debug_breakpoints,  _("\
+Set riscv breakpoint debugging."), _("\
+Show riscv breakpoint debugging."), _("\
+When non-zero, print debugging information for the riscv specific parts\n\
+of the breakpoint mechanism."),
+			     NULL,
+			     show_riscv_debug_variable,
+			     &setdebugriscvcmdlist, &showdebugriscvcmdlist);
+
   add_setshow_zuinteger_cmd ("infcall", class_maintenance,
 			     &riscv_debug_infcall,  _("\
 Set riscv inferior call debugging."), _("\
@@ -2982,9 +3001,9 @@ of the stack unwinding mechanism."),
 Set debugger's use of compressed breakpoints."), _("	\
 Show debugger's use of compressed breakpoints."), _("\
 Debugging compressed code requires compressed breakpoints to be used. If\n \
-left to 'auto' then gdb will use them if $misa indicates the C extension\n \
-is supported. If that doesn't give the correct behavior, then this option\n\
-can be used."),
+left to 'auto' then gdb will use them if the existing instruction is a\n \
+compressed instruction. If that doesn't give the correct behavior, then\n \
+this option can be used."),
 				NULL,
 				show_use_compressed_breakpoints,
 				&setriscvcmdlist,

-- 
John Baldwin

                                                                            

Re: [PATCH 2/4] Fall back to a default value of 0 for the MISA register.

[Jim Wilson]

On Thu, Sep 20, 2018 at 1:31 PM John Baldwin <jhb@freebsd.org> wrote:
>     Use the existing instruction to determine the RISC-V breakpoint kind.

Yes, this looks like a nice solution to the problem.

In riscv_breakpoint_kind_from_pc, you are setting a byte_order local
variable but not obviously using it.  You presumably don't need byte
order here as you are only reading one byte.

Jim

Re: [PATCH 2/4] Fall back to a default value of 0 for the MISA register.

[Andrew Burgess]

This look like a great improvement.  I have one issue inline below...


* John Baldwin <jhb@FreeBSD.org> [2018-09-20 13:31:46 -0700]:

> On 9/19/18 5:40 PM, John Baldwin wrote:
> > Whether or not 'C' is present is a bit more subtle as it isn't something
> > you could infer just from a register being present or not.  On the other
> > hand, for breakpoints, I wonder if a more straightforward approach would
> > be to examine the opcode of the existing instruction and use that to
> > decide which breakpoint to use?  That is, read the first byte of the
> > existing instruction and if the low 2 bits indicate a 16-bit instruction
> > use C.BREAK and otherwise use BREAK?
> 
> This seems to work for me in my testing (I used 'stepi' to step across a
> set of instructions with a mix of regular and 'C' instructions and it
> worked correctly).  You can use 'set debug riscv breakpoints 1' to see what
> it decides each time it sets a breakpoint.  I haven't yet tested if this
> means I can remove the MISA patch, but I suspect it does as the only
> place that needs MISA after this is the riscv_isa_flen() function.
> 
> Author: John Baldwin <jhb@FreeBSD.org>
> Date:   Thu Sep 20 10:12:22 2018 -0700
> 
>     Use the existing instruction to determine the RISC-V breakpoint kind.
>     
>     RISC-V supports instructions of varying lengths.  Standard existing
>     instructions in the base ISA are 4 bytes in length, but the 'C'
>     extension adds support for compressed, 2 byte instructions.  RISC-V
>     supports two different breakpoint instructions: EBREAK is a 4 byte
>     instruction in the base ISA, and C.EBREAK is a 2 byte instruction only
>     available on processors implementing the 'C' extension.  Using EBREAK
>     to set breakpoints on compressed instructions causes problems as the
>     second half of EBREAK will overwrite the first 2 bytes of the
>     following instruction breaking other threads in the process if their
>     PC is the following instruction.  Thus, breakpoints on compressed
>     instructions need to use C.EBREAK instead of EBREAK.
>     
>     Previously, the riscv architecture checked the MISA register to
>     determine if the 'C' extension was available.  If so, it used C.EBREAK
>     for all breakpoints.  However, the MISA register is not necessarily
>     available to supervisor mode operating systems.  While native targets
>     could provide a fake MISA register value, this patch instead examines
>     the existing instruction at a breakpoint target to determine which
>     breakpoint instruction to use.  If the existing instruction is a
>     compressed instruction, C.EBREAK is used, otherwise EBREAK is used.
>     
>     gdb/ChangeLog:
>     
>             * riscv-tdep.c (show_use_compressed_breakpoints): Remove
>             'additional_info' and related logic.
>             (riscv_debug_breakpoints): New variable.
>             (riscv_breakpoint_kind_from_pc): Use the length of the existing
>             instruction to determine the breakpoint kind.
>             (_initialize_riscv_tdep): Add 'set/show debug riscv breakpoints'
>             flag.  Update description of 'set/show riscv
>             use-compressed-breakpoints' flag.
> 
> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index 75032b7a4d..df4561d319 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,3 +1,14 @@
> +2018-09-20  John Baldwin  <jhb@FreeBSD.org>
> +
> +	* riscv-tdep.c (show_use_compressed_breakpoints): Remove
> +	'additional_info' and related logic.
> +	(riscv_debug_breakpoints): New variable.
> +	(riscv_breakpoint_kind_from_pc): Use the length of the existing
> +	instruction to determine the breakpoint kind.
> +	(_initialize_riscv_tdep): Add 'set/show debug riscv breakpoints'
> +	flag.  Update description of 'set/show riscv
> +	use-compressed-breakpoints' flag.
> +
>  2018-09-19  John Baldwin  <jhb@FreeBSD.org>
>  
>  	* Makefile.in (ALLDEPFILES): Add riscv-fbsd-nat.c.
> diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c
> index 4b385ed5da..cd8dbaf9f6 100644
> --- a/gdb/riscv-tdep.c
> +++ b/gdb/riscv-tdep.c
> @@ -210,20 +210,9 @@ show_use_compressed_breakpoints (struct ui_file *file, int from_tty,
>  				 struct cmd_list_element *c,
>  				 const char *value)
>  {
> -  const char *additional_info;
> -  struct gdbarch *gdbarch = target_gdbarch ();
> -
> -  if (use_compressed_breakpoints == AUTO_BOOLEAN_AUTO)
> -    if (riscv_has_feature (gdbarch, 'C'))
> -      additional_info = _(" (currently on)");
> -    else
> -      additional_info = _(" (currently off)");
> -  else
> -    additional_info = "";
> -
>    fprintf_filtered (file,
>  		    _("Debugger's use of compressed breakpoints is set "
> -		      "to %s%s.\n"), value, additional_info);
> +		      "to %s.\n"), value);
>  }
>  
>  /* The set and show lists for 'set riscv' and 'show riscv' prefixes.  */
> @@ -284,6 +273,11 @@ show_riscv_debug_variable (struct ui_file *file, int from_tty,
>  		    c->name, value);
>  }
>  
> +/* When this is set to non-zero debugging information about breakpoint
> +   kinds will be printed.  */
> +
> +static unsigned int riscv_debug_breakpoints = 0;
> +
>  /* When this is set to non-zero debugging information about inferior calls
>     will be printed.  */
>  
> @@ -426,7 +420,22 @@ riscv_breakpoint_kind_from_pc (struct gdbarch *gdbarch, CORE_ADDR *pcptr)
>  {
>    if (use_compressed_breakpoints == AUTO_BOOLEAN_AUTO)
>      {
> -      if (riscv_has_feature (gdbarch, 'C'))
> +      enum bfd_endian byte_order = gdbarch_byte_order_for_code (gdbarch);

byte_order is unused.

> +      gdb_byte buf[1];
> +      int status;
> +
> +      /* Read the opcode byte to determine the instruction length.  */
> +      status = target_read_memory (*pcptr, buf, 1);

This should use target_read_code.  I know that we already have some
(incorrect) uses of target_read_memory in riscv-tdep.c, but we can fix
those later.

However, this causes a testsuite regression for gdb.gdb/unittest.exp.
You can easily reproduce the issue with:

    (gdb) maintenance selftest 

We probably need to add a riscv specific case into
disasm-selftest.c:print_one_insn_test, lots of other targets already
do.

Otherwise, I think this is good.

Thanks,
Andrew

> +      if (status)
> +	memory_error (TARGET_XFER_E_IO, *pcptr);
> +
> +      if (riscv_debug_breakpoints)
> +	fprintf_unfiltered
> +	  (gdb_stdlog,
> +	   "Using %s for breakpoint at %s (instruction length %d)\n",
> +	   riscv_insn_length (buf[0]) == 2 ? "C.EBREAK" : "EBREAK",
> +	   paddress (gdbarch, *pcptr), riscv_insn_length (buf[0]));
> +      if (riscv_insn_length (buf[0]) == 2)
>  	return 2;
>        else
>  	return 4;
> @@ -2945,6 +2954,16 @@ _initialize_riscv_tdep (void)
>  		  &showdebugriscvcmdlist, "show debug riscv ", 0,
>  		  &showdebuglist);
>  
> +  add_setshow_zuinteger_cmd ("breakpoints", class_maintenance,
> +			     &riscv_debug_breakpoints,  _("\
> +Set riscv breakpoint debugging."), _("\
> +Show riscv breakpoint debugging."), _("\
> +When non-zero, print debugging information for the riscv specific parts\n\
> +of the breakpoint mechanism."),
> +			     NULL,
> +			     show_riscv_debug_variable,
> +			     &setdebugriscvcmdlist, &showdebugriscvcmdlist);
> +
>    add_setshow_zuinteger_cmd ("infcall", class_maintenance,
>  			     &riscv_debug_infcall,  _("\
>  Set riscv inferior call debugging."), _("\
> @@ -2982,9 +3001,9 @@ of the stack unwinding mechanism."),
>  Set debugger's use of compressed breakpoints."), _("	\
>  Show debugger's use of compressed breakpoints."), _("\
>  Debugging compressed code requires compressed breakpoints to be used. If\n \
> -left to 'auto' then gdb will use them if $misa indicates the C extension\n \
> -is supported. If that doesn't give the correct behavior, then this option\n\
> -can be used."),
> +left to 'auto' then gdb will use them if the existing instruction is a\n \
> +compressed instruction. If that doesn't give the correct behavior, then\n \
> +this option can be used."),
>  				NULL,
>  				show_use_compressed_breakpoints,
>  				&setriscvcmdlist,
> 
> -- 
> John Baldwin
> 
>                                                                             

Re: [PATCH 2/4] Fall back to a default value of 0 for the MISA register.

[John Baldwin]

On 9/20/18 1:56 PM, Jim Wilson wrote:
> On Thu, Sep 20, 2018 at 1:31 PM John Baldwin <jhb@freebsd.org> wrote:
>>     Use the existing instruction to determine the RISC-V breakpoint kind.
> 
> Yes, this looks like a nice solution to the problem.
> 
> In riscv_breakpoint_kind_from_pc, you are setting a byte_order local
> variable but not obviously using it.  You presumably don't need byte
> order here as you are only reading one byte.

Oh, very true.  I had just copied it from riscv_insn::fetch_instruction.
I'll fix and post a V2.

-- 
John Baldwin

                                                                            

Re: [PATCH 2/4] Fall back to a default value of 0 for the MISA register.

[John Baldwin]

On 9/20/18 2:51 PM, Andrew Burgess wrote:
> * John Baldwin <jhb@FreeBSD.org> [2018-09-20 13:31:46 -0700]:
>> @@ -426,7 +420,22 @@ riscv_breakpoint_kind_from_pc (struct gdbarch *gdbarch, CORE_ADDR *pcptr)
>>  {
>>    if (use_compressed_breakpoints == AUTO_BOOLEAN_AUTO)
>>      {
>> -      if (riscv_has_feature (gdbarch, 'C'))
>> +      enum bfd_endian byte_order = gdbarch_byte_order_for_code (gdbarch);
> 
> byte_order is unused.

Will fix.

>> +      gdb_byte buf[1];
>> +      int status;
>> +
>> +      /* Read the opcode byte to determine the instruction length.  */
>> +      status = target_read_memory (*pcptr, buf, 1);
> 
> This should use target_read_code.  I know that we already have some
> (incorrect) uses of target_read_memory in riscv-tdep.c, but we can fix
> those later.

Ok.

> However, this causes a testsuite regression for gdb.gdb/unittest.exp.
> You can easily reproduce the issue with:
> 
>     (gdb) maintenance selftest 
> 
> We probably need to add a riscv specific case into
> disasm-selftest.c:print_one_insn_test, lots of other targets already
> do.

Ok.  I'll reproduce that and figure out the fix and include it in a V2
patch.

One other question is if I drop the change to default MISA to 0, we should
perhaps fix the comment above riscv_read_misa?  The comment implies that
it falls back to zero if it can't read the register and it does that for
the !target_has_registers case already.  It's not clear from the comment
that targets are required to provide MISA.

-- 
John Baldwin

                                                                            

Re: [PATCH 2/4] Fall back to a default value of 0 for the MISA register.

[Andrew Burgess]

* John Baldwin <jhb@FreeBSD.org> [2018-09-20 16:01:04 -0700]:

> On 9/20/18 2:51 PM, Andrew Burgess wrote:
> > * John Baldwin <jhb@FreeBSD.org> [2018-09-20 13:31:46 -0700]:
> >> @@ -426,7 +420,22 @@ riscv_breakpoint_kind_from_pc (struct gdbarch *gdbarch, CORE_ADDR *pcptr)
> >>  {
> >>    if (use_compressed_breakpoints == AUTO_BOOLEAN_AUTO)
> >>      {
> >> -      if (riscv_has_feature (gdbarch, 'C'))
> >> +      enum bfd_endian byte_order = gdbarch_byte_order_for_code (gdbarch);
> > 
> > byte_order is unused.
> 
> Will fix.
> 
> >> +      gdb_byte buf[1];
> >> +      int status;
> >> +
> >> +      /* Read the opcode byte to determine the instruction length.  */
> >> +      status = target_read_memory (*pcptr, buf, 1);
> > 
> > This should use target_read_code.  I know that we already have some
> > (incorrect) uses of target_read_memory in riscv-tdep.c, but we can fix
> > those later.
> 
> Ok.
> 
> > However, this causes a testsuite regression for gdb.gdb/unittest.exp.
> > You can easily reproduce the issue with:
> > 
> >     (gdb) maintenance selftest 
> > 
> > We probably need to add a riscv specific case into
> > disasm-selftest.c:print_one_insn_test, lots of other targets already
> > do.
> 
> Ok.  I'll reproduce that and figure out the fix and include it in a V2
> patch.
> 
> One other question is if I drop the change to default MISA to 0, we should
> perhaps fix the comment above riscv_read_misa?  The comment implies that
> it falls back to zero if it can't read the register and it does that for
> the !target_has_registers case already.  It's not clear from the comment
> that targets are required to provide MISA.

I'm kind-of mixed about the default 0 patch.  The spec says:

    The misa CSR is an XLEN-bit WARL read-write register reporting the
    ISA supported by the hart. This register must be readable in any
    implementation, but a value of zero can be returned to indicate
    the misa register has not been implemented, requiring that CPU
    capabilities be determined through a separate non-standard
    mechanism.

So, it doesn't seem to crazy to say that in GDB, if we make not one,
but two attempts to find a MISA value, fail on both, then we could
assume a default of 0.  After all, the default of 0 just means, go
figure out an answer for yourself, so we shouldn't be any worse off.

Still, it would probably be a good thing if targets did just provide a
0 value for misa on their own as that would be more inline with the
spec (I think).

Having just looked at riscv_read_misa_reg again, it turns out that
it's completely broken anyway.  Take a look at how the READ_P
parameter is initialised in the caller and then (not) set in
function.

Further, the one and only caller, checks READ_P /or/ for a return
value of 0, so just defaulting to 0 would be fine.  At a minimum we
should apply this patch:

## START ##

diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c
index 254914c9c7e..52e101e6ab8 100644
--- a/gdb/riscv-tdep.c
+++ b/gdb/riscv-tdep.c
@@ -302,7 +302,7 @@ static unsigned int riscv_debug_unwinder = 0;
    default (false).  */
 
 static uint32_t
-riscv_read_misa_reg (bool *read_p)
+riscv_read_misa_reg ()
 {
   uint32_t value = 0;
 
@@ -334,13 +334,10 @@ riscv_read_misa_reg (bool *read_p)
 static bool
 riscv_has_feature (struct gdbarch *gdbarch, char feature)
 {
-  bool have_read_misa = false;
-  uint32_t misa;
-
   gdb_assert (feature >= 'A' && feature <= 'Z');
 
-  misa = riscv_read_misa_reg (&have_read_misa);
-  if (!have_read_misa || misa == 0)
+  uint32_t misa = riscv_read_misa_reg ();
+  if (misa == 0)
     misa = gdbarch_tdep (gdbarch)->core_features;
 
   return (misa & (1 << (feature - 'A'))) != 0;

## END ##

And, better still would be something more like your original patch:

## START ##

diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c
index 254914c9c7e..58e4c221a7c 100644
--- a/gdb/riscv-tdep.c
+++ b/gdb/riscv-tdep.c
@@ -302,7 +302,7 @@ static unsigned int riscv_debug_unwinder = 0;
    default (false).  */
 
 static uint32_t
-riscv_read_misa_reg (bool *read_p)
+riscv_read_misa_reg ()
 {
   uint32_t value = 0;
 
@@ -310,18 +310,23 @@ riscv_read_misa_reg (bool *read_p)
     {
       struct frame_info *frame = get_current_frame ();
 
-      TRY
-	{
-	  value = get_frame_register_unsigned (frame,
-					       RISCV_CSR_MISA_REGNUM);
-	}
-      CATCH (ex, RETURN_MASK_ERROR)
+      /* Old cores might have MISA located at a different offset.  */
+      int misa_regs[] =
+        { RISCV_CSR_MISA_REGNUM, RISCV_CSR_LEGACY_MISA_REGNUM };
+
+      for (int i = 0; i < ARRAY_SIZE (misa_regs); ++i)
 	{
-	  /* Old cores might have MISA located at a different offset.  */
-	  value = get_frame_register_unsigned (frame,
-					       RISCV_CSR_LEGACY_MISA_REGNUM);
+	  TRY
+	    {
+	      value = get_frame_register_unsigned (frame, misa_regs[i]);
+	      break;
+	    }
+	  CATCH (ex, RETURN_MASK_ERROR)
+	    {
+	      /* Ignore the error.  */
+	    }
+	  END_CATCH
 	}
-      END_CATCH
     }
 
   return value;
@@ -334,13 +339,10 @@ riscv_read_misa_reg (bool *read_p)
 static bool
 riscv_has_feature (struct gdbarch *gdbarch, char feature)
 {
-  bool have_read_misa = false;
-  uint32_t misa;
-
   gdb_assert (feature >= 'A' && feature <= 'Z');
 
-  misa = riscv_read_misa_reg (&have_read_misa);
-  if (!have_read_misa || misa == 0)
+  uint32_t misa = riscv_read_misa_reg ();
+  if (misa == 0)
     misa = gdbarch_tdep (gdbarch)->core_features;
 
   return (misa & (1 << (feature - 'A'))) != 0;

## END ##

Jim: Given that we agree that targets should definitely provide a
value for misa, at a minimum just returning the constant 0.  But,
given that GDB already defaults to 0 in some cases anyway.  And the
spec is quite clear that 0 is the right default value in the absence
of anything better, would you be OK with a patch that does return a
default of 0?

Thanks,
Andrew

> 
> -- 
> John Baldwin
> 
>                                                                             

Re: [PATCH 2/4] Fall back to a default value of 0 for the MISA register.

[Jim Wilson]

On Fri, Sep 21, 2018 at 2:27 AM Andrew Burgess
<andrew.burgess@embecosm.com> wrote:
> Jim: Given that we agree that targets should definitely provide a
> value for misa, at a minimum just returning the constant 0.  But,
> given that GDB already defaults to 0 in some cases anyway.  And the
> spec is quite clear that 0 is the right default value in the absence
> of anything better, would you be OK with a patch that does return a
> default of 0?

The patch to decode an instruction to decide whether to use a
compressed breakpoint or not solves my main problem.  There is also
the issue of finding FP register size, but since we only support
rv64gc at the moment, it isn't a serious problem.  Also, I think the
linker kernel may already be passing FP info via auxvec/hwcap, so I
think we already have an alternate solution for that which just needs
to be implemented.  I haven't looked at that yet.  So yes, I think it
is OK to start defaulting misa to 0.

FYI I have a qemu patch, which I may someday finish, that adds XML
register support to the RISC-V qemu system-mode port, which allows
qemu to provide a correct value of misa.  We know that misa accesses
already work with embedded targets via OpenOCD.  So it is just linux
and freebsd that need to worry about misa.

The qemu patch is here, though it looks like github is confused by
rebasing and the patch isn't readable anymore.
    https://github.com/riscv/riscv-qemu/pull/160
I'll have to figure out how to fix that.

Jim

Re: [PATCH 2/4] Fall back to a default value of 0 for the MISA register.

[John Baldwin]

On 9/21/18 2:27 AM, Andrew Burgess wrote:
> * John Baldwin <jhb@FreeBSD.org> [2018-09-20 16:01:04 -0700]:
> 
>> On 9/20/18 2:51 PM, Andrew Burgess wrote:
>>> * John Baldwin <jhb@FreeBSD.org> [2018-09-20 13:31:46 -0700]:
>> One other question is if I drop the change to default MISA to 0, we should
>> perhaps fix the comment above riscv_read_misa?  The comment implies that
>> it falls back to zero if it can't read the register and it does that for
>> the !target_has_registers case already.  It's not clear from the comment
>> that targets are required to provide MISA.
> 
> I'm kind-of mixed about the default 0 patch.  The spec says:
> 
>     The misa CSR is an XLEN-bit WARL read-write register reporting the
>     ISA supported by the hart. This register must be readable in any
>     implementation, but a value of zero can be returned to indicate
>     the misa register has not been implemented, requiring that CPU
>     capabilities be determined through a separate non-standard
>     mechanism.
> 
> So, it doesn't seem to crazy to say that in GDB, if we make not one,
> but two attempts to find a MISA value, fail on both, then we could
> assume a default of 0.  After all, the default of 0 just means, go
> figure out an answer for yourself, so we shouldn't be any worse off.
> 
> Still, it would probably be a good thing if targets did just provide a
> 0 value for misa on their own as that would be more inline with the
> spec (I think).
> 
> Having just looked at riscv_read_misa_reg again, it turns out that
> it's completely broken anyway.  Take a look at how the READ_P
> parameter is initialised in the caller and then (not) set in
> function.
> 
> Further, the one and only caller, checks READ_P /or/ for a return
> value of 0, so just defaulting to 0 would be fine.  At a minimum we
> should apply this patch:
> 
> ## START ##
> 
> diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c
> index 254914c9c7e..52e101e6ab8 100644
> --- a/gdb/riscv-tdep.c
> +++ b/gdb/riscv-tdep.c
> @@ -302,7 +302,7 @@ static unsigned int riscv_debug_unwinder = 0;
>     default (false).  */
>  
>  static uint32_t
> -riscv_read_misa_reg (bool *read_p)
> +riscv_read_misa_reg ()
>  {
>    uint32_t value = 0;
>  
> @@ -334,13 +334,10 @@ riscv_read_misa_reg (bool *read_p)
>  static bool
>  riscv_has_feature (struct gdbarch *gdbarch, char feature)
>  {
> -  bool have_read_misa = false;
> -  uint32_t misa;
> -
>    gdb_assert (feature >= 'A' && feature <= 'Z');
>  
> -  misa = riscv_read_misa_reg (&have_read_misa);
> -  if (!have_read_misa || misa == 0)
> +  uint32_t misa = riscv_read_misa_reg ();
> +  if (misa == 0)
>      misa = gdbarch_tdep (gdbarch)->core_features;
>  
>    return (misa & (1 << (feature - 'A'))) != 0;
> 
> ## END ##
> 
> And, better still would be something more like your original patch:
> 
> ## START ##
> 
> diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c
> index 254914c9c7e..58e4c221a7c 100644
> --- a/gdb/riscv-tdep.c
> +++ b/gdb/riscv-tdep.c
> @@ -302,7 +302,7 @@ static unsigned int riscv_debug_unwinder = 0;
>     default (false).  */
>  
>  static uint32_t
> -riscv_read_misa_reg (bool *read_p)
> +riscv_read_misa_reg ()
>  {
>    uint32_t value = 0;
>  
> @@ -310,18 +310,23 @@ riscv_read_misa_reg (bool *read_p)
>      {
>        struct frame_info *frame = get_current_frame ();
>  
> -      TRY
> -	{
> -	  value = get_frame_register_unsigned (frame,
> -					       RISCV_CSR_MISA_REGNUM);
> -	}
> -      CATCH (ex, RETURN_MASK_ERROR)
> +      /* Old cores might have MISA located at a different offset.  */
> +      int misa_regs[] =
> +        { RISCV_CSR_MISA_REGNUM, RISCV_CSR_LEGACY_MISA_REGNUM };
> +
> +      for (int i = 0; i < ARRAY_SIZE (misa_regs); ++i)
>  	{
> -	  /* Old cores might have MISA located at a different offset.  */
> -	  value = get_frame_register_unsigned (frame,
> -					       RISCV_CSR_LEGACY_MISA_REGNUM);
> +	  TRY
> +	    {
> +	      value = get_frame_register_unsigned (frame, misa_regs[i]);
> +	      break;
> +	    }
> +	  CATCH (ex, RETURN_MASK_ERROR)
> +	    {
> +	      /* Ignore the error.  */
> +	    }
> +	  END_CATCH
>  	}
> -      END_CATCH
>      }
>  
>    return value;
> @@ -334,13 +339,10 @@ riscv_read_misa_reg (bool *read_p)
>  static bool
>  riscv_has_feature (struct gdbarch *gdbarch, char feature)
>  {
> -  bool have_read_misa = false;
> -  uint32_t misa;
> -
>    gdb_assert (feature >= 'A' && feature <= 'Z');
>  
> -  misa = riscv_read_misa_reg (&have_read_misa);
> -  if (!have_read_misa || misa == 0)
> +  uint32_t misa = riscv_read_misa_reg ();
> +  if (misa == 0)
>      misa = gdbarch_tdep (gdbarch)->core_features;
>  
>    return (misa & (1 << (feature - 'A'))) != 0;
> 
> ## END ##
> 
> Jim: Given that we agree that targets should definitely provide a
> value for misa, at a minimum just returning the constant 0.  But,
> given that GDB already defaults to 0 in some cases anyway.  And the
> spec is quite clear that 0 is the right default value in the absence
> of anything better, would you be OK with a patch that does return a
> default of 0?

FWIW, I think the second patch looks fine.  I can confirm that the breakpoint
patch is sufficient for my limited testing on FreeBSD that I no longer need
the MISA patch so I've dropped it from my local series (I'll post a V2 in a
bit).

-- 
John Baldwin

                                                                            

Re: [PATCH 2/4] Fall back to a default value of 0 for the MISA register.

[Andrew Burgess]

* Jim Wilson <jimw@sifive.com> [2018-09-21 10:25:47 -0700]:

> On Fri, Sep 21, 2018 at 2:27 AM Andrew Burgess
> <andrew.burgess@embecosm.com> wrote:
> > Jim: Given that we agree that targets should definitely provide a
> > value for misa, at a minimum just returning the constant 0.  But,
> > given that GDB already defaults to 0 in some cases anyway.  And the
> > spec is quite clear that 0 is the right default value in the absence
> > of anything better, would you be OK with a patch that does return a
> > default of 0?
> 
> The patch to decode an instruction to decide whether to use a
> compressed breakpoint or not solves my main problem.  There is also
> the issue of finding FP register size, but since we only support
> rv64gc at the moment, it isn't a serious problem.

I regularly test embeded RiscV against:

  rv32im  rv32imc  rv32imf   rv32imfc

  rv64im  rv64imc  rv64imfd  rv64imfdc

with the last one of those being closes to rv64gc.  The pass rate is
broadly the same against all of these targets, so right now I consider
these equally supported for baremetal.

I understand Linux support might be different.

Thanks,

Andrew


>                                                    Also, I think the
> linker kernel may already be passing FP info via auxvec/hwcap, so I
> think we already have an alternate solution for that which just needs
> to be implemented.  I haven't looked at that yet.  So yes, I think it
> is OK to start defaulting misa to 0.
> 
> FYI I have a qemu patch, which I may someday finish, that adds XML
> register support to the RISC-V qemu system-mode port, which allows
> qemu to provide a correct value of misa.  We know that misa accesses
> already work with embedded targets via OpenOCD.  So it is just linux
> and freebsd that need to worry about misa.
> 
> The qemu patch is here, though it looks like github is confused by
> rebasing and the patch isn't readable anymore.
>     https://github.com/riscv/riscv-qemu/pull/160
> I'll have to figure out how to fix that.
> 
> Jim

Re: [PATCH 2/4] Fall back to a default value of 0 for the MISA register.

[Palmer Dabbelt]

On Fri, 28 Sep 2018 02:43:55 PDT (-0700), andrew.burgess@embecosm.com wrote:
> * Jim Wilson <jimw@sifive.com> [2018-09-21 10:25:47 -0700]:
>
>> On Fri, Sep 21, 2018 at 2:27 AM Andrew Burgess
>> <andrew.burgess@embecosm.com> wrote:
>> > Jim: Given that we agree that targets should definitely provide a
>> > value for misa, at a minimum just returning the constant 0.  But,
>> > given that GDB already defaults to 0 in some cases anyway.  And the
>> > spec is quite clear that 0 is the right default value in the absence
>> > of anything better, would you be OK with a patch that does return a
>> > default of 0?
>>
>> The patch to decode an instruction to decide whether to use a
>> compressed breakpoint or not solves my main problem.  There is also
>> the issue of finding FP register size, but since we only support
>> rv64gc at the moment, it isn't a serious problem.
>
> I regularly test embeded RiscV against:
>
>   rv32im  rv32imc  rv32imf   rv32imfc
>
>   rv64im  rv64imc  rv64imfd  rv64imfdc
>
> with the last one of those being closes to rv64gc.  The pass rate is
> broadly the same against all of these targets, so right now I consider
> these equally supported for baremetal.

For those uniniated in RISC-V, "rv64gc" is the same as "rv64imafdc" (the G is 
short for IMAFD).  Importantly it's probably close enough for embedded GDB 
testing, as all you're missing is the A extension and GDB doesn't really care 
about atomics.

> I understand Linux support might be different.

Right now all that's really supported in Linux land is rv64gc.  The kernel 
should also build on rv64imac, rv32gc, and rv32imac but they're much less 
mature.  The upstream glibc port supports rv64gc and rv64imac, but we test 
those only on rv64gc kernels.

We try our best to avoid breaking the other targets, but until we get some 
better CI up and running I'd expect that non-rv64gc targets do keep falling 
apart.  We're working on it :)

On Linux you should be able to look at the HWCAP in the auxvec, which is meant 
to tell you what user state is available.  The code to fill this out is here

    https://git.kernel.org/pub/scm/linux/kernel/git/palmer/riscv-linux.git/tree/arch/riscv/kernel/cpufeature.c#n26

>
> Thanks,
>
> Andrew
>
>
>>                                                    Also, I think the
>> linker kernel may already be passing FP info via auxvec/hwcap, so I
>> think we already have an alternate solution for that which just needs
>> to be implemented.  I haven't looked at that yet.  So yes, I think it
>> is OK to start defaulting misa to 0.
>>
>> FYI I have a qemu patch, which I may someday finish, that adds XML
>> register support to the RISC-V qemu system-mode port, which allows
>> qemu to provide a correct value of misa.  We know that misa accesses
>> already work with embedded targets via OpenOCD.  So it is just linux
>> and freebsd that need to worry about misa.
>>
>> The qemu patch is here, though it looks like github is confused by
>> rebasing and the patch isn't readable anymore.
>>     https://github.com/riscv/riscv-qemu/pull/160
>> I'll have to figure out how to fix that.
>>
>> Jim