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

Message ID 20210707003043.447755-3-lsix@lancelotsix.com
State New
Headers show
Series
  • Fix gdb.base/sigstep.exp for riscv64-linux
Related show

Commit Message

Simon Marchi via Gdb-patches July 7, 2021, 12:30 a.m.
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(+)

-- 
2.30.2

Comments

Pedro Alves July 8, 2021, 10 a.m. | #1
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" ?
Simon Marchi via Gdb-patches July 8, 2021, 11:49 a.m. | #2
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.
Simon Marchi via Gdb-patches July 8, 2021, 12:11 p.m. | #3
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.
Pedro Alves July 8, 2021, 12:34 p.m. | #4
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.

Patch

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