[PATCH] RISC-V: Fix opcode entries of "vmsge{,u}.vx"

[PATCH] RISC-V: Fix opcode entries of "vmsge{,u}.vx"

[Tsukasa OI]

From: Tsukasa OI <research_trasio@irq.a4lg.com>

Their check_func should be "match_never", not "match_opcode".  The reasons
this error did not cause any disassembler errors are:

1.  The problem will not reproduce if "no-aliases" is specified
    (because macro instructions are handled as aliases).
2.  If not, all affected compressed instructions or their aliases
    precede before "vmsge{,u}.vx" macro instructions.

However, it'll easily break if we reorder opcode entries.  This commit
fixes this issue before the *accident* occurs.

opcodes/ChangeLog:

	* riscv-opc.c (riscv_opcodes): Make sure that we never match to
	vmsge{,u}.vx instructions unless specified in the assembler.
---
 opcodes/riscv-opc.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/opcodes/riscv-opc.c b/opcodes/riscv-opc.c
index 6a854736fec0..37c7694999ad 100644
--- a/opcodes/riscv-opc.c
+++ b/opcodes/riscv-opc.c
@@ -1606,10 +1606,10 @@ const struct riscv_opcode riscv_opcodes[] =
 {"vmsgeu.vi",  0, INSN_CLASS_V,  "Vd,Vu,0Vm", MATCH_VMSEQVV, MASK_VMSEQVV, match_vs1_eq_vs2, INSN_ALIAS },
 {"vmsgeu.vi",  0, INSN_CLASS_V,  "Vd,Vt,VkVm", MATCH_VMSGTUVI, MASK_VMSGTUVI, match_opcode, INSN_ALIAS },
 
-{"vmsge.vx",   0, INSN_CLASS_V, "Vd,Vt,sVm", 0, (int) M_VMSGE, match_opcode, INSN_MACRO },
-{"vmsge.vx",   0, INSN_CLASS_V, "Vd,Vt,s,VM,VT", 0, (int) M_VMSGE, match_opcode, INSN_MACRO },
-{"vmsgeu.vx",  0, INSN_CLASS_V, "Vd,Vt,sVm", 0, (int) M_VMSGEU, match_opcode, INSN_MACRO },
-{"vmsgeu.vx",  0, INSN_CLASS_V, "Vd,Vt,s,VM,VT", 0, (int) M_VMSGEU, match_opcode, INSN_MACRO },
+{"vmsge.vx",   0, INSN_CLASS_V, "Vd,Vt,sVm", 0, (int) M_VMSGE, match_never, INSN_MACRO },
+{"vmsge.vx",   0, INSN_CLASS_V, "Vd,Vt,s,VM,VT", 0, (int) M_VMSGE, match_never, INSN_MACRO },
+{"vmsgeu.vx",  0, INSN_CLASS_V, "Vd,Vt,sVm", 0, (int) M_VMSGEU, match_never, INSN_MACRO },
+{"vmsgeu.vx",  0, INSN_CLASS_V, "Vd,Vt,s,VM,VT", 0, (int) M_VMSGEU, match_never, INSN_MACRO },
 
 {"vminu.vv",   0, INSN_CLASS_V,  "Vd,Vt,VsVm", MATCH_VMINUVV, MASK_VMINUVV, match_opcode, 0},
 {"vminu.vx",   0, INSN_CLASS_V,  "Vd,Vt,sVm", MATCH_VMINUVX, MASK_VMINUVX, match_opcode, 0},

base-commit: 5e66f55c62e306afbcc93856bf06e542ddd00997
-- 
2.41.0

Re: [PATCH] RISC-V: Fix opcode entries of "vmsge{,u}.vx"

[Tsukasa OI]

A little more about how important this fix is.

vmsge{,u}.vx matches wide range of compressed instructions and possibly
affect wide range of compressed extensions.

-   xxxx xxxx xxx0 00x0 (vmsge.vx;  M_VMSGE  == 29)
-   xxxx xxxx xxx0 000x (vmsgeu.vx; M_VMSGEU == 30)

The reason why we don't see any wrong disassembly is detailed in the
patch message.  And I found that the 'Zcmp' extension patch (a work by
Jiawei) actually breaks the disassembler.  This is not Jiawei's fault
but the consequence of this bug (I want to fix).

How to reproduce:

