public inbox for binutils@sourceware.org
 help / color / mirror / Atom feed
* [RFC PATCH 0/3] RISC-V: Check shift amount against XLEN (disassembler)
@ 2022-05-23 10:06 Tsukasa OI
  2022-05-23 10:06 ` [RFC PATCH 1/3] RISC-V: Add xlen to match_func Tsukasa OI
                   ` (4 more replies)
  0 siblings, 5 replies; 7+ messages in thread
From: Tsukasa OI @ 2022-05-23 10:06 UTC (permalink / raw)
  To: Tsukasa OI, Palmer Dabbelt, Nelson Chu, Kito Cheng; +Cc: binutils

** Note **
This patch is not compatible with my previous Zfinx fixes.  Actual PATCH
v1 is going to be submitted after this patchset is reviewed and Zfinx
fixes (relatively high priority) are applied.

Certain shift instructions have a constraint: shift amount must be less
than current XLEN.  This constraint is checked on assembler and
simulator, but not on disassembler.  It causes GDB to print wrong (and
invalid) instructions and can be a problem while ... for instance,
checking for invalid build configuration.

We have several methods to deal with it:

-   Add `xlen' argument to match_func to test XLEN
-   Split shift instructions to per-XLEN variants

Because shift instructions are so basic and XLEN-checking is so simple,
I chose the former.  The latter is used on my Zfinx fixes but this is
because register pair checking involves relatively complex per-operand
constraints.  Complex solution for simple problem can be a new problem.

But...

-   Is it okay to add an argument anyway?
-   Which is better extra argument?
    1.  unsigned xlen
    2.  const riscv_parse_subset_t * subset
        which contains ISA version, XLEN and extensions
        ... but a part of BFD, not opcodes.

I also (slightly) changed the assembler to suppress extra error message
just like I did on Zicbop instructions:
<https://sourceware.org/pipermail/binutils/2021-December/118910.html>
<https://sourceware.org/pipermail/binutils/2021-December/118938.html>




Tsukasa OI (3):
  RISC-V: Add xlen to match_func
  RISC-V: Check shift amount against XLEN
  RISC-V: Add disassembler tests for shift amount

 gas/config/tc-riscv.c                  |   8 +-
 gas/testsuite/gas/riscv/shamt-dis-32.d |  34 +++++++
 gas/testsuite/gas/riscv/shamt-dis-64.d |  34 +++++++
 gas/testsuite/gas/riscv/shamt-dis.s    |  45 +++++++++
 include/opcode/riscv.h                 |   3 +-
 opcodes/riscv-dis.c                    |   2 +-
 opcodes/riscv-opc.c                    | 122 +++++++++++++++----------
 sim/riscv/sim-main.c                   |   2 +-
 8 files changed, 195 insertions(+), 55 deletions(-)
 create mode 100644 gas/testsuite/gas/riscv/shamt-dis-32.d
 create mode 100644 gas/testsuite/gas/riscv/shamt-dis-64.d
 create mode 100644 gas/testsuite/gas/riscv/shamt-dis.s


base-commit: cb0d58bf4d274cfb1ae11b75bd2b3ba81c8d371d
-- 
2.34.1


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [RFC PATCH 1/3] RISC-V: Add xlen to match_func
  2022-05-23 10:06 [RFC PATCH 0/3] RISC-V: Check shift amount against XLEN (disassembler) Tsukasa OI
@ 2022-05-23 10:06 ` Tsukasa OI
  2022-05-23 10:06 ` [RFC PATCH 2/3] RISC-V: Check shift amount against XLEN Tsukasa OI
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 7+ messages in thread
From: Tsukasa OI @ 2022-05-23 10:06 UTC (permalink / raw)
  To: Tsukasa OI, Palmer Dabbelt, Nelson Chu, Kito Cheng; +Cc: binutils

For shift amount checking against current XLEN, we need to add third
argument `xlen' to opcode-matching function.

gas/ChangeLog:

	* config/tc-riscv.c (riscv_ip): Call match_func with XLEN.

include/ChangeLog:

	* opcode/riscv.h (struct riscv_opcode): Modify match_func so
	that XLEN can be matched.

