[PATCH] RISC-V: make .insn actually work for 64-bit insns

[PATCH] RISC-V: make .insn actually work for 64-bit insns

[Jan Beulich]

Presently in this case, due to an undefined behavior shift, at least
with x86 cross builds I'm observing:

Error: value conflicts with instruction length `8,0x0000003f'

Eliminate the UB and extend the respective testcase.

--- a/gas/config/tc-riscv.c
+++ b/gas/config/tc-riscv.c
@@ -3248,7 +3248,7 @@ riscv_ip_hardcode (char *str,
   insn->match = values[num - 1];
   create_insn (ip, insn);
   unsigned int bytes = riscv_insn_length (insn->match);
-  if (values[num - 1] >> (8 * bytes) != 0
+  if ((bytes < sizeof(values[0]) && values[num - 1] >> (8 * bytes) != 0)
       || (num == 2 && values[0] != bytes))
     return _("value conflicts with instruction length");
 
--- a/gas/testsuite/gas/riscv/insn.d
+++ b/gas/testsuite/gas/riscv/insn.d
@@ -71,5 +71,9 @@ Disassembly of section .text:
 [^:]+:[ 	]+00c58533[ 	]+add[ 	]+a0,a1,a2
 [^:]+:[ 	]+0001[ 	]+nop
 [^:]+:[ 	]+00000013[ 	]+nop
+[^:]+:[ 	]+001f 0000 0000[ 	].*
+[^:]+:[ 	]+0000003f 00000000[ 	].*
 [^:]+:[ 	]+0001[ 	]+nop
 [^:]+:[ 	]+00000013[ 	]+nop
+[^:]+:[ 	]+001f 0000 0000[ 	].*
+[^:]+:[ 	]+0000003f 00000000[ 	].*
--- a/gas/testsuite/gas/riscv/insn.s
+++ b/gas/testsuite/gas/riscv/insn.s
@@ -56,5 +56,9 @@ target:
 
 	.insn 0x0001
 	.insn 0x00000013
+	.insn 0x0000001f
+	.insn 0x0000003f
 	.insn 0x2, 0x0001
 	.insn 0x4, 0x00000013
+	.insn 6, 0x0000001f
+	.insn 8, 0x0000003f

Re: [PATCH] RISC-V: make .insn actually work for 64-bit insns

[Nelson Chu]

On Fri, Feb 25, 2022 at 7:06 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> Presently in this case, due to an undefined behavior shift, at least
> with x86 cross builds I'm observing:
>
> Error: value conflicts with instruction length `8,0x0000003f'
>
> Eliminate the UB and extend the respective testcase.
>
> --- a/gas/config/tc-riscv.c
> +++ b/gas/config/tc-riscv.c
> @@ -3248,7 +3248,7 @@ riscv_ip_hardcode (char *str,
>    insn->match = values[num - 1];
>    create_insn (ip, insn);
>    unsigned int bytes = riscv_insn_length (insn->match);
> -  if (values[num - 1] >> (8 * bytes) != 0
> +  if ((bytes < sizeof(values[0]) && values[num - 1] >> (8 * bytes) != 0)
>        || (num == 2 && values[0] != bytes))
>      return _("value conflicts with instruction length");

I only considered 2/4 bytes instructions before, but forgot to think
that insn_t (uint_64) shifts 64 bit will be the original value (not
equal zero), so the previous check was dangerous.  We probably need to
consider the case that instruction length will be more than 64-bit in
the future, but now your fix should be enough and good to me.  Please
commit the patch when you think it's time.

Thanks
Nelson

> --- a/gas/testsuite/gas/riscv/insn.d
> +++ b/gas/testsuite/gas/riscv/insn.d
> @@ -71,5 +71,9 @@ Disassembly of section .text:
>  [^:]+:[        ]+00c58533[     ]+add[  ]+a0,a1,a2
>  [^:]+:[        ]+0001[         ]+nop
>  [^:]+:[        ]+00000013[     ]+nop
> +[^:]+:[        ]+001f 0000 0000[       ].*
> +[^:]+:[        ]+0000003f 00000000[    ].*
>  [^:]+:[        ]+0001[         ]+nop
>  [^:]+:[        ]+00000013[     ]+nop
> +[^:]+:[        ]+001f 0000 0000[       ].*
> +[^:]+:[        ]+0000003f 00000000[    ].*
> --- a/gas/testsuite/gas/riscv/insn.s
> +++ b/gas/testsuite/gas/riscv/insn.s
> @@ -56,5 +56,9 @@ target:
>
>         .insn 0x0001
>         .insn 0x00000013
> +       .insn 0x0000001f
> +       .insn 0x0000003f
>         .insn 0x2, 0x0001
>         .insn 0x4, 0x00000013
> +       .insn 6, 0x0000001f
> +       .insn 8, 0x0000003f
>