1.  Apply and optionally fix Jiawei's Zcmp patchset
    (applying only push/pop patch is sufficient to reproduce)
    <https://sourceware.org/pipermail/binutils/2023-July/128626.html>
2.  Assemble "cm.push {ra}, -16"
    with -march=rv32if_zcmp_zve32f
3.  Disassemble the output
    and we see "vmsge.vx v16,v0,ra,v0.t", not "cm.push {ra}, -16".

This is because "cm.push {ra}, -16" mismatches as follows:

-   1011 1000 0100 0010 (cm.push {ra}, -16)
-   xxxx xxxx xxx0 00x0 (wrong matching pattern of vmsge.vx)

Note that "cm.push" entry on riscv_opcodes is placed after "vmsge.vx".
I don't recommend Jiawei to move "cm.push" riscv_opcode entry because
there's no good reason to do it.

I recognize that fixing this bug is practically a requirement of
adopting the 'Zcmp' / 'Zcmt' extensions (not absolute requirement
though) and hope that this bug fix patch is reviewed soon.

Sincerely,
Tsukasa

On 2023/08/06 10:53, Tsukasa OI wrote:
> From: Tsukasa OI <research_trasio@irq.a4lg.com>
> 
> Their check_func should be "match_never", not "match_opcode".  The reasons
> this error did not cause any disassembler errors are:
> 
> 1.  The problem will not reproduce if "no-aliases" is specified
>     (because macro instructions are handled as aliases).
> 2.  If not, all affected compressed instructions or their aliases
>     precede before "vmsge{,u}.vx" macro instructions.
> 
> However, it'll easily break if we reorder opcode entries.  This commit
> fixes this issue before the *accident* occurs.
> 
> opcodes/ChangeLog:
> 
> 	* riscv-opc.c (riscv_opcodes): Make sure that we never match to
> 	vmsge{,u}.vx instructions unless specified in the assembler.
> ---
>  opcodes/riscv-opc.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/opcodes/riscv-opc.c b/opcodes/riscv-opc.c
> index 6a854736fec0..37c7694999ad 100644
> --- a/opcodes/riscv-opc.c
> +++ b/opcodes/riscv-opc.c
> @@ -1606,10 +1606,10 @@ const struct riscv_opcode riscv_opcodes[] =
>  {"vmsgeu.vi",  0, INSN_CLASS_V,  "Vd,Vu,0Vm", MATCH_VMSEQVV, MASK_VMSEQVV, match_vs1_eq_vs2, INSN_ALIAS },
>  {"vmsgeu.vi",  0, INSN_CLASS_V,  "Vd,Vt,VkVm", MATCH_VMSGTUVI, MASK_VMSGTUVI, match_opcode, INSN_ALIAS },
>  
> -{"vmsge.vx",   0, INSN_CLASS_V, "Vd,Vt,sVm", 0, (int) M_VMSGE, match_opcode, INSN_MACRO },
> -{"vmsge.vx",   0, INSN_CLASS_V, "Vd,Vt,s,VM,VT", 0, (int) M_VMSGE, match_opcode, INSN_MACRO },
> -{"vmsgeu.vx",  0, INSN_CLASS_V, "Vd,Vt,sVm", 0, (int) M_VMSGEU, match_opcode, INSN_MACRO },
> -{"vmsgeu.vx",  0, INSN_CLASS_V, "Vd,Vt,s,VM,VT", 0, (int) M_VMSGEU, match_opcode, INSN_MACRO },
> +{"vmsge.vx",   0, INSN_CLASS_V, "Vd,Vt,sVm", 0, (int) M_VMSGE, match_never, INSN_MACRO },
> +{"vmsge.vx",   0, INSN_CLASS_V, "Vd,Vt,s,VM,VT", 0, (int) M_VMSGE, match_never, INSN_MACRO },
> +{"vmsgeu.vx",  0, INSN_CLASS_V, "Vd,Vt,sVm", 0, (int) M_VMSGEU, match_never, INSN_MACRO },
> +{"vmsgeu.vx",  0, INSN_CLASS_V, "Vd,Vt,s,VM,VT", 0, (int) M_VMSGEU, match_never, INSN_MACRO },
>  
>  {"vminu.vv",   0, INSN_CLASS_V,  "Vd,Vt,VsVm", MATCH_VMINUVV, MASK_VMINUVV, match_opcode, 0},
>  {"vminu.vx",   0, INSN_CLASS_V,  "Vd,Vt,sVm", MATCH_VMINUVX, MASK_VMINUVX, match_opcode, 0},
> 
> base-commit: 5e66f55c62e306afbcc93856bf06e542ddd00997

