[PATCH v3 0/2] Fix gdb.base/sigstep.exp for riscv64-linux

[PATCH v3 0/2] Fix gdb.base/sigstep.exp for riscv64-linux

[Lancelot SIX]

On a riscv64-linux-gnu platform, gdb.base/sigstep.exp has many failures:

	                === gdb Summary ===

	# of expected passes            711
	# of unexpected failures        58

This patch series fixes those errors.  Patch #1 disables tests that
cannot pass because the platform lacks support for hardware single
stepping.  Patch #2 implements stepping outside of signal handlers for
riscv*-*-linux* platforms.

The series was tested on riscv64-linux-gnu.

Since V2:

	- Patch #1 also disables tests that require hardwre single
	stepping for riscv32.
	- Add support for stepping outside of signal handler.

Lancelot SIX (2):
  gdb/testsuite: Declare that riscv64-*-linux* cannot
    hardware_single_step
  gdb: Support stepping out from signal handler on riscv*-linux

 gdb/riscv-linux-tdep.c    | 24 ++++++++++++++++++++++++
 gdb/riscv-tdep.c          | 10 ++++++++++
 gdb/riscv-tdep.h          |  4 ++++
 gdb/testsuite/lib/gdb.exp |  2 +-
 4 files changed, 39 insertions(+), 1 deletion(-)

-- 
2.30.2

[PATCH v3 1/2] gdb/testsuite: Declare that riscv*-*-linux* cannot hardware_single_step

[Lancelot SIX]

Many tests fail in gdb/testsuite/gdb.base/sigstep.exp on
riscv64-linux-gnu.  Those tests check that when stepping, if the
debuggee received a signal it should step inside the signal handler.

This feature requires hardware support for single stepping (or at least
kernel support), but none are available on riscv*-linux-gnu hosts.

This patch adds RISC-V to the list of configurations that does not
have hardware single step capability, disabling tests relying on such
feature.

