[PATCH 0/2] RISC-V: Improve "bits undefined" diagnostics

[PATCH 0/2] RISC-V: Improve "bits undefined" diagnostics

[Tsukasa OI]

Hello,

This small patch intends to improve one of the internal diagnostic messages
when an invalid RISC-V instruction is defined in riscv-opc.c.

Tracker on GitHub:
<https://github.com/a4lg/binutils-gdb/wiki/riscv_gas_diag_unused_bits>

    Sidenote:
    I started listing my Binutils submissions on my GitHub Wiki:
    <https://github.com/a4lg/binutils-gdb/wiki/Patch-Queue>
    hoping that current status and conflicting patches are clear.

So, priority to apply this patch is VERY LOW
(definitely, this is the last patchset sent by me to be reviewed).

However, I believe that applying this patch improves experient while testing
new RISC-V instructions (or modifying RISC-V instructions in somy way).



First -- just to experiment -- we change mask value of "fcvt.d.s"
instruction from MASK_FCVT_D_S|MASK_RM to MASK_FCVT_D_S while not touching
operands "D,S" and we run compiled assembler, we get following message:

    Assembler messages:
    Error: internal: bad RISC-V opcode (bits 0xffffffff00007000 undefined): fcvt.d.s D,S
    Fatal error: internal: broken assembler.  No assembly attempted

Bits 0x7000 corresponds to rm (rounding mode) bits we just removed and no
corresponding operands are found (making definition of fcvt.d.s instruction
invalid).

Then, what about 0xffffffff00000000 (upper 32-bits)?
Yes, they are non-instruction bits.  Because of ~ (bitwise complement)
operator while computing undefined bits, it also displays non-instruction
bits.


This patchset (PATCH 1/2) changes how undefined/invalid bits are computed.

before:
    ~(used & required)
after:
    (used ^ required)

After PATCH 1/2, following error message is generated.

    Assembler messages:
    Error: internal: bad RISC-V opcode (bits 0x7000 undefined or invalid): fcvt.d.s D,S
    Fatal error: internal: broken assembler.  No assembly attempted


Note that we are testing for "undefined or invalid" bits here, not just
undefined bits.  In fact, if we corrupt a variant of c.addi instruction with
ADDITIONAL "j" operand (which is an immediate for I-type instruction, upper
12-bits of **32-bit** instruction encoding), we get following message:

    Assembler messages:
    Error: internal: bad RISC-V opcode (bits 0xfff00000 undefined or invalid): addi d,CU,Cj,j
    Fatal error: internal: broken assembler.  No assembly attempted

Okay, extra "j" operand generates "extra" bits (that should not have been
defined considering its 16-bit encoding) and words "undefined or invalid"
are working here.  We are correctly capturing invalid extra bits.  Before
this patch, invalid bits are hidden by 0xffffffffffff0000 (48 non-
instruction bits).


Additionally, this patchset includes a minor fix when unsigned long long
type is larger than 64-bits.  It also works to correct computing required
bits (PATCH 2/2).


Again, this patch is not important but anyway, happy hacking!

Thanks,
Tsukasa




Tsukasa OI (2):
  RISC-V: Improve "bits undefined" diagnostics
  RISC-V: Fix required bits on certain environments

 gas/config/tc-riscv.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)


base-commit: d2acd4b0c5bab349aaa152d60268bc144634a844
-- 
2.34.1

[PATCH 1/2] RISC-V: Improve "bits undefined" diagnostics

[Tsukasa OI]

This commit improves internal error message
"internal: bad RISC-V opcode (bits 0x%lx undefined): %s %s"
to display actual unused bits (excluding non-instruction bits).

gas/ChangeLog:

	* config/tc-riscv.c (validate_riscv_insn): Exclude non-
	instruction bits from displaying internal diagnostics.
	Change error message slightly.
---
 gas/config/tc-riscv.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/gas/config/tc-riscv.c b/gas/config/tc-riscv.c