Re: [PATCH] RISC-V: Fix opcode entries of "vmsge{,u}.vx"

[Nelson Chu]

[-- Attachment #1: Type: text/plain, Size: 2621 bytes --]

Okay, in general INSN_MACRO should use match_never rather than match_opcode
and others, so looks correct and good to me.

Thanks
Nelson

On Sun, Aug 6, 2023 at 9:53 AM Tsukasa OI <research_trasio@irq.a4lg.com>
wrote:

> From: Tsukasa OI <research_trasio@irq.a4lg.com>
>
> Their check_func should be "match_never", not "match_opcode".  The reasons
> this error did not cause any disassembler errors are:
>
> 1.  The problem will not reproduce if "no-aliases" is specified
>     (because macro instructions are handled as aliases).
> 2.  If not, all affected compressed instructions or their aliases
>     precede before "vmsge{,u}.vx" macro instructions.
>
> However, it'll easily break if we reorder opcode entries.  This commit
> fixes this issue before the *accident* occurs.
>
> opcodes/ChangeLog:
>
>         * riscv-opc.c (riscv_opcodes): Make sure that we never match to
>         vmsge{,u}.vx instructions unless specified in the assembler.
> ---
>  opcodes/riscv-opc.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/opcodes/riscv-opc.c b/opcodes/riscv-opc.c
> index 6a854736fec0..37c7694999ad 100644
> --- a/opcodes/riscv-opc.c
> +++ b/opcodes/riscv-opc.c
> @@ -1606,10 +1606,10 @@ const struct riscv_opcode riscv_opcodes[] =
>  {"vmsgeu.vi",  0, INSN_CLASS_V,  "Vd,Vu,0Vm", MATCH_VMSEQVV,
> MASK_VMSEQVV, match_vs1_eq_vs2, INSN_ALIAS },
>  {"vmsgeu.vi",  0, INSN_CLASS_V,  "Vd,Vt,VkVm", MATCH_VMSGTUVI,
> MASK_VMSGTUVI, match_opcode, INSN_ALIAS },
>
> -{"vmsge.vx",   0, INSN_CLASS_V, "Vd,Vt,sVm", 0, (int) M_VMSGE,
> match_opcode, INSN_MACRO },
> -{"vmsge.vx",   0, INSN_CLASS_V, "Vd,Vt,s,VM,VT", 0, (int) M_VMSGE,
> match_opcode, INSN_MACRO },
> -{"vmsgeu.vx",  0, INSN_CLASS_V, "Vd,Vt,sVm", 0, (int) M_VMSGEU,
> match_opcode, INSN_MACRO },
> -{"vmsgeu.vx",  0, INSN_CLASS_V, "Vd,Vt,s,VM,VT", 0, (int) M_VMSGEU,
> match_opcode, INSN_MACRO },
> +{"vmsge.vx",   0, INSN_CLASS_V, "Vd,Vt,sVm", 0, (int) M_VMSGE,
> match_never, INSN_MACRO },
> +{"vmsge.vx",   0, INSN_CLASS_V, "Vd,Vt,s,VM,VT", 0, (int) M_VMSGE,
> match_never, INSN_MACRO },
> +{"vmsgeu.vx",  0, INSN_CLASS_V, "Vd,Vt,sVm", 0, (int) M_VMSGEU,
> match_never, INSN_MACRO },
> +{"vmsgeu.vx",  0, INSN_CLASS_V, "Vd,Vt,s,VM,VT", 0, (int) M_VMSGEU,
> match_never, INSN_MACRO },
>
>  {"vminu.vv",   0, INSN_CLASS_V,  "Vd,Vt,VsVm", MATCH_VMINUVV,
> MASK_VMINUVV, match_opcode, 0},
>  {"vminu.vx",   0, INSN_CLASS_V,  "Vd,Vt,sVm", MATCH_VMINUVX,
> MASK_VMINUVX, match_opcode, 0},
>
> base-commit: 5e66f55c62e306afbcc93856bf06e542ddd00997
> --
> 2.41.0
>
>