diff mbox series

[v2,1/2] RISC-V: Fallback for instructions longer than 64b

Message ID af45c5d57c221e43101c2aa38b14f4b4ac12d011.1665031170.git.research_trasio@irq.a4lg.com
State Accepted, archived
Headers
Series RISC-V: Improve "bits undefined" diagnostics |

Checks

Context Check Description
snail/binutils-gdb-check success Github commit url

Commit Message

Tsukasa OI Oct. 6, 2022, 4:40 a.m. UTC 
  We don't support instructions longer than 64-bits yet.  Still, we can
modify validate_riscv_insn function to prevent unexpected behavior by
limiting the "length" of an instruction to 64-bit (or less).

gas/ChangeLog:

	* config/tc-riscv.c (validate_riscv_insn): Fix function
	description comment based on current usage.  Limit instruction
	length up to 64-bit for now.  Make sure that required_bits does
	not corrupt even if unsigned long long is longer than 64-bit.
---
 gas/config/tc-riscv.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

Comments

Jan Beulich Oct. 6, 2022, 8:22 a.m. UTC | #1 
On 06.10.2022 06:40, Tsukasa OI via Binutils wrote:
> We don't support instructions longer than 64-bits yet.  Still, we can
> modify validate_riscv_insn function to prevent unexpected behavior by
> limiting the "length" of an instruction to 64-bit (or less).
> 
> gas/ChangeLog:
> 
> 	* config/tc-riscv.c (validate_riscv_insn): Fix function
> 	description comment based on current usage.  Limit instruction
> 	length up to 64-bit for now.  Make sure that required_bits does
> 	not corrupt even if unsigned long long is longer than 64-bit.

While I agree with the code change, I don't agree with the adjustment
to the comment - you're changing it to match the sole present caller,
but imo such a comment ought to describe the behavior of the function
irrespective of how it's used at any given point in time.

Jan
Tsukasa OI Oct. 6, 2022, 9:52 a.m. UTC | #2 
On 2022/10/06 17:22, Jan Beulich wrote:
> On 06.10.2022 06:40, Tsukasa OI via Binutils wrote:
>> We don't support instructions longer than 64-bits yet.  Still, we can
>> modify validate_riscv_insn function to prevent unexpected behavior by
>> limiting the "length" of an instruction to 64-bit (or less).
>>
>> gas/ChangeLog:
>>
>> 	* config/tc-riscv.c (validate_riscv_insn): Fix function
>> 	description comment based on current usage.  Limit instruction
>> 	length up to 64-bit for now.  Make sure that required_bits does
>> 	not corrupt even if unsigned long long is longer than 64-bit.
> 
> While I agree with the code change, I don't agree with the adjustment
> to the comment - you're changing it to match the sole present caller,
> but imo such a comment ought to describe the behavior of the function
> irrespective of how it's used at any given point in time.
> 
> Jan
> 

Okay, I revised the comment to reflect the specification itself.

Thanks,
Tsukasa
diff mbox series

Patch

diff --git a/gas/config/tc-riscv.c b/gas/config/tc-riscv.c
index 22385d1baa0..2e41cec5c9f 100644
--- a/gas/config/tc-riscv.c
+++ b/gas/config/tc-riscv.c
@@ -1109,7 +1109,7 @@  arg_lookup (char **s, const char *const *array, size_t size, unsigned *regnop)
 
 /* For consistency checking, verify that all bits are specified either
    by the match/mask part of the instruction definition, or by the
-   operand list. The `length` could be 0, 4 or 8, 0 for auto detection.  */
+   operand list. The `length` could be 0, 2 or 4, 0 for auto detection.  */
 
 static bool
 validate_riscv_insn (const struct riscv_opcode *opc, int length)
@@ -1120,11 +1120,13 @@  validate_riscv_insn (const struct riscv_opcode *opc, int length)
   insn_t required_bits;
 
   if (length == 0)
-    insn_width = 8 * riscv_insn_length (opc->match);
-  else
-    insn_width = 8 * length;
+    length = riscv_insn_length (opc->match);
+  /* We don't support instructions longer than 64-bits yet.  */
+  if (length > 8)
+    length = 8;
+  insn_width = 8 * length;
 
-  required_bits = ~0ULL >> (64 - insn_width);
+  required_bits = ((insn_t)~0ULL) >> (64 - insn_width);
 
   if ((used_bits & opc->match) != (opc->match & required_bits))
     {