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

Message ID d74de1fc1f7150666bee412ba9d48e5357c3316e.1535560591.git.andrew.burgess@embecosm.com
State New, archived
Headers

Commit Message

Andrew Burgess Aug. 29, 2018, 4:40 p.m. UTC
  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(-)
  

Comments

Jim Wilson Aug. 29, 2018, 10:05 p.m. UTC | #1
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
  
Palmer Dabbelt Aug. 29, 2018, 11:09 p.m. UTC | #2
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))
  

Patch

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