opcodes/ChangeLog:

	* riscv-dis.c (riscv_disassemble_insn): Call match_func
	with XLEN.
	* riscv-opc.c (match_opcode, match_never, match_rs1_eq_rs2,
	match_rd_nonzero, match_c_add, match_c_add_with_hint,
	match_c_nop, match_c_addi16sp, match_c_lui,
	match_c_lui_with_hint, match_c_addi4spn, match_c_slli,
	match_slli_as_c_slli, match_c_slli64, match_srxi_as_c_srxi,
	match_vs1_eq_vs2, match_vd_eq_vs1_eq_vs2): Add argument `xlen'.

ChangeLog:

	* sim/riscv/sim-main.c (step_once): Call match_func with XLEN.
---
 gas/config/tc-riscv.c  |  2 +-
 include/opcode/riscv.h |  3 +-
 opcodes/riscv-dis.c    |  2 +-
 opcodes/riscv-opc.c    | 76 ++++++++++++++++++++++++------------------
 sim/riscv/sim-main.c   |  2 +-
 5 files changed, 49 insertions(+), 36 deletions(-)

diff --git a/gas/config/tc-riscv.c b/gas/config/tc-riscv.c
index 1b730b4be36..b207b2d30c8 100644
--- a/gas/config/tc-riscv.c
+++ b/gas/config/tc-riscv.c
@@ -2309,7 +2309,7 @@ riscv_ip (char *str, struct riscv_cl_insn *ip, expressionS *imm_expr,
 	    case '\0': /* End of args.  */
 	      if (insn->pinfo != INSN_MACRO)
 		{
-		  if (!insn->match_func (insn, ip->insn_opcode))
+		  if (!insn->match_func (insn, ip->insn_opcode, xlen))
 		    break;
 
 		  /* For .insn, insn->match and insn->mask are 0.  */
diff --git a/include/opcode/riscv.h b/include/opcode/riscv.h
index 0d1fbcf8fc5..6ee2640cac1 100644
--- a/include/opcode/riscv.h
+++ b/include/opcode/riscv.h
@@ -426,7 +426,8 @@ struct riscv_opcode
 
   /* A function to determine if a word corresponds to this instruction.
      Usually, this computes ((word & mask) == match).  */
-  int (*match_func) (const struct riscv_opcode *op, insn_t word);
+  int (*match_func) (const struct riscv_opcode *op, insn_t word,
+		     unsigned xlen);
 
   /* For a macro, this is INSN_MACRO.  Otherwise, it is a collection
      of bits describing the instruction, notably any relevant hazard
diff --git a/opcodes/riscv-dis.c b/opcodes/riscv-dis.c
index 9ff31167775..4738c6a98c7 100644
--- a/opcodes/riscv-dis.c
+++ b/opcodes/riscv-dis.c
@@ -644,7 +644,7 @@ riscv_disassemble_insn (bfd_vma memaddr, insn_t word, disassemble_info *info)
       for (; op->name; op++)
 	{
 	  /* Does the opcode match?  */
-	  if (! (op->match_func) (op, word))
+	  if (! (op->match_func) (op, word, xlen))
 	    continue;
 	  /* Is this a pseudo-instruction and may we print it as such?  */
 	  if (no_aliases && (op->pinfo & INSN_ALIAS))
diff --git a/opcodes/riscv-opc.c b/opcodes/riscv-opc.c
index bbd4a3718f6..636e67a3eb0 100644
--- a/opcodes/riscv-opc.c
+++ b/opcodes/riscv-opc.c
@@ -132,65 +132,71 @@ const char * const riscv_vma[2] =
 #define MASK_VMASK (OP_MASK_VMASK << OP_SH_VMASK)
 
 static int
-match_opcode (const struct riscv_opcode *op, insn_t insn)
+match_opcode (const struct riscv_opcode *op, insn_t insn,
+	      unsigned xlen ATTRIBUTE_UNUSED)
 {
   return ((insn ^ op->match) & op->mask) == 0;
 }
 
 static int
 match_never (const struct riscv_opcode *op ATTRIBUTE_UNUSED,
-	     insn_t insn ATTRIBUTE_UNUSED)
+	     insn_t insn ATTRIBUTE_UNUSED,
+	     unsigned xlen ATTRIBUTE_UNUSED)
 {
   return 0;
 }
 
 static int
-match_rs1_eq_rs2 (const struct riscv_opcode *op, insn_t insn)
+match_rs1_eq_rs2 (const struct riscv_opcode *op, insn_t insn,
+		  unsigned xlen)
 {
   int rs1 = (insn & MASK_RS1) >> OP_SH_RS1;
   int rs2 = (insn & MASK_RS2) >> OP_SH_RS2;
-  return match_opcode (op, insn) && rs1 == rs2;
+  return match_opcode (op, insn, xlen) && rs1 == rs2;
 }
 
 static int
-match_rd_nonzero (const struct riscv_opcode *op, insn_t insn)
+match_rd_nonzero (const struct riscv_opcode *op, insn_t insn,
+		  unsigned xlen)
 {
-  return match_opcode (op, insn) && ((insn & MASK_RD) != 0);
+  return match_opcode (op, insn, xlen) && ((insn & MASK_RD) != 0);
 }
 
 static int
-match_c_add (const struct riscv_opcode *op, insn_t insn)
+match_c_add (const struct riscv_opcode *op, insn_t insn, unsigned xlen)
 {
-  return match_rd_nonzero (op, insn) && ((insn & MASK_CRS2) != 0);
+  return match_rd_nonzero (op, insn, xlen) && ((insn & MASK_CRS2) != 0);
 }
 
 /* We don't allow mv zero,X to become a c.mv hint, so we need a separate
    matching function for this.  */
 
 static int
-match_c_add_with_hint (const struct riscv_opcode *op, insn_t insn)
+match_c_add_with_hint (const struct riscv_opcode *op, insn_t insn,
+		       unsigned xlen)
 {
-  return match_opcode (op, insn) && ((insn & MASK_CRS2) != 0);
+  return match_opcode (op, insn, xlen) && ((insn & MASK_CRS2) != 0);
 }
 
 static int
-match_c_nop (const struct riscv_opcode *op, insn_t insn)
+match_c_nop (const struct riscv_opcode *op, insn_t insn, unsigned xlen)
 {
-  return (match_opcode (op, insn)
+  return (match_opcode (op, insn, xlen)
 	  && (((insn & MASK_RD) >> OP_SH_RD) == 0));
 }
 
 static int
-match_c_addi16sp (const struct riscv_opcode *op, insn_t insn)
+match_c_addi16sp (const struct riscv_opcode *op, insn_t insn,
+		  unsigned xlen)
 {
-  return (match_opcode (op, insn)
+  return (match_opcode (op, insn, xlen)
 	  && (((insn & MASK_RD) >> OP_SH_RD) == 2));
 }
 
 static int
-match_c_lui (const struct riscv_opcode *op, insn_t insn)
+match_c_lui (const struct riscv_opcode *op, insn_t insn, unsigned xlen)
 {
-  return (match_rd_nonzero (op, insn)
+  return (match_rd_nonzero (op, insn, xlen)
 	  && (((insn & MASK_RD) >> OP_SH_RD) != 2)
 	  && EXTRACT_CITYPE_LUI_IMM (insn) != 0);
 }
@@ -199,71 +205,77 @@ match_c_lui (const struct riscv_opcode *op, insn_t insn)
    matching function for this.  */
 
 static int
-match_c_lui_with_hint (const struct riscv_opcode *op, insn_t insn)
+match_c_lui_with_hint (const struct riscv_opcode *op, insn_t insn, 
+		       unsigned xlen)
 {
-  return (match_opcode (op, insn)
+  return (match_opcode (op, insn, xlen)
 	  && (((insn & MASK_RD) >> OP_SH_RD) != 2)
 	  && EXTRACT_CITYPE_LUI_IMM (insn) != 0);
 }
 
 static int
-match_c_addi4spn (const struct riscv_opcode *op, insn_t insn)
+match_c_addi4spn (const struct riscv_opcode *op, insn_t insn,
+		  unsigned xlen)
 {
-  return match_opcode (op, insn) && EXTRACT_CIWTYPE_ADDI4SPN_IMM (insn) != 0;
+  return match_opcode (op, insn, xlen)
+	 && EXTRACT_CIWTYPE_ADDI4SPN_IMM (insn) != 0;
 }
 
 /* This requires a non-zero shift.  A zero rd is a hint, so is allowed.  */
 
 static int
-match_c_slli (const struct riscv_opcode *op, insn_t insn)
+match_c_slli (const struct riscv_opcode *op, insn_t insn, unsigned xlen)
 {
-  return match_opcode (op, insn) && EXTRACT_CITYPE_IMM (insn) != 0;
+  return match_opcode (op, insn, xlen) && EXTRACT_CITYPE_IMM (insn) != 0;
 }
 
 /* This requires a non-zero rd, and a non-zero shift.  */
 
 static int
-match_slli_as_c_slli (const struct riscv_opcode *op, insn_t insn)
+match_slli_as_c_slli (const struct riscv_opcode *op, insn_t insn,
+		      unsigned xlen)
 {
-  return match_rd_nonzero (op, insn) && EXTRACT_CITYPE_IMM (insn) != 0;
+  return match_rd_nonzero (op, insn, xlen)
+	 && EXTRACT_CITYPE_IMM (insn) != 0;
 }
 
 /* This requires a zero shift.  A zero rd is a hint, so is allowed.  */
 
 static int
-match_c_slli64 (const struct riscv_opcode *op, insn_t insn)
+match_c_slli64 (const struct riscv_opcode *op, insn_t insn, unsigned xlen)
 {
-  return match_opcode (op, insn) && EXTRACT_CITYPE_IMM (insn) == 0;
+  return match_opcode (op, insn, xlen) && EXTRACT_CITYPE_IMM (insn) == 0;
 }
 
 /* This is used for both srli and srai.  This requires a non-zero shift.
    A zero rd is not possible.  */
 
 static int
-match_srxi_as_c_srxi (const struct riscv_opcode *op, insn_t insn)
+match_srxi_as_c_srxi (const struct riscv_opcode *op, insn_t insn,
+		      unsigned xlen)
 {
-  return match_opcode (op, insn) && EXTRACT_CITYPE_IMM (insn) != 0;
+  return match_opcode (op, insn, xlen) && EXTRACT_CITYPE_IMM (insn) != 0;
 }
 
 static int
 match_vs1_eq_vs2 (const struct riscv_opcode *op,
-		  insn_t insn)
+		  insn_t insn, unsigned xlen)
 {
   int vs1 = (insn & MASK_VS1) >> OP_SH_VS1;
   int vs2 = (insn & MASK_VS2) >> OP_SH_VS2;
 
-  return match_opcode (op, insn) && vs1 == vs2;
+  return match_opcode (op, insn, xlen) && vs1 == vs2;
 }
 
 static int
 match_vd_eq_vs1_eq_vs2 (const struct riscv_opcode *op,
-			insn_t insn)
+			insn_t insn, unsigned xlen)
 {
   int vd =  (insn & MASK_VD) >> OP_SH_VD;
   int vs1 = (insn & MASK_VS1) >> OP_SH_VS1;
   int vs2 = (insn & MASK_VS2) >> OP_SH_VS2;
 
-  return match_opcode (op, insn) && vd == vs1 && vs1 == vs2;
+  return match_opcode (op, insn, xlen) && vd == vs1 && vs1 == vs2;
 }
 
 const struct riscv_opcode riscv_opcodes[] =
diff --git a/sim/riscv/sim-main.c b/sim/riscv/sim-main.c
index 62f475671c9..0b0a021a1c6 100644
--- a/sim/riscv/sim-main.c
+++ b/sim/riscv/sim-main.c
@@ -983,7 +983,7 @@ void step_once (SIM_CPU *cpu)
   for (; op->name; op++)
     {
       /* Does the opcode match?  */
-      if (! op->match_func (op, iw))
+      if (! op->match_func (op, iw, xlen))
 	continue;
       /* Is this a pseudo-instruction and may we print it as such?  */
       if (op->pinfo & INSN_ALIAS)
-- 
2.34.1


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [RFC PATCH 2/3] RISC-V: Check shift amount against XLEN
  2022-05-23 10:06 [RFC PATCH 0/3] RISC-V: Check shift amount against XLEN (disassembler) Tsukasa OI
  2022-05-23 10:06 ` [RFC PATCH 1/3] RISC-V: Add xlen to match_func Tsukasa OI
