[PATCH 0/4] RISCV Non-DWARF stack unwinding

[PATCH 0/4] RISCV Non-DWARF stack unwinding

[Andrew Burgess]

A series of patches providing non-DWARF stack unwinding on RISC-V.
Tested on a set of targetes including with and without compressed, and
FP, and on 32 and 64 bit.

Patch #3 touches generic code and so will need global maintainer
review before I can commit.

Review and feedback is welcome for all of the other patches too.

Thanks,
Andrew

---

Andrew Burgess (4):
  gdb/riscv: remove extra caching of misa register
  gdb/riscv: Extend instruction decode to cover more instructions
  gdb: Extend the trad-frame API
  gdb/riscv: Provide non-DWARF stack unwinder

 gdb/ChangeLog    |  42 +++++++
 gdb/riscv-tdep.c | 361 +++++++++++++++++++++++++++++++++----------------------
 gdb/riscv-tdep.h |   2 +
 gdb/trad-frame.c |  21 +++-
 gdb/trad-frame.h |   8 ++
 5 files changed, 287 insertions(+), 147 deletions(-)

-- 
2.14.4

[PATCH 1/4] gdb/riscv: remove extra caching of misa register

[Andrew Burgess]

The RISC-V had a mechanism in place to cache the contents of the misa
register per-inferior, the original intention behind this was to
reduce the number of times the misa register had to be read (as the
contents should be constant), but it was pointed out on the mailing
list[1] that the register cache will mean the register is only
accessed once each time GDB stops, and any additional caching is
probably just unneeded extra complexity.

As such, until it can be shown that there's a real need for additional
caching, this commit removes all of the additional caching of the misa
register, and just accesses the misa register like a normal register.

[1] https://sourceware.org/ml/gdb-patches/2018-03/msg00136.html

gdb/ChangeLog:

	* riscv-tdep.c (struct riscv_inferior_data): Delete.
	(riscv_read_misa_reg): Don't cache value read into inferior data.
	(riscv_new_inferior_data): Delete.
	(riscv_inferior_data_cleanup): Delete.
	(riscv_inferior_data): Delete.
	(riscv_invalidate_inferior_data): Delete.
	(_initialize_riscv_tdep): Remove initialisation of inferior data.
---
 gdb/ChangeLog    |  10 ++++++
 gdb/riscv-tdep.c | 106 +++----------------------------------------------------
 2 files changed, 15 insertions(+), 101 deletions(-)

diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c
index 1df1300100c..2f619c35e75 100644
--- a/gdb/riscv-tdep.c
+++ b/gdb/riscv-tdep.c
@@ -60,8 +60,6 @@
 
 /* Forward declarations.  */
 static bool riscv_has_feature (struct gdbarch *gdbarch, char feature);
-struct riscv_inferior_data;
-struct riscv_inferior_data * riscv_inferior_data (struct inferior *const inf);
 
 /* Define a series of is_XXX_insn functions to check if the value INSN
    is an instance of instruction XXX.  */
@@ -73,22 +71,6 @@ static inline bool is_ ## INSN_NAME ## _insn (long insn) \
 #include "opcode/riscv-opc.h"
 #undef DECLARE_INSN
 
-/* Per inferior information for RiscV.  */
-
-struct riscv_inferior_data
-{
-  /* True when MISA_VALUE is valid, otherwise false.  */
-  bool misa_read;
-
-  /* If MISA_READ is true then MISA_VALUE holds the value of the MISA
-     register read from the target.  */
-  uint32_t misa_value;
-};
-
-/* Key created when the RiscV per-inferior data is registered.  */
-
-static const struct inferior_data *riscv_inferior_data_reg;
-
 /* Architectural name for core registers.  */
 
 static const char * const riscv_gdb_reg_names[RISCV_LAST_FP_REGNUM + 1] =
@@ -293,17 +275,16 @@ static unsigned int riscv_debug_infcall = 0;
 static uint32_t
 riscv_read_misa_reg (bool *read_p)
 {
-  struct riscv_inferior_data *inf_data
-    = riscv_inferior_data (current_inferior ());
+  uint32_t value = 0;
 
-  if (!inf_data->misa_read && target_has_registers)
+  if (target_has_registers)
     {
-      uint32_t value = 0;
       struct frame_info *frame = get_current_frame ();
 
       TRY
 	{
-	  value = get_frame_register_unsigned (frame, RISCV_CSR_MISA_REGNUM);
+	  value = get_frame_register_unsigned (frame,
+					       RISCV_CSR_MISA_REGNUM);
 	}
       CATCH (ex, RETURN_MASK_ERROR)
 	{
@@ -312,15 +293,9 @@ riscv_read_misa_reg (bool *read_p)
 					       RISCV_CSR_LEGACY_MISA_REGNUM);
 	}
       END_CATCH
-
-      inf_data->misa_read = true;
-      inf_data->misa_value = value;
     }
 
-  if (read_p != nullptr)
-    *read_p = inf_data->misa_read;
-
-  return inf_data->misa_value;
+  return value;
 }
 
 /* Return true if FEATURE is available for the architecture GDBARCH.  The
@@ -2646,69 +2621,6 @@ riscv_gdbarch_init (struct gdbarch_info info,
   return gdbarch;
 }
 
-
-/* Allocate new riscv_inferior_data object.  */
-
-static struct riscv_inferior_data *
-riscv_new_inferior_data (void)
-{
-  struct riscv_inferior_data *inf_data
-    = new (struct riscv_inferior_data);
-  inf_data->misa_read = false;
-  return inf_data;
-}
-
-/* Free inferior data.  */
-
-static void
-riscv_inferior_data_cleanup (struct inferior *inf, void *data)
-{
-  struct riscv_inferior_data *inf_data =
-    static_cast <struct riscv_inferior_data *> (data);
-  delete (inf_data);
-}
-
-/* Return riscv_inferior_data for the given INFERIOR.  If not yet created,
-   construct it.  */
-
-struct riscv_inferior_data *
-riscv_inferior_data (struct inferior *const inf)
-{
-  struct riscv_inferior_data *inf_data;
-
-  gdb_assert (inf != NULL);
-
-  inf_data
-    = (struct riscv_inferior_data *) inferior_data (inf, riscv_inferior_data_reg);
-  if (inf_data == NULL)
-    {
-      inf_data = riscv_new_inferior_data ();
-      set_inferior_data (inf, riscv_inferior_data_reg, inf_data);
-    }
-
-  return inf_data;
-}
-
-/* Free the inferior data when an inferior exits.  */
-
-static void
-riscv_invalidate_inferior_data (struct inferior *inf)
-{
-  struct riscv_inferior_data *inf_data;
-
-  gdb_assert (inf != NULL);
-
-  /* Don't call RISCV_INFERIOR_DATA as we don't want to create the data if
-     we've not already created it by this point.  */
-  inf_data
-    = (struct riscv_inferior_data *) inferior_data (inf, riscv_inferior_data_reg);
-  if (inf_data != NULL)
-    {
-      delete (inf_data);
-      set_inferior_data (inf, riscv_inferior_data_reg, NULL);
-    }
-}
-
 /* This decodes the current instruction and determines the address of the
    next instruction.  */
 
