diff mbox series

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

Message ID d52952119e15357c0e823f8a2398999359588b4d.1665050099.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, 9:56 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 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(-)

Comments

Nelson Chu Oct. 14, 2022, 1:32 a.m. UTC | #1 
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
>
Jan Beulich Oct. 14, 2022, 7:07 a.m. UTC | #2 
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
>>
Tsukasa OI Oct. 16, 2022, 1:32 p.m. UTC | #3 
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
>>
>
Nelson Chu Oct. 28, 2022, 9:41 a.m. UTC | #4 
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
> >>
> >
diff mbox series

Patch

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