Tested on riscv64-linux-gnu.
---
 gdb/testsuite/lib/gdb.exp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 38f40fdddb5..23798217bbb 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -2831,7 +2831,7 @@ proc can_hardware_single_step {} {
 
     if { [istarget "arm*-*-*"] || [istarget "mips*-*-*"]
 	 || [istarget "tic6x-*-*"] || [istarget "sparc*-*-linux*"]
-	 || [istarget "nios2-*-*"] } {
+	 || [istarget "nios2-*-*"] || [istarget "riscv*-*-linux*"] } {
 	return 0
     }
 
-- 
2.30.2

[PATCH v3 2/2] gdb: Support stepping out from signal handler on riscv*-linux

[Lancelot SIX]

Currently, gdb cannot step outside of a signal handler on RISC-V
platforms.  This causes multiple failures in gdb.base/sigstep.exp:

	FAIL: gdb.base/sigstep.exp: continue to handler, nothing in handler, step from handler: leave handler (timeout)
	FAIL: gdb.base/sigstep.exp: continue to handler, si+advance in handler, step from handler: leave handler (timeout)
	FAIL: gdb.base/sigstep.exp: continue to handler, nothing in handler, next from handler: leave handler (timeout)
	FAIL: gdb.base/sigstep.exp: continue to handler, si+advance in handler, next from handler: leave handler (timeout)
	FAIL: gdb.base/sigstep.exp: stepi from handleri: leave signal trampoline
	FAIL: gdb.base/sigstep.exp: nexti from handleri: leave signal trampoline

	                === gdb Summary ===

	# of expected passes            587
	# of unexpected failures        6

This patch adds support for stepping outside of a signal handler on
riscv*-*-linux*.

Implementation is heavily inspired from mips_linux_syscall_next_pc and
surroundings as advised by Pedro Alves.

After this patch, all tests in gdb.base/sigstep.exp pass.

Build and tested on riscv64-linux-gnu.
---
 gdb/riscv-linux-tdep.c | 24 ++++++++++++++++++++++++
 gdb/riscv-tdep.c       | 10 ++++++++++
 gdb/riscv-tdep.h       |  4 ++++
 3 files changed, 38 insertions(+)

diff --git a/gdb/riscv-linux-tdep.c b/gdb/riscv-linux-tdep.c
index ca97a60128f..2a16214d50d 100644
--- a/gdb/riscv-linux-tdep.c
+++ b/gdb/riscv-linux-tdep.c
@@ -27,6 +27,11 @@
 #include "trad-frame.h"
 #include "gdbarch.h"
 
+/* The following value is derived from __NR_rt_sigreturn in
+   <include/uapi/asm-generic/unistd.h> from the linux source tree.  */
+
+#define RISCV_NR_rt_sigreturn 139
+
 /* Define the general register mapping.  The kernel puts the PC at offset 0,
    gdb puts it at offset 32.  Register x0 is always 0 and can be ignored.
    Registers x1 to x31 are in the same place.  */
@@ -154,11 +159,28 @@ riscv_linux_sigframe_init (const struct tramp_frame *self,
   trad_frame_set_id (this_cache, frame_id_build (frame_sp, func));
 }
 
+/* When FRAME is at a syscall instruction (ECALL), return the PC of the next
+   instruction to be executed.  */
+
+static CORE_ADDR
+riscv_linux_syscall_next_pc (struct frame_info *frame)
+{
+  const CORE_ADDR pc = get_frame_pc (frame);
+  const ULONGEST a7 = get_frame_register_unsigned (frame, RISCV_A7_REGNUM);
+
+  if (a7 == RISCV_NR_rt_sigreturn)
+    return frame_unwind_caller_pc (frame);
+
+  return pc + 4 /* Length of the ECALL insn.  */;
+}
+
 /* Initialize RISC-V Linux ABI info.  */
 
 static void
 riscv_linux_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
 {
+  struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
+
   linux_init_abi (info, gdbarch, 0);
 
   set_gdbarch_software_single_step (gdbarch, riscv_software_single_step);
@@ -182,6 +204,8 @@ riscv_linux_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
     (gdbarch, riscv_linux_iterate_over_regset_sections);
 
   tramp_frame_prepend_unwinder (gdbarch, &riscv_linux_sigframe);
+
+  tdep->syscall_next_pc = riscv_linux_syscall_next_pc;
 }
 
 /* Initialize RISC-V Linux target support.  */
diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c
index 19e2616c9c0..b5b0d2d79de 100644
--- a/gdb/riscv-tdep.c
+++ b/gdb/riscv-tdep.c
@@ -1421,6 +1421,8 @@ class riscv_insn
       /* These are needed for stepping over atomic sequences.  */
       LR,
       SC,
+      /* This instruction is used to do a syscall.  */
+      ECALL,
 
       /* Other instructions are not interesting during the prologue scan, and
 	 are ignored.  */
@@ -1711,6 +1713,8 @@ riscv_insn::decode (struct gdbarch *gdbarch, CORE_ADDR pc)
 	decode_r_type_insn (SC, ival);
       else if (is_sc_d_insn (ival))
 	decode_r_type_insn (SC, ival);
+      else if (is_ecall_insn (ival))
+	decode_i_type_insn (ECALL, ival);
       else
 	/* None of the other fields are valid in this case.  */
 	m_opcode = OTHER;
@@ -3764,6 +3768,7 @@ static CORE_ADDR
 riscv_next_pc (struct regcache *regcache, CORE_ADDR pc)
 {
   struct gdbarch *gdbarch = regcache->arch ();
+  const struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
   struct riscv_insn insn;
   CORE_ADDR next_pc;
 
@@ -3826,6 +3831,11 @@ riscv_next_pc (struct regcache *regcache, CORE_ADDR pc)
       if (src1 >= src2)
 	next_pc = pc + insn.imm_signed ();
     }
+  else if (insn.opcode () == riscv_insn::ECALL)
+    {
+      if (tdep->syscall_next_pc != nullptr)
+	next_pc = tdep->syscall_next_pc (get_current_frame ());
+    }
 
   return next_pc;
 }