index a0e8456a0d1..8a961c05d95 100644
--- a/gas/config/tc-riscv.c
+++ b/gas/config/tc-riscv.c
@@ -1265,8 +1265,8 @@ validate_riscv_insn (const struct riscv_opcode *opc, int length)
   if (used_bits != required_bits)
     {
       as_bad (_("internal: bad RISC-V opcode "
-		"(bits 0x%lx undefined): %s %s"),
-	      ~(unsigned long)(used_bits & required_bits),
+		"(bits 0x%lx undefined or invalid): %s %s"),
+	      (unsigned long)(used_bits ^ required_bits),
 	      opc->name, opc->args);
       return false;
     }
-- 
2.34.1

[PATCH 2/2] RISC-V: Fix required bits on certain environments

[Tsukasa OI]

If `unsigned long long' type has more than 64-bits, validate_riscv_insn
function generated wrong required_bits value.  This commit fixes this
small issue (may be too pedantic though).

gas/ChangeLog:

	* config/tc-riscv.c (validate_riscv_insn): Compute correct
	required_bits value when unsigned long long is larger than 64b.
---
 gas/config/tc-riscv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gas/config/tc-riscv.c b/gas/config/tc-riscv.c
index 8a961c05d95..9581a5c6e03 100644
--- a/gas/config/tc-riscv.c
+++ b/gas/config/tc-riscv.c
@@ -1110,7 +1110,7 @@ validate_riscv_insn (const struct riscv_opcode *opc, int length)
   else
     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))
     {
-- 
2.34.1

[PATCH v2 0/2] RISC-V: Improve "bits undefined" diagnostics

[Tsukasa OI]

Hello,

This small patch intends to improve one of the internal diagnostic messages
when an invalid RISC-V instruction is defined in riscv-opc.c.

Tracker on GitHub:
<https://github.com/a4lg/binutils-gdb/wiki/riscv_gas_diag_unused_bits>

It was a very low-priority patch but since commit bb996692bd9 "RISC-V/gas:
allow generating up to 176-bit instructions with .insn" can break
validate_riscv_insn function (by negative shift width), it'a good chance to
improve this function entirely.

This is mostly a quote from the cover letter of PATCH v1 but the words
"PATCH 1/2" are replaced with "PATCH 2/2" to reflect PATCH v2.


> First -- just to experiment -- we change mask value of "fcvt.d.s"
> instruction from MASK_FCVT_D_S|MASK_RM to MASK_FCVT_D_S while not touching
> operands "D,S" and we run compiled assembler, we get following message:
>
>     Assembler messages:
>     Error: internal: bad RISC-V opcode (bits 0xffffffff00007000 undefined): fcvt.d.s D,S
>     Fatal error: internal: broken assembler.  No assembly attempted
>
> Bits 0x7000 corresponds to rm (rounding mode) bits we just removed and no
> corresponding operands are found (making definition of fcvt.d.s instruction
> invalid).
>
> Then, what about 0xffffffff00000000 (upper 32-bits)?
> Yes, they are non-instruction bits.  Because of ~ (bitwise complement)
> operator while computing undefined bits, it also displays non-instruction
> bits.
>
>
> This patchset (PATCH 2/2) changes how undefined/invalid bits are computed.
>
> before:
>     ~(used & required)
> after:
>     (used ^ required)
>
> After PATCH 2/2, following error message is generated.
>
>     Assembler messages:
>     Error: internal: bad RISC-V opcode (bits 0x7000 undefined or invalid): fcvt.d.s D,S
>     Fatal error: internal: broken assembler.  No assembly attempted
>
>
> Note that we are testing for "undefined or invalid" bits here, not just
> undefined bits.  In fact, if we corrupt a variant of c.addi instruction with
> ADDITIONAL "j" operand (which is an immediate for I-type instruction, upper
> 12-bits of **32-bit** instruction encoding), we get following message:
>
>     Assembler messages:
>     Error: internal: bad RISC-V opcode (bits 0xfff00000 undefined or invalid): addi d,CU,Cj,j
>     Fatal error: internal: broken assembler.  No assembly attempted
>
> Okay, extra "j" operand generates "extra" bits (that should not have been
> defined considering its 16-bit encoding) and words "undefined or invalid"
> are working here.  We are correctly capturing invalid extra bits.  Before
> this patch, invalid bits are hidden by 0xffffffffffff0000 (48 non-
> instruction bits).


Thanks,
Tsukasa




Tsukasa OI (2):
  RISC-V: Fallback for instructions longer than 64b
  RISC-V: Improve "bits undefined" diagnostics

 gas/config/tc-riscv.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)


base-commit: a13886e2198beb78b81c59839043b021ce6df78a
-- 
2.34.1

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

[Tsukasa OI]

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

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

[PATCH v2 2/2] RISC-V: Improve "bits undefined" diagnostics

[Tsukasa OI]

This commit improves internal error message
"internal: bad RISC-V opcode (bits 0x%lx undefined): %s %s"
to display actual unused bits (excluding non-instruction bits).

gas/ChangeLog:

	* config/tc-riscv.c (validate_riscv_insn): Exclude non-
	instruction bits from displaying internal diagnostics.
	Change error message slightly.
---
 gas/config/tc-riscv.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/gas/config/tc-riscv.c b/gas/config/tc-riscv.c
index 2e41cec5c9f..34973d7803c 100644
--- a/gas/config/tc-riscv.c
+++ b/gas/config/tc-riscv.c
@@ -1312,8 +1312,8 @@ validate_riscv_insn (const struct riscv_opcode *opc, int length)
   if (used_bits != required_bits)
     {
       as_bad (_("internal: bad RISC-V opcode "
-		"(bits 0x%lx undefined): %s %s"),
-	      ~(unsigned long)(used_bits & required_bits),
+		"(bits 0x%llx undefined or invalid): %s %s"),
+	      (unsigned long long)(used_bits ^ required_bits),
 	      opc->name, opc->args);
       return false;
     }
-- 
2.34.1

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

[Jan Beulich]

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

Re: [PATCH v2 2/2] RISC-V: Improve "bits undefined" diagnostics

[Jan Beulich]

On 06.10.2022 06:40, Tsukasa OI via Binutils wrote:
> --- a/gas/config/tc-riscv.c
> +++ b/gas/config/tc-riscv.c
> @@ -1312,8 +1312,8 @@ validate_riscv_insn (const struct riscv_opcode *opc, int length)
>    if (used_bits != required_bits)
>      {
>        as_bad (_("internal: bad RISC-V opcode "
> -		"(bits 0x%lx undefined): %s %s"),
> -	      ~(unsigned long)(used_bits & required_bits),
> +		"(bits 0x%llx undefined or invalid): %s %s"),
> +	      (unsigned long long)(used_bits ^ required_bits),

May I encourage the use of the # format modifier in cases like this
one (i.e. %#llx here), for producing a one character shorter string
literal? Iirc a respective adjustment was done pretty recently to
some other parts of binutils.

Jan

Re: [PATCH v2 2/2] RISC-V: Improve "bits undefined" diagnostics

[Tsukasa OI]

On 2022/10/06 17:26, Jan Beulich wrote:
> On 06.10.2022 06:40, Tsukasa OI via Binutils wrote:
>> --- a/gas/config/tc-riscv.c
>> +++ b/gas/config/tc-riscv.c
>> @@ -1312,8 +1312,8 @@ validate_riscv_insn (const struct riscv_opcode *opc, int length)
>>    if (used_bits != required_bits)
>>      {
>>        as_bad (_("internal: bad RISC-V opcode "
>> -		"(bits 0x%lx undefined): %s %s"),
>> -	      ~(unsigned long)(used_bits & required_bits),
>> +		"(bits 0x%llx undefined or invalid): %s %s"),
>> +	      (unsigned long long)(used_bits ^ required_bits),
> 
> May I encourage the use of the # format modifier in cases like this
> one (i.e. %#llx here), for producing a one character shorter string
> literal? Iirc a respective adjustment was done pretty recently to
> some other parts of binutils.

I would disagree if it was a part of the core disassembling portion
but... seems okay here (as exact formatting is not important).  It would
have changed the behavior if (used_bits ^ required_bits) is not zero
(e.g. with "%#x": "0" (0), "0x1" (1)...) but here, (used_bits ^
required_bits) cannot be zero.  So, the behavior won't change either.

Thanks,
Tsukasa

> 
> Jan
> 

Re: [PATCH v2 2/2] RISC-V: Improve "bits undefined" diagnostics

[Jan Beulich]

On 06.10.2022 10:34, Tsukasa OI wrote:
> On 2022/10/06 17:26, Jan Beulich wrote:
>> On 06.10.2022 06:40, Tsukasa OI via Binutils wrote:
>>> --- a/gas/config/tc-riscv.c
>>> +++ b/gas/config/tc-riscv.c
>>> @@ -1312,8 +1312,8 @@ validate_riscv_insn (const struct riscv_opcode *opc, int length)
>>>    if (used_bits != required_bits)
>>>      {
>>>        as_bad (_("internal: bad RISC-V opcode "
>>> -		"(bits 0x%lx undefined): %s %s"),
>>> -	      ~(unsigned long)(used_bits & required_bits),
>>> +		"(bits 0x%llx undefined or invalid): %s %s"),
>>> +	      (unsigned long long)(used_bits ^ required_bits),
>>
>> May I encourage the use of the # format modifier in cases like this
>> one (i.e. %#llx here), for producing a one character shorter string
>> literal? Iirc a respective adjustment was done pretty recently to
>> some other parts of binutils.
> 
> I would disagree if it was a part of the core disassembling portion
> but...

Sure - typically in disassembly you want to output leading zeros, and
in that case using # isn't desirable. (I've observed RISC-V disassembly
to omit leading zeros in certain cases though, which personally I find
confusing.)

Jan

> seems okay here (as exact formatting is not important).  It would
> have changed the behavior if (used_bits ^ required_bits) is not zero
> (e.g. with "%#x": "0" (0), "0x1" (1)...) but here, (used_bits ^
> required_bits) cannot be zero.  So, the behavior won't change either.
> 
> Thanks,
> Tsukasa

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

[Tsukasa OI]

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

[PATCH v3 0/2] RISC-V: Improve "bits undefined" diagnostics

[Tsukasa OI]

Hello,

This small patch intends to improve one of the internal diagnostic messages
when an invalid RISC-V instruction is defined in riscv-opc.c.

Tracker on GitHub:
<https://github.com/a4lg/binutils-gdb/wiki/riscv_gas_diag_unused_bits>

It was a very low-priority patch but since commit bb996692bd9 "RISC-V/gas:
allow generating up to 176-bit instructions with .insn" can break
validate_riscv_insn function (by negative shift width), it'a good chance to
improve this function entirely.


[Changes: v2 -> v3]

-   PATCH 1/2: Revised the function description to reflect
    the specification.
-   PATCH 2/2: Started using "%#llx" instead of "0x%llx".
    In this occurrence, it will not change the behavior.

Both changes are based on the feedback from Jan Beulich.



This is mostly a quote from the cover letter of PATCH v1 but the words
"PATCH 1/2" are replaced with "PATCH 2/2" to reflect PATCH v2 and v3.


> First -- just to experiment -- we change mask value of "fcvt.d.s"
> instruction from MASK_FCVT_D_S|MASK_RM to MASK_FCVT_D_S while not touching
> operands "D,S" and we run compiled assembler, we get following message:
>
>     Assembler messages:
>     Error: internal: bad RISC-V opcode (bits 0xffffffff00007000 undefined): fcvt.d.s D,S
>     Fatal error: internal: broken assembler.  No assembly attempted
>
> Bits 0x7000 corresponds to rm (rounding mode) bits we just removed and no
> corresponding operands are found (making definition of fcvt.d.s instruction
> invalid).
>
> Then, what about 0xffffffff00000000 (upper 32-bits)?
> Yes, they are non-instruction bits.  Because of ~ (bitwise complement)
> operator while computing undefined bits, it also displays non-instruction
> bits.
>
>
> This patchset (PATCH 2/2) changes how undefined/invalid bits are computed.
>
> before:
>     ~(used & required)
> after:
>     (used ^ required)
>
> After PATCH 2/2, following error message is generated.
>
>     Assembler messages:
>     Error: internal: bad RISC-V opcode (bits 0x7000 undefined or invalid): fcvt.d.s D,S
>     Fatal error: internal: broken assembler.  No assembly attempted
>
>
> Note that we are testing for "undefined or invalid" bits here, not just
> undefined bits.  In fact, if we corrupt a variant of c.addi instruction with
> ADDITIONAL "j" operand (which is an immediate for I-type instruction, upper
> 12-bits of **32-bit** instruction encoding), we get following message:
>
>     Assembler messages:
>     Error: internal: bad RISC-V opcode (bits 0xfff00000 undefined or invalid): addi d,CU,Cj,j
>     Fatal error: internal: broken assembler.  No assembly attempted
>
> Okay, extra "j" operand generates "extra" bits (that should not have been
> defined considering its 16-bit encoding) and words "undefined or invalid"
> are working here.  We are correctly capturing invalid extra bits.  Before
> this patch, invalid bits are hidden by 0xffffffffffff0000 (48 non-
> instruction bits).


Thanks,
Tsukasa




Tsukasa OI (2):
  RISC-V: Fallback for instructions longer than 64b
  RISC-V: Improve "bits undefined" diagnostics

 gas/config/tc-riscv.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)


base-commit: 80e0c6dc91f52fad32c3ff3cf20da889d77013ac
-- 
2.34.1

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

[Tsukasa OI]

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 spec.  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 | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/gas/config/tc-riscv.c b/gas/config/tc-riscv.c
index 22385d1baa0..41d6dfc6062 100644
--- a/gas/config/tc-riscv.c
+++ b/gas/config/tc-riscv.c
@@ -1109,7 +1109,8 @@ 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 the actual instruction length or
+   0 for auto-detection.  */
 
 static bool
 validate_riscv_insn (const struct riscv_opcode *opc, int length)
@@ -1120,11 +1121,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))
     {
-- 
2.34.1

[PATCH v3 2/2] RISC-V: Improve "bits undefined" diagnostics

[Tsukasa OI]

This commit improves internal error message
"internal: bad RISC-V opcode (bits 0x%lx undefined): %s %s"
to display actual unused bits (excluding non-instruction bits).

gas/ChangeLog:

	* config/tc-riscv.c (validate_riscv_insn): Exclude non-
	instruction bits from displaying internal diagnostics.
	Change error message slightly.
---
 gas/config/tc-riscv.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/gas/config/tc-riscv.c b/gas/config/tc-riscv.c
index 41d6dfc6062..dacfe0f1d25 100644
--- a/gas/config/tc-riscv.c
+++ b/gas/config/tc-riscv.c
@@ -1313,8 +1313,8 @@ validate_riscv_insn (const struct riscv_opcode *opc, int length)
   if (used_bits != required_bits)
     {
       as_bad (_("internal: bad RISC-V opcode "
-		"(bits 0x%lx undefined): %s %s"),
-	      ~(unsigned long)(used_bits & required_bits),
+		"(bits %#llx undefined or invalid): %s %s"),
+	      (unsigned long long)(used_bits ^ required_bits),
 	      opc->name, opc->args);
       return false;
     }
-- 
2.34.1

Re: [PATCH v3 1/2] RISC-V: Fallback for instructions longer than 64b

[Nelson Chu]

In fact we don't really need this change, since so far the parameter
length of validate_riscv_insn will only be 2 and 4,
https://github.com/bminor/binutils-gdb/blob/master/gas/config/tc-riscv.c#L1349

Nelson

On Thu, Oct 6, 2022 at 5:56 PM Tsukasa OI <research_trasio@irq.a4lg.com> 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 spec.  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 | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/gas/config/tc-riscv.c b/gas/config/tc-riscv.c
> index 22385d1baa0..41d6dfc6062 100644
> --- a/gas/config/tc-riscv.c
> +++ b/gas/config/tc-riscv.c
> @@ -1109,7 +1109,8 @@ 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 the actual instruction length or
> +   0 for auto-detection.  */
>
>  static bool
>  validate_riscv_insn (const struct riscv_opcode *opc, int length)
> @@ -1120,11 +1121,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))
>      {
> --
> 2.34.1
>

Re: [PATCH v3 1/2] RISC-V: Fallback for instructions longer than 64b

[Jan Beulich]

On 14.10.2022 03:32, Nelson Chu wrote:
> In fact we don't really need this change, since so far the parameter
> length of validate_riscv_insn will only be 2 and 4,
> https://github.com/bminor/binutils-gdb/blob/master/gas/config/tc-riscv.c#L1349

Hmm, it is clearly said ...

> On Thu, Oct 6, 2022 at 5:56 PM Tsukasa OI <research_trasio@irq.a4lg.com> 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).

... here that the change is to avoid surprises going forward.

Jan

>> gas/ChangeLog:
>>
>>         * config/tc-riscv.c (validate_riscv_insn): Fix function
>>         description comment based on current spec.  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 | 13 ++++++++-----
>>  1 file changed, 8 insertions(+), 5 deletions(-)
>>
>> diff --git a/gas/config/tc-riscv.c b/gas/config/tc-riscv.c
>> index 22385d1baa0..41d6dfc6062 100644
>> --- a/gas/config/tc-riscv.c
>> +++ b/gas/config/tc-riscv.c
>> @@ -1109,7 +1109,8 @@ 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 the actual instruction length or
>> +   0 for auto-detection.  */
>>
>>  static bool
>>  validate_riscv_insn (const struct riscv_opcode *opc, int length)
>> @@ -1120,11 +1121,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))
>>      {
>> --
>> 2.34.1
>>

Re: [PATCH v3 1/2] RISC-V: Fallback for instructions longer than 64b

[Tsukasa OI]

On 2022/10/14 10:32, Nelson Chu wrote:
> In fact we don't really need this change, since so far the parameter
> length of validate_riscv_insn will only be 2 and 4,
> https://github.com/bminor/binutils-gdb/blob/master/gas/config/tc-riscv.c#L134
The argument "length" of validate_riscv_insn will be 2, 4 OR 0 but this
is not the point.

I have to agree that this change alone will not change the behavior.
Still, I don't want to surprise future developers with "instruction
length support" status (as Jan added).  To note, commit message of PATCH
1/2 is an important part of this patchset.

Could you reconsider this patchset again?

Thanks,
Tsukasa

> 
> Nelson
> 
> On Thu, Oct 6, 2022 at 5:56 PM Tsukasa OI <research_trasio@irq.a4lg.com> 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 spec.  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 | 13 ++++++++-----
>>  1 file changed, 8 insertions(+), 5 deletions(-)
>>
>> diff --git a/gas/config/tc-riscv.c b/gas/config/tc-riscv.c
>> index 22385d1baa0..41d6dfc6062 100644
>> --- a/gas/config/tc-riscv.c
>> +++ b/gas/config/tc-riscv.c
>> @@ -1109,7 +1109,8 @@ 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 the actual instruction length or
>> +   0 for auto-detection.  */
>>
>>  static bool
>>  validate_riscv_insn (const struct riscv_opcode *opc, int length)
>> @@ -1120,11 +1121,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))
>>      {
>> --
>> 2.34.1
>>
> 

Re: [PATCH v3 1/2] RISC-V: Fallback for instructions longer than 64b

[Nelson Chu]

On Sun, Oct 16, 2022 at 9:32 PM Tsukasa OI <research_trasio@irq.a4lg.com> wrote:
>
> On 2022/10/14 10:32, Nelson Chu wrote:
> > In fact we don't really need this change, since so far the parameter
> > length of validate_riscv_insn will only be 2 and 4,
> > https://github.com/bminor/binutils-gdb/blob/master/gas/config/tc-riscv.c#L134
> The argument "length" of validate_riscv_insn will be 2, 4 OR 0 but this
> is not the point.
>
> I have to agree that this change alone will not change the behavior.
> Still, I don't want to surprise future developers with "instruction
> length support" status (as Jan added).  To note, commit message of PATCH
> 1/2 is an important part of this patchset.
>
> Could you reconsider this patchset again?

OK, please commit.

Thanks
Nelson

> Thanks,
> Tsukasa
>
> >
> > Nelson
> >
> > On Thu, Oct 6, 2022 at 5:56 PM Tsukasa OI <research_trasio@irq.a4lg.com> 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 spec.  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 | 13 ++++++++-----
> >>  1 file changed, 8 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/gas/config/tc-riscv.c b/gas/config/tc-riscv.c
> >> index 22385d1baa0..41d6dfc6062 100644
> >> --- a/gas/config/tc-riscv.c
> >> +++ b/gas/config/tc-riscv.c
> >> @@ -1109,7 +1109,8 @@ 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 the actual instruction length or
> >> +   0 for auto-detection.  */
> >>
> >>  static bool
> >>  validate_riscv_insn (const struct riscv_opcode *opc, int length)
> >> @@ -1120,11 +1121,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))
> >>      {
> >> --
> >> 2.34.1
> >>
> >