@@ -2853,14 +2765,6 @@ _initialize_riscv_tdep (void)
 {
   gdbarch_register (bfd_arch_riscv, riscv_gdbarch_init, NULL);
 
-  /* Register per-inferior data.  */
-  riscv_inferior_data_reg
-    = register_inferior_data_with_cleanup (NULL, riscv_inferior_data_cleanup);
-
-  /* Observers used to invalidate the inferior data when needed.  */
-  gdb::observers::inferior_exit.attach (riscv_invalidate_inferior_data);
-  gdb::observers::inferior_appeared.attach (riscv_invalidate_inferior_data);
-
   /* Add root prefix command for all "set debug riscv" and "show debug
      riscv" commands.  */
   add_prefix_cmd ("riscv", no_class, set_debug_riscv_command,
-- 
2.14.4

Re: [PATCH 1/4] gdb/riscv: remove extra caching of misa register

[Palmer Dabbelt]

On Wed, 29 Aug 2018 09:40:51 PDT (-0700), andrew.burgess@embecosm.com wrote:
> The RISC-V had a mechanism in place to cache the contents of the misa
> register per-inferior, the original intention behind this was to
> reduce the number of times the misa register had to be read (as the
> contents should be constant), but it was pointed out on the mailing
> list[1] that the register cache will mean the register is only
> accessed once each time GDB stops, and any additional caching is
> probably just unneeded extra complexity.
>
> As such, until it can be shown that there's a real need for additional
> caching, this commit removes all of the additional caching of the misa
> register, and just accesses the misa register like a normal register.
>
> [1] https://sourceware.org/ml/gdb-patches/2018-03/msg00136.html

I like this -- we've got too many special one-off behaviors for registers 
floating around the RISC-V debug stack and I'm not really convinced most of 
them do anything but cause bugs.  I think that unless there's a benchmark that 
demonstrates why they're necessary then we should just nuke the special 
behavior whenever we run into a bug.  Like you said, the generic caches will 
probably soak up the load anyway.

>
> gdb/ChangeLog:
>
> 	* riscv-tdep.c (struct riscv_inferior_data): Delete.
> 	(riscv_read_misa_reg): Don't cache value read into inferior data.
> 	(riscv_new_inferior_data): Delete.
> 	(riscv_inferior_data_cleanup): Delete.
> 	(riscv_inferior_data): Delete.
> 	(riscv_invalidate_inferior_data): Delete.
> 	(_initialize_riscv_tdep): Remove initialisation of inferior data.
> ---
>  gdb/ChangeLog    |  10 ++++++
>  gdb/riscv-tdep.c | 106 +++----------------------------------------------------
>  2 files changed, 15 insertions(+), 101 deletions(-)
>
> diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c
> index 1df1300100c..2f619c35e75 100644
> --- a/gdb/riscv-tdep.c
> +++ b/gdb/riscv-tdep.c
> @@ -60,8 +60,6 @@
>
>  /* Forward declarations.  */
>  static bool riscv_has_feature (struct gdbarch *gdbarch, char feature);
> -struct riscv_inferior_data;
> -struct riscv_inferior_data * riscv_inferior_data (struct inferior *const inf);
>
>  /* Define a series of is_XXX_insn functions to check if the value INSN
>     is an instance of instruction XXX.  */
> @@ -73,22 +71,6 @@ static inline bool is_ ## INSN_NAME ## _insn (long insn) \
>  #include "opcode/riscv-opc.h"
>  #undef DECLARE_INSN
>
> -/* Per inferior information for RiscV.  */
> -
> -struct riscv_inferior_data
> -{
> -  /* True when MISA_VALUE is valid, otherwise false.  */
> -  bool misa_read;
> -
> -  /* If MISA_READ is true then MISA_VALUE holds the value of the MISA
> -     register read from the target.  */
> -  uint32_t misa_value;
> -};
> -
> -/* Key created when the RiscV per-inferior data is registered.  */
> -
> -static const struct inferior_data *riscv_inferior_data_reg;
> -
>  /* Architectural name for core registers.  */
>
>  static const char * const riscv_gdb_reg_names[RISCV_LAST_FP_REGNUM + 1] =
> @@ -293,17 +275,16 @@ static unsigned int riscv_debug_infcall = 0;
>  static uint32_t
>  riscv_read_misa_reg (bool *read_p)
>  {
> -  struct riscv_inferior_data *inf_data
> -    = riscv_inferior_data (current_inferior ());
> +  uint32_t value = 0;
>
> -  if (!inf_data->misa_read && target_has_registers)
> +  if (target_has_registers)
>      {
> -      uint32_t value = 0;
>        struct frame_info *frame = get_current_frame ();
>
>        TRY
>  	{
> -	  value = get_frame_register_unsigned (frame, RISCV_CSR_MISA_REGNUM);
> +	  value = get_frame_register_unsigned (frame,
> +					       RISCV_CSR_MISA_REGNUM);
>  	}
>        CATCH (ex, RETURN_MASK_ERROR)
>  	{
> @@ -312,15 +293,9 @@ riscv_read_misa_reg (bool *read_p)
>  					       RISCV_CSR_LEGACY_MISA_REGNUM);
>  	}
>        END_CATCH
> -
> -      inf_data->misa_read = true;
> -      inf_data->misa_value = value;
>      }
>
> -  if (read_p != nullptr)
> -    *read_p = inf_data->misa_read;
> -
> -  return inf_data->misa_value;
> +  return value;
>  }
>
>  /* Return true if FEATURE is available for the architecture GDBARCH.  The
> @@ -2646,69 +2621,6 @@ riscv_gdbarch_init (struct gdbarch_info info,
>    return gdbarch;
>  }
>
> -
> -/* Allocate new riscv_inferior_data object.  */
> -
> -static struct riscv_inferior_data *
> -riscv_new_inferior_data (void)
> -{
> -  struct riscv_inferior_data *inf_data
> -    = new (struct riscv_inferior_data);
> -  inf_data->misa_read = false;
> -  return inf_data;
> -}
> -
> -/* Free inferior data.  */
> -
> -static void
> -riscv_inferior_data_cleanup (struct inferior *inf, void *data)
> -{
> -  struct riscv_inferior_data *inf_data =
> -    static_cast <struct riscv_inferior_data *> (data);
> -  delete (inf_data);
> -}
> -
> -/* Return riscv_inferior_data for the given INFERIOR.  If not yet created,
> -   construct it.  */
> -
> -struct riscv_inferior_data *
> -riscv_inferior_data (struct inferior *const inf)
> -{
> -  struct riscv_inferior_data *inf_data;
> -
> -  gdb_assert (inf != NULL);
> -
> -  inf_data
> -    = (struct riscv_inferior_data *) inferior_data (inf, riscv_inferior_data_reg);
> -  if (inf_data == NULL)
> -    {
> -      inf_data = riscv_new_inferior_data ();
> -      set_inferior_data (inf, riscv_inferior_data_reg, inf_data);
> -    }
t
> -
> -  return inf_data;
> -}
> -
> -/* Free the inferior data when an inferior exits.  */
> -
> -static void
> -riscv_invalidate_inferior_data (struct inferior *inf)
> -{
> -  struct riscv_inferior_data *inf_data;
> -
> -  gdb_assert (inf != NULL);
> -
> -  /* Don't call RISCV_INFERIOR_DATA as we don't want to create the data if
> -     we've not already created it by this point.  */
> -  inf_data
> -    = (struct riscv_inferior_data *) inferior_data (inf, riscv_inferior_data_reg);
> -  if (inf_data != NULL)
> -    {
> -      delete (inf_data);
> -      set_inferior_data (inf, riscv_inferior_data_reg, NULL);
> -    }
> -}
> -
>  /* This decodes the current instruction and determines the address of the
>     next instruction.  */
>
> @@ -2853,14 +2765,6 @@ _initialize_riscv_tdep (void)
>  {
>    gdbarch_register (bfd_arch_riscv, riscv_gdbarch_init, NULL);
>
> -  /* Register per-inferior data.  */
> -  riscv_inferior_data_reg
> -    = register_inferior_data_with_cleanup (NULL, riscv_inferior_data_cleanup);
> -
> -  /* Observers used to invalidate the inferior data when needed.  */
> -  gdb::observers::inferior_exit.attach (riscv_invalidate_inferior_data);
> -  gdb::observers::inferior_appeared.attach (riscv_invalidate_inferior_data);
> -
>    /* Add root prefix command for all "set debug riscv" and "show debug
>       riscv" commands.  */
>    add_prefix_cmd ("riscv", no_class, set_debug_riscv_command,

[PATCH 4/4] gdb/riscv: Provide non-DWARF stack unwinder

[Andrew Burgess]

Collects information during the prologue scan and uses this to unwind
registers when no DWARF information is available.

This patch has been tested by disabling the DWARF stack unwinders, and
running the complete GDB testsuite against a range of RISC-V targets.
The results are comparable to running with the DWARF unwinders in
place.

gdb/ChangeLog:

	* riscv-tdep.c: Add 'prologue-value.h' include.
	(struct riscv_unwind_cache): New struct.
	(riscv_debug_unwinder): New global.
	(riscv_scan_prologue): Update arguments, capture register details
	from prologue scan.
	(riscv_skip_prologue): Reformat arguments line, move end of
	prologue calculation into riscv_scan_prologue.
	(riscv_frame_cache): Update return type, create
	riscv_unwind_cache, scan the prologue, and fill in remaining cache
	details.
	(riscv_frame_this_id): Use frame id computed in riscv_frame_cache.
	(riscv_frame_prev_register): Use the trad_frame within the
	riscv_unwind_cache.
	(_initialize_riscv_tdep): Add 'set/show debug riscv unwinder'
	flag.
---
 gdb/ChangeLog    |  18 +++++
 gdb/riscv-tdep.c | 227 +++++++++++++++++++++++++++++++++++++++++++------------
 gdb/riscv-tdep.h |   2 +
 3 files changed, 200 insertions(+), 47 deletions(-)

diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c
index b4ac83a6dd9..35ff27f2bcb 100644
--- a/gdb/riscv-tdep.c
+++ b/gdb/riscv-tdep.c
@@ -54,6 +54,7 @@
 #include "opcode/riscv-opc.h"
 #include "cli/cli-decode.h"
 #include "observable.h"
+#include "prologue-value.h"
 
 /* The stack must be 16-byte aligned.  */
 #define SP_ALIGNMENT 16
@@ -71,6 +72,29 @@ static inline bool is_ ## INSN_NAME ## _insn (long insn) \
 #include "opcode/riscv-opc.h"
 #undef DECLARE_INSN
 
+/* Cached information about a frame.  */
+
+struct riscv_unwind_cache
+{
+  /* The register from which we can calculate the frame base.  This is
+     usually $sp or $fp.  */
+  int frame_base_reg;
+
+  /* The offset from the current value in register FRAME_BASE_REG to the
+     actual frame base address.  */
+  int frame_base_offset;
+
+  /* Information about previous register values.  */
+  struct trad_frame_saved_reg *regs;
+
+  /* The id for this frame.  */
+  struct frame_id this_id;
+
+  /* The base (stack) address for this frame.  This is the stack pointer
+     value on entry to this frame before any adjustments are made.  */
+  CORE_ADDR frame_base;
+};
+
 /* Architectural name for core registers.  */
 
 static const char * const riscv_gdb_reg_names[RISCV_LAST_FP_REGNUM + 1] =
@@ -265,6 +289,11 @@ show_riscv_debug_variable (struct ui_file *file, int from_tty,
 
 static unsigned int riscv_debug_infcall = 0;
 
+/* When this is set to non-zero debugging information about stack unwinding
+   will be printed.  */
+
+static unsigned int riscv_debug_unwinder = 0;
+
 /* Read the MISA register from the target.  The register will only be read
    once, and the value read will be cached.  If the register can't be read
    from the target then a default value (0) will be returned.  If the
@@ -1240,16 +1269,34 @@ riscv_insn::decode (struct gdbarch *gdbarch, CORE_ADDR pc)
 
 static CORE_ADDR
 riscv_scan_prologue (struct gdbarch *gdbarch,
-		     CORE_ADDR start_pc, CORE_ADDR limit_pc)
+		     CORE_ADDR start_pc, CORE_ADDR end_pc,
+		     struct riscv_unwind_cache *cache)
 {
-  CORE_ADDR cur_pc, next_pc;
-  long frame_offset = 0;
+  CORE_ADDR cur_pc, next_pc, after_prologue_pc;
   CORE_ADDR end_prologue_addr = 0;
 
-  if (limit_pc > start_pc + 200)
-    limit_pc = start_pc + 200;
-
-  for (next_pc = cur_pc = start_pc; cur_pc < limit_pc; cur_pc = next_pc)
+  /* Find an upper limit on the function prologue using the debug
+     information.  If the debug information could not be used to provide
+     that bound, then use an arbitrary large number as the upper bound.  */
+  after_prologue_pc = skip_prologue_using_sal (gdbarch, start_pc);
+  if (after_prologue_pc == 0)
+    after_prologue_pc = start_pc + 100;   /* Arbitrary large number.  */
+  if (after_prologue_pc < end_pc)
+    end_pc = after_prologue_pc;
+
+  pv_t regs[RISCV_NUM_INTEGER_REGS]; /* Number of GPR.  */
+  for (int regno = 0; regno < RISCV_NUM_INTEGER_REGS; regno++)
+    regs[regno] = pv_register (regno, 0);
+  pv_area stack (RISCV_SP_REGNUM, gdbarch_addr_bit (gdbarch));
+
+  if (riscv_debug_unwinder)
+    fprintf_unfiltered
+      (gdb_stdlog,
+       "Prologue scan for function starting at %s (limit %s)\n",
+       core_addr_to_string (start_pc),
+       core_addr_to_string (end_pc));
+
+  for (next_pc = cur_pc = start_pc; cur_pc < end_pc; cur_pc = next_pc)
     {
       struct riscv_insn insn;
 
@@ -1267,10 +1314,10 @@ riscv_scan_prologue (struct gdbarch *gdbarch,
 	{
 	  /* Handle: addi sp, sp, -i
 	     or:     addiw sp, sp, -i  */
-	  if (insn.imm_signed () < 0)
-	    frame_offset += insn.imm_signed ();
-	  else
-	    break;
+          gdb_assert (insn.rd () < RISCV_NUM_INTEGER_REGS);
+          gdb_assert (insn.rs1 () < RISCV_NUM_INTEGER_REGS);
+          regs[insn.rd ()]
+            = pv_add_constant (regs[insn.rs1 ()], insn.imm_signed ());
 	}
       else if ((insn.opcode () == riscv_insn::SW
 		|| insn.opcode () == riscv_insn::SD)
@@ -1282,6 +1329,11 @@ riscv_scan_prologue (struct gdbarch *gdbarch,
 	     or:     sw reg, offset(s0)
 	     or:     sd reg, offset(s0)  */
 	  /* Instruction storing a register onto the stack.  */
+          gdb_assert (insn.rs1 () < RISCV_NUM_INTEGER_REGS);
+          gdb_assert (insn.rs2 () < RISCV_NUM_INTEGER_REGS);
+          stack.store (pv_add_constant (regs[insn.rs1 ()], insn.imm_signed ()),
+                        (insn.opcode () == riscv_insn::SW ? 4 : 8),
+                        regs[insn.rs2 ()]);
 	}
       else if (insn.opcode () == riscv_insn::ADDI
 	       && insn.rd () == RISCV_FP_REGNUM
@@ -1289,6 +1341,10 @@ riscv_scan_prologue (struct gdbarch *gdbarch,
 	{
 	  /* Handle: addi s0, sp, size  */
 	  /* Instructions setting up the frame pointer.  */
+          gdb_assert (insn.rd () < RISCV_NUM_INTEGER_REGS);
+          gdb_assert (insn.rs1 () < RISCV_NUM_INTEGER_REGS);
+          regs[insn.rd ()]
+            = pv_add_constant (regs[insn.rs1 ()], insn.imm_signed ());
 	}
       else if ((insn.opcode () == riscv_insn::ADD
 		|| insn.opcode () == riscv_insn::ADDW)
@@ -1299,6 +1355,9 @@ riscv_scan_prologue (struct gdbarch *gdbarch,
 	  /* Handle: add s0, sp, 0
 	     or:     addw s0, sp, 0  */
 	  /* Instructions setting up the frame pointer.  */
+          gdb_assert (insn.rd () < RISCV_NUM_INTEGER_REGS);
+          gdb_assert (insn.rs1 () < RISCV_NUM_INTEGER_REGS);
+          regs[insn.rd ()] = pv_add_constant (regs[insn.rs1 ()], 0);
 	}
       else if ((insn.rd () == RISCV_GP_REGNUM
 		&& (insn.opcode () == riscv_insn::AUIPC
@@ -1324,24 +1383,63 @@ riscv_scan_prologue (struct gdbarch *gdbarch,
 	}
       else
 	{
-	  if (end_prologue_addr == 0)
-	    end_prologue_addr = cur_pc;
+	  end_prologue_addr = cur_pc;
+	  break;
 	}
     }
 
   if (end_prologue_addr == 0)
     end_prologue_addr = cur_pc;
 
+  if (riscv_debug_unwinder)
+    fprintf_unfiltered (gdb_stdlog, "End of prologue at %s\n",
+			core_addr_to_string (end_prologue_addr));
+
+  if (cache != NULL)
+    {
+      /* Figure out if it is a frame pointer or just a stack pointer.  Also
+         the offset held in the pv_t is from the original register value to
+         the current value, which for a grows down stack means a negative
+         value.  The FRAME_BASE_OFFSET is the negation of this, how to get
+         from the current value to the original value.  */
+      if (pv_is_register (regs[RISCV_FP_REGNUM], RISCV_SP_REGNUM))
+	{
+          cache->frame_base_reg = RISCV_FP_REGNUM;
+          cache->frame_base_offset = -regs[RISCV_FP_REGNUM].k;
+	}
+      else
+	{
+          cache->frame_base_reg = RISCV_SP_REGNUM;
+          cache->frame_base_offset = -regs[RISCV_SP_REGNUM].k;
+	}
+
+      /* Assign offset from old SP to all saved registers.  As we don't
+         have the previous value for the frame base register at this
+         point, we store the offset as the address in the trad_frame, and
+         then convert this to an actual address later.  */
+      for (int i = 0; i <= RISCV_NUM_INTEGER_REGS; i++)
+	{
+	  CORE_ADDR offset;
+	  if (stack.find_reg (gdbarch, i, &offset))
+            {
+              if (riscv_debug_unwinder)
+                fprintf_unfiltered (gdb_stdlog,
+                                    "Register $%s at stack offset %ld\n",
+                                    gdbarch_register_name (gdbarch, i),
+                                    offset);
+              trad_frame_set_addr (cache->regs, i, offset);
+            }
+	}
+    }
+
   return end_prologue_addr;
 }
 
 /* Implement the riscv_skip_prologue gdbarch method.  */
 
 static CORE_ADDR
-riscv_skip_prologue (struct gdbarch *gdbarch,
-		     CORE_ADDR       pc)
+riscv_skip_prologue (struct gdbarch *gdbarch, CORE_ADDR pc)
 {
-  CORE_ADDR limit_pc;
   CORE_ADDR func_addr;
 
   /* See if we can determine the end of the prologue via the symbol
@@ -1357,16 +1455,9 @@ riscv_skip_prologue (struct gdbarch *gdbarch,
     }
 
   /* Can't determine prologue from the symbol table, need to examine
-     instructions.  */
-
-  /* Find an upper limit on the function prologue using the debug
-     information.  If the debug information could not be used to provide
-     that bound, then use an arbitrary large number as the upper bound.  */
-  limit_pc = skip_prologue_using_sal (gdbarch, pc);
-  if (limit_pc == 0)
-    limit_pc = pc + 100;   /* MAGIC! */
-
-  return riscv_scan_prologue (gdbarch, pc, limit_pc);
+     instructions.  Pass -1 for the end address to indicate the prologue
+     scanner can scan as far as it needs to find the end of the prologue.  */
+  return riscv_scan_prologue (gdbarch, pc, ((CORE_ADDR) -1), NULL);
 }
 
 /* Implement the gdbarch push dummy code callback.  */
@@ -2444,31 +2535,63 @@ riscv_dummy_id (struct gdbarch *gdbarch, struct frame_info *this_frame)
 /* Generate, or return the cached frame cache for the RiscV frame
    unwinder.  */
 
-static struct trad_frame_cache *
+static struct riscv_unwind_cache *
 riscv_frame_cache (struct frame_info *this_frame, void **this_cache)
 {
-  CORE_ADDR pc;
-  CORE_ADDR start_addr;
-  CORE_ADDR stack_addr;
-  struct trad_frame_cache *this_trad_cache;
+  CORE_ADDR pc, start_addr;
+  struct riscv_unwind_cache *cache;
   struct gdbarch *gdbarch = get_frame_arch (this_frame);
+  int numregs, regno;
 
   if ((*this_cache) != NULL)
-    return (struct trad_frame_cache *) *this_cache;
-  this_trad_cache = trad_frame_cache_zalloc (this_frame);
-  (*this_cache) = this_trad_cache;
+    return (struct riscv_unwind_cache *) *this_cache;
 
-  trad_frame_set_reg_realreg (this_trad_cache, gdbarch_pc_regnum (gdbarch),
-			      RISCV_RA_REGNUM);
+  cache = FRAME_OBSTACK_ZALLOC (struct riscv_unwind_cache);
+  cache->regs = trad_frame_alloc_saved_regs (this_frame);
+  (*this_cache) = cache;
 
+  /* Scan the prologue, filling in the cache.  */
+  start_addr = get_frame_func (this_frame);
   pc = get_frame_pc (this_frame);
-  find_pc_partial_function (pc, NULL, &start_addr, NULL);
-  stack_addr = get_frame_register_signed (this_frame, RISCV_SP_REGNUM);
-  trad_frame_set_id (this_trad_cache, frame_id_build (stack_addr, start_addr));
+  riscv_scan_prologue (gdbarch, start_addr, pc, cache);
+
+  /* We can now calculate the frame base address.  */
+  cache->frame_base
+    = (get_frame_register_signed (this_frame, cache->frame_base_reg) +
+       cache->frame_base_offset);
+  if (riscv_debug_unwinder)
+    fprintf_unfiltered (gdb_stdlog, "Frame base is %s ($%s + 0x%x)\n",
+                        core_addr_to_string (cache->frame_base),
+                        gdbarch_register_name (gdbarch,
+                                               cache->frame_base_reg),
+                        cache->frame_base_offset);
+
+  /* The prologue scanner sets the address of registers stored to the stack
+     as the offset of that register from the frame base.  The prologue
+     scanner doesn't know the actual frame base value, and so is unable to
+     compute the exact address.  We do now know the frame base value, so
+     update the address of registers stored to the stack.  */
+  numregs = gdbarch_num_regs (gdbarch) + gdbarch_num_pseudo_regs (gdbarch);
+  for (regno = 0; regno < numregs; ++regno)
+    {
+      if (trad_frame_addr_p (cache->regs, regno))
+	cache->regs[regno].addr += cache->frame_base;
+    }
+
+  /* The previous $pc can be found wherever the $ra value can be found.
+     The previous $ra value is gone, this would have been stored be the
+     previous frame if required.  */
+  cache->regs[gdbarch_pc_regnum (gdbarch)] = cache->regs[RISCV_RA_REGNUM];
+  trad_frame_set_unknown (cache->regs, RISCV_RA_REGNUM);
+
+  /* Build the frame id.  */
+  cache->this_id = frame_id_build (cache->frame_base, start_addr);
 
-  trad_frame_set_this_base (this_trad_cache, stack_addr);
+  /* The previous $sp value is the frame base value.  */
+  trad_frame_set_value (cache->regs, gdbarch_sp_regnum (gdbarch),
+			cache->frame_base);
 
-  return this_trad_cache;
+  return cache;
 }
 
 /* Implement the this_id callback for RiscV frame unwinder.  */
@@ -2478,10 +2601,10 @@ riscv_frame_this_id (struct frame_info *this_frame,
 		     void **prologue_cache,
 		     struct frame_id *this_id)
 {
-  struct trad_frame_cache *info;
+  struct riscv_unwind_cache *cache;
 
-  info = riscv_frame_cache (this_frame, prologue_cache);
-  trad_frame_get_id (info, this_id);
+  cache = riscv_frame_cache (this_frame, prologue_cache);
+  *this_id = cache->this_id;
 }
 
 /* Implement the prev_register callback for RiscV frame unwinder.  */
@@ -2491,10 +2614,10 @@ riscv_frame_prev_register (struct frame_info *this_frame,
 			   void **prologue_cache,
 			   int regnum)
 {
-  struct trad_frame_cache *info;
+  struct riscv_unwind_cache *cache;
 
-  info = riscv_frame_cache (this_frame, prologue_cache);
-  return trad_frame_get_register (info, this_frame, regnum);
+  cache = riscv_frame_cache (this_frame, prologue_cache);
+  return trad_frame_get_prev_register (this_frame, cache->regs, regnum);
 }
 
 /* Structure defining the RiscV normal frame unwind functions.  Since we
@@ -2823,6 +2946,16 @@ of the inferior call mechanism."),
 			     show_riscv_debug_variable,
 			     &setdebugriscvcmdlist, &showdebugriscvcmdlist);
 
+  add_setshow_zuinteger_cmd ("unwinder", class_maintenance,
+			     &riscv_debug_unwinder,  _("\
+Set riscv stack unwinding debugging."), _("\
+Show riscv stack unwinding debugging."), _("\
+When non-zero, print debugging information for the riscv specific parts\n\
+of the stack unwinding mechanism."),
+			     NULL,
+			     show_riscv_debug_variable,
+			     &setdebugriscvcmdlist, &showdebugriscvcmdlist);
+
   /* Add root prefix command for all "set riscv" and "show riscv" commands.  */
   add_prefix_cmd ("riscv", no_class, set_riscv_command,
 		  _("RISC-V specific commands."),
diff --git a/gdb/riscv-tdep.h b/gdb/riscv-tdep.h
index 8358d4e00ba..8a2454eb668 100644
--- a/gdb/riscv-tdep.h
+++ b/gdb/riscv-tdep.h
@@ -34,6 +34,8 @@ enum
   RISCV_A1_REGNUM = 11,		/* Second argument.  */
   RISCV_PC_REGNUM = 32,		/* Program Counter.  */
 
+  RISCV_NUM_INTEGER_REGS = 32,
+
   RISCV_FIRST_FP_REGNUM = 33,	/* First Floating Point Register */
   RISCV_FA0_REGNUM = 43,
   RISCV_FA1_REGNUM = RISCV_FA0_REGNUM + 1,
-- 
2.14.4

Re: [PATCH 4/4] gdb/riscv: Provide non-DWARF stack unwinder

[Palmer Dabbelt]

On Wed, 29 Aug 2018 09:40:54 PDT (-0700), andrew.burgess@embecosm.com wrote:
> Collects information during the prologue scan and uses this to unwind
> registers when no DWARF information is available.
>
> This patch has been tested by disabling the DWARF stack unwinders, and
> running the complete GDB testsuite against a range of RISC-V targets.
> The results are comparable to running with the DWARF unwinders in
> place.
>
> gdb/ChangeLog:
>
> 	* riscv-tdep.c: Add 'prologue-value.h' include.
> 	(struct riscv_unwind_cache): New struct.
> 	(riscv_debug_unwinder): New global.
> 	(riscv_scan_prologue): Update arguments, capture register details
> 	from prologue scan.
> 	(riscv_skip_prologue): Reformat arguments line, move end of
> 	prologue calculation into riscv_scan_prologue.
> 	(riscv_frame_cache): Update return type, create
> 	riscv_unwind_cache, scan the prologue, and fill in remaining cache
> 	details.
> 	(riscv_frame_this_id): Use frame id computed in riscv_frame_cache.
> 	(riscv_frame_prev_register): Use the trad_frame within the
> 	riscv_unwind_cache.
> 	(_initialize_riscv_tdep): Add 'set/show debug riscv unwinder'
> 	flag.
> ---
>  gdb/ChangeLog    |  18 +++++
>  gdb/riscv-tdep.c | 227 +++++++++++++++++++++++++++++++++++++++++++------------
>  gdb/riscv-tdep.h |   2 +
>  3 files changed, 200 insertions(+), 47 deletions(-)
>
> diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c
> index b4ac83a6dd9..35ff27f2bcb 100644
> --- a/gdb/riscv-tdep.c
> +++ b/gdb/riscv-tdep.c
> @@ -54,6 +54,7 @@
>  #include "opcode/riscv-opc.h"
>  #include "cli/cli-decode.h"
>  #include "observable.h"
> +#include "prologue-value.h"
>
>  /* The stack must be 16-byte aligned.  */
>  #define SP_ALIGNMENT 16
> @@ -71,6 +72,29 @@ static inline bool is_ ## INSN_NAME ## _insn (long insn) \
>  #include "opcode/riscv-opc.h"
>  #undef DECLARE_INSN
>
> +/* Cached information about a frame.  */
> +
> +struct riscv_unwind_cache
> +{
> +  /* The register from which we can calculate the frame base.  This is
> +     usually $sp or $fp.  */
> +  int frame_base_reg;
> +
> +  /* The offset from the current value in register FRAME_BASE_REG to the
> +     actual frame base address.  */
> +  int frame_base_offset;
> +
> +  /* Information about previous register values.  */
> +  struct trad_frame_saved_reg *regs;
> +
> +  /* The id for this frame.  */
> +  struct frame_id this_id;
> +
> +  /* The base (stack) address for this frame.  This is the stack pointer
> +     value on entry to this frame before any adjustments are made.  */
> +  CORE_ADDR frame_base;
> +};
> +
>  /* Architectural name for core registers.  */
>
>  static const char * const riscv_gdb_reg_names[RISCV_LAST_FP_REGNUM + 1] =
> @@ -265,6 +289,11 @@ show_riscv_debug_variable (struct ui_file *file, int from_tty,
>
>  static unsigned int riscv_debug_infcall = 0;
>
> +/* When this is set to non-zero debugging information about stack unwinding
> +   will be printed.  */
> +
> +static unsigned int riscv_debug_unwinder = 0;
> +
>  /* Read the MISA register from the target.  The register will only be read
>     once, and the value read will be cached.  If the register can't be read
>     from the target then a default value (0) will be returned.  If the
> @@ -1240,16 +1269,34 @@ riscv_insn::decode (struct gdbarch *gdbarch, CORE_ADDR pc)
>
>  static CORE_ADDR
>  riscv_scan_prologue (struct gdbarch *gdbarch,
> -		     CORE_ADDR start_pc, CORE_ADDR limit_pc)
> +		     CORE_ADDR start_pc, CORE_ADDR end_pc,
> +		     struct riscv_unwind_cache *cache)
>  {
> -  CORE_ADDR cur_pc, next_pc;
> -  long frame_offset = 0;
> +  CORE_ADDR cur_pc, next_pc, after_prologue_pc;
>    CORE_ADDR end_prologue_addr = 0;
>
> -  if (limit_pc > start_pc + 200)
> -    limit_pc = start_pc + 200;
> -
> -  for (next_pc = cur_pc = start_pc; cur_pc < limit_pc; cur_pc = next_pc)
> +  /* Find an upper limit on the function prologue using the debug
> +     information.  If the debug information could not be used to provide
> +     that bound, then use an arbitrary large number as the upper bound.  */
> +  after_prologue_pc = skip_prologue_using_sal (gdbarch, start_pc);
> +  if (after_prologue_pc == 0)
> +    after_prologue_pc = start_pc + 100;   /* Arbitrary large number.  */
> +  if (after_prologue_pc < end_pc)
> +    end_pc = after_prologue_pc;
> +
> +  pv_t regs[RISCV_NUM_INTEGER_REGS]; /* Number of GPR.  */
> +  for (int regno = 0; regno < RISCV_NUM_INTEGER_REGS; regno++)
> +    regs[regno] = pv_register (regno, 0);
> +  pv_area stack (RISCV_SP_REGNUM, gdbarch_addr_bit (gdbarch));
> +
> +  if (riscv_debug_unwinder)
> +    fprintf_unfiltered
> +      (gdb_stdlog,
> +       "Prologue scan for function starting at %s (limit %s)\n",
> +       core_addr_to_string (start_pc),
> +       core_addr_to_string (end_pc));
> +
> +  for (next_pc = cur_pc = start_pc; cur_pc < end_pc; cur_pc = next_pc)
>      {
>        struct riscv_insn insn;
>
> @@ -1267,10 +1314,10 @@ riscv_scan_prologue (struct gdbarch *gdbarch,
>  	{
>  	  /* Handle: addi sp, sp, -i
>  	     or:     addiw sp, sp, -i  */
> -	  if (insn.imm_signed () < 0)
> -	    frame_offset += insn.imm_signed ();
> -	  else
> -	    break;
> +          gdb_assert (insn.rd () < RISCV_NUM_INTEGER_REGS);
> +          gdb_assert (insn.rs1 () < RISCV_NUM_INTEGER_REGS);
> +          regs[insn.rd ()]
> +            = pv_add_constant (regs[insn.rs1 ()], insn.imm_signed ());
>  	}
>        else if ((insn.opcode () == riscv_insn::SW
>  		|| insn.opcode () == riscv_insn::SD)
> @@ -1282,6 +1329,11 @@ riscv_scan_prologue (struct gdbarch *gdbarch,
>  	     or:     sw reg, offset(s0)
>  	     or:     sd reg, offset(s0)  */
>  	  /* Instruction storing a register onto the stack.  */
> +          gdb_assert (insn.rs1 () < RISCV_NUM_INTEGER_REGS);
> +          gdb_assert (insn.rs2 () < RISCV_NUM_INTEGER_REGS);
> +          stack.store (pv_add_constant (regs[insn.rs1 ()], insn.imm_signed ()),
> +                        (insn.opcode () == riscv_insn::SW ? 4 : 8),
> +                        regs[insn.rs2 ()]);
>  	}
>        else if (insn.opcode () == riscv_insn::ADDI
>  	       && insn.rd () == RISCV_FP_REGNUM
> @@ -1289,6 +1341,10 @@ riscv_scan_prologue (struct gdbarch *gdbarch,
>  	{
>  	  /* Handle: addi s0, sp, size  */
>  	  /* Instructions setting up the frame pointer.  */
> +          gdb_assert (insn.rd () < RISCV_NUM_INTEGER_REGS);
> +          gdb_assert (insn.rs1 () < RISCV_NUM_INTEGER_REGS);
> +          regs[insn.rd ()]
> +            = pv_add_constant (regs[insn.rs1 ()], insn.imm_signed ());
>  	}
>        else if ((insn.opcode () == riscv_insn::ADD
>  		|| insn.opcode () == riscv_insn::ADDW)
> @@ -1299,6 +1355,9 @@ riscv_scan_prologue (struct gdbarch *gdbarch,
>  	  /* Handle: add s0, sp, 0
>  	     or:     addw s0, sp, 0  */
>  	  /* Instructions setting up the frame pointer.  */
> +          gdb_assert (insn.rd () < RISCV_NUM_INTEGER_REGS);
> +          gdb_assert (insn.rs1 () < RISCV_NUM_INTEGER_REGS);
> +          regs[insn.rd ()] = pv_add_constant (regs[insn.rs1 ()], 0);
>  	}
>        else if ((insn.rd () == RISCV_GP_REGNUM
>  		&& (insn.opcode () == riscv_insn::AUIPC
> @@ -1324,24 +1383,63 @@ riscv_scan_prologue (struct gdbarch *gdbarch,
>  	}
>        else
>  	{
> -	  if (end_prologue_addr == 0)
> -	    end_prologue_addr = cur_pc;
> +	  end_prologue_addr = cur_pc;
> +	  break;
>  	}
>      }
>
>    if (end_prologue_addr == 0)
>      end_prologue_addr = cur_pc;
>
> +  if (riscv_debug_unwinder)
> +    fprintf_unfiltered (gdb_stdlog, "End of prologue at %s\n",
> +			core_addr_to_string (end_prologue_addr));
> +
> +  if (cache != NULL)
> +    {
> +      /* Figure out if it is a frame pointer or just a stack pointer.  Also
> +         the offset held in the pv_t is from the original register value to
> +         the current value, which for a grows down stack means a negative
> +         value.  The FRAME_BASE_OFFSET is the negation of this, how to get
> +         from the current value to the original value.  */
> +      if (pv_is_register (regs[RISCV_FP_REGNUM], RISCV_SP_REGNUM))
> +	{
> +          cache->frame_base_reg = RISCV_FP_REGNUM;
> +          cache->frame_base_offset = -regs[RISCV_FP_REGNUM].k;
> +	}
> +      else
> +	{
> +          cache->frame_base_reg = RISCV_SP_REGNUM;
> +          cache->frame_base_offset = -regs[RISCV_SP_REGNUM].k;
> +	}
> +
> +      /* Assign offset from old SP to all saved registers.  As we don't
> +         have the previous value for the frame base register at this
> +         point, we store the offset as the address in the trad_frame, and
> +         then convert this to an actual address later.  */
> +      for (int i = 0; i <= RISCV_NUM_INTEGER_REGS; i++)
> +	{
> +	  CORE_ADDR offset;
> +	  if (stack.find_reg (gdbarch, i, &offset))
> +            {
> +              if (riscv_debug_unwinder)
> +                fprintf_unfiltered (gdb_stdlog,
> +                                    "Register $%s at stack offset %ld\n",
> +                                    gdbarch_register_name (gdbarch, i),
> +                                    offset);
> +              trad_frame_set_addr (cache->regs, i, offset);
> +            }
> +	}
> +    }
> +
>    return end_prologue_addr;
>  }
>
>  /* Implement the riscv_skip_prologue gdbarch method.  */
>
>  static CORE_ADDR
> -riscv_skip_prologue (struct gdbarch *gdbarch,
> -		     CORE_ADDR       pc)
> +riscv_skip_prologue (struct gdbarch *gdbarch, CORE_ADDR pc)
>  {
> -  CORE_ADDR limit_pc;
>    CORE_ADDR func_addr;
>
>    /* See if we can determine the end of the prologue via the symbol
> @@ -1357,16 +1455,9 @@ riscv_skip_prologue (struct gdbarch *gdbarch,
>      }
>
>    /* Can't determine prologue from the symbol table, need to examine
> -     instructions.  */
> -
> -  /* Find an upper limit on the function prologue using the debug
> -     information.  If the debug information could not be used to provide
> -     that bound, then use an arbitrary large number as the upper bound.  */
> -  limit_pc = skip_prologue_using_sal (gdbarch, pc);
> -  if (limit_pc == 0)
> -    limit_pc = pc + 100;   /* MAGIC! */
> -
> -  return riscv_scan_prologue (gdbarch, pc, limit_pc);
> +     instructions.  Pass -1 for the end address to indicate the prologue
> +     scanner can scan as far as it needs to find the end of the prologue.  */
> +  return riscv_scan_prologue (gdbarch, pc, ((CORE_ADDR) -1), NULL);
>  }
>
>  /* Implement the gdbarch push dummy code callback.  */
> @@ -2444,31 +2535,63 @@ riscv_dummy_id (struct gdbarch *gdbarch, struct frame_info *this_frame)
>  /* Generate, or return the cached frame cache for the RiscV frame
>     unwinder.  */
>
> -static struct trad_frame_cache *
> +static struct riscv_unwind_cache *
>  riscv_frame_cache (struct frame_info *this_frame, void **this_cache)
>  {
> -  CORE_ADDR pc;
> -  CORE_ADDR start_addr;
> -  CORE_ADDR stack_addr;
> -  struct trad_frame_cache *this_trad_cache;
> +  CORE_ADDR pc, start_addr;
> +  struct riscv_unwind_cache *cache;
>    struct gdbarch *gdbarch = get_frame_arch (this_frame);
> +  int numregs, regno;
>
>    if ((*this_cache) != NULL)
> -    return (struct trad_frame_cache *) *this_cache;
> -  this_trad_cache = trad_frame_cache_zalloc (this_frame);
> -  (*this_cache) = this_trad_cache;
> +    return (struct riscv_unwind_cache *) *this_cache;
>
> -  trad_frame_set_reg_realreg (this_trad_cache, gdbarch_pc_regnum (gdbarch),
> -			      RISCV_RA_REGNUM);
> +  cache = FRAME_OBSTACK_ZALLOC (struct riscv_unwind_cache);
> +  cache->regs = trad_frame_alloc_saved_regs (this_frame);
> +  (*this_cache) = cache;
>
> +  /* Scan the prologue, filling in the cache.  */
> +  start_addr = get_frame_func (this_frame);
>    pc = get_frame_pc (this_frame);
> -  find_pc_partial_function (pc, NULL, &start_addr, NULL);
> -  stack_addr = get_frame_register_signed (this_frame, RISCV_SP_REGNUM);
> -  trad_frame_set_id (this_trad_cache, frame_id_build (stack_addr, start_addr));
> +  riscv_scan_prologue (gdbarch, start_addr, pc, cache);
> +
> +  /* We can now calculate the frame base address.  */
> +  cache->frame_base
> +    = (get_frame_register_signed (this_frame, cache->frame_base_reg) +
> +       cache->frame_base_offset);
> +  if (riscv_debug_unwinder)
> +    fprintf_unfiltered (gdb_stdlog, "Frame base is %s ($%s + 0x%x)\n",
> +                        core_addr_to_string (cache->frame_base),
> +                        gdbarch_register_name (gdbarch,
> +                                               cache->frame_base_reg),
> +                        cache->frame_base_offset);
> +
> +  /* The prologue scanner sets the address of registers stored to the stack
> +     as the offset of that register from the frame base.  The prologue
> +     scanner doesn't know the actual frame base value, and so is unable to
> +     compute the exact address.  We do now know the frame base value, so
> +     update the address of registers stored to the stack.  */
> +  numregs = gdbarch_num_regs (gdbarch) + gdbarch_num_pseudo_regs (gdbarch);
> +  for (regno = 0; regno < numregs; ++regno)
> +    {
> +      if (trad_frame_addr_p (cache->regs, regno))
> +	cache->regs[regno].addr += cache->frame_base;
> +    }
> +
> +  /* The previous $pc can be found wherever the $ra value can be found.
> +     The previous $ra value is gone, this would have been stored be the
> +     previous frame if required.  */
> +  cache->regs[gdbarch_pc_regnum (gdbarch)] = cache->regs[RISCV_RA_REGNUM];
> +  trad_frame_set_unknown (cache->regs, RISCV_RA_REGNUM);
> +
> +  /* Build the frame id.  */
> +  cache->this_id = frame_id_build (cache->frame_base, start_addr);
>
> -  trad_frame_set_this_base (this_trad_cache, stack_addr);
> +  /* The previous $sp value is the frame base value.  */
> +  trad_frame_set_value (cache->regs, gdbarch_sp_regnum (gdbarch),
> +			cache->frame_base);
>
> -  return this_trad_cache;
> +  return cache;
>  }
>
>  /* Implement the this_id callback for RiscV frame unwinder.  */
> @@ -2478,10 +2601,10 @@ riscv_frame_this_id (struct frame_info *this_frame,
>  		     void **prologue_cache,
>  		     struct frame_id *this_id)
>  {
> -  struct trad_frame_cache *info;
> +  struct riscv_unwind_cache *cache;
>
> -  info = riscv_frame_cache (this_frame, prologue_cache);
> -  trad_frame_get_id (info, this_id);
> +  cache = riscv_frame_cache (this_frame, prologue_cache);
> +  *this_id = cache->this_id;
>  }
>
>  /* Implement the prev_register callback for RiscV frame unwinder.  */
> @@ -2491,10 +2614,10 @@ riscv_frame_prev_register (struct frame_info *this_frame,
>  			   void **prologue_cache,
>  			   int regnum)
>  {
> -  struct trad_frame_cache *info;
> +  struct riscv_unwind_cache *cache;
>
> -  info = riscv_frame_cache (this_frame, prologue_cache);
> -  return trad_frame_get_register (info, this_frame, regnum);
> +  cache = riscv_frame_cache (this_frame, prologue_cache);
> +  return trad_frame_get_prev_register (this_frame, cache->regs, regnum);
>  }
>
>  /* Structure defining the RiscV normal frame unwind functions.  Since we
> @@ -2823,6 +2946,16 @@ of the inferior call mechanism."),
>  			     show_riscv_debug_variable,
>  			     &setdebugriscvcmdlist, &showdebugriscvcmdlist);
>
> +  add_setshow_zuinteger_cmd ("unwinder", class_maintenance,
> +			     &riscv_debug_unwinder,  _("\
> +Set riscv stack unwinding debugging."), _("\
> +Show riscv stack unwinding debugging."), _("\
> +When non-zero, print debugging information for the riscv specific parts\n\
> +of the stack unwinding mechanism."),
> +			     NULL,
> +			     show_riscv_debug_variable,
> +			     &setdebugriscvcmdlist, &showdebugriscvcmdlist);
> +
>    /* Add root prefix command for all "set riscv" and "show riscv" commands.  */
>    add_prefix_cmd ("riscv", no_class, set_riscv_command,
>  		  _("RISC-V specific commands."),
> diff --git a/gdb/riscv-tdep.h b/gdb/riscv-tdep.h
> index 8358d4e00ba..8a2454eb668 100644
> --- a/gdb/riscv-tdep.h
> +++ b/gdb/riscv-tdep.h
> @@ -34,6 +34,8 @@ enum
>    RISCV_A1_REGNUM = 11,		/* Second argument.  */
>    RISCV_PC_REGNUM = 32,		/* Program Counter.  */
>
> +  RISCV_NUM_INTEGER_REGS = 32,
> +
>    RISCV_FIRST_FP_REGNUM = 33,	/* First Floating Point Register */
>    RISCV_FA0_REGNUM = 43,
>    RISCV_FA1_REGNUM = RISCV_FA0_REGNUM + 1,

There only thing I can think of here is that this won't handle large stack 
frames where we have to use some sort of lui-based sequence to increment the 
stack pointer.  For example, if I do something like

    extern int func(int *a);
    
    int _start(void) {
        int a[4096];
        return func(a);
    }

    $ riscv64-sifive-elf-gcc test.c -O3 -S -o-
    	.file	"test.c"
    	.option nopic
    	.text
    	.align	1
    	.globl	_start
    	.type	_start, @function
    _start:
    	li	t1,-16384
    	addi	sp,sp,-32
    	addi	t1,t1,16
    	sd	ra,24(sp)
    	li	a5,16384
    	add	sp,sp,t1
    	add	a5,a5,sp
    	li	a0,-16384
    	add	a0,a5,a0
    	call	func
    	li	t1,16384
    	addi	t1,t1,-16
    	add	sp,sp,t1
    	ld	ra,24(sp)
    	addi	sp,sp,32
    	jr	ra
    	.size	_start, .-_start
    	.ident	"GCC: (GNU) 8.1.0"

I don't see why this would block merging the patches, though.

I'm also not sure why you'd end up with something like "auipc gp, IMM" or "addi 
gp, gp, IMM" in a prologue, is that a holdover from MIPS where they have a GP 
per shared object?  If we've actually got any of those it seems suspicious at 
best.

[PATCH 3/4] gdb: Extend the trad-frame API

[Andrew Burgess]

Adds two new functions to the trad-frame API and update the internals
of trad-frame to use the new functions.  These functions will be used
in later commits.

gdb/ChangeLog:

	* trad-frame.h (trad_frame_set_realreg): Declare.
	(trad_frame_set_addr): Declare.
	* trad-frame.c (trad_frame_set_realreg): Define new function.
	(trad_frame_set_addr): Define new function.
	(trad_frame_set_reg_realreg): Use new function.
	(trad_frame_set_reg_addr): Use new function.
---
 gdb/ChangeLog    |  9 +++++++++
 gdb/trad-frame.c | 21 ++++++++++++++++++---
 gdb/trad-frame.h |  8 ++++++++
 3 files changed, 35 insertions(+), 3 deletions(-)

diff --git a/gdb/trad-frame.c b/gdb/trad-frame.c
index a6e24158279..e3e48f2b7b0 100644
--- a/gdb/trad-frame.c
+++ b/gdb/trad-frame.c
@@ -105,6 +105,22 @@ trad_frame_set_value (struct trad_frame_saved_reg this_saved_regs[],
   this_saved_regs[regnum].addr = val;
 }
 
+void
+trad_frame_set_realreg (struct trad_frame_saved_reg this_saved_regs[],
+			int regnum, int realreg)
+{
+  this_saved_regs[regnum].realreg = realreg;
+  this_saved_regs[regnum].addr = -1;
+}
+
+void
+trad_frame_set_addr (struct trad_frame_saved_reg this_saved_regs[],
+		     int regnum, CORE_ADDR addr)
+{
+  this_saved_regs[regnum].realreg = regnum;
+  this_saved_regs[regnum].addr = addr;
+}
+
 void
 trad_frame_set_reg_value (struct trad_frame_cache *this_trad_cache,
 			  int regnum, LONGEST val)
@@ -118,15 +134,14 @@ void
 trad_frame_set_reg_realreg (struct trad_frame_cache *this_trad_cache,
 			    int regnum, int realreg)
 {
-  this_trad_cache->prev_regs[regnum].realreg = realreg;
-  this_trad_cache->prev_regs[regnum].addr = -1;
+  trad_frame_set_realreg (this_trad_cache->prev_regs, regnum, realreg);
 }
 
 void
 trad_frame_set_reg_addr (struct trad_frame_cache *this_trad_cache,
 			 int regnum, CORE_ADDR addr)
 {
-  this_trad_cache->prev_regs[regnum].addr = addr;
+  trad_frame_set_addr (this_trad_cache->prev_regs, regnum, addr);
 }
 
 void
diff --git a/gdb/trad-frame.h b/gdb/trad-frame.h
index f7ec43cc80c..a11a2c29c9c 100644
--- a/gdb/trad-frame.h
+++ b/gdb/trad-frame.h
@@ -88,6 +88,14 @@ struct trad_frame_saved_reg
 void trad_frame_set_value (struct trad_frame_saved_reg this_saved_regs[],
 			   int regnum, LONGEST val);
 
+/* Encode REGNUM is in REALREG in the trad-frame.  */
+void trad_frame_set_realreg (struct trad_frame_saved_reg this_saved_regs[],
+			     int regnum, int realreg);
+
+/* Encode REGNUM is at address ADDR in the trad-frame.  */
+void trad_frame_set_addr (struct trad_frame_saved_reg this_trad_cache[],
+			  int regnum, CORE_ADDR addr);
+
 /* Mark REGNUM as unknown.  */
 void trad_frame_set_unknown (struct trad_frame_saved_reg this_saved_regs[],
 			     int regnum);
-- 
2.14.4

Re: [PATCH 3/4] gdb: Extend the trad-frame API

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:

Andrew> Adds two new functions to the trad-frame API and update the internals
Andrew> of trad-frame to use the new functions.  These functions will be used
Andrew> in later commits.

Andrew> gdb/ChangeLog:

Andrew> 	* trad-frame.h (trad_frame_set_realreg): Declare.
Andrew> 	(trad_frame_set_addr): Declare.
Andrew> 	* trad-frame.c (trad_frame_set_realreg): Define new function.
Andrew> 	(trad_frame_set_addr): Define new function.
Andrew> 	(trad_frame_set_reg_realreg): Use new function.
Andrew> 	(trad_frame_set_reg_addr): Use new function.

IIUC this is just exposing a bit more of trad frame, so that you can
manipulate the information without having a frame cache.

This seems reasonable enough.  This is ok.

I suppose another approach would have been to just keep a trad frame
cache as a member of your new object.  But there's nothing wrong with
your chosen approach either, IMO.

Tom

[PATCH 2/4] gdb/riscv: Extend instruction decode to cover more instructions

[Andrew Burgess]

Extends the instruction decoder used during prologue scan and software
single step to cover more instructions.  These instructions are
encountered when running the current GDB testsuite with the DWARF
stack unwinders turned off.

gdb/ChangeLog:

	* riscv-tdep.c (riscv_insn::decode): Decode c.addi4spn, c.sd,
	c.sw, c.swsp, and c.sdsp.
---
 gdb/ChangeLog    |  5 +++++
 gdb/riscv-tdep.c | 40 ++++++++++++++++++++++++++++++++++++++--
 2 files changed, 43 insertions(+), 2 deletions(-)

diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c
index 2f619c35e75..b4ac83a6dd9 100644
--- a/gdb/riscv-tdep.c
+++ b/gdb/riscv-tdep.c
@@ -972,6 +972,31 @@ private:
     m_imm.s = EXTRACT_STYPE_IMM (ival);
   }
 
+  /* Helper for DECODE, decode 16-bit CS-type instruction.  The immediate
+     encoding is different for each CSS format instruction, so extracting
+     the immediate is left up to the caller, who should pass the extracted
+     immediate value through in IMM.  */
+  void decode_cs_type_insn (enum opcode opcode, ULONGEST ival, int imm)
+  {
+    m_opcode = opcode;
+    m_imm.s = imm;
+    m_rs1 = decode_register_index_short (ival, OP_SH_CRS1S);
+    m_rs2 = decode_register_index_short (ival, OP_SH_CRS2S);
+  }
+
+  /* Helper for DECODE, decode 16-bit CSS-type instruction.  The immediate
+     encoding is different for each CSS format instruction, so extracting
+     the immediate is left up to the caller, who should pass the extracted
+     immediate value through in IMM.  */
+  void decode_css_type_insn (enum opcode opcode, ULONGEST ival, int imm)
+  {
+    m_opcode = opcode;
+    m_imm.s = imm;
+    m_rs1 = RISCV_SP_REGNUM;
+    /* Not a compressed register number in this case.  */
+    m_rs2 = decode_register_index (ival, OP_SH_CRS2);
+  }
+
   /* Helper for DECODE, decode 32-bit U-type instruction.  */
   void decode_u_type_insn (enum opcode opcode, ULONGEST ival)
   {
@@ -1165,14 +1190,25 @@ riscv_insn::decode (struct gdbarch *gdbarch, CORE_ADDR pc)
 	  m_rd = m_rs1 = decode_register_index (ival, OP_SH_RD);
 	  m_imm.s = EXTRACT_RVC_ADDI16SP_IMM (ival);
 	}
+      else if (xlen < 16 && is_c_addi4spn_insn (ival))
+	{
+	  m_opcode = ADDI;
+	  m_rd = decode_register_index_short (ival, OP_SH_CRS2S);
+	  m_rs1 = RISCV_SP_REGNUM;
+	  m_imm.s = EXTRACT_RVC_ADDI4SPN_IMM (ival);
+	}
       else if (is_c_lui_insn (ival))
 	m_opcode = OTHER;
       /* C_SD and C_FSW have the same opcode.  C_SD is RV64 and RV128 only,
 	 and C_FSW is RV32 only.  */
       else if (xlen != 4 && is_c_sd_insn (ival))
-	m_opcode = OTHER;
+	decode_cs_type_insn (SD, ival, EXTRACT_RVC_LD_IMM (ival));
       else if (is_c_sw_insn (ival))
-	m_opcode = OTHER;
+	decode_cs_type_insn (SW, ival, EXTRACT_RVC_LW_IMM (ival));
+      else if (is_c_swsp_insn (ival))
+	decode_css_type_insn (SW, ival, EXTRACT_RVC_SWSP_IMM (ival));
+      else if (is_c_sdsp_insn (ival))
+	decode_css_type_insn (SW, ival, EXTRACT_RVC_SDSP_IMM (ival));
       /* C_JR and C_MV have the same opcode.  If RS2 is 0, then this is a C_JR.
 	 So must try to match C_JR first as it ahs more bits in mask.  */
       else if (is_c_jr_insn (ival))
-- 
2.14.4

Re: [PATCH 2/4] gdb/riscv: Extend instruction decode to cover more instructions

[Jim Wilson]

On Wed, Aug 29, 2018 at 9:40 AM, Andrew Burgess
<andrew.burgess@embecosm.com> wrote:
> +      else if (xlen < 16 && is_c_addi4spn_insn (ival))
> +       {
> +         m_opcode = ADDI;
> +         m_rd = decode_register_index_short (ival, OP_SH_CRS2S);
> +         m_rs1 = RISCV_SP_REGNUM;
> +         m_imm.s = EXTRACT_RVC_ADDI4SPN_IMM (ival);
> +       }

c.addi4spn is always valid.  I don't think that there should be a xlen
check here.

> +      else if (is_c_sdsp_insn (ival))
> +       decode_css_type_insn (SW, ival, EXTRACT_RVC_SDSP_IMM (ival));

c.sdsp is only valid for RV64I and RV128I.  For RV32I this is a
c.fswsp instruction.  So there needs to be a xlen != 4 check here

Jim

Re: [PATCH 2/4] gdb/riscv: Extend instruction decode to cover more instructions

[Palmer Dabbelt]

On Wed, 29 Aug 2018 09:40:52 PDT (-0700), andrew.burgess@embecosm.com wrote:
> Extends the instruction decoder used during prologue scan and software
> single step to cover more instructions.  These instructions are
> encountered when running the current GDB testsuite with the DWARF
> stack unwinders turned off.
>
> gdb/ChangeLog:
>
> 	* riscv-tdep.c (riscv_insn::decode): Decode c.addi4spn, c.sd,
> 	c.sw, c.swsp, and c.sdsp.
> ---
>  gdb/ChangeLog    |  5 +++++
>  gdb/riscv-tdep.c | 40 ++++++++++++++++++++++++++++++++++++++--
>  2 files changed, 43 insertions(+), 2 deletions(-)
>
> diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c
> index 2f619c35e75..b4ac83a6dd9 100644
> --- a/gdb/riscv-tdep.c
> +++ b/gdb/riscv-tdep.c
> @@ -972,6 +972,31 @@ private:
>      m_imm.s = EXTRACT_STYPE_IMM (ival);
>    }
>
> +  /* Helper for DECODE, decode 16-bit CS-type instruction.  The immediate
> +     encoding is different for each CSS format instruction, so extracting

I think you mean "each CS format" here -- it's true for CSS formats as well, 
just not relevant until below.

> +     the immediate is left up to the caller, who should pass the extracted
> +     immediate value through in IMM.  */
> +  void decode_cs_type_insn (enum opcode opcode, ULONGEST ival, int imm)
> +  {
> +    m_opcode = opcode;
> +    m_imm.s = imm;
> +    m_rs1 = decode_register_index_short (ival, OP_SH_CRS1S);
> +    m_rs2 = decode_register_index_short (ival, OP_SH_CRS2S);
> +  }
> +
> +  /* Helper for DECODE, decode 16-bit CSS-type instruction.  The immediate
> +     encoding is different for each CSS format instruction, so extracting
> +     the immediate is left up to the caller, who should pass the extracted
> +     immediate value through in IMM.  */
> +  void decode_css_type_insn (enum opcode opcode, ULONGEST ival, int imm)
> +  {
> +    m_opcode = opcode;
> +    m_imm.s = imm;
> +    m_rs1 = RISCV_SP_REGNUM;
> +    /* Not a compressed register number in this case.  */
> +    m_rs2 = decode_register_index (ival, OP_SH_CRS2);
> +  }
> +
>    /* Helper for DECODE, decode 32-bit U-type instruction.  */
>    void decode_u_type_insn (enum opcode opcode, ULONGEST ival)
>    {
> @@ -1165,14 +1190,25 @@ riscv_insn::decode (struct gdbarch *gdbarch, CORE_ADDR pc)
>  	  m_rd = m_rs1 = decode_register_index (ival, OP_SH_RD);
>  	  m_imm.s = EXTRACT_RVC_ADDI16SP_IMM (ival);
>  	}
> +      else if (xlen < 16 && is_c_addi4spn_insn (ival))
> +	{
> +	  m_opcode = ADDI;
> +	  m_rd = decode_register_index_short (ival, OP_SH_CRS2S);
> +	  m_rs1 = RISCV_SP_REGNUM;
> +	  m_imm.s = EXTRACT_RVC_ADDI4SPN_IMM (ival);

I think this is OK: the immediate is actually unsigned, but it's only 10 bits 
long so it doesn't matter.

> +	}
>        else if (is_c_lui_insn (ival))
>  	m_opcode = OTHER;
>        /* C_SD and C_FSW have the same opcode.  C_SD is RV64 and RV128 only,
>  	 and C_FSW is RV32 only.  */
>        else if (xlen != 4 && is_c_sd_insn (ival))
> -	m_opcode = OTHER;
> +	decode_cs_type_insn (SD, ival, EXTRACT_RVC_LD_IMM (ival));
>        else if (is_c_sw_insn (ival))
> -	m_opcode = OTHER;
> +	decode_cs_type_insn (SW, ival, EXTRACT_RVC_LW_IMM (ival));
> +      else if (is_c_swsp_insn (ival))
> +	decode_css_type_insn (SW, ival, EXTRACT_RVC_SWSP_IMM (ival));
> +      else if (is_c_sdsp_insn (ival))
> +	decode_css_type_insn (SW, ival, EXTRACT_RVC_SDSP_IMM (ival));
>        /* C_JR and C_MV have the same opcode.  If RS2 is 0, then this is a C_JR.
>  	 So must try to match C_JR first as it ahs more bits in mask.  */
>        else if (is_c_jr_insn (ival))

Re: [PATCH 0/4] RISCV Non-DWARF stack unwinding

[Palmer Dabbelt]

On Wed, 29 Aug 2018 09:40:50 PDT (-0700), andrew.burgess@embecosm.com wrote:
> A series of patches providing non-DWARF stack unwinding on RISC-V.
> Tested on a set of targetes including with and without compressed, and
> FP, and on 32 and 64 bit.
>
> Patch #3 touches generic code and so will need global maintainer
> review before I can commit.
>
> Review and feedback is welcome for all of the other patches too.

I have a few minor comments, but nothing that should block merging the patches 
on my end.  No idea about the global stuff, though, as I'm far from a GDB 
expert.

Thanks!

>
> Thanks,
> Andrew
>
> ---
>
> Andrew Burgess (4):
>   gdb/riscv: remove extra caching of misa register
>   gdb/riscv: Extend instruction decode to cover more instructions
>   gdb: Extend the trad-frame API
>   gdb/riscv: Provide non-DWARF stack unwinder
>
>  gdb/ChangeLog    |  42 +++++++
>  gdb/riscv-tdep.c | 361 +++++++++++++++++++++++++++++++++----------------------
>  gdb/riscv-tdep.h |   2 +
>  gdb/trad-frame.c |  21 +++-
>  gdb/trad-frame.h |   8 ++
>  5 files changed, 287 insertions(+), 147 deletions(-)