diff --git a/gdb/riscv-tdep.h b/gdb/riscv-tdep.h
index 62bf4797d02..a69fa489b96 100644
--- a/gdb/riscv-tdep.h
+++ b/gdb/riscv-tdep.h
@@ -34,6 +34,7 @@ enum
   RISCV_FP_REGNUM = 8,		/* Frame Pointer.  */
   RISCV_A0_REGNUM = 10,		/* First argument.  */
   RISCV_A1_REGNUM = 11,		/* Second argument.  */
+  RISCV_A7_REGNUM = 17,		/* Seventh argument.  */
   RISCV_PC_REGNUM = 32,		/* Program Counter.  */
 
   RISCV_NUM_INTEGER_REGS = 32,
@@ -102,6 +103,9 @@ struct gdbarch_tdep
   int duplicate_frm_regnum = -1;
   int duplicate_fcsr_regnum = -1;
 
+  /* Return the expected next PC if FRAME is stopped at a syscall
+     instruction.  */
+  CORE_ADDR (*syscall_next_pc) (struct frame_info *frame);
 };
 
 
-- 
2.30.2

Re: [PATCH v3 0/2] Fix gdb.base/sigstep.exp for riscv64-linux

[Jim Wilson]

On Tue, Jul 6, 2021 at 5:31 PM Lancelot SIX via Gdb-patches <
gdb-patches@sourceware.org> wrote:

> This patch series fixes those errors.  Patch #1 disables tests that
> cannot pass because the platform lacks support for hardware single
> stepping.  Patch #2 implements stepping outside of signal handlers for
> riscv*-*-linux* platforms.
>

FYI On the HiFive Unleashed, it isn't possible to implement hardware
breakpoints because the debug registers can only be accessed from debug
mode, i.e. via jtag.  They can't be accessed by the linux kernel, so I only
implemented software breakpoints.  The debug spec was later changed to
allow a few debug registers to be accessed from machine mode, and this is
implemented in the HiFive Unmatched.  So in theory we can support hardware
breakpoints (and tracepoints) on the HiFive Unmatched and similar new
targets, like the BeagleV.  This will require OpenSBI changes to add hooks
to access the new registers, linux kernel changes to make the registers
available, linux kernel changes to add new ptrace features to access them,
and gdb changes to test for them and use them when available.  But
unfortunately there is no one actively working on RISC-V Linux gdb support
that I know of, so I have no idea when this work will be done.

When we do have hardware breakpoint support these tests will have to be
re-enabled, though it will need to be conditional depending on whether the
target under test supports hardware breakpoints or not.

Jim

Re: [PATCH v3 0/2] Fix gdb.base/sigstep.exp for riscv64-linux

[Lancelot SIX]

On Wed, Jul 07, 2021 at 02:01:39PM -0700, Jim Wilson wrote:
> On Tue, Jul 6, 2021 at 5:31 PM Lancelot SIX via Gdb-patches <
> gdb-patches@sourceware.org> wrote:
> 
> > This patch series fixes those errors.  Patch #1 disables tests that
> > cannot pass because the platform lacks support for hardware single
> > stepping.  Patch #2 implements stepping outside of signal handlers for
> > riscv*-*-linux* platforms.
> >
> 
> FYI On the HiFive Unleashed, it isn't possible to implement hardware
> breakpoints because the debug registers can only be accessed from debug
> mode, i.e. via jtag.  They can't be accessed by the linux kernel, so I only
> implemented software breakpoints.  The debug spec was later changed to
> allow a few debug registers to be accessed from machine mode, and this is
> implemented in the HiFive Unmatched.  So in theory we can support hardware
> breakpoints (and tracepoints) on the HiFive Unmatched and similar new
> targets, like the BeagleV.  This will require OpenSBI changes to add hooks
> to access the new registers, linux kernel changes to make the registers
> available, linux kernel changes to add new ptrace features to access them,
> and gdb changes to test for them and use them when available.  But
> unfortunately there is no one actively working on RISC-V Linux gdb support
> that I know of, so I have no idea when this work will be done.
> 
> When we do have hardware breakpoint support these tests will have to be
> re-enabled, though it will need to be conditional depending on whether the
> target under test supports hardware breakpoints or not.
> 
> Jim

