diff mbox series

RISC-V: Dump instruction without checking architecture support as usual.

Message ID 20231027003917.67308-1-nelson@rivosinc.com
State Accepted
Headers
Series RISC-V: Dump instruction without checking architecture support as usual. |

Checks

Context Check Description
snail/binutils-gdb-check success Github commit url

Commit Message

Nelson Chu Oct. 27, 2023, 12:39 a.m. UTC 
  Since QEMU have supported -Max option to to enable all normal extensions,
the dis-assembler should also add an option, -M,max to do the same thing.
For the instruction, which have overlapped encodings like zfinx, will not
be considered by the -M,max option.

opcodes/
	* riscv-dis.c (all_ext): New static boolean.  If set, disassemble
	without checking architectire string.
	(riscv_disassemble_insn): Likewise.
	(parse_riscv_dis_option_without_args): Recognized -M,max option.
---
 opcodes/riscv-dis.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

Comments

Tsukasa OI Oct. 27, 2023, 2:17 a.m. UTC | #1 
On 2023/10/27 9:39, Nelson Chu wrote:
> Since QEMU have supported -Max option to to enable all normal extensions,
> the dis-assembler should also add an option, -M,max to do the same thing.
> For the instruction, which have overlapped encodings like zfinx, will not
> be considered by the -M,max option.
> 
> opcodes/
> 	* riscv-dis.c (all_ext): New static boolean.  If set, disassemble
> 	without checking architectire string.
> 	(riscv_disassemble_insn): Likewise.
> 	(parse_riscv_dis_option_without_args): Recognized -M,max option.
> ---
>  opcodes/riscv-dis.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)

LGTM.

For overlapped encodings, riscv_opcodes ordering will get more important
but I don't think this change will "break" anything ("max" should be
used only when the specific architecture IS NOT important).

I remember I submitted a proposal (long time ago) to add a disassembler
option to specify custom ISA string for the disassembler (where specific
architecture IS important) and it might be the time to rework on this
(because "max" and my past proposal "arch=ARCH" would work as a pair).

Tsukasa


p.s.

Also looking at your patch, I started to think disassembler options that
would visualize custom instructions (or custom instruction/CSR space)
might be helpful for some reverse engineering needs (the priority is
low, even for me though).
Nelson Chu Oct. 27, 2023, 2:55 a.m. UTC | #2 
On Fri, Oct 27, 2023 at 10:17 AM Tsukasa OI <research_trasio@irq.a4lg.com>
wrote:

> On 2023/10/27 9:39, Nelson Chu wrote:
> > Since QEMU have supported -Max option to to enable all normal extensions,
> > the dis-assembler should also add an option, -M,max to do the same thing.
> > For the instruction, which have overlapped encodings like zfinx, will not
> > be considered by the -M,max option.
> >
> > opcodes/
> >       * riscv-dis.c (all_ext): New static boolean.  If set, disassemble
> >       without checking architectire string.
> >       (riscv_disassemble_insn): Likewise.
> >       (parse_riscv_dis_option_without_args): Recognized -M,max option.
> > ---
> >  opcodes/riscv-dis.c | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
>
> LGTM.
>
> For overlapped encodings, riscv_opcodes ordering will get more important
> but I don't think this change will "break" anything ("max" should be
> used only when the specific architecture IS NOT important).
>
> I remember I submitted a proposal (long time ago) to add a disassembler
> option to specify custom ISA string for the disassembler (where specific
> architecture IS important) and it might be the time to rework on this
> (because "max" and my past proposal "arch=ARCH" would work as a pair).
>

 Yeah probably it's time to get thi, too.  Since it gives more chances for
users to choose what they want to dump, although they may dump the wrong
results, that's their choice ;)

Thanks
Nelson
diff mbox series

Patch

diff --git a/opcodes/riscv-dis.c b/opcodes/riscv-dis.c
index 216916e9426..6a910595f6d 100644
--- a/opcodes/riscv-dis.c
+++ b/opcodes/riscv-dis.c
@@ -75,6 +75,9 @@  static const char (*riscv_fpr_names)[NRC];
 /* If set, disassemble as most general instruction.  */
 static bool no_aliases = false;
 
+/* If set, disassemble without checking architectire string, just like what
+   we did at the beginning.  */
+static bool all_ext = false;
 
 /* Set default RISC-V disassembler options.  */
 
@@ -98,6 +101,8 @@  parse_riscv_dis_option_without_args (const char *option)
       riscv_gpr_names = riscv_gpr_names_numeric;
       riscv_fpr_names = riscv_fpr_names_numeric;
     }
+  else if (strcmp (option, "max") == 0)
+    all_ext = true;
   else
     return false;
   return true;
@@ -772,7 +777,8 @@  riscv_disassemble_insn (bfd_vma memaddr,
 	  if ((op->xlen_requirement != 0) && (op->xlen_requirement != xlen))
 	    continue;
 	  /* Is this instruction supported by the current architecture?  */
-	  if (!riscv_multi_subset_supports (&riscv_rps_dis, op->insn_class))
+	  if (!all_ext
+	      && !riscv_multi_subset_supports (&riscv_rps_dis, op->insn_class))
 	    continue;
 
 	  /* It's a match.  */