diff mbox series

[v5,1/2,sim/riscv] Fix crash during instruction decoding

Message ID 20231222052658.2102802-2-jaydeep.patil@imgtec.com
State New
Headers
Series [v5,1/2,sim/riscv] Fix crash during instruction decoding |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_gdb_build--master-arm success Testing passed
linaro-tcwg-bot/tcwg_gdb_check--master-arm success Testing passed
linaro-tcwg-bot/tcwg_gdb_check--master-aarch64 fail Testing failed

Commit Message

Jaydeep Patil Dec. 22, 2023, 5:26 a.m. UTC 
  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(-)

Comments

Andrew Burgess Jan. 10, 2024, 10:22 a.m. UTC | #1 
<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
Jaydeep Patil Jan. 10, 2024, 11:34 a.m. UTC | #2 
<aburgess@redhat.com> writes:

>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?
>

Looking at the riscv_opcodes[] table in opcodes/riscv-opc.c, we don't need to modify opcodes/riscv-dis.c.
The match_never() has been replaced with NULL for INSN_MACRO types. Thus disassembler will never segfault because of following code:

@@ -818,7 +818,7 @@ riscv_disassemble_insn (bfd_vma memaddr,
	  if (op->pinfo == INSN_MACRO)
	    continue;

The segfault happens only in step_once() of sim/riscv/sim-main.c.

Regards,
Jaydeep
diff mbox series

Patch

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