Hi Jim,

Thanks for the clarifications.

I went through (skimmed through to be more precise) the debug
specification for RISC-V.  Considering that 1) support of the debugger
spec is (as far as I know) optional, 2) within this spec, trigger
functionality is optional and 3) there is no support in the Linux kernel
for it so far anyway, I could assume that from GDB's perspective there
is no hardware assisted single-step available for the riscv*-linux
platform.  At least my take as far as the testsuite is concerned is that
we expect GDB behave as if there is no hardware support.

I guess I could have made it clearer that hardware support is not
inherently missing on the platform, “just” optional and de facto
unavailable on all current configurations.  I could maybe set XFAILS on
the tests to indicate that it is expected that GDB behaves as if there
is no hardware single step available,  but that could change in the
future.  Would you find this more accurate?

As for people working on RISC-V Linux gdb support, I am obviously not
there yet, but I use my Unmatched board to work on GDB.  So over time I
expect to help improve the support.  I am mainly running the testsuite,
and trying to understand what is happening when I hit failing tests.
This is how I am looking for entry points in the codebase for the
moment.

Lancelot.

Re: [PATCH v3 0/2] Fix gdb.base/sigstep.exp for riscv64-linux

[Jim Wilson]

On Wed, Jul 7, 2021 at 3:55 PM Lancelot SIX <lsix@lancelotsix.com> wrote:

> platform.  At least my take as far as the testsuite is concerned is that
> we expect GDB behave as if there is no hardware support.
>

No problem.  We can fix this when we eventually write the missing gdb
hardware support.

In addition to tests failing, you might want to look at tests not being
run.  gdb.asm for instance doesn't run because there is no risc-v.inc file,
and no stanza in asm-source.exp to use it.

gdb.trace/entry-values.exp should be an easy fix.  It just needs a regexp
to match RISC-V call instructions.

The gdb.compile stuff is a mess.  I got the simple tests working.  But we
might not be able to get the more complex ones working.  I just vaguely
remember something about a conflict between the RISC-V ABI and gdb's
expectations of how this stuff should work.

And I think that is all of the notes I have for gdb testsuite results.  I
haven't had time to look at this in a while.

Jim

Re: [PATCH v3 0/2] Fix gdb.base/sigstep.exp for riscv64-linux

[Pedro Alves]

Hi Jim,

On 2021-07-07 10:01 p.m., Jim Wilson wrote:
> On Tue, Jul 6, 2021 at 5:31 PM Lancelot SIX via Gdb-patches <
> gdb-patches@sourceware.org> wrote:
> 
>> This patch series fixes those errors.  Patch #1 disables tests that
>> cannot pass because the platform lacks support for hardware single
>> stepping.  Patch #2 implements stepping outside of signal handlers for
>> riscv*-*-linux* platforms.
>>
> 
> FYI On the HiFive Unleashed, it isn't possible to implement hardware
> breakpoints because the debug registers can only be accessed from debug
> mode, i.e. via jtag.  They can't be accessed by the linux kernel, so I only
> implemented software breakpoints.  The debug spec was later changed to
> allow a few debug registers to be accessed from machine mode, and this is
> implemented in the HiFive Unmatched.  So in theory we can support hardware
> breakpoints (and tracepoints) on the HiFive Unmatched and similar new
> targets, like the BeagleV.  This will require OpenSBI changes to add hooks
> to access the new registers, linux kernel changes to make the registers
> available, linux kernel changes to add new ptrace features to access them,
> and gdb changes to test for them and use them when available.  But
> unfortunately there is no one actively working on RISC-V Linux gdb support
> that I know of, so I have no idea when this work will be done.
> 
> When we do have hardware breakpoint support these tests will have to be
> re-enabled, though it will need to be conditional depending on whether the
> target under test supports hardware breakpoints or not.
> 


