Re: [PATCH v5 1/2] [sim/riscv] Fix crash during instruction decoding

[Andrew Burgess <aburgess@redhat.com>]

<jaydeep.patil@imgtec.com> writes:

> From: Jaydeep Patil <jaydeep.patil@imgtec.com>
>
> The match_never() function has been removed and thus step_once() crashes
> during instruction decoding. Fixed it by checking for null pointer before
> invoking function attached to match_func member of riscv_opcode structure.
> ---
>  opcodes/riscv-dis.c  | 2 +-
>  sim/riscv/sim-main.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/opcodes/riscv-dis.c b/opcodes/riscv-dis.c
> index 68674380797..a89ebdd32ac 100644
> --- a/opcodes/riscv-dis.c
> +++ b/opcodes/riscv-dis.c
> @@ -818,7 +818,7 @@ riscv_disassemble_insn (bfd_vma memaddr,
>  	  if (op->pinfo == INSN_MACRO)
>  	    continue;
>  	  /* Does the opcode match?  */
> -	  if (! (op->match_func) (op, word))
> +	  if (! op->match_func || ! (op->match_func) (op, word))
>  	    continue;
>  	  /* Is this a pseudo-instruction and may we print it as such?  */
>  	  if (no_aliases && (op->pinfo & INSN_ALIAS))

Sorry to be a pain, but changes in opcodes/ need to go through the
binutils@sourceware.org mailing list.

I took a little dive into the history of the match_never() removal, and
found these two commits:

  commit 2ec31e54dff83130fbde8d2f674469078ee203d5
  Date:   Fri Nov 24 10:15:59 2023 +0100
  
      RISC-V: drop leftover match_never() references

And maybe more interesting:

  commit 27b33966b18ed8bf1701a60999448224b1d28273
  Date:   Fri Nov 24 09:53:15 2023 +0100
  
      RISC-V: disallow x0 with certain macro-insns

This second commit talks about treating a NULL as actually meaning
match_always.  I've not dug into this beyond looking at those commits,
but that second commit does include some code similar to yours, except
in that case they've gone with something like:

  if (op->match_func && !(op->match_func) (op, word))
    continue;

I'd like to see a commit message that references this history, and
explains why this commit does something different.

Also, does your change indicate that there exists an instruction
encoding which, if we try to disassemble it, will cause the disassembler
to segfault?  That would be a good candidate for making into a test,
maybe in the gas testsuite?

Thanks,
Andrew



> diff --git a/sim/riscv/sim-main.c b/sim/riscv/sim-main.c
> index 4d205345395..65c0ea245b2 100644
> --- a/sim/riscv/sim-main.c
> +++ b/sim/riscv/sim-main.c
> @@ -1042,7 +1042,7 @@ void step_once (SIM_CPU *cpu)
>    for (; op->name; op++)
>      {
>        /* Does the opcode match?  */
> -      if (! op->match_func (op, iw))
> +      if (! op->match_func || ! op->match_func (op, iw))
>  	continue;
>        /* Is this a pseudo-instruction and may we print it as such?  */
>        if (op->pinfo & INSN_ALIAS)
> -- 
> 2.25.1