@ 2022-05-23 10:06 ` Tsukasa OI
  2022-05-23 10:06 ` [RFC PATCH 3/3] RISC-V: Add disassembler tests for shift amount Tsukasa OI
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 7+ messages in thread
From: Tsukasa OI @ 2022-05-23 10:06 UTC (permalink / raw)
  To: Tsukasa OI, Palmer Dabbelt, Nelson Chu, Kito Cheng; +Cc: binutils

Although assembler does the equivalent, disassembler did not have shift
amount checking.  This commit makes such invalid RV32 instruction
invalid by checking shift amount against current XLEN.

gas/ChangeLog:

	* config/tc-riscv.c (riscv_ip): Mask shift amount to suppress
	extra assembler error messages.

opcodes/ChangeLog:

	* riscv-opc.c (match_shamt): New.
	(match_c_slli, match_slli_as_c_slli, match_srxi_as_c_srxi):
	Check shift amount against current xlen.
---
 gas/config/tc-riscv.c |  6 +++--
 opcodes/riscv-opc.c   | 52 ++++++++++++++++++++++++++-----------------
 2 files changed, 36 insertions(+), 22 deletions(-)

diff --git a/gas/config/tc-riscv.c b/gas/config/tc-riscv.c
index b207b2d30c8..66907bdf0f2 100644
--- a/gas/config/tc-riscv.c
+++ b/gas/config/tc-riscv.c
@@ -2403,7 +2403,8 @@ riscv_ip (char *str, struct riscv_cl_insn *ip, expressionS *imm_expr,
 		      || imm_expr->X_op != O_constant
 		      || (unsigned long) imm_expr->X_add_number >= xlen)
 		    break;
-		  ip->insn_opcode |= ENCODE_CITYPE_IMM (imm_expr->X_add_number);
+		  ip->insn_opcode |=
+		    ENCODE_CITYPE_IMM (imm_expr->X_add_number & (xlen - 1));
 		rvc_imm_done:
 		  asarg = expr_end;
 		  imm_expr->X_op = O_absent;