Note this is about hardware single-stepping, not hardware breakpoints.
In Linux/ptrace, this would mean the kernel would support PTRACE_SINGLESTEP.
The kernel would set some debug trace register flag on so that the CPU stops after
executing one instruction, and then report a SIGTRAP stop.

Re: [PATCH v3 2/2] gdb: Support stepping out from signal handler on riscv*-linux

[Pedro Alves]

On 2021-07-07 1:30 a.m., Lancelot SIX via Gdb-patches wrote:

> After this patch, all tests in gdb.base/sigstep.exp pass.
> 
> Build and tested on riscv64-linux-gnu.

Thanks.  Other than a couple easyfix issues below, this LGTM.

>  #include "gdbarch.h"
>  
> +/* The following value is derived from __NR_rt_sigreturn in
> +   <include/uapi/asm-generic/unistd.h> from the linux source tree.  */

s/linux/Linux/

>  
>        /* Other instructions are not interesting during the prologue scan, and
>  	 are ignored.  */
> @@ -1711,6 +1713,8 @@ riscv_insn::decode (struct gdbarch *gdbarch, CORE_ADDR pc)
>  	decode_r_type_insn (SC, ival);
>        else if (is_sc_d_insn (ival))
>  	decode_r_type_insn (SC, ival);
> +      else if (is_ecall_insn (ival))
> +	decode_i_type_insn (ECALL, ival);

OOC, where are these is_FOO_insn functions declared/defined?

> @@ -3826,6 +3831,11 @@ riscv_next_pc (struct regcache *regcache, CORE_ADDR pc)
>        if (src1 >= src2)
>  	next_pc = pc + insn.imm_signed ();
>      }
> +  else if (insn.opcode () == riscv_insn::ECALL)
> +    {
> +      if (tdep->syscall_next_pc != nullptr)
> +	next_pc = tdep->syscall_next_pc (get_current_frame ());


	else
	  next_pc += 4;

?

> +    }
>  
>    return next_pc;
>  }

>  
> +  /* Return the expected next PC if FRAME is stopped at a syscall
> +     instruction.  */
> +  CORE_ADDR (*syscall_next_pc) (struct frame_info *frame);

"if" -> "assuming" ?

Re: [PATCH v3 1/2] gdb/testsuite: Declare that riscv*-*-linux* cannot hardware_single_step

[Pedro Alves]

On 2021-07-07 1:30 a.m., Lancelot SIX via Gdb-patches wrote:
> Many tests fail in gdb/testsuite/gdb.base/sigstep.exp on
> riscv64-linux-gnu.  Those tests check that when stepping, if the
> debuggee received a signal it should step inside the signal handler.
> 
> This feature requires hardware support for single stepping (or at least
> kernel support), but none are available on riscv*-linux-gnu hosts.
> 
> This patch adds RISC-V to the list of configurations that does not
> have hardware single step capability, disabling tests relying on such
> feature.
> 
> Tested on riscv64-linux-gnu.

Still OK.

Re: [PATCH v3 2/2] gdb: Support stepping out from signal handler on riscv*-linux

[Lancelot SIX]

