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

[Lancelot SIX <lsix@lancelotsix.com>]

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.