@@ -2843,7 +2844,8 @@ riscv_ip (char *str, struct riscv_cl_insn *ip, expressionS *imm_expr,
 	      if ((unsigned long) imm_expr->X_add_number >= xlen)
 		as_bad (_("improper shift amount (%lu)"),
 			(unsigned long) imm_expr->X_add_number);
-	      INSERT_OPERAND (SHAMT, *ip, imm_expr->X_add_number);
+	      INSERT_OPERAND (SHAMT, *ip,
+			      imm_expr->X_add_number & (xlen - 1));
 	      imm_expr->X_op = O_absent;
 	      asarg = expr_end;
 	      continue;
diff --git a/opcodes/riscv-opc.c b/opcodes/riscv-opc.c
index 636e67a3eb0..b71a9f4711a 100644
--- a/opcodes/riscv-opc.c
+++ b/opcodes/riscv-opc.c
@@ -162,6 +162,13 @@ match_rd_nonzero (const struct riscv_opcode *op, insn_t insn,
   return match_opcode (op, insn, xlen) && ((insn & MASK_RD) != 0);
 }
 
+static int
+match_shamt (const struct riscv_opcode *op, insn_t insn, unsigned xlen)
+{
+  unsigned shamt = (insn & MASK_SHAMT) >> OP_SH_SHAMT;
+  return match_opcode (op, insn, xlen) && shamt < xlen;
+}
+
 static int
 match_c_add (const struct riscv_opcode *op, insn_t insn, unsigned xlen)
 {
@@ -226,7 +233,9 @@ match_c_addi4spn (const struct riscv_opcode *op, insn_t insn,
 static int
 match_c_slli (const struct riscv_opcode *op, insn_t insn, unsigned xlen)
 {
-  return match_opcode (op, insn, xlen) && EXTRACT_CITYPE_IMM (insn) != 0;
+  return match_opcode (op, insn, xlen)
+	 && EXTRACT_CITYPE_IMM (insn) != 0
+	 && (EXTRACT_CITYPE_IMM (insn) & (RISCV_RVC_IMM_REACH - 1)) < xlen;
 }
 
 /* This requires a non-zero rd, and a non-zero shift.  */
@@ -236,7 +245,8 @@ match_slli_as_c_slli (const struct riscv_opcode *op, insn_t insn,
 		      unsigned xlen)
 {
   return match_rd_nonzero (op, insn, xlen)
-	 && EXTRACT_CITYPE_IMM (insn) != 0;
+	 && EXTRACT_CITYPE_IMM (insn) != 0
+	 && (EXTRACT_CITYPE_IMM (insn) & (RISCV_RVC_IMM_REACH - 1)) < xlen;
 }
 
 /* This requires a zero shift.  A zero rd is a hint, so is allowed.  */
@@ -254,7 +264,9 @@ static int
 match_srxi_as_c_srxi (const struct riscv_opcode *op, insn_t insn,
 		      unsigned xlen)
 {
-  return match_opcode (op, insn, xlen) && EXTRACT_CITYPE_IMM (insn) != 0;
+  return match_opcode (op, insn, xlen)
+	 && EXTRACT_CITYPE_IMM (insn) != 0
+	 && (EXTRACT_CITYPE_IMM (insn) & (RISCV_RVC_IMM_REACH - 1)) < xlen;
 }
 
 static int
@@ -371,20 +383,20 @@ const struct riscv_opcode riscv_opcodes[] =
 {"la.tls.ie",   0, INSN_CLASS_I, "d,A",       0, (int) M_LA_TLS_IE, match_never, INSN_MACRO },
 {"neg",         0, INSN_CLASS_I, "d,t",       MATCH_SUB, MASK_SUB|MASK_RS1, match_opcode, INSN_ALIAS }, /* sub 0  */
 {"slli",        0, INSN_CLASS_C, "d,CU,C>",   MATCH_C_SLLI, MASK_C_SLLI, match_slli_as_c_slli, INSN_ALIAS },
-{"slli",        0, INSN_CLASS_I, "d,s,>",     MATCH_SLLI, MASK_SLLI, match_opcode, 0 },
+{"slli",        0, INSN_CLASS_I, "d,s,>",     MATCH_SLLI, MASK_SLLI, match_shamt, 0 },
 {"sll",         0, INSN_CLASS_C, "d,CU,C>",   MATCH_C_SLLI, MASK_C_SLLI, match_slli_as_c_slli, INSN_ALIAS },
 {"sll",         0, INSN_CLASS_I, "d,s,t",     MATCH_SLL, MASK_SLL, match_opcode, 0 },
-{"sll",         0, INSN_CLASS_I, "d,s,>",     MATCH_SLLI, MASK_SLLI, match_opcode, INSN_ALIAS },
+{"sll",         0, INSN_CLASS_I, "d,s,>",     MATCH_SLLI, MASK_SLLI, match_shamt, INSN_ALIAS },
 {"srli",        0, INSN_CLASS_C, "Cs,Cw,C>",  MATCH_C_SRLI, MASK_C_SRLI, match_srxi_as_c_srxi, INSN_ALIAS },
-{"srli",        0, INSN_CLASS_I, "d,s,>",     MATCH_SRLI, MASK_SRLI, match_opcode, 0 },
+{"srli",        0, INSN_CLASS_I, "d,s,>",     MATCH_SRLI, MASK_SRLI, match_shamt, 0 },
 {"srl",         0, INSN_CLASS_C, "Cs,Cw,C>",  MATCH_C_SRLI, MASK_C_SRLI, match_srxi_as_c_srxi, INSN_ALIAS },
 {"srl",         0, INSN_CLASS_I, "d,s,t",     MATCH_SRL, MASK_SRL, match_opcode, 0 },
-{"srl",         0, INSN_CLASS_I, "d,s,>",     MATCH_SRLI, MASK_SRLI, match_opcode, INSN_ALIAS },
+{"srl",         0, INSN_CLASS_I, "d,s,>",     MATCH_SRLI, MASK_SRLI, match_shamt, INSN_ALIAS },
 {"srai",        0, INSN_CLASS_C, "Cs,Cw,C>",  MATCH_C_SRAI, MASK_C_SRAI, match_srxi_as_c_srxi, INSN_ALIAS },
-{"srai",        0, INSN_CLASS_I, "d,s,>",     MATCH_SRAI, MASK_SRAI, match_opcode, 0 },
+{"srai",        0, INSN_CLASS_I, "d,s,>",     MATCH_SRAI, MASK_SRAI, match_shamt, 0 },
 {"sra",         0, INSN_CLASS_C, "Cs,Cw,C>",  MATCH_C_SRAI, MASK_C_SRAI, match_srxi_as_c_srxi, INSN_ALIAS },
 {"sra",         0, INSN_CLASS_I, "d,s,t",     MATCH_SRA, MASK_SRA, match_opcode, 0 },
-{"sra",         0, INSN_CLASS_I, "d,s,>",     MATCH_SRAI, MASK_SRAI, match_opcode, INSN_ALIAS },
+{"sra",         0, INSN_CLASS_I, "d,s,>",     MATCH_SRAI, MASK_SRAI, match_shamt, INSN_ALIAS },
 {"sub",         0, INSN_CLASS_C, "Cs,Cw,Ct",  MATCH_C_SUB, MASK_C_SUB, match_opcode, INSN_ALIAS },
 {"sub",         0, INSN_CLASS_I, "d,s,t",     MATCH_SUB, MASK_SUB, match_opcode, 0 },
 {"lb",          0, INSN_CLASS_I, "d,o(s)",    MATCH_LB, MASK_LB, match_opcode, INSN_DREF|INSN_1_BYTE },
@@ -963,9 +975,9 @@ const struct riscv_opcode riscv_opcodes[] =
 {"orn",        0, INSN_CLASS_ZBB_OR_ZBKB,  "d,s,t", MATCH_ORN, MASK_ORN, match_opcode, 0 },
 {"xnor",       0, INSN_CLASS_ZBB_OR_ZBKB,  "d,s,t", MATCH_XNOR, MASK_XNOR, match_opcode, 0 },
 {"rol",        0, INSN_CLASS_ZBB_OR_ZBKB,  "d,s,t", MATCH_ROL, MASK_ROL, match_opcode, 0 },
-{"rori",       0, INSN_CLASS_ZBB_OR_ZBKB,  "d,s,>", MATCH_RORI, MASK_RORI, match_opcode, 0 },
+{"rori",       0, INSN_CLASS_ZBB_OR_ZBKB,  "d,s,>", MATCH_RORI, MASK_RORI, match_shamt, 0 },
 {"ror",        0, INSN_CLASS_ZBB_OR_ZBKB,  "d,s,t", MATCH_ROR, MASK_ROR, match_opcode, 0 },
-{"ror",        0, INSN_CLASS_ZBB_OR_ZBKB,  "d,s,>", MATCH_RORI, MASK_RORI, match_opcode, INSN_ALIAS },
+{"ror",        0, INSN_CLASS_ZBB_OR_ZBKB,  "d,s,>", MATCH_RORI, MASK_RORI, match_shamt, INSN_ALIAS },
 {"rev8",      32, INSN_CLASS_ZBB_OR_ZBKB,  "d,s",   MATCH_GREVI | MATCH_SHAMT_REV8_32, MASK_GREVI | MASK_SHAMT, match_opcode, 0 },
 {"rev8",      64, INSN_CLASS_ZBB_OR_ZBKB,  "d,s",   MATCH_GREVI | MATCH_SHAMT_REV8_64, MASK_GREVI | MASK_SHAMT, match_opcode, 0 },
 {"rolw",      64, INSN_CLASS_ZBB_OR_ZBKB,  "d,s,t", MATCH_ROLW, MASK_ROLW, match_opcode, 0 },
@@ -983,7 +995,7 @@ const struct riscv_opcode riscv_opcodes[] =
 {"zext.w",    64, INSN_CLASS_ZBA,  "d,s",   MATCH_ADD_UW, MASK_ADD_UW | MASK_RS2, match_opcode, INSN_ALIAS },
 {"zext.w",    64, INSN_CLASS_I, "d,s",       0, (int) M_ZEXTW, match_never, INSN_MACRO },
 {"add.uw",    64, INSN_CLASS_ZBA,  "d,s,t", MATCH_ADD_UW, MASK_ADD_UW, match_opcode, 0 },
-{"slli.uw",   64, INSN_CLASS_ZBA,  "d,s,>", MATCH_SLLI_UW, MASK_SLLI_UW, match_opcode, 0 },
+{"slli.uw",   64, INSN_CLASS_ZBA,  "d,s,>", MATCH_SLLI_UW, MASK_SLLI_UW, match_shamt, 0 },
 
 /* Zbc or zbkc instructions.  */
 {"clmul",      0, INSN_CLASS_ZBC_OR_ZBKC,  "d,s,t", MATCH_CLMUL, MASK_CLMUL, match_opcode, 0 },
@@ -991,18 +1003,18 @@ const struct riscv_opcode riscv_opcodes[] =
 {"clmulr",     0, INSN_CLASS_ZBC,  "d,s,t", MATCH_CLMULR, MASK_CLMULR, match_opcode, 0 },
 
 /* Zbs instructions.  */
-{"bclri",     0, INSN_CLASS_ZBS,   "d,s,>",  MATCH_BCLRI, MASK_BCLRI, match_opcode, 0 },
-{"bseti",     0, INSN_CLASS_ZBS,   "d,s,>",  MATCH_BSETI, MASK_BSETI, match_opcode, 0 },
-{"binvi",     0, INSN_CLASS_ZBS,   "d,s,>",  MATCH_BINVI, MASK_BINVI, match_opcode, 0 },
-{"bexti",     0, INSN_CLASS_ZBS,   "d,s,>",  MATCH_BEXTI, MASK_BEXTI, match_opcode, 0 },
+{"bclri",     0, INSN_CLASS_ZBS,   "d,s,>",  MATCH_BCLRI, MASK_BCLRI, match_shamt, 0 },
+{"bseti",     0, INSN_CLASS_ZBS,   "d,s,>",  MATCH_BSETI, MASK_BSETI, match_shamt, 0 },
+{"binvi",     0, INSN_CLASS_ZBS,   "d,s,>",  MATCH_BINVI, MASK_BINVI, match_shamt, 0 },
+{"bexti",     0, INSN_CLASS_ZBS,   "d,s,>",  MATCH_BEXTI, MASK_BEXTI, match_shamt, 0 },
 {"bclr",      0, INSN_CLASS_ZBS,   "d,s,t",  MATCH_BCLR, MASK_BCLR, match_opcode, 0 },
-{"bclr",      0, INSN_CLASS_ZBS,   "d,s,>",  MATCH_BCLRI, MASK_BCLRI, match_opcode, INSN_ALIAS },
+{"bclr",      0, INSN_CLASS_ZBS,   "d,s,>",  MATCH_BCLRI, MASK_BCLRI, match_shamt, INSN_ALIAS },
 {"bset",      0, INSN_CLASS_ZBS,   "d,s,t",  MATCH_BSET, MASK_BSET, match_opcode, 0 },
-{"bset",      0, INSN_CLASS_ZBS,   "d,s,>",  MATCH_BSETI, MASK_BSETI, match_opcode, INSN_ALIAS },
+{"bset",      0, INSN_CLASS_ZBS,   "d,s,>",  MATCH_BSETI, MASK_BSETI, match_shamt, INSN_ALIAS },
 {"binv",      0, INSN_CLASS_ZBS,   "d,s,t",  MATCH_BINV, MASK_BINV, match_opcode, 0 },
-{"binv",      0, INSN_CLASS_ZBS,   "d,s,>",  MATCH_BINVI, MASK_BINVI, match_opcode, INSN_ALIAS },
+{"binv",      0, INSN_CLASS_ZBS,   "d,s,>",  MATCH_BINVI, MASK_BINVI, match_shamt, INSN_ALIAS },
 {"bext",      0, INSN_CLASS_ZBS,   "d,s,t",  MATCH_BEXT, MASK_BEXT, match_opcode, 0 },
-{"bext",      0, INSN_CLASS_ZBS,   "d,s,>",  MATCH_BEXTI, MASK_BEXTI, match_opcode, INSN_ALIAS },
+{"bext",      0, INSN_CLASS_ZBS,   "d,s,>",  MATCH_BEXTI, MASK_BEXTI, match_shamt, INSN_ALIAS },
 
 /* Zbkx instructions.  */
 {"xperm4",     0, INSN_CLASS_ZBKX,  "d,s,t",  MATCH_XPERM4, MASK_XPERM4, match_opcode, 0 },
-- 
2.34.1


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [RFC PATCH 3/3] RISC-V: Add disassembler tests for shift amount
  2022-05-23 10:06 [RFC PATCH 0/3] RISC-V: Check shift amount against XLEN (disassembler) Tsukasa OI
  2022-05-23 10:06 ` [RFC PATCH 1/3] RISC-V: Add xlen to match_func Tsukasa OI
  2022-05-23 10:06 ` [RFC PATCH 2/3] RISC-V: Check shift amount against XLEN Tsukasa OI
@ 2022-05-23 10:06 ` Tsukasa OI
  2022-07-30  3:47 ` [RFC PATCH 0/3] RISC-V: Check shift amount against XLEN (disassembler) Tsukasa OI
  2022-07-30  3:51 ` [PATCH 0/1] " Tsukasa OI
  4 siblings, 0 replies; 7+ messages in thread
From: Tsukasa OI @ 2022-05-23 10:06 UTC (permalink / raw)
  To: Tsukasa OI, Palmer Dabbelt, Nelson Chu, Kito Cheng; +Cc: binutils

This commit adds tests for shift instruction to make sure that the
disassembler detects invalid shift amount.

gas/ChangeLog:

	* testsuite/gas/riscv/shamt-dis.s: New disassembler test.
	* testsuite/gas/riscv/shamt-dis-32.d: Likewise.
	* testsuite/gas/riscv/shamt-dis-64.d: Likewise.
---
 gas/testsuite/gas/riscv/shamt-dis-32.d | 34 +++++++++++++++++++
 gas/testsuite/gas/riscv/shamt-dis-64.d | 34 +++++++++++++++++++
 gas/testsuite/gas/riscv/shamt-dis.s    | 45 ++++++++++++++++++++++++++
 3 files changed, 113 insertions(+)
 create mode 100644 gas/testsuite/gas/riscv/shamt-dis-32.d
 create mode 100644 gas/testsuite/gas/riscv/shamt-dis-64.d
 create mode 100644 gas/testsuite/gas/riscv/shamt-dis.s

diff --git a/gas/testsuite/gas/riscv/shamt-dis-32.d b/gas/testsuite/gas/riscv/shamt-dis-32.d
new file mode 100644
index 00000000000..b160353f297
--- /dev/null
+++ b/gas/testsuite/gas/riscv/shamt-dis-32.d
@@ -0,0 +1,34 @@
+#as: -march=rv32ic_zba_zbb_zbs
+#source: shamt-dis.s
+#objdump: -d -M no-aliases
+
+.*:[ 	]+file format .*
+
+
+Disassembly of section .text:
+
+0+000 <target>:
+[ 	]+[0-9a-f]+:[ 	]+01f59513[ 	]+slli[ 	]+a0,a1,0x1f
+[ 	]+[0-9a-f]+:[ 	]+02059513[ 	]+\.4byte[ 	]+0x2059513
+[ 	]+[0-9a-f]+:[ 	]+01f5d513[ 	]+srli[ 	]+a0,a1,0x1f
+[ 	]+[0-9a-f]+:[ 	]+0205d513[ 	]+\.4byte[ 	]+0x205d513
+[ 	]+[0-9a-f]+:[ 	]+41f5d513[ 	]+srai[ 	]+a0,a1,0x1f
+[ 	]+[0-9a-f]+:[ 	]+4205d513[ 	]+\.4byte[ 	]+0x4205d513
+[ 	]+[0-9a-f]+:[ 	]+057e[ 	]+c\.slli[ 	]+a0,0x1f
+[ 	]+[0-9a-f]+:[ 	]+1502[ 	]+\.2byte[ 	]+0x1502
+[ 	]+[0-9a-f]+:[ 	]+817d[ 	]+c\.srli[ 	]+a0,0x1f
+[ 	]+[0-9a-f]+:[ 	]+9101[ 	]+\.2byte[ 	]+0x9101
+[ 	]+[0-9a-f]+:[ 	]+857d[ 	]+c\.srai[ 	]+a0,0x1f
+[ 	]+[0-9a-f]+:[ 	]+9501[ 	]+\.2byte[ 	]+0x9501
+[ 	]+[0-9a-f]+:[ 	]+61f5d513[ 	]+rori[ 	]+a0,a1,0x1f
+[ 	]+[0-9a-f]+:[ 	]+6205d513[ 	]+\.4byte[ 	]+0x6205d513
+[ 	]+[0-9a-f]+:[ 	]+09f5951b[ 	]+\.4byte[ 	]+0x9f5951b
+[ 	]+[0-9a-f]+:[ 	]+0a05951b[ 	]+\.4byte[ 	]+0xa05951b
+[ 	]+[0-9a-f]+:[ 	]+49f59513[ 	]+bclri[ 	]+a0,a1,0x1f
+[ 	]+[0-9a-f]+:[ 	]+4a059513[ 	]+\.4byte[ 	]+0x4a059513
+[ 	]+[0-9a-f]+:[ 	]+29f59513[ 	]+bseti[ 	]+a0,a1,0x1f
+[ 	]+[0-9a-f]+:[ 	]+2a059513[ 	]+\.4byte[ 	]+0x2a059513
+[ 	]+[0-9a-f]+:[ 	]+69f59513[ 	]+binvi[ 	]+a0,a1,0x1f
+[ 	]+[0-9a-f]+:[ 	]+6a059513[ 	]+\.4byte[ 	]+0x6a059513
+[ 	]+[0-9a-f]+:[ 	]+49f5d513[ 	]+bexti[ 	]+a0,a1,0x1f
+[ 	]+[0-9a-f]+:[ 	]+4a05d513[ 	]+\.4byte[ 	]+0x4a05d513
diff --git a/gas/testsuite/gas/riscv/shamt-dis-64.d b/gas/testsuite/gas/riscv/shamt-dis-64.d
new file mode 100644
index 00000000000..d10e90d9d30
--- /dev/null
+++ b/gas/testsuite/gas/riscv/shamt-dis-64.d
@@ -0,0 +1,34 @@
+#as: -march=rv64ic_zba_zbb_zbs
+#source: shamt-dis.s
+#objdump: -d -M no-aliases
+
+.*:[ 	]+file format .*
+
+
+Disassembly of section .text:
+
+0+000 <target>:
+[ 	]+[0-9a-f]+:[ 	]+01f59513[ 	]+slli[ 	]+a0,a1,0x1f
+[ 	]+[0-9a-f]+:[ 	]+02059513[ 	]+slli[ 	]+a0,a1,0x20
+[ 	]+[0-9a-f]+:[ 	]+01f5d513[ 	]+srli[ 	]+a0,a1,0x1f
+[ 	]+[0-9a-f]+:[ 	]+0205d513[ 	]+srli[ 	]+a0,a1,0x20
+[ 	]+[0-9a-f]+:[ 	]+41f5d513[ 	]+srai[ 	]+a0,a1,0x1f
+[ 	]+[0-9a-f]+:[ 	]+4205d513[ 	]+srai[ 	]+a0,a1,0x20
+[ 	]+[0-9a-f]+:[ 	]+057e[ 	]+c\.slli[ 	]+a0,0x1f
+[ 	]+[0-9a-f]+:[ 	]+1502[ 	]+c\.slli[ 	]+a0,0x20
+[ 	]+[0-9a-f]+:[ 	]+817d[ 	]+c\.srli[ 	]+a0,0x1f
+[ 	]+[0-9a-f]+:[ 	]+9101[ 	]+c\.srli[ 	]+a0,0x20
+[ 	]+[0-9a-f]+:[ 	]+857d[ 	]+c\.srai[ 	]+a0,0x1f
+[ 	]+[0-9a-f]+:[ 	]+9501[ 	]+c\.srai[ 	]+a0,0x20
+[ 	]+[0-9a-f]+:[ 	]+61f5d513[ 	]+rori[ 	]+a0,a1,0x1f
+[ 	]+[0-9a-f]+:[ 	]+6205d513[ 	]+rori[ 	]+a0,a1,0x20
+[ 	]+[0-9a-f]+:[ 	]+09f5951b[ 	]+slli\.uw[ 	]+a0,a1,0x1f
+[ 	]+[0-9a-f]+:[ 	]+0a05951b[ 	]+slli\.uw[ 	]+a0,a1,0x20
+[ 	]+[0-9a-f]+:[ 	]+49f59513[ 	]+bclri[ 	]+a0,a1,0x1f
+[ 	]+[0-9a-f]+:[ 	]+4a059513[ 	]+bclri[ 	]+a0,a1,0x20
+[ 	]+[0-9a-f]+:[ 	]+29f59513[ 	]+bseti[ 	]+a0,a1,0x1f
+[ 	]+[0-9a-f]+:[ 	]+2a059513[ 	]+bseti[ 	]+a0,a1,0x20
+[ 	]+[0-9a-f]+:[ 	]+69f59513[ 	]+binvi[ 	]+a0,a1,0x1f
+[ 	]+[0-9a-f]+:[ 	]+6a059513[ 	]+binvi[ 	]+a0,a1,0x20
+[ 	]+[0-9a-f]+:[ 	]+49f5d513[ 	]+bexti[ 	]+a0,a1,0x1f
+[ 	]+[0-9a-f]+:[ 	]+4a05d513[ 	]+bexti[ 	]+a0,a1,0x20
diff --git a/gas/testsuite/gas/riscv/shamt-dis.s b/gas/testsuite/gas/riscv/shamt-dis.s
new file mode 100644
index 00000000000..edade302562
--- /dev/null
+++ b/gas/testsuite/gas/riscv/shamt-dis.s
@@ -0,0 +1,45 @@
+target:
+	# slli a0,a1,SHAMT [31/32]
+	.insn 0x01f59513
+	.insn 0x02059513
+	# srli a0,a1,SHAMT [31/32]
+	.insn 0x01f5d513
+	.insn 0x0205d513
+	# srai a0,a1,SHAMT [31/32]
+	.insn 0x41f5d513
+	.insn 0x4205d513
+
+	# RVC
+	# c.slli a0,SHAMT [31/32]
+	.insn 0x057e
+	.insn 0x1502
+	# c.srli a0,SHAMT [31/32]
+	.insn 0x817d
+	.insn 0x9101
+	# c.srai a0,SHAMT [31/32]
+	.insn 0x857d
+	.insn 0x9501
+
+	# Zbb extension (or Zbkb)
+	# rori a0,a1,SHAMT [31/32]
+	.insn 0x61f5d513
+	.insn 0x6205d513
+
+	# Zba extension
+	# slli.uw a0,a1,SHAMT [31/32] (invalid on RV32)
+	.insn 0x09f5951b
+	.insn 0x0a05951b
+
+	# Zbs extension
+	# bclri a0,a1,SHAMT [31/32]
+	.insn 0x49f59513
+	.insn 0x4a059513
+	# bseti a0,a1,SHAMT [31/32]
+	.insn 0x29f59513
+	.insn 0x2a059513
+	# binvi a0,a1,SHAMT [31/32]
+	.insn 0x69f59513
+	.insn 0x6a059513
+	# bexti a0,a1,SHAMT [31/32]
+	.insn 0x49f5d513
+	.insn 0x4a05d513
-- 
2.34.1


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RFC PATCH 0/3] RISC-V: Check shift amount against XLEN (disassembler)
  2022-05-23 10:06 [RFC PATCH 0/3] RISC-V: Check shift amount against XLEN (disassembler) Tsukasa OI
                   ` (2 preceding siblings ...)
  2022-05-23 10:06 ` [RFC PATCH 3/3] RISC-V: Add disassembler tests for shift amount Tsukasa OI
@ 2022-07-30  3:47 ` Tsukasa OI
  2022-07-30  3:51 ` [PATCH 0/1] " Tsukasa OI
  4 siblings, 0 replies; 7+ messages in thread
From: Tsukasa OI @ 2022-07-30  3:47 UTC (permalink / raw)
  To: Palmer Dabbelt, Nelson Chu, Kito Cheng; +Cc: binutils

This patchset is officially withdrawn as I will submit new simpler
patchset for the exact same issue.

Thanks,
Tsukasa

On 2022/05/23 19:06, Tsukasa OI wrote:
> ** Note **
> This patch is not compatible with my previous Zfinx fixes.  Actual PATCH
> v1 is going to be submitted after this patchset is reviewed and Zfinx
> fixes (relatively high priority) are applied.
> 
> Certain shift instructions have a constraint: shift amount must be less
> than current XLEN.  This constraint is checked on assembler and
> simulator, but not on disassembler.  It causes GDB to print wrong (and
> invalid) instructions and can be a problem while ... for instance,
> checking for invalid build configuration.
> 
> We have several methods to deal with it:
> 
> -   Add `xlen' argument to match_func to test XLEN
> -   Split shift instructions to per-XLEN variants
> 
> Because shift instructions are so basic and XLEN-checking is so simple,
> I chose the former.  The latter is used on my Zfinx fixes but this is
> because register pair checking involves relatively complex per-operand
> constraints.  Complex solution for simple problem can be a new problem.
> 
> But...
> 
> -   Is it okay to add an argument anyway?
> -   Which is better extra argument?
>     1.  unsigned xlen
>     2.  const riscv_parse_subset_t * subset
>         which contains ISA version, XLEN and extensions
>         ... but a part of BFD, not opcodes.
> 
> I also (slightly) changed the assembler to suppress extra error message
> just like I did on Zicbop instructions:
> <https://sourceware.org/pipermail/binutils/2021-December/118910.html>
> <https://sourceware.org/pipermail/binutils/2021-December/118938.html>
> 
> 
> 
> 
> Tsukasa OI (3):
>   RISC-V: Add xlen to match_func
>   RISC-V: Check shift amount against XLEN
>   RISC-V: Add disassembler tests for shift amount
> 
>  gas/config/tc-riscv.c                  |   8 +-
>  gas/testsuite/gas/riscv/shamt-dis-32.d |  34 +++++++
>  gas/testsuite/gas/riscv/shamt-dis-64.d |  34 +++++++
>  gas/testsuite/gas/riscv/shamt-dis.s    |  45 +++++++++
>  include/opcode/riscv.h                 |   3 +-
>  opcodes/riscv-dis.c                    |   2 +-
>  opcodes/riscv-opc.c                    | 122 +++++++++++++++----------
>  sim/riscv/sim-main.c                   |   2 +-
>  8 files changed, 195 insertions(+), 55 deletions(-)
>  create mode 100644 gas/testsuite/gas/riscv/shamt-dis-32.d
>  create mode 100644 gas/testsuite/gas/riscv/shamt-dis-64.d
>  create mode 100644 gas/testsuite/gas/riscv/shamt-dis.s
> 
> 
> base-commit: cb0d58bf4d274cfb1ae11b75bd2b3ba81c8d371d

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH 0/1] RISC-V: Check shift amount against XLEN (disassembler)
  2022-05-23 10:06 [RFC PATCH 0/3] RISC-V: Check shift amount against XLEN (disassembler) Tsukasa OI
                   ` (3 preceding siblings ...)
  2022-07-30  3:47 ` [RFC PATCH 0/3] RISC-V: Check shift amount against XLEN (disassembler) Tsukasa OI
@ 2022-07-30  3:51 ` Tsukasa OI
  2022-07-30  3:51   ` [PATCH 1/1] RISC-V: Check shift amount against XLEN (disasm) Tsukasa OI
  4 siblings, 1 reply; 7+ messages in thread
From: Tsukasa OI @ 2022-07-30  3:51 UTC (permalink / raw)
  To: Tsukasa OI, Nelson Chu, Palmer Dabbelt, Kito Cheng; +Cc: binutils

Hello,

This patchset is going to support shift amount checking on the disassembler.

Previous Patchset:
<https://sourceware.org/pipermail/binutils/2022-May/120956.html>
<https://github.com/a4lg/binutils-gdb/wiki/riscv_dis_shift_xlen>
Tracker on GitHub:
<https://github.com/a4lg/binutils-gdb/wiki/riscv_dis_shift_xlen_2>


In the disassembler, there's no validation whether shift amount is valid on the
specific architecture (RV32/64).

My previous Idea 1 patchset tried to fix that by adding xlen argument to every
match_func (in opcode entries).  However, it requires bigger core disassembler
changes and maintaining is relatively hard.

Idea 2 patchset (this patchset) fixes that by printing invalid0x[SHAMT] instead
of actual shift width (SHAMT in hexadecimal like regular shift amounts).  If we
don't have to print "unrecognized instruction" (such as .4byte) on instrucitons
with invalid shift amounts, this patch would be the simplest solution for the
invalid shift amount checking support on the disassembler.


This patchset supports styling. invalid shift amounts are printed as text, not
immediate.  It makes invalid shift amounts default white opposed to blue on
valid ones.
See an example with the GitHub Wiki page linked above.


Thanks,
Tsukasa




Tsukasa OI (1):
  RISC-V: Check shift amount against XLEN (disasm)

 gas/testsuite/gas/riscv/shamt-dis-32.d | 34 +++++++++++++++++++
 gas/testsuite/gas/riscv/shamt-dis-64.d | 34 +++++++++++++++++++
 gas/testsuite/gas/riscv/shamt-dis.s    | 47 ++++++++++++++++++++++++++
 opcodes/riscv-dis.c                    | 16 ++++++---
 4 files changed, 127 insertions(+), 4 deletions(-)
 create mode 100644 gas/testsuite/gas/riscv/shamt-dis-32.d
 create mode 100644 gas/testsuite/gas/riscv/shamt-dis-64.d
 create mode 100644 gas/testsuite/gas/riscv/shamt-dis.s


base-commit: b245c595aaa59812f8f3a0e8b70ea5f52e045627
-- 
2.34.1


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH 1/1] RISC-V: Check shift amount against XLEN (disasm)
  2022-07-30  3:51 ` [PATCH 0/1] " Tsukasa OI
@ 2022-07-30  3:51   ` Tsukasa OI
  0 siblings, 0 replies; 7+ messages in thread
From: Tsukasa OI @ 2022-07-30  3:51 UTC (permalink / raw)
  To: Tsukasa OI, Nelson Chu, Palmer Dabbelt, Kito Cheng; +Cc: binutils

Although the assembler does the equivalent, the disassembler did not
have shift amount checking.  Instead of actually rejecting invalid shift
amount, this commit prints invalid shift amount as "invalid0x[SHAMT]".

It also changes the style so that style-aware disassembler can distinguish
regular shift amount (immediate) and invalid one.

gas/ChangeLog:

	* testsuite/gas/riscv/shamt-dis.s: New disassembler test.
	* testsuite/gas/riscv/shamt-dis-32.d: Likewise.
	* testsuite/gas/riscv/shamt-dis-64.d: Likewise.

opcodes/ChangeLog:

	* riscv-dis.c (print_insn_args): Print invalid shift amount as
	"invalid0x[SHAMT]" where SHAMT is represented in hexadecimal.
---
 gas/testsuite/gas/riscv/shamt-dis-32.d | 34 +++++++++++++++++++
 gas/testsuite/gas/riscv/shamt-dis-64.d | 34 +++++++++++++++++++
 gas/testsuite/gas/riscv/shamt-dis.s    | 47 ++++++++++++++++++++++++++
 opcodes/riscv-dis.c                    | 16 ++++++---
 4 files changed, 127 insertions(+), 4 deletions(-)
 create mode 100644 gas/testsuite/gas/riscv/shamt-dis-32.d
 create mode 100644 gas/testsuite/gas/riscv/shamt-dis-64.d
 create mode 100644 gas/testsuite/gas/riscv/shamt-dis.s

diff --git a/gas/testsuite/gas/riscv/shamt-dis-32.d b/gas/testsuite/gas/riscv/shamt-dis-32.d
new file mode 100644
index 00000000000..85a247f7e25
--- /dev/null
+++ b/gas/testsuite/gas/riscv/shamt-dis-32.d
@@ -0,0 +1,34 @@
+#as: -march=rv32ic_zba_zbb_zbs
+#source: shamt-dis.s
+#objdump: -d -M no-aliases
+
+.*:[ 	]+file format .*
+
+
+Disassembly of section .text:
+
+0+000 <target>:
+[ 	]+[0-9a-f]+:[ 	]+01f59513[ 	]+slli[ 	]+a0,a1,0x1f
+[ 	]+[0-9a-f]+:[ 	]+02059513[ 	]+slli[ 	]+a0,a1,invalid0x20
+[ 	]+[0-9a-f]+:[ 	]+01f5d513[ 	]+srli[ 	]+a0,a1,0x1f
+[ 	]+[0-9a-f]+:[ 	]+0205d513[ 	]+srli[ 	]+a0,a1,invalid0x20
+[ 	]+[0-9a-f]+:[ 	]+41f5d513[ 	]+srai[ 	]+a0,a1,0x1f
+[ 	]+[0-9a-f]+:[ 	]+4205d513[ 	]+srai[ 	]+a0,a1,invalid0x20
+[ 	]+[0-9a-f]+:[ 	]+057e[ 	]+c\.slli[ 	]+a0,0x1f
+[ 	]+[0-9a-f]+:[ 	]+1502[ 	]+c\.slli[ 	]+a0,invalid0x20
+[ 	]+[0-9a-f]+:[ 	]+817d[ 	]+c\.srli[ 	]+a0,0x1f
+[ 	]+[0-9a-f]+:[ 	]+9101[ 	]+c\.srli[ 	]+a0,invalid0x20
+[ 	]+[0-9a-f]+:[ 	]+857d[ 	]+c\.srai[ 	]+a0,0x1f
+[ 	]+[0-9a-f]+:[ 	]+9501[ 	]+c\.srai[ 	]+a0,invalid0x20
+[ 	]+[0-9a-f]+:[ 	]+61f5d513[ 	]+rori[ 	]+a0,a1,0x1f
+[ 	]+[0-9a-f]+:[ 	]+6205d513[ 	]+rori[ 	]+a0,a1,invalid0x20
+[ 	]+[0-9a-f]+:[ 	]+09f5951b[ 	]+\.4byte[ 	]+0x9f5951b
+[ 	]+[0-9a-f]+:[ 	]+0a05951b[ 	]+\.4byte[ 	]+0xa05951b
+[ 	]+[0-9a-f]+:[ 	]+49f59513[ 	]+bclri[ 	]+a0,a1,0x1f
+[ 	]+[0-9a-f]+:[ 	]+4a059513[ 	]+bclri[ 	]+a0,a1,invalid0x20
+[ 	]+[0-9a-f]+:[ 	]+29f59513[ 	]+bseti[ 	]+a0,a1,0x1f
+[ 	]+[0-9a-f]+:[ 	]+2a059513[ 	]+bseti[ 	]+a0,a1,invalid0x20
+[ 	]+[0-9a-f]+:[ 	]+69f59513[ 	]+binvi[ 	]+a0,a1,0x1f
+[ 	]+[0-9a-f]+:[ 	]+6a059513[ 	]+binvi[ 	]+a0,a1,invalid0x20
+[ 	]+[0-9a-f]+:[ 	]+49f5d513[ 	]+bexti[ 	]+a0,a1,0x1f
+[ 	]+[0-9a-f]+:[ 	]+4a05d513[ 	]+bexti[ 	]+a0,a1,invalid0x20
diff --git a/gas/testsuite/gas/riscv/shamt-dis-64.d b/gas/testsuite/gas/riscv/shamt-dis-64.d
new file mode 100644
index 00000000000..d10e90d9d30
--- /dev/null
+++ b/gas/testsuite/gas/riscv/shamt-dis-64.d
@@ -0,0 +1,34 @@
+#as: -march=rv64ic_zba_zbb_zbs
+#source: shamt-dis.s
+#objdump: -d -M no-aliases
+
+.*:[ 	]+file format .*
+
+
+Disassembly of section .text:
+
+0+000 <target>:
+[ 	]+[0-9a-f]+:[ 	]+01f59513[ 	]+slli[ 	]+a0,a1,0x1f
+[ 	]+[0-9a-f]+:[ 	]+02059513[ 	]+slli[ 	]+a0,a1,0x20
+[ 	]+[0-9a-f]+:[ 	]+01f5d513[ 	]+srli[ 	]+a0,a1,0x1f
+[ 	]+[0-9a-f]+:[ 	]+0205d513[ 	]+srli[ 	]+a0,a1,0x20
+[ 	]+[0-9a-f]+:[ 	]+41f5d513[ 	]+srai[ 	]+a0,a1,0x1f
+[ 	]+[0-9a-f]+:[ 	]+4205d513[ 	]+srai[ 	]+a0,a1,0x20
+[ 	]+[0-9a-f]+:[ 	]+057e[ 	]+c\.slli[ 	]+a0,0x1f
+[ 	]+[0-9a-f]+:[ 	]+1502[ 	]+c\.slli[ 	]+a0,0x20
+[ 	]+[0-9a-f]+:[ 	]+817d[ 	]+c\.srli[ 	]+a0,0x1f
+[ 	]+[0-9a-f]+:[ 	]+9101[ 	]+c\.srli[ 	]+a0,0x20
+[ 	]+[0-9a-f]+:[ 	]+857d[ 	]+c\.srai[ 	]+a0,0x1f
+[ 	]+[0-9a-f]+:[ 	]+9501[ 	]+c\.srai[ 	]+a0,0x20
+[ 	]+[0-9a-f]+:[ 	]+61f5d513[ 	]+rori[ 	]+a0,a1,0x1f
+[ 	]+[0-9a-f]+:[ 	]+6205d513[ 	]+rori[ 	]+a0,a1,0x20
+[ 	]+[0-9a-f]+:[ 	]+09f5951b[ 	]+slli\.uw[ 	]+a0,a1,0x1f
+[ 	]+[0-9a-f]+:[ 	]+0a05951b[ 	]+slli\.uw[ 	]+a0,a1,0x20
+[ 	]+[0-9a-f]+:[ 	]+49f59513[ 	]+bclri[ 	]+a0,a1,0x1f
+[ 	]+[0-9a-f]+:[ 	]+4a059513[ 	]+bclri[ 	]+a0,a1,0x20
+[ 	]+[0-9a-f]+:[ 	]+29f59513[ 	]+bseti[ 	]+a0,a1,0x1f
+[ 	]+[0-9a-f]+:[ 	]+2a059513[ 	]+bseti[ 	]+a0,a1,0x20
+[ 	]+[0-9a-f]+:[ 	]+69f59513[ 	]+binvi[ 	]+a0,a1,0x1f
+[ 	]+[0-9a-f]+:[ 	]+6a059513[ 	]+binvi[ 	]+a0,a1,0x20
+[ 	]+[0-9a-f]+:[ 	]+49f5d513[ 	]+bexti[ 	]+a0,a1,0x1f
+[ 	]+[0-9a-f]+:[ 	]+4a05d513[ 	]+bexti[ 	]+a0,a1,0x20
diff --git a/gas/testsuite/gas/riscv/shamt-dis.s b/gas/testsuite/gas/riscv/shamt-dis.s
new file mode 100644
index 00000000000..a6e292242d6
--- /dev/null
+++ b/gas/testsuite/gas/riscv/shamt-dis.s
@@ -0,0 +1,47 @@
+target:
+	# slli a0,a1,SHAMT [31/32]
+	.insn i	OP_IMM, 1, a0, a1, 0x000 | 31
+	.insn i	OP_IMM, 1, a0, a1, 0x000 | 32
+	# srli a0,a1,SHAMT [31/32]
+	.insn i	OP_IMM, 5, a0, a1, 0x000 | 31
+	.insn i	OP_IMM, 5, a0, a1, 0x000 | 32
+	# srai a0,a1,SHAMT [31/32]
+	.insn i	OP_IMM, 5, a0, a1, 0x400 | 31
+	.insn i	OP_IMM, 5, a0, a1, 0x400 | 32
+
+	# RVC
+	# c.slli a0,SHAMT [31/32]
+	.insn ci	2, 0, a0, 31
+	.insn ci	2, 0, a0, 32 - 0x40
+	# Although c.sr[la]i are CB-format instructions,
+	# encode them as CI-type for immediate consistency.
+	# c.srli a0,SHAMT [31/32]
+	.insn ci	1, 4, x2, 31
+	.insn ci	1, 4, x2, 32 - 0x40
+	# c.srai a0,SHAMT [31/32]
+	.insn ci	1, 4, x10, 31
+	.insn ci	1, 4, x10, 32 - 0x40
+
+	# Zbb extension (or Zbkb)
+	# rori a0,a1,SHAMT [31/32]
+	.insn i	OP_IMM, 5, a0, a1, 0x600 | 31
+	.insn i	OP_IMM, 5, a0, a1, 0x600 | 32
+
+	# Zba extension
+	# slli.uw a0,a1,SHAMT [31/32] (invalid on RV32)
+	.insn i	OP_IMM_32, 1, a0, a1, 0x080 | 31
+	.insn i	OP_IMM_32, 1, a0, a1, 0x080 | 32
+
+	# Zbs extension
+	# bclri a0,a1,SHAMT [31/32]
+	.insn i	OP_IMM, 1, a0, a1, 0x480 | 31
+	.insn i	OP_IMM, 1, a0, a1, 0x480 | 32
+	# bseti a0,a1,SHAMT [31/32]
+	.insn i	OP_IMM, 1, a0, a1, 0x280 | 31
+	.insn i	OP_IMM, 1, a0, a1, 0x280 | 32
+	# binvi a0,a1,SHAMT [31/32]
+	.insn i	OP_IMM, 1, a0, a1, 0x680 | 31
+	.insn i	OP_IMM, 1, a0, a1, 0x680 | 32
+	# bexti a0,a1,SHAMT [31/32]
+	.insn i	OP_IMM, 5, a0, a1, 0x480 | 31
+	.insn i	OP_IMM, 5, a0, a1, 0x480 | 32
diff --git a/opcodes/riscv-dis.c b/opcodes/riscv-dis.c
index 164fd209dbd..3e9519525d4 100644
--- a/opcodes/riscv-dis.c
+++ b/opcodes/riscv-dis.c
@@ -286,8 +286,12 @@ print_insn_args (const char *oparg, insn_t l, bfd_vma pc, disassemble_info *info
 		     (int)(EXTRACT_CITYPE_IMM (l) & (RISCV_BIGIMM_REACH-1)));
 	      break;
 	    case '>':
-	      print (info->stream, dis_style_immediate, "0x%x",
-		     (int)EXTRACT_CITYPE_IMM (l) & 0x3f);
+	      if (((unsigned)EXTRACT_CITYPE_IMM (l) & 0x3fU) >= xlen)
+		print (info->stream, dis_style_text, "invalid0x%x",
+		       (unsigned)EXTRACT_CITYPE_IMM (l) & 0x3fU);
+	      else
+		print (info->stream, dis_style_immediate, "0x%x",
+		       (unsigned)EXTRACT_CITYPE_IMM (l) & 0x3fU);
 	      break;
 	    case '<':
 	      print (info->stream, dis_style_immediate, "0x%x",
@@ -481,8 +485,12 @@ print_insn_args (const char *oparg, insn_t l, bfd_vma pc, disassemble_info *info
 	  break;
 
 	case '>':
-	  print (info->stream, dis_style_immediate, "0x%x",
-		 (int)EXTRACT_OPERAND (SHAMT, l));
+	  if ((unsigned)EXTRACT_OPERAND (SHAMT, l) >= xlen)
+	    print (info->stream, dis_style_text, "invalid0x%x",
+		   (unsigned)EXTRACT_OPERAND (SHAMT, l));
+	  else
+	    print (info->stream, dis_style_immediate, "0x%x",
+		   (unsigned)EXTRACT_OPERAND (SHAMT, l));
 	  break;
 
 	case '<':
-- 
2.34.1


^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2022-07-30  3:51 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-23 10:06 [RFC PATCH 0/3] RISC-V: Check shift amount against XLEN (disassembler) Tsukasa OI
2022-05-23 10:06 ` [RFC PATCH 1/3] RISC-V: Add xlen to match_func Tsukasa OI
2022-05-23 10:06 ` [RFC PATCH 2/3] RISC-V: Check shift amount against XLEN Tsukasa OI
2022-05-23 10:06 ` [RFC PATCH 3/3] RISC-V: Add disassembler tests for shift amount Tsukasa OI
2022-07-30  3:47 ` [RFC PATCH 0/3] RISC-V: Check shift amount against XLEN (disassembler) Tsukasa OI
2022-07-30  3:51 ` [PATCH 0/1] " Tsukasa OI
2022-07-30  3:51   ` [PATCH 1/1] RISC-V: Check shift amount against XLEN (disasm) Tsukasa OI

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).