On Thu, Jul 08, 2021 at 11:00:33AM +0100, Pedro Alves wrote:
> On 2021-07-07 1:30 a.m., Lancelot SIX via Gdb-patches wrote:
> 
> > After this patch, all tests in gdb.base/sigstep.exp pass.
> > 
> > Build and tested on riscv64-linux-gnu.
> 
> Thanks.  Other than a couple easyfix issues below, this LGTM.
> 
> >  #include "gdbarch.h"
> >  
> > +/* The following value is derived from __NR_rt_sigreturn in
> > +   <include/uapi/asm-generic/unistd.h> from the linux source tree.  */
> 
> s/linux/Linux/
> 
> >  
> >        /* Other instructions are not interesting during the prologue scan, and
> >  	 are ignored.  */
> > @@ -1711,6 +1713,8 @@ riscv_insn::decode (struct gdbarch *gdbarch, CORE_ADDR pc)
> >  	decode_r_type_insn (SC, ival);
> >        else if (is_sc_d_insn (ival))
> >  	decode_r_type_insn (SC, ival);
> > +      else if (is_ecall_insn (ival))
> > +	decode_i_type_insn (ECALL, ival);
> 
> OOC, where are these is_FOO_insn functions declared/defined?
> 

Hi,

There is a DECLARE_INSN macro that does that in gdb/riscv-tdep.c

	/* Define a series of is_XXX_insn functions to check if the value INSN
	   is an instance of instruction XXX.  */
	#define DECLARE_INSN(INSN_NAME, INSN_MATCH, INSN_MASK) \
	static inline bool is_ ## INSN_NAME ## _insn (long insn) \
	{ \
	  return (insn & INSN_MASK) == INSN_MATCH; \
	}
	#include "opcode/riscv-opc.h"
	#undef DECLARE_INSN

The macro is instanciated in include/opcode/riscv-opc.h as follows:

	#define MATCH_EBREAK 0x100073
	#define MASK_EBREAK  0xffffffff
	[…]
	#ifdef DECLARE_INSN
	DECLARE_INSN(ebreak, MATCH_EBREAK, MASK_EBREAK)
	#endif /* DECLARE_INSN */

> > @@ -3826,6 +3831,11 @@ riscv_next_pc (struct regcache *regcache, CORE_ADDR pc)
> >        if (src1 >= src2)
> >  	next_pc = pc + insn.imm_signed ();
> >      }
> > +  else if (insn.opcode () == riscv_insn::ECALL)
> > +    {
> > +      if (tdep->syscall_next_pc != nullptr)
> > +	next_pc = tdep->syscall_next_pc (get_current_frame ());
> 
> 
> 	else
> 	  next_pc += 4;
> 
> ?

OK, I’ll change that.

> 
> > +    }
> >  
> >    return next_pc;
> >  }
> 
> >  
> > +  /* Return the expected next PC if FRAME is stopped at a syscall
> > +     instruction.  */
> > +  CORE_ADDR (*syscall_next_pc) (struct frame_info *frame);
> 
> "if" -> "assuming" ?

OK, seems more appropriate.

Thanks,
Lancelot.

Re: [PATCH v3 0/2] Fix gdb.base/sigstep.exp for riscv64-linux

[Lancelot SIX]

On Thu, Jul 08, 2021 at 10:44:07AM +0100, Pedro Alves wrote:
> Hi Jim,
> 
> On 2021-07-07 10:01 p.m., Jim Wilson wrote:
> > On Tue, Jul 6, 2021 at 5:31 PM Lancelot SIX via Gdb-patches <
> > gdb-patches@sourceware.org> wrote:
> > 
> >> This patch series fixes those errors.  Patch #1 disables tests that
> >> cannot pass because the platform lacks support for hardware single
> >> stepping.  Patch #2 implements stepping outside of signal handlers for
> >> riscv*-*-linux* platforms.
> >>
> > 
> > FYI On the HiFive Unleashed, it isn't possible to implement hardware
> > breakpoints because the debug registers can only be accessed from debug
> > mode, i.e. via jtag.  They can't be accessed by the linux kernel, so I only
> > implemented software breakpoints.  The debug spec was later changed to
> > allow a few debug registers to be accessed from machine mode, and this is
> > implemented in the HiFive Unmatched.  So in theory we can support hardware
> > breakpoints (and tracepoints) on the HiFive Unmatched and similar new
> > targets, like the BeagleV.  This will require OpenSBI changes to add hooks
> > to access the new registers, linux kernel changes to make the registers
> > available, linux kernel changes to add new ptrace features to access them,
> > and gdb changes to test for them and use them when available.  But
> > unfortunately there is no one actively working on RISC-V Linux gdb support
> > that I know of, so I have no idea when this work will be done.
> > 
> > When we do have hardware breakpoint support these tests will have to be
> > re-enabled, though it will need to be conditional depending on whether the
> > target under test supports hardware breakpoints or not.
> > 
> 
> 
> Note this is about hardware single-stepping, not hardware breakpoints.
> In Linux/ptrace, this would mean the kernel would support PTRACE_SINGLESTEP.
> The kernel would set some debug trace register flag on so that the CPU stops after
> executing one instruction, and then report a SIGTRAP stop.

Hi,

In the RISC-V debugging spec[1], there is support for this via the
“Instruction Count” trigger (icount[2]).  I guess this is what an
implementation for PTRACE_SINGLESTEP will be looking for.

Lancelot.

[1] https://raw.githubusercontent.com/riscv/riscv-debug-spec/master/riscv-debug-stable.pdf
[2] Sect 5.5.13, p78

Re: [PATCH v3 2/2] gdb: Support stepping out from signal handler on riscv*-linux

[Lancelot SIX]

On Thu, Jul 08, 2021 at 11:49:02AM +0000, Lancelot SIX via Gdb-patches wrote:
> > > @@ -3826,6 +3831,11 @@ riscv_next_pc (struct regcache *regcache, CORE_ADDR pc)
> > >        if (src1 >= src2)
> > >  	next_pc = pc + insn.imm_signed ();
> > >      }
> > > +  else if (insn.opcode () == riscv_insn::ECALL)
> > > +    {
> > > +      if (tdep->syscall_next_pc != nullptr)
> > > +	next_pc = tdep->syscall_next_pc (get_current_frame ());
> > 
> > 
> > 	else
> > 	  next_pc += 4;
> > 
> > ?
> 
> OK, I’ll change that.

Actually, no.  I was a bit fast reading your comment and thought we
where in the riscv_syscall_nexc_pc function.

The function is currently implemented as:

	static CORE_ADDR
	riscv_next_pc (struct regcache *regcache, CORE_ADDR pc)
	{
	  struct riscv_insn insn;
	  CORE_ADDR next_pc
	  insn.decode (gdbarch, pc);
	  next_pc = pc + insn.length ();   // <--- The default case is already handled here
	
	  /* a bunch of ifs that can assign a new value to next_pc.  */
	  if (insn.opcode() == …)
	      next_pc = …
	  else if (insn.opcode() == …)
	      next_pc = …
	  […]
	
	  return next_pc;
	}

To make it clearer, the 'next_pc = pc + insn.length ();' statement could
be the one to go in a final “catchall” else where you proposed to add
the '+= 4' (note that when using the compressed instruction set,
instructions can have a size of 2 bytes).

Lancelot.

Re: [PATCH v3 2/2] gdb: Support stepping out from signal handler on riscv*-linux

[Pedro Alves]

On 2021-07-08 1:11 p.m., Lancelot SIX wrote:
> On Thu, Jul 08, 2021 at 11:49:02AM +0000, Lancelot SIX via Gdb-patches wrote:
>>>> @@ -3826,6 +3831,11 @@ riscv_next_pc (struct regcache *regcache, CORE_ADDR pc)
>>>>        if (src1 >= src2)
>>>>  	next_pc = pc + insn.imm_signed ();
>>>>      }
>>>> +  else if (insn.opcode () == riscv_insn::ECALL)
>>>> +    {
>>>> +      if (tdep->syscall_next_pc != nullptr)
>>>> +	next_pc = tdep->syscall_next_pc (get_current_frame ());
>>>
>>>
>>> 	else
>>> 	  next_pc += 4;
>>>
>>> ?
>>
>> OK, I’ll change that.
> 
> Actually, no.  I was a bit fast reading your comment and thought we
> where in the riscv_syscall_nexc_pc function.
> 
> The function is currently implemented as:
> 
> 	static CORE_ADDR
> 	riscv_next_pc (struct regcache *regcache, CORE_ADDR pc)
> 	{
> 	  struct riscv_insn insn;
> 	  CORE_ADDR next_pc
> 	  insn.decode (gdbarch, pc);
> 	  next_pc = pc + insn.length ();   // <--- The default case is already handled here
> 	
> 	  /* a bunch of ifs that can assign a new value to next_pc.  */
> 	  if (insn.opcode() == …)
> 	      next_pc = …
> 	  else if (insn.opcode() == …)
> 	      next_pc = …
> 	  […]
> 	
> 	  return next_pc;
> 	}
> 
> To make it clearer, the 'next_pc = pc + insn.length ();' statement could
> be the one to go in a final “catchall” else where you proposed to add
> the '+= 4' (note that when using the compressed instruction set,
> instructions can have a size of 2 bytes).

Ah, OK, sorry, I should have looked at the code instead of assuming.
Thanks for double checking.

Re: [PATCH v3 0/2] Fix gdb.base/sigstep.exp for riscv64-linux

[Jim Wilson]

On Thu, Jul 8, 2021 at 2:44 AM Pedro Alves <pedro@palves.net> wrote:

> Note this is about hardware single-stepping, not hardware breakpoints.
> In Linux/ptrace, this would mean the kernel would support
> PTRACE_SINGLESTEP.
> The kernel would set some debug trace register flag on so that the CPU
> stops after
> executing one instruction, and then report a SIGTRAP stop.
>

Hardware single-stepping, hardware breakpoints, and hardware tracepoints
are all effectively the same problem for RISC-V at the moment.  Unavailable
on the HiFive Unleashed, available on the Unmatched in theory but not
implemented in the linux kernel yet.

Lancelot SIX mentioned that the debug spec is optional, but the Application
(aka linux) Platform spec requires all of these features.

https://github.com/riscv/riscv-platform-specs/blob/main/riscv-platform-spec.adoc#debug
Except that the application platform spec hasn't been formally approved yet
as far as I know, and there is no guarantee that all linux targets will
actually follow the spec.

Jim

Re: [PATCH v3 0/2] Fix gdb.base/sigstep.exp for riscv64-linux

[Lancelot SIX]

Hi,

I have just pushed the 2 patches (with Pedro's comments taken into
account) to master.

Lancelot.

On Wed, Jul 07, 2021 at 12:30:41AM +0000, Lancelot SIX wrote:
> On a riscv64-linux-gnu platform, gdb.base/sigstep.exp has many failures:
> 
> 	                === gdb Summary ===
> 
> 	# of expected passes            711
> 	# of unexpected failures        58
> 
> This patch series fixes those errors.  Patch #1 disables tests that
> cannot pass because the platform lacks support for hardware single
> stepping.  Patch #2 implements stepping outside of signal handlers for
> riscv*-*-linux* platforms.
> 
> The series was tested on riscv64-linux-gnu.
> 
> Since V2:
> 
> 	- Patch #1 also disables tests that require hardwre single
> 	stepping for riscv32.
> 	- Add support for stepping outside of signal handler.
> 
> Lancelot SIX (2):
>   gdb/testsuite: Declare that riscv64-*-linux* cannot
>     hardware_single_step
>   gdb: Support stepping out from signal handler on riscv*-linux
> 
>  gdb/riscv-linux-tdep.c    | 24 ++++++++++++++++++++++++
>  gdb/riscv-tdep.c          | 10 ++++++++++
>  gdb/riscv-tdep.h          |  4 ++++
>  gdb/testsuite/lib/gdb.exp |  2 +-
>  4 files changed, 39 insertions(+), 1 deletion(-)
> 
> -- 
> 2.30.2
>