[PATCH 00/11] RISC-V: Requirements for disassembler optimizations batch 1

[PATCH 00/11] RISC-V: Requirements for disassembler optimizations batch 1

[Tsukasa OI]

Hello,

To improve the core disassembler (both for performance and feature), this
patchset now prepares for further optimization.

It will be a prerequisite of upcoming core disassembler changes including
two major optimizations.

Tracker on GitHub (with detailed benchmark results):
<https://github.com/a4lg/binutils-gdb/wiki/riscv_dis_opt1_req_reorganize>

This is the part 2 of 4-part project:
<https://github.com/a4lg/binutils-gdb/wiki/proj_dis_perf_improvements_1>


Individual changes are detailed in each patch.
In this cover letter, I will explain...

-   Aggregate Performance Improvements
-   Some Backgrounds behind some patches


1.  Aggregate Performance Improvements
2.  Split Match/Print Steps on the Disassembler (PATCH 8/11)
3.  State Initialization (PATCH 9/11)
    3.1. Disassembler Lifecycle
    3.2. Moving the Initialization
    3.3. Fixed Issues as a Side Effect
    3.4. Fixed Issue 1: Unsetting Options on GDB (and new smaller issue)
    3.5. Fixed Issue 2: Location of Error Message
4.  Architecture Initialization and Management (PATCH 10/11)



Aggregate Performance Improvements
===================================

Since commit 40f1a1a4564b ("RISC-V: Output mapping symbols with ISA string."
), the RISC-V disassembler was slower than before.  I'm not blaming this
because it has a good reason to be changed (to support mapping symbols with
ISA string).

Despite that this patchset itself is not main optimization part, it roughly
restores the disassembler performance before that commit (if the input ELF
file contains mapping symbols with ISA string, this patchset makes
disassembling such files significantly faster).

Note that, this patchset contains two "optimization" commits that
significantly (>30%) improves the performance under certain conditions:

-   When thousands of CSR instructions are being disassembled
    (PATCH 5/11)
-   When disassembling large chunk of code using GDB (not objdump)
    (PATCH 10/11)
-   When the input has mapping symbols with ISA string (both on GDB/objdump)
    (PATCH 10/11)



Split Match/Print Steps on the Disassembler (PATCH 8/11)
=========================================================

This is more like a future-proof commit.

riscv_disassemble_insn function (before this patch) works like this:

    FOREACH (opcode)
    {
        if (!opcode.MATCH(insn))
            continue;
        //
        // Print insn using matched opcode info.
        //
        return insnlen;
    }
    //
    // opcode not matched.
    //

But if we split this to two steps: matching and printing like this:

    matched_opcode = NULL;
    // Step 1: opcode matching
    FOREACH (opcode)
    {
        if (!opcode.MATCH(insn))
            continue;
        matched_opcode = opcode;
        break;
    }
    // Step 2: opcode printing
    if (matched_opcode != NULL)
    {
        //
        // Print insn using matched_opcode info.
        //
        return insnlen;
    }
    //
    // opcode not matched.
    //

We can independently improve the opcode matching step and makes a room to
implement more complex opcode matching.  Despite that this patch alone
imposes a small amount of performance penalty, it can be easily paid back by
other optimizations.



State Initialization (PATCH 9/11)
==================================

Disassembler Lifecycle
-----------------------

In arch-independent libopcodes, we use following functions to disassemble
the object file/memory range:

-   disassembler
    To get real disassembler function for specific BFD and architecture.
    The result is print_insn_riscv on RISC-V.
-   (the result of "disassembler")
    as explained above.
-   disassemble_init_for_target
    This function initializes disassemble_info structure (e.g. allocates
    target-specific private data and sets certain fields).
-   disassemble_free_for_target
    This function frees private_data in the disassemble_info.  It also
    allows the target to run custom logic.

Ideally, all disassembler state should be contained within
disassemble_info.private_data (like in PowerPC) but many (including Arm,
MIPS and RISC-V) don't do that.  It means that we should handle this
abstract "disassembler" as a singleton for now.

It's used from objdump (binutils) and GDB but note that they have a
different execution order.  This is shown below:

objdump (completely target-independent):
    (Source)
    -   binutils/objdump.c
    (Note)
    -   This cycle happens per BFD object.  This is usually once on objdump
        but may occur twice or more (or even 0 times) on archive files.
    (Initialization Step)
    a1. Call "disassembler" function
        (with input BFD, arch, endian and mach)
    a2. Set initial disassemble_info structure contents
        (including flavour, arch, mach and disassembler_options)
    a3. Call disassemble_init_for_target
    (For Every Instruction)
    b1. Call the result of "disassembler" (print_insn_riscv on RISC-V)
    (Finalization Step)
    c1. Call disassemble_free_for_target

GDB (some may change through gdbarch but this is the general sequence):
    (Source)
    -   gdb/disasm.c
    -   gdb/disasm.h
    -   gdb/gdbarch.c
    -   gdb/arch-utils.c
    (Note)
    -   This cycle happens per one disassembler request
        (e.g. per one "disas" command).
    -   A disassembler lifecycle is represented as a
        gdb_disassemble_info class object.
    (Initialization Step - constructor)
    a1. Set initial disassemble_info structure contents
        (including flavour [unknown], arch, mach and disassembler_options)
    a2. Call disassemble_init_for_target
    (For Every Instruction - by default)
        Call trace:
        1.  gdb_disassembler::print_insn method
            (gdb_disassembler is a subclass of gdb_disassemble_info)
        2.  gdb_print_insn_1 function
        3.  gdbarch_print_insn
        4.  gdbarch->print_insn
            (default_print_insn by default but target-overridable)
        5.  default_print_insn (by default)
    b1. Call "disassembler" function
        (with input BFD, and arch, endian and mach as in disassemble_info)
    b2. Call the result of "disassembler" (print_insn_riscv on RISC-V)
    (Finalization Step - destructor)
    c1. Call disassemble_free_for_target

In both usecases, disassembler_options, arch and mach are set only once
(before disassemble_init_for_target) and never changes in multiple
print_insn_riscv calls.  That means, we are safe to parse some
disassemble_info fields such as mach and disassembler_options in
disassemble_init_for_target (it's even expected to do that, considering a
side effect I will explain later).

Note that we need to take care of two possible calling order:

-   disassembler -> disassemble_init_for_target (objdump)
-   disassemble_init_for_target -> disassembler (GDB)

Moving the Initialization
--------------------------

For instance, PowerPC and WebAssembly targets parse disassembler options in
the disassembler_init_for_target call (PowerPC parses mach, too).

Many architectures instead try to initialize its state on per-instruction
call (in the "result of disassembler" call to be specific) but it's
clearly a waste of time and only a tradition (a bad one, to be honest).

As objdump works well in PowerPC and WebAssembly and GDB works well in
PowerPC (note: GDB for WebAssembly is not implemented), moving RISC-V's
disassembler state initialization (including parsing disassembler_options)
is safe as PowerPC and makes state management simpler and faster.  Some of
my optimizations depend on this change otherwise will be too complex and/or
doesn't speed up the disassembler as expected.

But not everything can be moved.  Because symbol table in the
disassemble_info (symtab, symtab_size...) is not available on
disassembler_init_for_target and disassemble_info object is not available
on "diassembler", we have no stable way to initialize pd->gp prior to the
first riscv_disassemble_insn call.  So, info->private_data == NULL
branch on riscv_disassemble_insn is preserved.

Fixed Issues as a Side Effect
------------------------------

Also, changing RISC-V disassembler like this has two good side effects:

1.  We can really "unset" the disassembler options in the GDB
2.  Error message regarding the disassembler options is at the right place
    on objdump

Fixed Issue 1: Unsetting Options on GDB (and new smaller issue)
----------------------------------------------------------------

First, GDB expects to allow "set disassembler-options XXX..." to set
disassembler options "XXX..." **and** "set disassembler-options" to
**unset** them, reverting to the default.  It didn't work like this in
RISC-V but my patchset fixes this issue as a side effect.

Before:
    (gdb) set disassembler-options
    (gdb) disas _start
    Dump of assembler code for function _start:
    0x0000000080000028 <+0>:     ret
    End of assembler dump.
    (gdb) set disassembler-options no-aliases
    (gdb) disas _start
    Dump of assembler code for function _start:
    0x0000000080000028 <+0>:     c.jr    ra
    End of assembler dump.
    (gdb) set disassembler-options
    (gdb) disas _start
    Dump of assembler code for function _start:
    0x0000000080000028 <+0>:     c.jr    ra     <-- should be "ret"!
    End of assembler dump.

After (snip):
    (gdb) set disassembler-options              <-- 1st/2nd time
    (gdb) disas _start
    Dump of assembler code for function _start:
    0x0000000080000028 <+0>:     ret            <-- restored!
    End of assembler dump.

This is because, after "set disassembler-options" to unset disassembler
options, disassemble_info.disassembler_options is set to NULL, not an empty
string.  Because current print_insn_riscv function checks whether
disassembler_options is NULL and parses the options when it's not,
"set disassembler-options" works as "do nothing" (instead of
"reset to default") in RISC-V.

Beware that, PATCH 9/11 introduces a smaller bug in GDB which suppresses an
error message when mismatched "priv-spec" option is specified once before
the first disassembly.  In the following example, we assume that we are
debugging an ELF file with privileged specification 1.11 ELF attributes.

It will not occur in objdump.

Before (okay):
    (gdb) file test.elf
    (gdb) set disassembler-options priv-spec=1.12
    (gdb) disas _start
    Dump of assembler code for function _start:
    BFD: mis-matched privilege spec set by priv-spec=1.12, the elf privilege attribute is 1.11
    0x0000000080000028 <+0>:     csrr    a0,0x3d0
                                            ^^^^^
                                    "pmpaddr32" CSR but not defined in 1.11.
    0x000000008000002c <+4>:     ret
    End of assembler dump.
    (gdb) disas _start
    Dump of assembler code for function _start:
    BFD: mis-matched privilege spec set by priv-spec=1.12, the elf privilege attribute is 1.11
    0x0000000080000028 <+0>:     csrr    a0,0x3d0
    0x000000008000002c <+4>:     ret
    End of assembler dump.

After PATCH 9/11 (an error message is suppressed):
    (gdb) file test.elf
    (gdb) set disassembler-options priv-spec=1.12
    (gdb) disas _start
    Dump of assembler code for function _start:
    0x0000000080000028 <+0>:     csrr    a0,0x3d0
                                            ^^^^^
                                    "pmpaddr32" CSR but not defined in 1.11.
                        An error message just before this line is suppressed
                          but "priv-spec" override does not actually happen.
    0x000000008000002c <+4>:     ret
    End of assembler dump.
    (gdb) disas _start
    Dump of assembler code for function _start:
    BFD: mis-matched privilege spec set by priv-spec=1.12, the elf privilege attribute is 1.11
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
                           correctly printed at the second time (and later).
    0x0000000080000028 <+0>:     csrr    a0,0x3d0
    0x000000008000002c <+4>:     ret
    End of assembler dump.

This new smaller issue is taken care in my upcoming patchset.

Fixed Issue 2: Location of Error Message
-----------------------------------------

Second, because PATCH 9/11 changes where error message is generated, this
patch makes error message location (when an invalid disassembler option is
specified) more "right", more sensible.

Before:

    test.elf:     file format elf64-littleriscv


    Disassembly of section .text:

    0000000080000000 <__start>:
        80000000:   riscv64-unknown-elf-objdump: mis-matched privilege spec set by priv-spec=1.12, the elf privilege attribute is 1.11
    00002197                auipc   gp,0x2
        80000004:   82e18193                addi    gp,gp,-2002 # 8000182e <__global_pointer$>
        ...

After:

    test.elf:     file format elf64-littleriscv

    riscv64-unknown-elf-objdump: mis-matched privilege spec set by priv-spec=1.12, the elf privilege attribute is 1.11

    Disassembly of section .text:

    0000000080000000 <__start>:
        80000000:   00002197                auipc   gp,0x2
        80000004:   82e18193                addi    gp,gp,-2002 # 8000182e <__global_pointer$>
        ...


Architecture Initialization and Management (PATCH 10/11)
=========================================================

This commit makes major refinements to the architecture initialization
and management code.  The purpose of this is to:

-   Provide easy interface to initialize when the architecture is changed
-   Enable switching between default architecture (from an ELF attribute
    or from default string "rv64gc") and the architecture override
    (from mapping symbol with ISA string).
-   Avoid repeated initialization cost.

Repeated architecture (riscv_rps_dis reinitialization) can occur per
instruction on following cases:

-   When disassembling with GDB (does not occur on objdump due to the
    execution order of libopcodes shown in the PATCH 9/11 section)
-   When an input ELF file has mapping symbols with ISA string.

Like Nelson's proposal tried to enable switching using riscv_file_subsets,
my PATCH 9/11 provides new type "context" to store riscv_subset_list_t,
XLEN and management-related variables.

The major differences between this change and Nelson's are:

-   PATCH 10/11 does not reinitialize the architecture (riscv_rps_dis) if
    the architecture string is not changed.  This makes a huge difference
    in performance.
-   PATCH 10/11 stores "XLEN per context".

The second property provides interesting "feature" although not realistic
in the real world: multi-XLEN object disassembling.  If an object has
mapping symbols with ISA string, we can switch between architectures,
EVEN IF XLEN HAS CHANGED (e.g. "$xrv32i" and "$xrv64i").

It's not the default behavior (by default, a XLEN-specific machine either
"riscv:rv32" or "riscv:rv64" is set and *this* XLEN is preferred than the
mapping symbols) and requires explicitly specifying the XLEN-neutral RISC-V
machine (objdump: "-m riscv").  Although it's not realistic in the real
world, it may help testing Binutils / GAS.




Tsukasa OI (11):
  opcodes/riscv-dis.c: More tidying
  RISC-V: Add test for 'Zfinx' register switching
  RISC-V: Make mapping symbol checking consistent
  RISC-V: Split riscv_get_map_state into two steps
  RISC-V: One time CSR hash table initialization
  RISC-V: Use static xlen on ADDIW sequence
  opcodes/riscv-dis.c: Add form feed for separation
  RISC-V: Split match/print steps on disassembler
  RISC-V: Reorganize disassembler state initialization
  RISC-V: Reorganize arch-related initialization and management
  RISC-V: Move disassembler private data initialization

 gas/testsuite/gas/riscv/mapping-dis.d     |   7 +
 gas/testsuite/gas/riscv/mapping-symbols.d |   4 +
 gas/testsuite/gas/riscv/mapping.s         |  10 +
 include/dis-asm.h                         |   1 +
 opcodes/disassemble.c                     |   2 +-
 opcodes/riscv-dis.c                       | 485 +++++++++++++++-------
 6 files changed, 366 insertions(+), 143 deletions(-)


base-commit: 8148339a741b37df6df3c4b3c4a7b9e812a79be7
-- 
2.37.2

[PATCH 01/11] opcodes/riscv-dis.c: More tidying

[Tsukasa OI]

This is a general tidying commit.

opcodes/ChangeLog:

	* riscv-dis.c (struct riscv_private_data) Add summary.
	Make length of hi_addr more meaningful.
---
 opcodes/riscv-dis.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/opcodes/riscv-dis.c b/opcodes/riscv-dis.c
index 3a31647a2f8..ea45a631a25 100644
--- a/opcodes/riscv-dis.c
+++ b/opcodes/riscv-dis.c
@@ -52,11 +52,12 @@ static riscv_parse_subset_t riscv_rps_dis =
   false,		/* check_unknown_prefixed_ext.  */
 };
 
+/* Private data structure for the RISC-V disassembler.  */
 struct riscv_private_data
 {
   bfd_vma gp;
   bfd_vma print_addr;
-  bfd_vma hi_addr[OP_MASK_RD + 1];
+  bfd_vma hi_addr[NGPR];
   bool to_print_addr;
   bool has_gp;
 };
-- 
2.37.2

[PATCH 02/11] RISC-V: Add test for 'Zfinx' register switching

[Tsukasa OI]

Because the author is going to reorganize core RISC-V disassembler, we have
to make sure that nothing is broken when disassembling with mapping symbols
with ISA string.

This commit adds a testcase for 'F' and 'Zfinx' instructions to make sure
that "FPR" register names are correctly switched when necessary.

gas/ChangeLog:

	* testsuite/gas/riscv/mapping.s: Add 'F' and 'Zfinx' testcase.
	* testsuite/gas/riscv/mapping-dis.d: Likewise.
	* testsuite/gas/riscv/mapping-symbols.d: Likewise.
---
 gas/testsuite/gas/riscv/mapping-dis.d     |  7 +++++++
 gas/testsuite/gas/riscv/mapping-symbols.d |  4 ++++
 gas/testsuite/gas/riscv/mapping.s         | 10 ++++++++++
 3 files changed, 21 insertions(+)

diff --git a/gas/testsuite/gas/riscv/mapping-dis.d b/gas/testsuite/gas/riscv/mapping-dis.d
index b1a26fbd151..f0508499b72 100644
--- a/gas/testsuite/gas/riscv/mapping-dis.d
+++ b/gas/testsuite/gas/riscv/mapping-dis.d
@@ -91,3 +91,10 @@ Disassembly of section .text.relax.align:
 [ 	]+[0-9a-f]+:[ 	]+00000013[ 	]+nop
 [ 	]+[0-9a-f]+:[ 	]+00200513[ 	]+li[ 	]+a0,2
 [ 	]+[0-9a-f]+:[ 	]+00000013[ 	]+nop
+
+Disassembly of section .text.dis.zfinx:
+
+0+000 <.text.dis.zfinx>:
+[ 	]+[0-9a-f]+:[ 	]+00c5f553[ 	]+fadd\.s[ 	]+fa0,fa1,fa2
+[ 	]+[0-9a-f]+:[ 	]+00c5f553[ 	]+fadd\.s[ 	]+a0,a1,a2
+[ 	]+[0-9a-f]+:[ 	]+00c5f553[ 	]+fadd\.s[ 	]+fa0,fa1,fa2
diff --git a/gas/testsuite/gas/riscv/mapping-symbols.d b/gas/testsuite/gas/riscv/mapping-symbols.d
index 40df3409736..b28e3306b1b 100644
--- a/gas/testsuite/gas/riscv/mapping-symbols.d
+++ b/gas/testsuite/gas/riscv/mapping-symbols.d
@@ -42,6 +42,10 @@ SYMBOL TABLE:
 0+00 l    d  .text.relax.align	0+00 .text.relax.align
 0+00 l       .text.relax.align	0+00 \$xrv32i2p1_c2p0
 0+08 l       .text.relax.align	0+00 \$xrv32i2p1
+0+00 l    d  .text.dis.zfinx	0+00 .text.dis.zfinx
+0+00 l       .text.dis.zfinx	0+00 \$xrv32i2p1_f2p2_zicsr2p0
+0+04 l       .text.dis.zfinx	0+00 \$xrv32i2p1_zicsr2p0_zfinx1p0
+0+08 l       .text.dis.zfinx	0+00 \$xrv32i2p1_f2p2_zicsr2p0
 0+0a l       .text.section.padding	0+00 \$x
 0+03 l       .text.odd.align.start.insn	0+00 \$d
 0+04 l       .text.odd.align.start.insn	0+00 \$x
diff --git a/gas/testsuite/gas/riscv/mapping.s b/gas/testsuite/gas/riscv/mapping.s
index 3014a69e792..4fee2b420f0 100644
--- a/gas/testsuite/gas/riscv/mapping.s
+++ b/gas/testsuite/gas/riscv/mapping.s
@@ -119,3 +119,13 @@ addi	a0, zero, 1		# $x, won't added
 .align	3			# $x, won't added
 addi	a0, zero, 2		# $xrv32i
 .option pop
+
+.section .text.dis.zfinx, "ax"
+.option push
+.option arch, rv32if
+fadd.s	fa0, fa1, fa2		# $xrv32if
+.option arch, rv32i_zfinx
+fadd.s	a0, a1, a2		# $xrv32i_zfinx
+.option arch, rv32if
+fadd.s	fa0, fa1, fa2		# $xrv32if
+.option pop
-- 
2.37.2

[PATCH 03/11] RISC-V: Make mapping symbol checking consistent

[Tsukasa OI]

There were two places where the mapping symbols are checked but had
different conditions.

-    riscv_get_map_state:          "$d" or starts with "$x"
-    riscv_elf_is_mapping_symbols: Starts with either "$x" or "$d"

Considering recent mapping symbol proposal, it's better to make symbol
checking consistent (whether the symbol _starts_ with "$[xd]").

It only checks prefix "$xrv" (mapping symbol with ISA string) only when the
prefix "$x" is matched.

opcodes/ChangeLog:

	* riscv-dis.c (riscv_get_map_state): Change the condition for
	consistency.
---
 opcodes/riscv-dis.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/opcodes/riscv-dis.c b/opcodes/riscv-dis.c
index ea45a631a25..d3bd4ceec1e 100644
--- a/opcodes/riscv-dis.c
+++ b/opcodes/riscv-dis.c
@@ -832,16 +832,17 @@ riscv_get_map_state (int n,
     return false;
 
   name = bfd_asymbol_name(info->symtab[n]);
-  if (strcmp (name, "$x") == 0)
-    *state = MAP_INSN;
-  else if (strcmp (name, "$d") == 0)
-    *state = MAP_DATA;
-  else if (strncmp (name, "$xrv", 4) == 0)
+  if (startswith (name, "$x"))
     {
+      if (startswith (name + 2, "rv"))
+	{
+	  riscv_release_subset_list (&riscv_subsets);
+	  riscv_parse_subset (&riscv_rps_dis, name + 2);
+	}
       *state = MAP_INSN;
-      riscv_release_subset_list (&riscv_subsets);
-      riscv_parse_subset (&riscv_rps_dis, name + 2);
     }
+  else if (startswith (name, "$d"))
+    *state = MAP_DATA;
   else
     return false;
 
-- 
2.37.2

[PATCH 04/11] RISC-V: Split riscv_get_map_state into two steps

[Tsukasa OI]

Because mapping symbol optimization would remove riscv_get_map_state
function, this commit splits symbol name checking step into a separate
function riscv_get_map_state_by_name.

Let alone the optimization, splitting the code improves readability.

opcodes/ChangeLog:

	* riscv-dis.c (riscv_get_map_state): Split symbol name checking
	into a separate function.  (riscv_get_map_state_by_name): New.
---
 opcodes/riscv-dis.c | 41 +++++++++++++++++++++++++++--------------
 1 file changed, 27 insertions(+), 14 deletions(-)

diff --git a/opcodes/riscv-dis.c b/opcodes/riscv-dis.c
index d3bd4ceec1e..3135db30ccd 100644
--- a/opcodes/riscv-dis.c
+++ b/opcodes/riscv-dis.c
@@ -816,6 +816,24 @@ riscv_disassemble_insn (bfd_vma memaddr, insn_t word, disassemble_info *info)
   return insnlen;
 }
 
+/* Return new mapping state if a given symbol name is of mapping symbols',
+   MAP_NONE otherwise.  If arch is not NULL and name denotes a mapping symbol
+   with ISA string, *arch is updated to the ISA string.  */
+
+static enum riscv_seg_mstate
+riscv_get_map_state_by_name (const char *name, const char** arch)
+{
+  if (startswith (name, "$x"))
+    {
+      if (arch && startswith (name + 2, "rv"))
+	*arch = name + 2;
+      return MAP_INSN;
+    }
+  else if (startswith (name, "$d"))
+    return MAP_DATA;
+  return MAP_NONE;
+}
+
 /* Return true if we find the suitable mapping symbol,
    and also update the STATE.  Otherwise, return false.  */
 
@@ -824,28 +842,23 @@ riscv_get_map_state (int n,
 		     enum riscv_seg_mstate *state,
 		     struct disassemble_info *info)
 {
-  const char *name;
+  const char *name, *arch = NULL;
 
   /* If the symbol is in a different section, ignore it.  */
   if (info->section != NULL
       && info->section != info->symtab[n]->section)
     return false;
 
-  name = bfd_asymbol_name(info->symtab[n]);
-  if (startswith (name, "$x"))
+  name = bfd_asymbol_name (info->symtab[n]);
+  enum riscv_seg_mstate newstate = riscv_get_map_state_by_name (name, &arch);
+  if (newstate == MAP_NONE)
+    return false;
+  *state = newstate;
+  if (arch)
     {
-      if (startswith (name + 2, "rv"))
-	{
-	  riscv_release_subset_list (&riscv_subsets);
-	  riscv_parse_subset (&riscv_rps_dis, name + 2);
-	}
-      *state = MAP_INSN;
+      riscv_release_subset_list (&riscv_subsets);
+      riscv_parse_subset (&riscv_rps_dis, arch);
     }
-  else if (startswith (name, "$d"))
-    *state = MAP_DATA;
-  else
-    return false;
-
   return true;
 }
 
-- 
2.37.2

[PATCH 05/11] RISC-V: One time CSR hash table initialization

[Tsukasa OI]

The current disassembler intends to initialize the CSR hash table when CSR
name parsing is required at the first time. This is managed by a function-
scope static variable init_csr but... there's a problem.

It's never set to true.

Because of this issue, current disassembler actually initializes the CSR
hash table every time when CSR name parsing is required.  This commit sets
init_csr to true once the CSR hash table is initialized.

It is expected to have about 30% performance improvements alone when
thousands of only CSR instructions are disassembled (CSR instructions are
rare in general so real world performance improvement is not that high).

This commit alone will not affect real world performance that much but
after the efficient opcode hash is implemented, it will be much effective
(sometimes >x10 effect than this commit alone) so that even some regular
programs can benefit from it.

opcodes/ChangeLog:

	* riscv-dis.c (print_insn_args): Make sure that CSR hash table
	initialization occurs only once.
---
 opcodes/riscv-dis.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/opcodes/riscv-dis.c b/opcodes/riscv-dis.c
index 3135db30ccd..ec38bed747a 100644
--- a/opcodes/riscv-dis.c
+++ b/opcodes/riscv-dis.c
@@ -564,6 +564,7 @@ print_insn_args (const char *oparg, insn_t l, bfd_vma pc, disassemble_info *info
 		DECLARE_CSR (name, num, class, define_version, abort_version)
 #include "opcode/riscv-opc.h"
 #undef DECLARE_CSR
+		init_csr = true;
 	      }
 
 	    if (riscv_csr_hash[csr] != NULL)
-- 
2.37.2

[PATCH 06/11] RISC-V: Use static xlen on ADDIW sequence

[Tsukasa OI]

Because XLEN for the disassembler is computed and stored in the xlen
variable, this commit replaces uses of info->mach with xlen
(when testing for ADDIW / C.ADDIW address sequence).

Not just we used two ways to determine current XLEN, info->mach and xlen,
xlen is going to be more important in the future commits.

opcodes/ChangeLog:

	* riscv-dis.c (print_insn_args): Use xlen variable to determine
	whether XLEN is larger than 32.
---
 opcodes/riscv-dis.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/opcodes/riscv-dis.c b/opcodes/riscv-dis.c
index ec38bed747a..d4a6cdd4d4e 100644
--- a/opcodes/riscv-dis.c
+++ b/opcodes/riscv-dis.c
@@ -261,7 +261,7 @@ print_insn_args (const char *oparg, insn_t l, bfd_vma pc, disassemble_info *info
 	    case 'j':
 	      if (((l & MASK_C_ADDI) == MATCH_C_ADDI) && rd != 0)
 		maybe_print_address (pd, rd, EXTRACT_CITYPE_IMM (l), 0);
-	      if (info->mach == bfd_mach_riscv64
+	      if (xlen > 32
 		  && ((l & MASK_C_ADDIW) == MATCH_C_ADDIW) && rd != 0)
 		maybe_print_address (pd, rd, EXTRACT_CITYPE_IMM (l), 1);
 	      print (info->stream, dis_style_immediate, "%d",
@@ -461,7 +461,7 @@ print_insn_args (const char *oparg, insn_t l, bfd_vma pc, disassemble_info *info
 	  if (((l & MASK_ADDI) == MATCH_ADDI && rs1 != 0)
 	      || (l & MASK_JALR) == MATCH_JALR)
 	    maybe_print_address (pd, rs1, EXTRACT_ITYPE_IMM (l), 0);
-	  if (info->mach == bfd_mach_riscv64
+	  if (xlen > 32
 	      && ((l & MASK_ADDIW) == MATCH_ADDIW) && rs1 != 0)
 	    maybe_print_address (pd, rs1, EXTRACT_ITYPE_IMM (l), 1);
 	  print (info->stream, dis_style_immediate, "%d",
-- 
2.37.2

[PATCH 07/11] opcodes/riscv-dis.c: Add form feed for separation

[Tsukasa OI]

This is a general tidying commit.

opcodes/ChangeLog:

	* riscv-dis.c: Add lines with form feed to separate functional
	sections.
---
 opcodes/riscv-dis.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/opcodes/riscv-dis.c b/opcodes/riscv-dis.c
index d4a6cdd4d4e..d9c16dad615 100644
--- a/opcodes/riscv-dis.c
+++ b/opcodes/riscv-dis.c
@@ -72,7 +72,7 @@ static const char * const *riscv_fpr_names;
 
 /* If set, disassemble as most general instruction.  */
 static bool no_aliases = false;
-
+\f
 
 /* Set default RISC-V disassembler options.  */
 
@@ -174,6 +174,7 @@ parse_riscv_dis_options (const char *opts_in)
 
   free (opts);
 }
+\f
 
 /* Print one argument from an array.  */
 
-- 
2.37.2

[PATCH 08/11] RISC-V: Split match/print steps on disassembler

[Tsukasa OI]

For further optimization and more disassembler features, we may need to
change the core RISC-V instruction matching.  For this purpose, it is
inconvenient to have "match" and "print" steps in the same loop.

This commit rewrites riscv_disassemble_insn function so that we store
matched_op for matching RISC-V opcode and then print it (if not NULL).

Although it looks a bit inefficient, it also lowers the indent of opcode
matching loop to clarify the opcode matching changes on the next
optimization commit.

Unfortunately, this commit alone will impose some performance penalty (<5%
on most cases but sometimes about 15% worse) but it can be easily paid back
by other optimizations.

opcodes/ChangeLog:

	* riscv-dis.c (riscv_disassemble_insn): Split instruction
	handling to two separate steps - opcode matching and printing.
---
 opcodes/riscv-dis.c | 151 +++++++++++++++++++++++---------------------
 1 file changed, 79 insertions(+), 72 deletions(-)

diff --git a/opcodes/riscv-dis.c b/opcodes/riscv-dis.c
index d9c16dad615..316c6c97607 100644
--- a/opcodes/riscv-dis.c
+++ b/opcodes/riscv-dis.c
@@ -646,7 +646,7 @@ print_insn_args (const char *oparg, insn_t l, bfd_vma pc, disassemble_info *info
 static int
 riscv_disassemble_insn (bfd_vma memaddr, insn_t word, disassemble_info *info)
 {
-  const struct riscv_opcode *op;
+  const struct riscv_opcode *op, *matched_op;
   static bool init = false;
   static const struct riscv_opcode *riscv_hash[OP_MASK_OP + 1];
   struct riscv_private_data *pd;
@@ -702,85 +702,92 @@ riscv_disassemble_insn (bfd_vma memaddr, insn_t word, disassemble_info *info)
   info->target = 0;
   info->target2 = 0;
 
+  matched_op = NULL;
   op = riscv_hash[OP_HASH_IDX (word)];
-  if (op != NULL)
+
+  /* If XLEN is not known, get its value from the ELF class.  */
+  if (info->mach == bfd_mach_riscv64)
+    xlen = 64;
+  else if (info->mach == bfd_mach_riscv32)
+    xlen = 32;
+  else if (info->section != NULL)
     {
-      /* If XLEN is not known, get its value from the ELF class.  */
-      if (info->mach == bfd_mach_riscv64)
-	xlen = 64;
-      else if (info->mach == bfd_mach_riscv32)
-	xlen = 32;
-      else if (info->section != NULL)
-	{
-	  Elf_Internal_Ehdr *ehdr = elf_elfheader (info->section->owner);
-	  xlen = ehdr->e_ident[EI_CLASS] == ELFCLASS64 ? 64 : 32;
-	}
+      Elf_Internal_Ehdr *ehdr = elf_elfheader (info->section->owner);
+      xlen = ehdr->e_ident[EI_CLASS] == ELFCLASS64 ? 64 : 32;
+    }
 
-      /* If arch has the Zfinx extension, replace FPR with GPR.  */
-      if (riscv_subset_supports (&riscv_rps_dis, "zfinx"))
-	riscv_fpr_names = riscv_gpr_names;
-      else
-	riscv_fpr_names = riscv_gpr_names == riscv_gpr_names_abi ?
-			  riscv_fpr_names_abi : riscv_fpr_names_numeric;
+  /* If arch has the Zfinx extension, replace FPR with GPR.  */
+  if (riscv_subset_supports (&riscv_rps_dis, "zfinx"))
+    riscv_fpr_names = riscv_gpr_names;
+  else
+    riscv_fpr_names = riscv_gpr_names == riscv_gpr_names_abi
+			  ? riscv_fpr_names_abi
+			  : riscv_fpr_names_numeric;
 
-      for (; op->name; op++)
-	{
-	  /* Does the opcode match?  */
-	  if (! (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))
-	    continue;
-	  /* Is this instruction restricted to a certain value of XLEN?  */
-	  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))
-	    continue;
-
-	  /* It's a match.  */
-	  (*info->fprintf_styled_func) (info->stream, dis_style_mnemonic,
-					"%s", op->name);
-	  print_insn_args (op->args, word, memaddr, info);
-
-	  /* Try to disassemble multi-instruction addressing sequences.  */
-	  if (pd->to_print_addr)
-	    {
-	      info->target = pd->print_addr;
-	      (*info->fprintf_styled_func)
-		(info->stream, dis_style_comment_start, " # ");
-	      (*info->print_address_func) (info->target, info);
-	      pd->to_print_addr = false;
-	    }
+  for (; op && op->name; op++)
+    {
+      /* Does the opcode match?  */
+      if (!(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))
+	continue;
+      /* Is this instruction restricted to a certain value of XLEN?  */
+      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))
+	continue;
 
-	  /* Finish filling out insn_info fields.  */
-	  switch (op->pinfo & INSN_TYPE)
-	    {
-	    case INSN_BRANCH:
-	      info->insn_type = dis_branch;
-	      break;
-	    case INSN_CONDBRANCH:
-	      info->insn_type = dis_condbranch;
-	      break;
-	    case INSN_JSR:
-	      info->insn_type = dis_jsr;
-	      break;
-	    case INSN_DREF:
-	      info->insn_type = dis_dref;
-	      break;
-	    default:
-	      break;
-	    }
+      matched_op = op;
+      break;
+    }
 
-	  if (op->pinfo & INSN_DATA_SIZE)
-	    {
-	      int size = ((op->pinfo & INSN_DATA_SIZE)
-			  >> INSN_DATA_SIZE_SHIFT);
-	      info->data_size = 1 << (size - 1);
-	    }
+  if (matched_op != NULL)
+    {
+      /* There is a match.  */
+      op = matched_op;
+
+      (*info->fprintf_styled_func) (info->stream, dis_style_mnemonic,
+				    "%s", op->name);
+      print_insn_args (op->args, word, memaddr, info);
 
-	  return insnlen;
+      /* Try to disassemble multi-instruction addressing sequences.  */
+      if (pd->to_print_addr)
+	{
+	  info->target = pd->print_addr;
+	  (*info->fprintf_styled_func) (info->stream, dis_style_comment_start,
+					" # ");
+	  (*info->print_address_func) (info->target, info);
+	  pd->to_print_addr = false;
 	}
+
+      /* Finish filling out insn_info fields.  */
+      switch (op->pinfo & INSN_TYPE)
+	{
+	case INSN_BRANCH:
+	  info->insn_type = dis_branch;
+	  break;
+	case INSN_CONDBRANCH:
+	  info->insn_type = dis_condbranch;
+	  break;
+	case INSN_JSR:
+	  info->insn_type = dis_jsr;
+	  break;
+	case INSN_DREF:
+	  info->insn_type = dis_dref;
+	  break;
+	default:
+	  break;
+	}
+
+      if (op->pinfo & INSN_DATA_SIZE)
+	{
+	  int size = ((op->pinfo & INSN_DATA_SIZE) >> INSN_DATA_SIZE_SHIFT);
+	  info->data_size = 1 << (size - 1);
+	}
+
+      return insnlen;
     }
 
   /* We did not find a match, so just print the instruction bits.  */
-- 
2.37.2

[PATCH 09/11] RISC-V: Reorganize disassembler state initialization

[Tsukasa OI]

The current disassembler repeatedly checks current state per instruction:

-   Whether riscv_gpr_names is initialized to a non-NULL value
-   Whether the Zfinx extension is available
-   Whether the hash table is initialized

... but they are not frequently changed.

riscv_gpr_names is initialized to a non-NULL value when

-   The first disassembler option is specified, or
-   Not initialized with disassembler options
    (in the first print_insn_riscv function call).

We can safely initialize the default disassembler options prior to the
print_insn_riscv function call and this per-instruction checking of
riscv_gpr_names can be safely removed.

The opcode hash table initialization will be taken care with a later commit.

To group disassembler state initialization, this commit utilizes the
disassemble_init_for_target function.  As a side effect, this will also fix
a potential issue when disassembler options is set and then unset on GDB.
This idea is based on opcodes/ppc-dis.c and opcodes/wasm32-dis.c.

New callback function init_riscv_dis_state_for_arch_and_options is called
when either the architecture string or an option is possibly changed.

We can now group the disassembler state initialization together.
It makes state initialization clearer and makes further changes easier.

In performance perspective, this commit has various effects (-5% to 6%).
However, this commit makes implementing large optimizations easier
as well as "RISC-V: Split match/print steps on disassembler".

include/ChangeLog:

	* dis-asm.h (disassemble_init_riscv): Add declaration of
	disassemble_init_riscv.

opcodes/ChangeLog:

	* disassemble.c (disassemble_init_for_target): Call
	disassemble_init_riscv to group state initialization together.
	* riscv-dis.c
	(xlen_by_mach, xlen_by_elf): New variables to store
	environment-inferred XLEN.
	(is_numeric): New.  Instead of directly setting
	riscv_{gpr,fpr}_names, use this to store an option.
	(update_riscv_dis_xlen): New function to set actual XLEN from
	xlen_by_mach and xlen_by_elf variables.
	(init_riscv_dis_state_for_arch_and_options): New callback function
	called when either the architecture or an option is changed.  Set
	riscv_{gpr,fpr}_names here.
	(set_default_riscv_dis_options): Initialize is_numeric instead of
	riscv_gpr_names and riscv_fpr_names.
	(parse_riscv_dis_option_without_args): When the "numeric" option
	is specified, write to is_numeric instead of register names.
	(parse_riscv_dis_options): Suppress setting the default options
	here and let disassemble_init_riscv to initialize them.
	(riscv_disassemble_insn): Move probing Zfinx and setting XLEN
	portions to init_riscv_dis_state_for_arch_and_options and
	update_riscv_dis_xlen.
	(riscv_get_map_state): If a mapping symbol with ISA string is
	suitable, call init_riscv_dis_state_for_arch_and_options function
	to update disassembler state.
	(print_insn_riscv): Update XLEN only if we haven't guessed correct
	XLEN for the disassembler.  Stop checking disassembler options for
	every instruction and let disassemble_init_riscv to parse options.
	(riscv_get_disassembler): Call
	init_riscv_dis_state_for_arch_and_options because the architecture
	string is updated here.
	(disassemble_init_riscv): New function to initialize the
	structure, reset/guess correct XLEN and reset/parse disassembler
	options.
---
 include/dis-asm.h     |   1 +
 opcodes/disassemble.c |   2 +-
 opcodes/riscv-dis.c   | 112 +++++++++++++++++++++++++++++-------------
 3 files changed, 79 insertions(+), 36 deletions(-)

diff --git a/include/dis-asm.h b/include/dis-asm.h
index 4921c040710..11537b432ff 100644
--- a/include/dis-asm.h
+++ b/include/dis-asm.h
@@ -393,6 +393,7 @@ extern bool arm_symbol_is_valid (asymbol *, struct disassemble_info *);
 extern bool csky_symbol_is_valid (asymbol *, struct disassemble_info *);
 extern bool riscv_symbol_is_valid (asymbol *, struct disassemble_info *);
 extern void disassemble_init_powerpc (struct disassemble_info *);
+extern void disassemble_init_riscv (struct disassemble_info *);
 extern void disassemble_init_s390 (struct disassemble_info *);
 extern void disassemble_init_wasm32 (struct disassemble_info *);
 extern void disassemble_init_nds32 (struct disassemble_info *);
diff --git a/opcodes/disassemble.c b/opcodes/disassemble.c
index 0a8f2da629f..704fb476ea9 100644
--- a/opcodes/disassemble.c
+++ b/opcodes/disassemble.c
@@ -717,7 +717,7 @@ disassemble_init_for_target (struct disassemble_info * info)
 #endif
 #ifdef ARCH_riscv
     case bfd_arch_riscv:
-      info->symbol_is_valid = riscv_symbol_is_valid;
+      disassemble_init_riscv (info);
       info->created_styled_output = true;
       break;
 #endif
diff --git a/opcodes/riscv-dis.c b/opcodes/riscv-dis.c
index 316c6c97607..76387efbe4b 100644
--- a/opcodes/riscv-dis.c
+++ b/opcodes/riscv-dis.c
@@ -35,6 +35,12 @@
 /* Current XLEN for the disassembler.  */
 static unsigned xlen = 0;
 
+/* XLEN as inferred by the machine architecture.  */
+static unsigned xlen_by_mach = 0;
+
+/* XLEN as inferred by ELF header.  */
+static unsigned xlen_by_elf = 0;
+
 /* Default ISA specification version (constant as of now).  */
 static enum riscv_spec_class default_isa_spec = ISA_SPEC_CLASS_DRAFT - 1;
 
@@ -72,6 +78,48 @@ static const char * const *riscv_fpr_names;
 
 /* If set, disassemble as most general instruction.  */
 static bool no_aliases = false;
+
+/* If set, disassemble with numeric register names.  */
+static bool is_numeric = false;
+\f
+
+/* Guess and update current XLEN.  */
+
+static void
+update_riscv_dis_xlen (struct disassemble_info *info)
+{
+  /* Set XLEN with following precedence rules:
+     1. BFD machine architecture set by either:
+       a. -m riscv:rv[32|64] option (GDB: set arch riscv:rv[32|64])
+       b. ELF class in actual ELF header (only on RISC-V ELF)
+       This is only effective if XLEN-specific BFD machine architecture is
+       chosen.  If XLEN-neutral (like riscv), BFD machine architecture is
+       ignored on XLEN selection.
+     2. ELF class in dummy ELF header.  */
+  if (xlen_by_mach != 0)
+    xlen = xlen_by_mach;
+  else if (xlen_by_elf != 0)
+    xlen = xlen_by_elf;
+  else if (info != NULL && info->section != NULL)
+    {
+      Elf_Internal_Ehdr *ehdr = elf_elfheader (info->section->owner);
+      xlen = xlen_by_elf = ehdr->e_ident[EI_CLASS] == ELFCLASS64 ? 64 : 32;
+    }
+}
+
+/* Initialization (for arch and options).  */
+
+static void
+init_riscv_dis_state_for_arch_and_options (void)
+{
+  /* Set GPR register names to disassemble.  */
+  riscv_gpr_names = is_numeric ? riscv_gpr_names_numeric : riscv_gpr_names_abi;
+  /* Set FPR register names to disassemble.  */
+  riscv_fpr_names
+      = !riscv_subset_supports (&riscv_rps_dis, "zfinx")
+	    ? (is_numeric ? riscv_fpr_names_numeric : riscv_fpr_names_abi)
+	    : riscv_gpr_names;
+}
 \f
 
 /* Set default RISC-V disassembler options.  */
@@ -79,9 +127,8 @@ static bool no_aliases = false;
 static void
 set_default_riscv_dis_options (void)
 {
-  riscv_gpr_names = riscv_gpr_names_abi;
-  riscv_fpr_names = riscv_fpr_names_abi;
   no_aliases = false;
+  is_numeric = false;
 }
 
 /* Parse RISC-V disassembler option (without arguments).  */
@@ -92,10 +139,7 @@ parse_riscv_dis_option_without_args (const char *option)
   if (strcmp (option, "no-aliases") == 0)
     no_aliases = true;
   else if (strcmp (option, "numeric") == 0)
-    {
-      riscv_gpr_names = riscv_gpr_names_numeric;
-      riscv_fpr_names = riscv_fpr_names_numeric;
-    }
+    is_numeric = true;
   else
     return false;
   return true;
@@ -163,8 +207,6 @@ parse_riscv_dis_options (const char *opts_in)
 {
   char *opts = xstrdup (opts_in), *opt = opts, *opt_end = opts;
 
-  set_default_riscv_dis_options ();
-
   for ( ; opt_end != NULL; opt = opt_end + 1)
     {
       if ((opt_end = strchr (opt, ',')) != NULL)
@@ -705,25 +747,6 @@ riscv_disassemble_insn (bfd_vma memaddr, insn_t word, disassemble_info *info)
   matched_op = NULL;
   op = riscv_hash[OP_HASH_IDX (word)];
 
-  /* If XLEN is not known, get its value from the ELF class.  */
-  if (info->mach == bfd_mach_riscv64)
-    xlen = 64;
-  else if (info->mach == bfd_mach_riscv32)
-    xlen = 32;
-  else if (info->section != NULL)
-    {
-      Elf_Internal_Ehdr *ehdr = elf_elfheader (info->section->owner);
-      xlen = ehdr->e_ident[EI_CLASS] == ELFCLASS64 ? 64 : 32;
-    }
-
-  /* If arch has the Zfinx extension, replace FPR with GPR.  */
-  if (riscv_subset_supports (&riscv_rps_dis, "zfinx"))
-    riscv_fpr_names = riscv_gpr_names;
-  else
-    riscv_fpr_names = riscv_gpr_names == riscv_gpr_names_abi
-			  ? riscv_fpr_names_abi
-			  : riscv_fpr_names_numeric;
-
   for (; op && op->name; op++)
     {
       /* Does the opcode match?  */
@@ -867,6 +890,7 @@ riscv_get_map_state (int n,
     {
       riscv_release_subset_list (&riscv_subsets);
       riscv_parse_subset (&riscv_rps_dis, arch);
+      init_riscv_dis_state_for_arch_and_options ();
     }
   return true;
 }
@@ -1063,14 +1087,9 @@ print_insn_riscv (bfd_vma memaddr, struct disassemble_info *info)
   enum riscv_seg_mstate mstate;
   int (*riscv_disassembler) (bfd_vma, insn_t, struct disassemble_info *);
 
-  if (info->disassembler_options != NULL)
-    {
-      parse_riscv_dis_options (info->disassembler_options);
-      /* Avoid repeatedly parsing the options.  */
-      info->disassembler_options = NULL;
-    }
-  else if (riscv_gpr_names == NULL)
-    set_default_riscv_dis_options ();
+  /* Guess and update XLEN if we haven't determined it yet.  */
+  if (xlen == 0)
+    update_riscv_dis_xlen (info);
 
   mstate = riscv_search_mapping_symbol (memaddr, info);
 
@@ -1132,9 +1151,32 @@ riscv_get_disassembler (bfd *abfd)
 
   riscv_release_subset_list (&riscv_subsets);
   riscv_parse_subset (&riscv_rps_dis, default_arch);
+  init_riscv_dis_state_for_arch_and_options ();
   return print_insn_riscv;
 }
 
+/* Initialize disassemble_info and parse options.  */
+
+void
+disassemble_init_riscv (struct disassemble_info *info)
+{
+  info->symbol_is_valid = riscv_symbol_is_valid;
+  /* Clear previous XLEN and guess by mach.  */
+  xlen = 0;
+  xlen_by_mach = 0;
+  xlen_by_elf = 0;
+  if (info->mach == bfd_mach_riscv64)
+    xlen_by_mach = 64;
+  else if (info->mach == bfd_mach_riscv32)
+    xlen_by_mach = 32;
+  update_riscv_dis_xlen (info);
+  /* Parse disassembler options.  */
+  set_default_riscv_dis_options ();
+  if (info->disassembler_options != NULL)
+    parse_riscv_dis_options (info->disassembler_options);
+  init_riscv_dis_state_for_arch_and_options ();
+}
+
 /* Prevent use of the fake labels that are generated as part of the DWARF
    and for relaxable relocations in the assembler.  */
 
-- 
2.37.2

[PATCH 10/11] RISC-V: Reorganize arch-related initialization and management

[Tsukasa OI]

objdump reuses the disassembler function returned by the disassembler
function.  That's good and benefits well from various optimizations.

However, by default, GDB (default_print_insn in gdb/arch-utils.c) assumes
that the disassembler function logic is simple and calls that function for
every instruction to be disassembled.  This is clearly a waste of time
because it probes BFD (ELF information) and re-initializes
riscv_rps_dis for every instruction.

After the support of mapping symbol with ISA string with commit 40f1a1a4564b
("RISC-V: Output mapping symbols with ISA string."), this kind of per-
instruction initialization of riscv_rps_dis can also occur on ELF files with
mapping symbol + ISA string (in riscv_get_map_state function).  It can be
worse if the object is assembled using Binutils commit 0ce50fc900a5
("RISC-V: Always generate mapping symbols at the start of the sections.")
or later.

To avoid repeated initialization, this commit

-   Caches the default / override architectures (in two "contexts") and
-   enables switching between them.

riscv_dis_arch_context_t and new utility functions are defined for this
purpose.  We still have to read the ".riscv.attributes" section on every
instruction on GDB but at least the most time-consuming part (updating the
actual architecture from the architecture string) is avoided.
Likewise, we still have to probe whether we have to update the architecture
for every instruction with a mapping symbol with ISA string is suitable but
at least we don't actually update the architecture unless necessary
(either context itself or the ISA string of the current context is changed).

This commit improves the disassembler performance well in those situations:

-   When long "disas" command is used on GDB
-   When ELF files with mapping symbols + ISA string is used

This commit now implements new mapping symbol handling ("$x" means revert
to the previous architecture [with "$x+arch"] if exists, otherwise revert to
the default one usually read from an ELF attribute), the recent consensus
made by Kito and Nelson.

On the benchmark using GDB batch files, the author measured significant
performance improvements (35-96% on various big RISC-V programs).
Unfortunately, on interactive usecases of GDB, this improvement is rarely
observable since we don't usually disassemble such a big chunk at once and
the current disassembler is not very slow.

On the benchmark using unstripped ELF files with mapping symbols + ISA
string "$xrv...", performance improvements are significant and easily
observable in the real world (150%-264% performance improvments).

Aside from optimization, this commit, along with "RISC-V: Reorganize
disassembler state initialization", makes state initialization clearer and
makes further changes easier.

Also, although not practical in the real world, this commit now allows
multi-XLEN object disassembling if the object file has mapping symbols
with ISA string and the machine is XLEN-neutral (e.g. objdump with
"-m riscv" option).  It may help testing Binutils / GAS.

opcodes/ChangeLog:

	* riscv-dis.c
	(initial_default_arch): Special default architecture
	string which is handled separately.
	(riscv_dis_arch_context_t): New type to manage RISC-V architecture
	context for the disassembler.  Two instance of this type is
	defined in this file - "default" and "override".
	(dis_arch_context_default): New.  Architecture context inferred
	from either an ELF attribute or initial_default_arch.
	(dis_arch_context_override): New.  Architecture context inferred
	from mapping symbols with ISA string.
	(dis_arch_context_current): New.  A pointer to either
	dis_arch_context_default or dis_arch_context_override.
	(riscv_rps_dis): Add summary.  Use initial values from the initial
	value of dis_arch_context_current - dis_arch_context_default.
	(from_last_map_symbol): Make it file scope to decide whether we
	should revert the architecture to the default in
	riscv_get_map_state function.
	(set_riscv_current_dis_arch_context): New function to update
	riscv_rps_dis and dis_arch_context_current.
	(set_riscv_dis_arch_context): New function to update the
	architecture for the given context.
	(update_riscv_dis_xlen): Consider dis_arch_context_current->xlen
	when guessing correct XLEN.
	(is_arch_changed): New.  Set to true if the architecture is
	changed.
	(init_riscv_dis_state_for_arch): New function to track whether
	the architecture string is changed.
	(init_riscv_dis_state_for_arch_and_options): Keep track of the
	architecture string change and update XLEN if it has changed.
	(update_riscv_dis_arch): New function to set both the architecture
	and the context.  Call initialization functions if needed.
	(riscv_get_map_state): Add update argument.  Keep track of the
	mapping symbols with ISA string and update the architecture and
	the context if required.
	(riscv_search_mapping_symbol): Move from_last_map_symbol to
	file scope.  Call riscv_get_map_state function with architecture
	and context updates enabled.
	(riscv_data_length): Call riscv_get_map_state function with
	architecture and context updates disabled.
	(riscv_get_disassembler): Add an error handling on Tag_RISCV_arch.
	Call update_riscv_dis_arch function to update the architecture
	and the context.

Co-developed-by: Nelson Chu <nelson@rivosinc.com>
---
 opcodes/riscv-dis.c | 174 ++++++++++++++++++++++++++++++++++++++------
 1 file changed, 152 insertions(+), 22 deletions(-)

diff --git a/opcodes/riscv-dis.c b/opcodes/riscv-dis.c
index 76387efbe4b..a57f4208f68 100644
--- a/opcodes/riscv-dis.c
+++ b/opcodes/riscv-dis.c
@@ -32,6 +32,9 @@
 #include <stdint.h>
 #include <ctype.h>
 
+/* Default architecture string (if not available).  */
+static const char *const initial_default_arch = "rv64gc";
+
 /* Current XLEN for the disassembler.  */
 static unsigned xlen = 0;
 
@@ -48,14 +51,36 @@ static enum riscv_spec_class default_isa_spec = ISA_SPEC_CLASS_DRAFT - 1;
    (as specified by the ELF attributes or the `priv-spec' option).  */
 static enum riscv_spec_class default_priv_spec = PRIV_SPEC_CLASS_NONE;
 
-static riscv_subset_list_t riscv_subsets;
+/* RISC-V disassembler architecture context type.  */
+typedef struct
+{
+  const char *arch_str;
+  const char *default_arch;
+  riscv_subset_list_t subsets;
+  unsigned xlen;
+  bool no_xlen_if_default;
+} riscv_dis_arch_context_t;
+
+/* Context: default (either initial_default_arch or ELF attribute).  */
+static riscv_dis_arch_context_t dis_arch_context_default
+    = { NULL, initial_default_arch, { }, 0, true };
+
+/* Context: override (mapping symbols with ISA string). */
+static riscv_dis_arch_context_t dis_arch_context_override
+    = { NULL, NULL, {}, 0, false };
+
+/* Pointer to the current disassembler architecture context.  */
+static riscv_dis_arch_context_t *dis_arch_context_current
+    = &dis_arch_context_default;
+
+/* RISC-V ISA string parser structure (current).  */
 static riscv_parse_subset_t riscv_rps_dis =
 {
-  &riscv_subsets,	/* subset_list.  */
-  opcodes_error_handler,/* error_handler.  */
-  &xlen,		/* xlen.  */
-  &default_isa_spec,	/* isa_spec.  */
-  false,		/* check_unknown_prefixed_ext.  */
+  &(dis_arch_context_default.subsets),	/* subset_list.  */
+  opcodes_error_handler,		/* error_handler.  */
+  &(dis_arch_context_default.xlen),	/* xlen.  */
+  &default_isa_spec,			/* isa_spec.  */
+  false,				/* check_unknown_prefixed_ext.  */
 };
 
 /* Private data structure for the RISC-V disassembler.  */
@@ -71,6 +96,7 @@ struct riscv_private_data
 /* Used for mapping symbols.  */
 static int last_map_symbol = -1;
 static bfd_vma last_stop_offset = 0;
+static bool from_last_map_symbol = false;
 
 /* Register names as used by the disassembler.  */
 static const char * const *riscv_gpr_names;
@@ -83,6 +109,59 @@ static bool no_aliases = false;
 static bool is_numeric = false;
 \f
 
+/* Set current disassembler context (dis_arch_context_current).
+   Return true if successfully updated.  */
+
+static bool
+set_riscv_current_dis_arch_context (riscv_dis_arch_context_t* context)
+{
+  if (context == dis_arch_context_current)
+    return false;
+  dis_arch_context_current = context;
+  riscv_rps_dis.subset_list = &(context->subsets);
+  riscv_rps_dis.xlen = &(context->xlen);
+  return true;
+}
+
+/* Update riscv_dis_arch_context_t by an ISA string.
+   Return true if the architecture is updated by arch.  */
+
+static bool
+set_riscv_dis_arch_context (riscv_dis_arch_context_t *context,
+			    const char *arch)
+{
+  /* Check whether the architecture is changed and
+     return false if the architecture will not be changed.  */
+  if (context->arch_str)
+    {
+      if (context->default_arch && arch == context->default_arch)
+	return false;
+      if (strcmp (context->arch_str, arch) == 0)
+	return false;
+    }
+  /* Update architecture string.  */
+  if (context->arch_str != context->default_arch)
+    free ((void *) context->arch_str);
+  context->arch_str = (arch != context->default_arch)
+			    ? xstrdup (arch)
+			    : context->default_arch;
+  /* Update other contents (subset list and XLEN).  */
+  riscv_subset_list_t *prev_subsets = riscv_rps_dis.subset_list;
+  unsigned *prev_xlen = riscv_rps_dis.xlen;
+  riscv_rps_dis.subset_list = &(context->subsets);
+  riscv_rps_dis.xlen = &(context->xlen);
+  context->xlen = 0;
+  riscv_release_subset_list (&context->subsets);
+  riscv_parse_subset (&riscv_rps_dis, context->arch_str);
+  riscv_rps_dis.subset_list = prev_subsets;
+  riscv_rps_dis.xlen = prev_xlen;
+  /* Special handling on the default architecture.  */
+  if (context->no_xlen_if_default && arch == context->default_arch)
+    context->xlen = 0;
+  return true;
+}
+\f
+
 /* Guess and update current XLEN.  */
 
 static void
@@ -95,9 +174,13 @@ update_riscv_dis_xlen (struct disassemble_info *info)
        This is only effective if XLEN-specific BFD machine architecture is
        chosen.  If XLEN-neutral (like riscv), BFD machine architecture is
        ignored on XLEN selection.
-     2. ELF class in dummy ELF header.  */
+     2. Non-default RISC-V architecture string set by either an ELF
+	attribute or a mapping symbol with ISA string.
+     3. ELF class in dummy ELF header.  */
   if (xlen_by_mach != 0)
     xlen = xlen_by_mach;
+  else if (dis_arch_context_current->xlen != 0)
+    xlen = dis_arch_context_current->xlen;
   else if (xlen_by_elf != 0)
     xlen = xlen_by_elf;
   else if (info != NULL && info->section != NULL)
@@ -107,11 +190,24 @@ update_riscv_dis_xlen (struct disassemble_info *info)
     }
 }
 
+/* Initialization (for arch).  */
+
+static bool is_arch_changed = false;
+
+static void
+init_riscv_dis_state_for_arch (void)
+{
+  is_arch_changed = true;
+}
+
 /* Initialization (for arch and options).  */
 
 static void
 init_riscv_dis_state_for_arch_and_options (void)
 {
+  /* If the architecture string is changed, update XLEN.  */
+  if (is_arch_changed)
+    update_riscv_dis_xlen (NULL);
   /* Set GPR register names to disassemble.  */
   riscv_gpr_names = is_numeric ? riscv_gpr_names_numeric : riscv_gpr_names_abi;
   /* Set FPR register names to disassemble.  */
@@ -119,6 +215,25 @@ init_riscv_dis_state_for_arch_and_options (void)
       = !riscv_subset_supports (&riscv_rps_dis, "zfinx")
 	    ? (is_numeric ? riscv_fpr_names_numeric : riscv_fpr_names_abi)
 	    : riscv_gpr_names;
+  /* Save previous options and mark them "unchanged".  */
+  is_arch_changed = false;
+}
+
+/* Update architecture for disassembler with its context.
+   Call initialization functions if either:
+   -  the architecture for current context is changed or
+   -  context is updated to a new one.  */
+
+static void
+update_riscv_dis_arch (riscv_dis_arch_context_t *context, const char *arch)
+{
+  if ((set_riscv_dis_arch_context (context, arch)
+       && dis_arch_context_current == context)
+      || set_riscv_current_dis_arch_context (context))
+    {
+      init_riscv_dis_state_for_arch ();
+      init_riscv_dis_state_for_arch_and_options ();
+    }
 }
 \f
 
@@ -872,7 +987,8 @@ riscv_get_map_state_by_name (const char *name, const char** arch)
 static bool
 riscv_get_map_state (int n,
 		     enum riscv_seg_mstate *state,
-		     struct disassemble_info *info)
+		     struct disassemble_info *info,
+		     bool update)
 {
   const char *name, *arch = NULL;
 
@@ -886,12 +1002,25 @@ riscv_get_map_state (int n,
   if (newstate == MAP_NONE)
     return false;
   *state = newstate;
-  if (arch)
-    {
-      riscv_release_subset_list (&riscv_subsets);
-      riscv_parse_subset (&riscv_rps_dis, arch);
-      init_riscv_dis_state_for_arch_and_options ();
-    }
+  if (newstate == MAP_INSN && update)
+  {
+    if (arch)
+      {
+	/* Override the architecture.  */
+	update_riscv_dis_arch (&dis_arch_context_override, arch);
+      }
+    else if (!from_last_map_symbol
+	     && set_riscv_current_dis_arch_context (&dis_arch_context_default))
+      {
+	/* Revert to the default architecture and call init functions if:
+	   - there's no ISA string in the mapping symbol,
+	   - mapping symbol is not reused and
+	   - current disassembler context is changed to the default one.
+	   This is a shortcut path to avoid full update_riscv_dis_arch.  */
+	init_riscv_dis_state_for_arch ();
+	init_riscv_dis_state_for_arch_and_options ();
+      }
+  }
   return true;
 }
 
@@ -903,7 +1032,6 @@ riscv_search_mapping_symbol (bfd_vma memaddr,
 			     struct disassemble_info *info)
 {
   enum riscv_seg_mstate mstate;
-  bool from_last_map_symbol;
   bool found = false;
   int symbol = -1;
   int n;
@@ -943,7 +1071,7 @@ riscv_search_mapping_symbol (bfd_vma memaddr,
       /* We have searched all possible symbols in the range.  */
       if (addr > memaddr)
 	break;
-      if (riscv_get_map_state (n, &mstate, info))
+      if (riscv_get_map_state (n, &mstate, info, true))
 	{
 	  symbol = n;
 	  found = true;
@@ -970,7 +1098,7 @@ riscv_search_mapping_symbol (bfd_vma memaddr,
 	  if (addr < (info->section ? info->section->vma : 0))
 	    break;
 	  /* Stop searching once we find the closed mapping symbol.  */
-	  if (riscv_get_map_state (n, &mstate, info))
+	  if (riscv_get_map_state (n, &mstate, info, true))
 	    {
 	      symbol = n;
 	      found = true;
@@ -1006,7 +1134,7 @@ riscv_data_length (bfd_vma memaddr,
 	{
 	  bfd_vma addr = bfd_asymbol_value (info->symtab[n]);
 	  if (addr > memaddr
-	      && riscv_get_map_state (n, &m, info))
+	      && riscv_get_map_state (n, &m, info, false))
 	    {
 	      if (addr - memaddr < length)
 		length = addr - memaddr;
@@ -1130,7 +1258,7 @@ print_insn_riscv (bfd_vma memaddr, struct disassemble_info *info)
 disassembler_ftype
 riscv_get_disassembler (bfd *abfd)
 {
-  const char *default_arch = "rv64gc";
+  const char *default_arch = initial_default_arch;
 
   if (abfd && bfd_get_flavour (abfd) == bfd_target_elf_flavour)
     {
@@ -1146,12 +1274,14 @@ riscv_get_disassembler (bfd *abfd)
 						  attr[Tag_c].i,
 						  &default_priv_spec);
 	  default_arch = attr[Tag_RISCV_arch].s;
+	  /* For ELF files with (somehow) no architecture string
+	     in the attributes, use the default value.  */
+	  if (!default_arch)
+	    default_arch = initial_default_arch;
 	}
     }
 
-  riscv_release_subset_list (&riscv_subsets);
-  riscv_parse_subset (&riscv_rps_dis, default_arch);
-  init_riscv_dis_state_for_arch_and_options ();
+  update_riscv_dis_arch (&dis_arch_context_default, default_arch);
   return print_insn_riscv;
 }
 
-- 
2.37.2

[PATCH 11/11] RISC-V: Move disassembler private data initialization

[Tsukasa OI]

Because disassemble_info.private_data could be used not only by
riscv_disassemble_insn, this commit splits the initialization of the
private data to a separate function.

This commit now allows storing mapping symbol and/or section-related
information to riscv_private_data.

In performance perspective, it also has a penalty.  However, it can be
easily paid back by other optimizations and it makes implementing
some optimizations easier.

opcodes/ChangeLog:

	* riscv-dis.c (init_riscv_dis_private_data): New.
	(riscv_disassemble_insn): Move private data initialization to
	init_riscv_dis_private_data.
	(print_insn_riscv): Start initializing the private data
	instead of instruction only riscv_disassemble_insn function.
---
 opcodes/riscv-dis.c | 51 +++++++++++++++++++++++++--------------------
 1 file changed, 28 insertions(+), 23 deletions(-)

diff --git a/opcodes/riscv-dis.c b/opcodes/riscv-dis.c
index a57f4208f68..80eb8cbbea4 100644
--- a/opcodes/riscv-dis.c
+++ b/opcodes/riscv-dis.c
@@ -219,6 +219,29 @@ init_riscv_dis_state_for_arch_and_options (void)
   is_arch_changed = false;
 }
 
+/* Initialize private data of the disassemble_info.  */
+
+static void
+init_riscv_dis_private_data (struct disassemble_info *info)
+{
+  struct riscv_private_data *pd;
+
+  pd = info->private_data = xcalloc (1, sizeof (struct riscv_private_data));
+  pd->gp = 0;
+  pd->print_addr = 0;
+  for (int i = 0; i < (int)ARRAY_SIZE (pd->hi_addr); i++)
+    pd->hi_addr[i] = -1;
+  pd->to_print_addr = false;
+  pd->has_gp = false;
+
+  for (int i = 0; i < info->symtab_size; i++)
+    if (strcmp (bfd_asymbol_name (info->symtab[i]), RISCV_GP_SYMBOL) == 0)
+      {
+	pd->gp = bfd_asymbol_value (info->symtab[i]);
+	pd->has_gp = true;
+      }
+}
+
 /* Update architecture for disassembler with its context.
    Call initialization functions if either:
    -  the architecture for current context is changed or
@@ -806,7 +829,7 @@ riscv_disassemble_insn (bfd_vma memaddr, insn_t word, disassemble_info *info)
   const struct riscv_opcode *op, *matched_op;
   static bool init = false;
   static const struct riscv_opcode *riscv_hash[OP_MASK_OP + 1];
-  struct riscv_private_data *pd;
+  struct riscv_private_data *pd = info->private_data;
   int insnlen;
 
 #define OP_HASH_IDX(i) ((i) & (riscv_insn_length (i) == 2 ? 0x3 : OP_MASK_OP))
@@ -821,28 +844,6 @@ riscv_disassemble_insn (bfd_vma memaddr, insn_t word, disassemble_info *info)
       init = true;
     }
 
-  if (info->private_data == NULL)
-    {
-      int i;
-
-      pd = info->private_data = xcalloc (1, sizeof (struct riscv_private_data));
-      pd->gp = 0;
-      pd->print_addr = 0;
-      for (i = 0; i < (int)ARRAY_SIZE (pd->hi_addr); i++)
-	pd->hi_addr[i] = -1;
-      pd->to_print_addr = false;
-      pd->has_gp = false;
-
-      for (i = 0; i < info->symtab_size; i++)
-	if (strcmp (bfd_asymbol_name (info->symtab[i]), RISCV_GP_SYMBOL) == 0)
-	  {
-	    pd->gp = bfd_asymbol_value (info->symtab[i]);
-	    pd->has_gp = true;
-	  }
-    }
-  else
-    pd = info->private_data;
-
   insnlen = riscv_insn_length (word);
 
   /* RISC-V instructions are always little-endian.  */
@@ -1215,6 +1216,10 @@ print_insn_riscv (bfd_vma memaddr, struct disassemble_info *info)
   enum riscv_seg_mstate mstate;
   int (*riscv_disassembler) (bfd_vma, insn_t, struct disassemble_info *);
 
+  /* Initialize the private data.  */
+  if (info->private_data == NULL)
+    init_riscv_dis_private_data (info);
+
   /* Guess and update XLEN if we haven't determined it yet.  */
   if (xlen == 0)
     update_riscv_dis_xlen (info);
-- 
2.37.2

[PATCH v2 00/11] RISC-V: Requirements for disassembler optimizations batch 1

[Tsukasa OI]

Hello,

To improve the core disassembler (both for performance and feature), this
patchset now prepares for further optimization.

It will be a prerequisite of upcoming core disassembler changes including
two major optimizations.

Tracker on GitHub (with detailed benchmark results):
<https://github.com/a4lg/binutils-gdb/wiki/riscv_dis_opt1_req_reorganize>

This is the part 2 of 4-part project:
<https://github.com/a4lg/binutils-gdb/wiki/proj_dis_perf_improvements_1>

[Changes: v1 -> v2]
-   Rebased against commit 97f006bc56af:
    "RISC-V: Better support for long instructions (disassembler)"


Individual changes are detailed in each patch.
In this cover letter, I will explain...

-   Aggregate Performance Improvements
-   Some Backgrounds behind some patches


1.  Aggregate Performance Improvements
2.  Split Match/Print Steps on the Disassembler (PATCH 8/11)
3.  State Initialization (PATCH 9/11)
    3.1. Disassembler Lifecycle
    3.2. Moving the Initialization
    3.3. Fixed Issues as a Side Effect
    3.4. Fixed Issue 1: Unsetting Options on GDB (and new smaller issue)
    3.5. Fixed Issue 2: Location of Error Message
4.  Architecture Initialization and Management (PATCH 10/11)



Aggregate Performance Improvements
===================================

Since commit 40f1a1a4564b ("RISC-V: Output mapping symbols with ISA string."
), the RISC-V disassembler was slower than before.  I'm not blaming this
because it has a good reason to be changed (to support mapping symbols with
ISA string).

Despite that this patchset itself is not main optimization part, it roughly
restores the disassembler performance before that commit (if the input ELF
file contains mapping symbols with ISA string, this patchset makes
disassembling such files significantly faster).

Note that, this patchset contains two "optimization" commits that
significantly (>30%) improves the performance under certain conditions:

-   When thousands of CSR instructions are being disassembled
    (PATCH 5/11)
-   When disassembling large chunk of code using GDB (not objdump)
    (PATCH 10/11)
-   When the input has mapping symbols with ISA string (both on GDB/objdump)
    (PATCH 10/11)



Split Match/Print Steps on the Disassembler (PATCH 8/11)
=========================================================

This is more like a future-proof commit.

riscv_disassemble_insn function (before this patch) works like this:

    FOREACH (opcode)
    {
        if (!opcode.MATCH(insn))
            continue;
        //
        // Print insn using matched opcode info.
        //
        return insnlen;
    }
    //
    // opcode not matched.
    //

But if we split this to two steps: matching and printing like this:

    matched_opcode = NULL;
    // Step 1: opcode matching
    FOREACH (opcode)
    {
        if (!opcode.MATCH(insn))
            continue;
        matched_opcode = opcode;
        break;
    }
    // Step 2: opcode printing
    if (matched_opcode != NULL)
    {
        //
        // Print insn using matched_opcode info.
        //
        return insnlen;
    }
    //
    // opcode not matched.
    //

We can independently improve the opcode matching step and makes a room to
implement more complex opcode matching.  Despite that this patch alone
imposes a small amount of performance penalty, it can be easily paid back by
other optimizations.



State Initialization (PATCH 9/11)
==================================

Disassembler Lifecycle
-----------------------

In arch-independent libopcodes, we use following functions to disassemble
the object file/memory range:

-   disassembler
    To get real disassembler function for specific BFD and architecture.
    The result is print_insn_riscv on RISC-V.
-   (the result of "disassembler")
    as explained above.
-   disassemble_init_for_target
    This function initializes disassemble_info structure (e.g. allocates
    target-specific private data and sets certain fields).
-   disassemble_free_for_target
    This function frees private_data in the disassemble_info.  It also
    allows the target to run custom logic.

Ideally, all disassembler state should be contained within
disassemble_info.private_data (like in PowerPC) but many (including Arm,
MIPS and RISC-V) don't do that.  It means that we should handle this
abstract "disassembler" as a singleton for now.

It's used from objdump (binutils) and GDB but note that they have a
different execution order.  This is shown below:

objdump (completely target-independent):
    (Source)
    -   binutils/objdump.c
    (Note)
    -   This cycle happens per BFD object.  This is usually once on objdump
        but may occur twice or more (or even 0 times) on archive files.
    (Initialization Step)
    a1. Call "disassembler" function
        (with input BFD, arch, endian and mach)
    a2. Set initial disassemble_info structure contents
        (including flavour, arch, mach and disassembler_options)
    a3. Call disassemble_init_for_target
    (For Every Instruction)
    b1. Call the result of "disassembler" (print_insn_riscv on RISC-V)
    (Finalization Step)
    c1. Call disassemble_free_for_target

GDB (some may change through gdbarch but this is the general sequence):
    (Source)
    -   gdb/disasm.c
    -   gdb/disasm.h
    -   gdb/gdbarch.c
    -   gdb/arch-utils.c
    (Note)
    -   This cycle happens per one disassembler request
        (e.g. per one "disas" command).
    -   A disassembler lifecycle is represented as a
        gdb_disassemble_info class object.
    (Initialization Step - constructor)
    a1. Set initial disassemble_info structure contents
        (including flavour [unknown], arch, mach and disassembler_options)
    a2. Call disassemble_init_for_target
    (For Every Instruction - by default)
        Call trace:
        1.  gdb_disassembler::print_insn method
            (gdb_disassembler is a subclass of gdb_disassemble_info)
        2.  gdb_print_insn_1 function
        3.  gdbarch_print_insn
        4.  gdbarch->print_insn
            (default_print_insn by default but target-overridable)
        5.  default_print_insn (by default)
    b1. Call "disassembler" function
        (with input BFD, and arch, endian and mach as in disassemble_info)
    b2. Call the result of "disassembler" (print_insn_riscv on RISC-V)
    (Finalization Step - destructor)
    c1. Call disassemble_free_for_target

In both usecases, disassembler_options, arch and mach are set only once
(before disassemble_init_for_target) and never changes in multiple
print_insn_riscv calls.  That means, we are safe to parse some
disassemble_info fields such as mach and disassembler_options in
disassemble_init_for_target (it's even expected to do that, considering a
side effect I will explain later).

Note that we need to take care of two possible calling order:

-   disassembler -> disassemble_init_for_target (objdump)
-   disassemble_init_for_target -> disassembler (GDB)

Moving the Initialization
--------------------------

For instance, PowerPC and WebAssembly targets parse disassembler options in
the disassembler_init_for_target call (PowerPC parses mach, too).

Many architectures instead try to initialize its state on per-instruction
call (in the "result of disassembler" call to be specific) but it's
clearly a waste of time and only a tradition (a bad one, to be honest).

As objdump works well in PowerPC and WebAssembly and GDB works well in
PowerPC (note: GDB for WebAssembly is not implemented), moving RISC-V's
disassembler state initialization (including parsing disassembler_options)
is safe as PowerPC and makes state management simpler and faster.  Some of
my optimizations depend on this change otherwise will be too complex and/or
doesn't speed up the disassembler as expected.

But not everything can be moved.  Because symbol table in the
disassemble_info (symtab, symtab_size...) is not available on
disassembler_init_for_target and disassemble_info object is not available
on "diassembler", we have no stable way to initialize pd->gp prior to the
first riscv_disassemble_insn call.  So, info->private_data == NULL
branch on riscv_disassemble_insn is preserved.

Fixed Issues as a Side Effect
------------------------------

Also, changing RISC-V disassembler like this has two good side effects:

1.  We can really "unset" the disassembler options in the GDB
2.  Error message regarding the disassembler options is at the right place
    on objdump

Fixed Issue 1: Unsetting Options on GDB (and new smaller issue)
----------------------------------------------------------------

First, GDB expects to allow "set disassembler-options XXX..." to set
disassembler options "XXX..." **and** "set disassembler-options" to
**unset** them, reverting to the default.  It didn't work like this in
RISC-V but my patchset fixes this issue as a side effect.

Before:
    (gdb) set disassembler-options
    (gdb) disas _start
    Dump of assembler code for function _start:
    0x0000000080000028 <+0>:     ret
    End of assembler dump.
    (gdb) set disassembler-options no-aliases
    (gdb) disas _start
    Dump of assembler code for function _start:
    0x0000000080000028 <+0>:     c.jr    ra
    End of assembler dump.
    (gdb) set disassembler-options
    (gdb) disas _start
    Dump of assembler code for function _start:
    0x0000000080000028 <+0>:     c.jr    ra     <-- should be "ret"!
    End of assembler dump.

After (snip):
    (gdb) set disassembler-options              <-- 1st/2nd time
    (gdb) disas _start
    Dump of assembler code for function _start:
    0x0000000080000028 <+0>:     ret            <-- restored!
    End of assembler dump.

This is because, after "set disassembler-options" to unset disassembler
options, disassemble_info.disassembler_options is set to NULL, not an empty
string.  Because current print_insn_riscv function checks whether
disassembler_options is NULL and parses the options when it's not,
"set disassembler-options" works as "do nothing" (instead of
"reset to default") in RISC-V.

Beware that, PATCH 9/11 introduces a smaller bug in GDB which suppresses an
error message when mismatched "priv-spec" option is specified once before
the first disassembly.  In the following example, we assume that we are
debugging an ELF file with privileged specification 1.11 ELF attributes.

It will not occur in objdump.

Before (okay):
    (gdb) file test.elf
    (gdb) set disassembler-options priv-spec=1.12
    (gdb) disas _start
    Dump of assembler code for function _start:
    BFD: mis-matched privilege spec set by priv-spec=1.12, the elf privilege attribute is 1.11
    0x0000000080000028 <+0>:     csrr    a0,0x3d0
                                            ^^^^^
                                    "pmpaddr32" CSR but not defined in 1.11.
    0x000000008000002c <+4>:     ret
    End of assembler dump.
    (gdb) disas _start
    Dump of assembler code for function _start:
    BFD: mis-matched privilege spec set by priv-spec=1.12, the elf privilege attribute is 1.11
    0x0000000080000028 <+0>:     csrr    a0,0x3d0
    0x000000008000002c <+4>:     ret
    End of assembler dump.

After PATCH 9/11 (an error message is suppressed):
    (gdb) file test.elf
    (gdb) set disassembler-options priv-spec=1.12
    (gdb) disas _start
    Dump of assembler code for function _start:
    0x0000000080000028 <+0>:     csrr    a0,0x3d0
                                            ^^^^^
                                    "pmpaddr32" CSR but not defined in 1.11.
                        An error message just before this line is suppressed
                          but "priv-spec" override does not actually happen.
    0x000000008000002c <+4>:     ret
    End of assembler dump.
    (gdb) disas _start
    Dump of assembler code for function _start:
    BFD: mis-matched privilege spec set by priv-spec=1.12, the elf privilege attribute is 1.11
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
                           correctly printed at the second time (and later).
    0x0000000080000028 <+0>:     csrr    a0,0x3d0
    0x000000008000002c <+4>:     ret
    End of assembler dump.

This new smaller issue is taken care in my upcoming patchset.

Fixed Issue 2: Location of Error Message
-----------------------------------------

Second, because PATCH 9/11 changes where error message is generated, this
patch makes error message location (when an invalid disassembler option is
specified) more "right", more sensible.

Before:

    test.elf:     file format elf64-littleriscv


    Disassembly of section .text:

    0000000080000000 <__start>:
        80000000:   riscv64-unknown-elf-objdump: mis-matched privilege spec set by priv-spec=1.12, the elf privilege attribute is 1.11
    00002197                auipc   gp,0x2
        80000004:   82e18193                addi    gp,gp,-2002 # 8000182e <__global_pointer$>
        ...

After:

    test.elf:     file format elf64-littleriscv

    riscv64-unknown-elf-objdump: mis-matched privilege spec set by priv-spec=1.12, the elf privilege attribute is 1.11

    Disassembly of section .text:

    0000000080000000 <__start>:
        80000000:   00002197                auipc   gp,0x2
        80000004:   82e18193                addi    gp,gp,-2002 # 8000182e <__global_pointer$>
        ...


Architecture Initialization and Management (PATCH 10/11)
=========================================================

This commit makes major refinements to the architecture initialization
and management code.  The purpose of this is to:

-   Provide easy interface to initialize when the architecture is changed
-   Enable switching between default architecture (from an ELF attribute
    or from default string "rv64gc") and the architecture override
    (from mapping symbol with ISA string).
-   Avoid repeated initialization cost.

Repeated architecture (riscv_rps_dis reinitialization) can occur per
instruction on following cases:

-   When disassembling with GDB (does not occur on objdump due to the
    execution order of libopcodes shown in the PATCH 9/11 section)
-   When an input ELF file has mapping symbols with ISA string.

Like Nelson's proposal tried to enable switching using riscv_file_subsets,
my PATCH 9/11 provides new type "context" to store riscv_subset_list_t,
XLEN and management-related variables.

The major differences between this change and Nelson's are:

-   PATCH 10/11 does not reinitialize the architecture (riscv_rps_dis) if
    the architecture string is not changed.  This makes a huge difference
    in performance.
-   PATCH 10/11 stores "XLEN per context".

The second property provides interesting "feature" although not realistic
in the real world: multi-XLEN object disassembling.  If an object has
mapping symbols with ISA string, we can switch between architectures,
EVEN IF XLEN HAS CHANGED (e.g. "$xrv32i" and "$xrv64i").

It's not the default behavior (by default, a XLEN-specific machine either
"riscv:rv32" or "riscv:rv64" is set and *this* XLEN is preferred than the
mapping symbols) and requires explicitly specifying the XLEN-neutral RISC-V
machine (objdump: "-m riscv").  Although it's not realistic in the real
world, it may help testing Binutils / GAS.




Tsukasa OI (11):
  opcodes/riscv-dis.c: More tidying
  RISC-V: Add test for 'Zfinx' register switching
  RISC-V: Make mapping symbol checking consistent
  RISC-V: Split riscv_get_map_state into two steps
  RISC-V: One time CSR hash table initialization
  RISC-V: Use static xlen on ADDIW sequence
  opcodes/riscv-dis.c: Add form feed for separation
  RISC-V: Split match/print steps on disassembler
  RISC-V: Reorganize disassembler state initialization
  RISC-V: Reorganize arch-related initialization and management
  RISC-V: Move disassembler private data initialization

 gas/testsuite/gas/riscv/mapping-dis.d     |   7 +
 gas/testsuite/gas/riscv/mapping-symbols.d |   4 +
 gas/testsuite/gas/riscv/mapping.s         |  10 +
 include/dis-asm.h                         |   1 +
 opcodes/disassemble.c                     |   2 +-
 opcodes/riscv-dis.c                       | 485 +++++++++++++++-------
 6 files changed, 366 insertions(+), 143 deletions(-)


base-commit: c341f4676af4f9922ca61e1b093d103ed808ae6e
-- 
2.38.1

[PATCH v2 01/11] opcodes/riscv-dis.c: More tidying

[Tsukasa OI]

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

This is a general tidying commit.

opcodes/ChangeLog:

	* riscv-dis.c (struct riscv_private_data) Add summary.
	Make length of hi_addr more meaningful.
---
 opcodes/riscv-dis.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/opcodes/riscv-dis.c b/opcodes/riscv-dis.c
index 0e1f3b4610aa..768b44ee6003 100644
--- a/opcodes/riscv-dis.c
+++ b/opcodes/riscv-dis.c
@@ -52,11 +52,12 @@ static riscv_parse_subset_t riscv_rps_dis =
   false,		/* check_unknown_prefixed_ext.  */
 };
 
+/* Private data structure for the RISC-V disassembler.  */
 struct riscv_private_data
 {
   bfd_vma gp;
   bfd_vma print_addr;
-  bfd_vma hi_addr[OP_MASK_RD + 1];
+  bfd_vma hi_addr[NGPR];
   bool to_print_addr;
   bool has_gp;
 };
-- 
2.38.1

[PATCH v2 02/11] RISC-V: Add test for 'Zfinx' register switching

[Tsukasa OI]

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

Because the author is going to reorganize core RISC-V disassembler, we have
to make sure that nothing is broken when disassembling with mapping symbols
with ISA string.

This commit adds a testcase for 'F' and 'Zfinx' instructions to make sure
that "FPR" register names are correctly switched when necessary.

gas/ChangeLog:

	* testsuite/gas/riscv/mapping.s: Add 'F' and 'Zfinx' testcase.
	* testsuite/gas/riscv/mapping-dis.d: Likewise.
	* testsuite/gas/riscv/mapping-symbols.d: Likewise.
---
 gas/testsuite/gas/riscv/mapping-dis.d     |  7 +++++++
 gas/testsuite/gas/riscv/mapping-symbols.d |  4 ++++
 gas/testsuite/gas/riscv/mapping.s         | 10 ++++++++++
 3 files changed, 21 insertions(+)

diff --git a/gas/testsuite/gas/riscv/mapping-dis.d b/gas/testsuite/gas/riscv/mapping-dis.d
index b1a26fbd151b..f0508499b726 100644
--- a/gas/testsuite/gas/riscv/mapping-dis.d
+++ b/gas/testsuite/gas/riscv/mapping-dis.d
@@ -91,3 +91,10 @@ Disassembly of section .text.relax.align:
 [ 	]+[0-9a-f]+:[ 	]+00000013[ 	]+nop
 [ 	]+[0-9a-f]+:[ 	]+00200513[ 	]+li[ 	]+a0,2
 [ 	]+[0-9a-f]+:[ 	]+00000013[ 	]+nop
+
+Disassembly of section .text.dis.zfinx:
+
+0+000 <.text.dis.zfinx>:
+[ 	]+[0-9a-f]+:[ 	]+00c5f553[ 	]+fadd\.s[ 	]+fa0,fa1,fa2
+[ 	]+[0-9a-f]+:[ 	]+00c5f553[ 	]+fadd\.s[ 	]+a0,a1,a2
+[ 	]+[0-9a-f]+:[ 	]+00c5f553[ 	]+fadd\.s[ 	]+fa0,fa1,fa2
diff --git a/gas/testsuite/gas/riscv/mapping-symbols.d b/gas/testsuite/gas/riscv/mapping-symbols.d
index 40df34097369..b28e3306b1b4 100644
--- a/gas/testsuite/gas/riscv/mapping-symbols.d
+++ b/gas/testsuite/gas/riscv/mapping-symbols.d
@@ -42,6 +42,10 @@ SYMBOL TABLE:
 0+00 l    d  .text.relax.align	0+00 .text.relax.align
 0+00 l       .text.relax.align	0+00 \$xrv32i2p1_c2p0
 0+08 l       .text.relax.align	0+00 \$xrv32i2p1
+0+00 l    d  .text.dis.zfinx	0+00 .text.dis.zfinx
+0+00 l       .text.dis.zfinx	0+00 \$xrv32i2p1_f2p2_zicsr2p0
+0+04 l       .text.dis.zfinx	0+00 \$xrv32i2p1_zicsr2p0_zfinx1p0
+0+08 l       .text.dis.zfinx	0+00 \$xrv32i2p1_f2p2_zicsr2p0
 0+0a l       .text.section.padding	0+00 \$x
 0+03 l       .text.odd.align.start.insn	0+00 \$d
 0+04 l       .text.odd.align.start.insn	0+00 \$x
diff --git a/gas/testsuite/gas/riscv/mapping.s b/gas/testsuite/gas/riscv/mapping.s
index 3014a69e7920..4fee2b420f0c 100644
--- a/gas/testsuite/gas/riscv/mapping.s
+++ b/gas/testsuite/gas/riscv/mapping.s
@@ -119,3 +119,13 @@ addi	a0, zero, 1		# $x, won't added
 .align	3			# $x, won't added
 addi	a0, zero, 2		# $xrv32i
 .option pop
+
+.section .text.dis.zfinx, "ax"
+.option push
+.option arch, rv32if
+fadd.s	fa0, fa1, fa2		# $xrv32if
+.option arch, rv32i_zfinx
+fadd.s	a0, a1, a2		# $xrv32i_zfinx
+.option arch, rv32if
+fadd.s	fa0, fa1, fa2		# $xrv32if
+.option pop
-- 
2.38.1

[PATCH v2 03/11] RISC-V: Make mapping symbol checking consistent

[Tsukasa OI]

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

There were two places where the mapping symbols are checked but had
different conditions.

-    riscv_get_map_state:          "$d" or starts with "$x"
-    riscv_elf_is_mapping_symbols: Starts with either "$x" or "$d"

Considering recent mapping symbol proposal, it's better to make symbol
checking consistent (whether the symbol _starts_ with "$[xd]").

It only checks prefix "$xrv" (mapping symbol with ISA string) only when the
prefix "$x" is matched.

opcodes/ChangeLog:

	* riscv-dis.c (riscv_get_map_state): Change the condition for
	consistency.
---
 opcodes/riscv-dis.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/opcodes/riscv-dis.c b/opcodes/riscv-dis.c
index 768b44ee6003..f6fdd5badfe6 100644
--- a/opcodes/riscv-dis.c
+++ b/opcodes/riscv-dis.c
@@ -834,16 +834,17 @@ riscv_get_map_state (int n,
     return false;
 
   name = bfd_asymbol_name(info->symtab[n]);
-  if (strcmp (name, "$x") == 0)
-    *state = MAP_INSN;
-  else if (strcmp (name, "$d") == 0)
-    *state = MAP_DATA;
-  else if (strncmp (name, "$xrv", 4) == 0)
+  if (startswith (name, "$x"))
     {
+      if (startswith (name + 2, "rv"))
+	{
+	  riscv_release_subset_list (&riscv_subsets);
+	  riscv_parse_subset (&riscv_rps_dis, name + 2);
+	}
       *state = MAP_INSN;
-      riscv_release_subset_list (&riscv_subsets);
-      riscv_parse_subset (&riscv_rps_dis, name + 2);
     }
+  else if (startswith (name, "$d"))
+    *state = MAP_DATA;
   else
     return false;
 
-- 
2.38.1

[PATCH v2 04/11] RISC-V: Split riscv_get_map_state into two steps

[Tsukasa OI]

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

Because mapping symbol optimization would remove riscv_get_map_state
function, this commit splits symbol name checking step into a separate
function riscv_get_map_state_by_name.

Let alone the optimization, splitting the code improves readability.

opcodes/ChangeLog:

	* riscv-dis.c (riscv_get_map_state): Split symbol name checking
	into a separate function.  (riscv_get_map_state_by_name): New.
---
 opcodes/riscv-dis.c | 41 +++++++++++++++++++++++++++--------------
 1 file changed, 27 insertions(+), 14 deletions(-)

diff --git a/opcodes/riscv-dis.c b/opcodes/riscv-dis.c
index f6fdd5badfe6..38eb91349d9a 100644
--- a/opcodes/riscv-dis.c
+++ b/opcodes/riscv-dis.c
@@ -818,6 +818,24 @@ riscv_disassemble_insn (bfd_vma memaddr,
   return insnlen;
 }
 
+/* Return new mapping state if a given symbol name is of mapping symbols',
+   MAP_NONE otherwise.  If arch is not NULL and name denotes a mapping symbol
+   with ISA string, *arch is updated to the ISA string.  */
+
+static enum riscv_seg_mstate
+riscv_get_map_state_by_name (const char *name, const char** arch)
+{
+  if (startswith (name, "$x"))
+    {
+      if (arch && startswith (name + 2, "rv"))
+	*arch = name + 2;
+      return MAP_INSN;
+    }
+  else if (startswith (name, "$d"))
+    return MAP_DATA;
+  return MAP_NONE;
+}
+
 /* Return true if we find the suitable mapping symbol,
    and also update the STATE.  Otherwise, return false.  */
 
@@ -826,28 +844,23 @@ riscv_get_map_state (int n,
 		     enum riscv_seg_mstate *state,
 		     struct disassemble_info *info)
 {
-  const char *name;
+  const char *name, *arch = NULL;
 
   /* If the symbol is in a different section, ignore it.  */
   if (info->section != NULL
       && info->section != info->symtab[n]->section)
     return false;
 
-  name = bfd_asymbol_name(info->symtab[n]);
-  if (startswith (name, "$x"))
+  name = bfd_asymbol_name (info->symtab[n]);
+  enum riscv_seg_mstate newstate = riscv_get_map_state_by_name (name, &arch);
+  if (newstate == MAP_NONE)
+    return false;
+  *state = newstate;
+  if (arch)
     {
-      if (startswith (name + 2, "rv"))
-	{
-	  riscv_release_subset_list (&riscv_subsets);
-	  riscv_parse_subset (&riscv_rps_dis, name + 2);
-	}
-      *state = MAP_INSN;
+      riscv_release_subset_list (&riscv_subsets);
+      riscv_parse_subset (&riscv_rps_dis, arch);
     }
-  else if (startswith (name, "$d"))
-    *state = MAP_DATA;
-  else
-    return false;
-
   return true;
 }
 
-- 
2.38.1

[PATCH v2 05/11] RISC-V: One time CSR hash table initialization

[Tsukasa OI]

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

The current disassembler intends to initialize the CSR hash table when CSR
name parsing is required at the first time. This is managed by a function-
scope static variable init_csr but... there's a problem.

It's never set to true.

Because of this issue, current disassembler actually initializes the CSR
hash table every time when CSR name parsing is required.  This commit sets
init_csr to true once the CSR hash table is initialized.

It is expected to have about 30% performance improvements alone when
thousands of only CSR instructions are disassembled (CSR instructions are
rare in general so real world performance improvement is not that high).

This commit alone will not affect real world performance that much but
after the efficient opcode hash is implemented, it will be much effective
(sometimes >x10 effect than this commit alone) so that even some regular
programs can benefit from it.

opcodes/ChangeLog:

	* riscv-dis.c (print_insn_args): Make sure that CSR hash table
	initialization occurs only once.
---
 opcodes/riscv-dis.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/opcodes/riscv-dis.c b/opcodes/riscv-dis.c
index 38eb91349d9a..51a08d847b10 100644
--- a/opcodes/riscv-dis.c
+++ b/opcodes/riscv-dis.c
@@ -564,6 +564,7 @@ print_insn_args (const char *oparg, insn_t l, bfd_vma pc, disassemble_info *info
 		DECLARE_CSR (name, num, class, define_version, abort_version)
 #include "opcode/riscv-opc.h"
 #undef DECLARE_CSR
+		init_csr = true;
 	      }
 
 	    if (riscv_csr_hash[csr] != NULL)
-- 
2.38.1

[PATCH v2 06/11] RISC-V: Use static xlen on ADDIW sequence

[Tsukasa OI]

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

Because XLEN for the disassembler is computed and stored in the xlen
variable, this commit replaces uses of info->mach with xlen
(when testing for ADDIW / C.ADDIW address sequence).

Not just we used two ways to determine current XLEN, info->mach and xlen,
xlen is going to be more important in the future commits.

opcodes/ChangeLog:

	* riscv-dis.c (print_insn_args): Use xlen variable to determine
	whether XLEN is larger than 32.
---
 opcodes/riscv-dis.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/opcodes/riscv-dis.c b/opcodes/riscv-dis.c
index 51a08d847b10..4c193624039e 100644
--- a/opcodes/riscv-dis.c
+++ b/opcodes/riscv-dis.c
@@ -261,7 +261,7 @@ print_insn_args (const char *oparg, insn_t l, bfd_vma pc, disassemble_info *info
 	    case 'j':
 	      if (((l & MASK_C_ADDI) == MATCH_C_ADDI) && rd != 0)
 		maybe_print_address (pd, rd, EXTRACT_CITYPE_IMM (l), 0);
-	      if (info->mach == bfd_mach_riscv64
+	      if (xlen > 32
 		  && ((l & MASK_C_ADDIW) == MATCH_C_ADDIW) && rd != 0)
 		maybe_print_address (pd, rd, EXTRACT_CITYPE_IMM (l), 1);
 	      print (info->stream, dis_style_immediate, "%d",
@@ -461,7 +461,7 @@ print_insn_args (const char *oparg, insn_t l, bfd_vma pc, disassemble_info *info
 	  if (((l & MASK_ADDI) == MATCH_ADDI && rs1 != 0)
 	      || (l & MASK_JALR) == MATCH_JALR)
 	    maybe_print_address (pd, rs1, EXTRACT_ITYPE_IMM (l), 0);
-	  if (info->mach == bfd_mach_riscv64
+	  if (xlen > 32
 	      && ((l & MASK_ADDIW) == MATCH_ADDIW) && rs1 != 0)
 	    maybe_print_address (pd, rs1, EXTRACT_ITYPE_IMM (l), 1);
 	  print (info->stream, dis_style_immediate, "%d",
-- 
2.38.1

[PATCH v2 07/11] opcodes/riscv-dis.c: Add form feed for separation

[Tsukasa OI]

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

This is a general tidying commit.

opcodes/ChangeLog:

	* riscv-dis.c: Add lines with form feed to separate functional
	sections.
---
 opcodes/riscv-dis.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/opcodes/riscv-dis.c b/opcodes/riscv-dis.c
index 4c193624039e..f5dc1907c6ec 100644
--- a/opcodes/riscv-dis.c
+++ b/opcodes/riscv-dis.c
@@ -72,7 +72,7 @@ static const char * const *riscv_fpr_names;
 
 /* If set, disassemble as most general instruction.  */
 static bool no_aliases = false;
-
+\f
 
 /* Set default RISC-V disassembler options.  */
 
@@ -174,6 +174,7 @@ parse_riscv_dis_options (const char *opts_in)
 
   free (opts);
 }
+\f
 
 /* Print one argument from an array.  */
 
-- 
2.38.1

[PATCH v2 08/11] RISC-V: Split match/print steps on disassembler

[Tsukasa OI]

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

For further optimization and more disassembler features, we may need to
change the core RISC-V instruction matching.  For this purpose, it is
inconvenient to have "match" and "print" steps in the same loop.

This commit rewrites riscv_disassemble_insn function so that we store
matched_op for matching RISC-V opcode and then print it (if not NULL).

Although it looks a bit inefficient, it also lowers the indent of opcode
matching loop to clarify the opcode matching changes on the next
optimization commit.

Unfortunately, this commit alone will impose some performance penalty (<5%
on most cases but sometimes about 15% worse) but it can be easily paid back
by other optimizations.

opcodes/ChangeLog:

	* riscv-dis.c (riscv_disassemble_insn): Split instruction
	handling to two separate steps - opcode matching and printing.
---
 opcodes/riscv-dis.c | 151 +++++++++++++++++++++++---------------------
 1 file changed, 79 insertions(+), 72 deletions(-)

diff --git a/opcodes/riscv-dis.c b/opcodes/riscv-dis.c
index f5dc1907c6ec..c74827ed19df 100644
--- a/opcodes/riscv-dis.c
+++ b/opcodes/riscv-dis.c
@@ -649,7 +649,7 @@ riscv_disassemble_insn (bfd_vma memaddr,
 			const bfd_byte *packet,
 			disassemble_info *info)
 {
-  const struct riscv_opcode *op;
+  const struct riscv_opcode *op, *matched_op;
   static bool init = false;
   static const struct riscv_opcode *riscv_hash[OP_MASK_OP + 1];
   struct riscv_private_data *pd;
@@ -705,85 +705,92 @@ riscv_disassemble_insn (bfd_vma memaddr,
   info->target = 0;
   info->target2 = 0;
 
+  matched_op = NULL;
   op = riscv_hash[OP_HASH_IDX (word)];
-  if (op != NULL)
+
+  /* If XLEN is not known, get its value from the ELF class.  */
+  if (info->mach == bfd_mach_riscv64)
+    xlen = 64;
+  else if (info->mach == bfd_mach_riscv32)
+    xlen = 32;
+  else if (info->section != NULL)
     {
-      /* If XLEN is not known, get its value from the ELF class.  */
-      if (info->mach == bfd_mach_riscv64)
-	xlen = 64;
-      else if (info->mach == bfd_mach_riscv32)
-	xlen = 32;
-      else if (info->section != NULL)
-	{
-	  Elf_Internal_Ehdr *ehdr = elf_elfheader (info->section->owner);
-	  xlen = ehdr->e_ident[EI_CLASS] == ELFCLASS64 ? 64 : 32;
-	}
+      Elf_Internal_Ehdr *ehdr = elf_elfheader (info->section->owner);
+      xlen = ehdr->e_ident[EI_CLASS] == ELFCLASS64 ? 64 : 32;
+    }
 
-      /* If arch has the Zfinx extension, replace FPR with GPR.  */
-      if (riscv_subset_supports (&riscv_rps_dis, "zfinx"))
-	riscv_fpr_names = riscv_gpr_names;
-      else
-	riscv_fpr_names = riscv_gpr_names == riscv_gpr_names_abi ?
-			  riscv_fpr_names_abi : riscv_fpr_names_numeric;
+  /* If arch has the Zfinx extension, replace FPR with GPR.  */
+  if (riscv_subset_supports (&riscv_rps_dis, "zfinx"))
+    riscv_fpr_names = riscv_gpr_names;
+  else
+    riscv_fpr_names = riscv_gpr_names == riscv_gpr_names_abi
+			  ? riscv_fpr_names_abi
+			  : riscv_fpr_names_numeric;
 
-      for (; op->name; op++)
-	{
-	  /* Does the opcode match?  */
-	  if (! (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))
-	    continue;
-	  /* Is this instruction restricted to a certain value of XLEN?  */
-	  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))
-	    continue;
-
-	  /* It's a match.  */
-	  (*info->fprintf_styled_func) (info->stream, dis_style_mnemonic,
-					"%s", op->name);
-	  print_insn_args (op->args, word, memaddr, info);
-
-	  /* Try to disassemble multi-instruction addressing sequences.  */
-	  if (pd->to_print_addr)
-	    {
-	      info->target = pd->print_addr;
-	      (*info->fprintf_styled_func)
-		(info->stream, dis_style_comment_start, " # ");
-	      (*info->print_address_func) (info->target, info);
-	      pd->to_print_addr = false;
-	    }
+  for (; op && op->name; op++)
+    {
+      /* Does the opcode match?  */
+      if (!(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))
+	continue;
+      /* Is this instruction restricted to a certain value of XLEN?  */
+      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))
+	continue;
 
-	  /* Finish filling out insn_info fields.  */
-	  switch (op->pinfo & INSN_TYPE)
-	    {
-	    case INSN_BRANCH:
-	      info->insn_type = dis_branch;
-	      break;
-	    case INSN_CONDBRANCH:
-	      info->insn_type = dis_condbranch;
-	      break;
-	    case INSN_JSR:
-	      info->insn_type = dis_jsr;
-	      break;
-	    case INSN_DREF:
-	      info->insn_type = dis_dref;
-	      break;
-	    default:
-	      break;
-	    }
+      matched_op = op;
+      break;
+    }
 
-	  if (op->pinfo & INSN_DATA_SIZE)
-	    {
-	      int size = ((op->pinfo & INSN_DATA_SIZE)
-			  >> INSN_DATA_SIZE_SHIFT);
-	      info->data_size = 1 << (size - 1);
-	    }
+  if (matched_op != NULL)
+    {
+      /* There is a match.  */
+      op = matched_op;
+
+      (*info->fprintf_styled_func) (info->stream, dis_style_mnemonic,
+				    "%s", op->name);
+      print_insn_args (op->args, word, memaddr, info);
 
-	  return insnlen;
+      /* Try to disassemble multi-instruction addressing sequences.  */
+      if (pd->to_print_addr)
+	{
+	  info->target = pd->print_addr;
+	  (*info->fprintf_styled_func) (info->stream, dis_style_comment_start,
+					" # ");
+	  (*info->print_address_func) (info->target, info);
+	  pd->to_print_addr = false;
 	}
+
+      /* Finish filling out insn_info fields.  */
+      switch (op->pinfo & INSN_TYPE)
+	{
+	case INSN_BRANCH:
+	  info->insn_type = dis_branch;
+	  break;
+	case INSN_CONDBRANCH:
+	  info->insn_type = dis_condbranch;
+	  break;
+	case INSN_JSR:
+	  info->insn_type = dis_jsr;
+	  break;
+	case INSN_DREF:
+	  info->insn_type = dis_dref;
+	  break;
+	default:
+	  break;
+	}
+
+      if (op->pinfo & INSN_DATA_SIZE)
+	{
+	  int size = ((op->pinfo & INSN_DATA_SIZE) >> INSN_DATA_SIZE_SHIFT);
+	  info->data_size = 1 << (size - 1);
+	}
+
+      return insnlen;
     }
 
   /* We did not find a match, so just print the instruction bits.  */
-- 
2.38.1

[PATCH v2 09/11] RISC-V: Reorganize disassembler state initialization

[Tsukasa OI]

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

The current disassembler repeatedly checks current state per instruction:

-   Whether riscv_gpr_names is initialized to a non-NULL value
-   Whether the Zfinx extension is available
-   Whether the hash table is initialized

... but they are not frequently changed.

riscv_gpr_names is initialized to a non-NULL value when

-   The first disassembler option is specified, or
-   Not initialized with disassembler options
    (in the first print_insn_riscv function call).

We can safely initialize the default disassembler options prior to the
print_insn_riscv function call and this per-instruction checking of
riscv_gpr_names can be safely removed.

The opcode hash table initialization will be taken care with a later commit.

To group disassembler state initialization, this commit utilizes the
disassemble_init_for_target function.  As a side effect, this will also fix
a potential issue when disassembler options is set and then unset on GDB.
This idea is based on opcodes/ppc-dis.c and opcodes/wasm32-dis.c.

New callback function init_riscv_dis_state_for_arch_and_options is called
when either the architecture string or an option is possibly changed.

We can now group the disassembler state initialization together.
It makes state initialization clearer and makes further changes easier.

In performance perspective, this commit has various effects (-5% to 6%).
However, this commit makes implementing large optimizations easier
as well as "RISC-V: Split match/print steps on disassembler".

include/ChangeLog:

	* dis-asm.h (disassemble_init_riscv): Add declaration of
	disassemble_init_riscv.

opcodes/ChangeLog:

	* disassemble.c (disassemble_init_for_target): Call
	disassemble_init_riscv to group state initialization together.
	* riscv-dis.c
	(xlen_by_mach, xlen_by_elf): New variables to store
	environment-inferred XLEN.
	(is_numeric): New.  Instead of directly setting
	riscv_{gpr,fpr}_names, use this to store an option.
	(update_riscv_dis_xlen): New function to set actual XLEN from
	xlen_by_mach and xlen_by_elf variables.
	(init_riscv_dis_state_for_arch_and_options): New callback function
	called when either the architecture or an option is changed.  Set
	riscv_{gpr,fpr}_names here.
	(set_default_riscv_dis_options): Initialize is_numeric instead of
	riscv_gpr_names and riscv_fpr_names.
	(parse_riscv_dis_option_without_args): When the "numeric" option
	is specified, write to is_numeric instead of register names.
	(parse_riscv_dis_options): Suppress setting the default options
	here and let disassemble_init_riscv to initialize them.
	(riscv_disassemble_insn): Move probing Zfinx and setting XLEN
	portions to init_riscv_dis_state_for_arch_and_options and
	update_riscv_dis_xlen.
	(riscv_get_map_state): If a mapping symbol with ISA string is
	suitable, call init_riscv_dis_state_for_arch_and_options function
	to update disassembler state.
	(print_insn_riscv): Update XLEN only if we haven't guessed correct
	XLEN for the disassembler.  Stop checking disassembler options for
	every instruction and let disassemble_init_riscv to parse options.
	(riscv_get_disassembler): Call
	init_riscv_dis_state_for_arch_and_options because the architecture
	string is updated here.
	(disassemble_init_riscv): New function to initialize the
	structure, reset/guess correct XLEN and reset/parse disassembler
	options.
---
 include/dis-asm.h     |   1 +
 opcodes/disassemble.c |   2 +-
 opcodes/riscv-dis.c   | 112 +++++++++++++++++++++++++++++-------------
 3 files changed, 79 insertions(+), 36 deletions(-)

diff --git a/include/dis-asm.h b/include/dis-asm.h
index 4921c0407102..11537b432ff0 100644
--- a/include/dis-asm.h
+++ b/include/dis-asm.h
@@ -393,6 +393,7 @@ extern bool arm_symbol_is_valid (asymbol *, struct disassemble_info *);
 extern bool csky_symbol_is_valid (asymbol *, struct disassemble_info *);
 extern bool riscv_symbol_is_valid (asymbol *, struct disassemble_info *);
 extern void disassemble_init_powerpc (struct disassemble_info *);
+extern void disassemble_init_riscv (struct disassemble_info *);
 extern void disassemble_init_s390 (struct disassemble_info *);
 extern void disassemble_init_wasm32 (struct disassemble_info *);
 extern void disassemble_init_nds32 (struct disassemble_info *);
diff --git a/opcodes/disassemble.c b/opcodes/disassemble.c
index 0a8f2da629f3..704fb476ea9f 100644
--- a/opcodes/disassemble.c
+++ b/opcodes/disassemble.c
@@ -717,7 +717,7 @@ disassemble_init_for_target (struct disassemble_info * info)
 #endif
 #ifdef ARCH_riscv
     case bfd_arch_riscv:
-      info->symbol_is_valid = riscv_symbol_is_valid;
+      disassemble_init_riscv (info);
       info->created_styled_output = true;
       break;
 #endif
diff --git a/opcodes/riscv-dis.c b/opcodes/riscv-dis.c
index c74827ed19df..174b6e180543 100644
--- a/opcodes/riscv-dis.c
+++ b/opcodes/riscv-dis.c
@@ -35,6 +35,12 @@
 /* Current XLEN for the disassembler.  */
 static unsigned xlen = 0;
 
+/* XLEN as inferred by the machine architecture.  */
+static unsigned xlen_by_mach = 0;
+
+/* XLEN as inferred by ELF header.  */
+static unsigned xlen_by_elf = 0;
+
 /* Default ISA specification version (constant as of now).  */
 static enum riscv_spec_class default_isa_spec = ISA_SPEC_CLASS_DRAFT - 1;
 
@@ -72,6 +78,48 @@ static const char * const *riscv_fpr_names;
 
 /* If set, disassemble as most general instruction.  */
 static bool no_aliases = false;
+
+/* If set, disassemble with numeric register names.  */
+static bool is_numeric = false;
+\f
+
+/* Guess and update current XLEN.  */
+
+static void
+update_riscv_dis_xlen (struct disassemble_info *info)
+{
+  /* Set XLEN with following precedence rules:
+     1. BFD machine architecture set by either:
+       a. -m riscv:rv[32|64] option (GDB: set arch riscv:rv[32|64])
+       b. ELF class in actual ELF header (only on RISC-V ELF)
+       This is only effective if XLEN-specific BFD machine architecture is
+       chosen.  If XLEN-neutral (like riscv), BFD machine architecture is
+       ignored on XLEN selection.
+     2. ELF class in dummy ELF header.  */
+  if (xlen_by_mach != 0)
+    xlen = xlen_by_mach;
+  else if (xlen_by_elf != 0)
+    xlen = xlen_by_elf;
+  else if (info != NULL && info->section != NULL)
+    {
+      Elf_Internal_Ehdr *ehdr = elf_elfheader (info->section->owner);
+      xlen = xlen_by_elf = ehdr->e_ident[EI_CLASS] == ELFCLASS64 ? 64 : 32;
+    }
+}
+
+/* Initialization (for arch and options).  */
+
+static void
+init_riscv_dis_state_for_arch_and_options (void)
+{
+  /* Set GPR register names to disassemble.  */
+  riscv_gpr_names = is_numeric ? riscv_gpr_names_numeric : riscv_gpr_names_abi;
+  /* Set FPR register names to disassemble.  */
+  riscv_fpr_names
+      = !riscv_subset_supports (&riscv_rps_dis, "zfinx")
+	    ? (is_numeric ? riscv_fpr_names_numeric : riscv_fpr_names_abi)
+	    : riscv_gpr_names;
+}
 \f
 
 /* Set default RISC-V disassembler options.  */
@@ -79,9 +127,8 @@ static bool no_aliases = false;
 static void
 set_default_riscv_dis_options (void)
 {
-  riscv_gpr_names = riscv_gpr_names_abi;
-  riscv_fpr_names = riscv_fpr_names_abi;
   no_aliases = false;
+  is_numeric = false;
 }
 
 /* Parse RISC-V disassembler option (without arguments).  */
@@ -92,10 +139,7 @@ parse_riscv_dis_option_without_args (const char *option)
   if (strcmp (option, "no-aliases") == 0)
     no_aliases = true;
   else if (strcmp (option, "numeric") == 0)
-    {
-      riscv_gpr_names = riscv_gpr_names_numeric;
-      riscv_fpr_names = riscv_fpr_names_numeric;
-    }
+    is_numeric = true;
   else
     return false;
   return true;
@@ -163,8 +207,6 @@ parse_riscv_dis_options (const char *opts_in)
 {
   char *opts = xstrdup (opts_in), *opt = opts, *opt_end = opts;
 
-  set_default_riscv_dis_options ();
-
   for ( ; opt_end != NULL; opt = opt_end + 1)
     {
       if ((opt_end = strchr (opt, ',')) != NULL)
@@ -708,25 +750,6 @@ riscv_disassemble_insn (bfd_vma memaddr,
   matched_op = NULL;
   op = riscv_hash[OP_HASH_IDX (word)];
 
-  /* If XLEN is not known, get its value from the ELF class.  */
-  if (info->mach == bfd_mach_riscv64)
-    xlen = 64;
-  else if (info->mach == bfd_mach_riscv32)
-    xlen = 32;
-  else if (info->section != NULL)
-    {
-      Elf_Internal_Ehdr *ehdr = elf_elfheader (info->section->owner);
-      xlen = ehdr->e_ident[EI_CLASS] == ELFCLASS64 ? 64 : 32;
-    }
-
-  /* If arch has the Zfinx extension, replace FPR with GPR.  */
-  if (riscv_subset_supports (&riscv_rps_dis, "zfinx"))
-    riscv_fpr_names = riscv_gpr_names;
-  else
-    riscv_fpr_names = riscv_gpr_names == riscv_gpr_names_abi
-			  ? riscv_fpr_names_abi
-			  : riscv_fpr_names_numeric;
-
   for (; op && op->name; op++)
     {
       /* Does the opcode match?  */
@@ -869,6 +892,7 @@ riscv_get_map_state (int n,
     {
       riscv_release_subset_list (&riscv_subsets);
       riscv_parse_subset (&riscv_rps_dis, arch);
+      init_riscv_dis_state_for_arch_and_options ();
     }
   return true;
 }
@@ -1067,14 +1091,9 @@ print_insn_riscv (bfd_vma memaddr, struct disassemble_info *info)
   int (*riscv_disassembler) (bfd_vma, insn_t, const bfd_byte *,
 			     struct disassemble_info *);
 
-  if (info->disassembler_options != NULL)
-    {
-      parse_riscv_dis_options (info->disassembler_options);
-      /* Avoid repeatedly parsing the options.  */
-      info->disassembler_options = NULL;
-    }
-  else if (riscv_gpr_names == NULL)
-    set_default_riscv_dis_options ();
+  /* Guess and update XLEN if we haven't determined it yet.  */
+  if (xlen == 0)
+    update_riscv_dis_xlen (info);
 
   mstate = riscv_search_mapping_symbol (memaddr, info);
 
@@ -1136,9 +1155,32 @@ riscv_get_disassembler (bfd *abfd)
 
   riscv_release_subset_list (&riscv_subsets);
   riscv_parse_subset (&riscv_rps_dis, default_arch);
+  init_riscv_dis_state_for_arch_and_options ();
   return print_insn_riscv;
 }
 
+/* Initialize disassemble_info and parse options.  */
+
+void
+disassemble_init_riscv (struct disassemble_info *info)
+{
+  info->symbol_is_valid = riscv_symbol_is_valid;
+  /* Clear previous XLEN and guess by mach.  */
+  xlen = 0;
+  xlen_by_mach = 0;
+  xlen_by_elf = 0;
+  if (info->mach == bfd_mach_riscv64)
+    xlen_by_mach = 64;
+  else if (info->mach == bfd_mach_riscv32)
+    xlen_by_mach = 32;
+  update_riscv_dis_xlen (info);
+  /* Parse disassembler options.  */
+  set_default_riscv_dis_options ();
+  if (info->disassembler_options != NULL)
+    parse_riscv_dis_options (info->disassembler_options);
+  init_riscv_dis_state_for_arch_and_options ();
+}
+
 /* Prevent use of the fake labels that are generated as part of the DWARF
    and for relaxable relocations in the assembler.  */
 
-- 
2.38.1

[PATCH v2 10/11] RISC-V: Reorganize arch-related initialization and management

[Tsukasa OI]

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

objdump reuses the disassembler function returned by the disassembler
function.  That's good and benefits well from various optimizations.

However, by default, GDB (default_print_insn in gdb/arch-utils.c) assumes
that the disassembler function logic is simple and calls that function for
every instruction to be disassembled.  This is clearly a waste of time
because it probes BFD (ELF information) and re-initializes
riscv_rps_dis for every instruction.

After the support of mapping symbol with ISA string with commit 40f1a1a4564b
("RISC-V: Output mapping symbols with ISA string."), this kind of per-
instruction initialization of riscv_rps_dis can also occur on ELF files with
mapping symbol + ISA string (in riscv_get_map_state function).  It can be
worse if the object is assembled using Binutils commit 0ce50fc900a5
("RISC-V: Always generate mapping symbols at the start of the sections.")
or later.

To avoid repeated initialization, this commit

-   Caches the default / override architectures (in two "contexts") and
-   enables switching between them.

riscv_dis_arch_context_t and new utility functions are defined for this
purpose.  We still have to read the ".riscv.attributes" section on every
instruction on GDB but at least the most time-consuming part (updating the
actual architecture from the architecture string) is avoided.
Likewise, we still have to probe whether we have to update the architecture
for every instruction with a mapping symbol with ISA string is suitable but
at least we don't actually update the architecture unless necessary
(either context itself or the ISA string of the current context is changed).

This commit improves the disassembler performance well in those situations:

-   When long "disas" command is used on GDB
-   When ELF files with mapping symbols + ISA string is used

This commit now implements new mapping symbol handling ("$x" means revert
to the previous architecture [with "$x+arch"] if exists, otherwise revert to
the default one usually read from an ELF attribute), the recent consensus
made by Kito and Nelson.

On the benchmark using GDB batch files, the author measured significant
performance improvements (35-96% on various big RISC-V programs).
Unfortunately, on interactive usecases of GDB, this improvement is rarely
observable since we don't usually disassemble such a big chunk at once and
the current disassembler is not very slow.

On the benchmark using unstripped ELF files with mapping symbols + ISA
string "$xrv...", performance improvements are significant and easily
observable in the real world (150%-264% performance improvments).

Aside from optimization, this commit, along with "RISC-V: Reorganize
disassembler state initialization", makes state initialization clearer and
makes further changes easier.

Also, although not practical in the real world, this commit now allows
multi-XLEN object disassembling if the object file has mapping symbols
with ISA string and the machine is XLEN-neutral (e.g. objdump with
"-m riscv" option).  It may help testing Binutils / GAS.

opcodes/ChangeLog:

	* riscv-dis.c
	(initial_default_arch): Special default architecture
	string which is handled separately.
	(riscv_dis_arch_context_t): New type to manage RISC-V architecture
	context for the disassembler.  Two instance of this type is
	defined in this file - "default" and "override".
	(dis_arch_context_default): New.  Architecture context inferred
	from either an ELF attribute or initial_default_arch.
	(dis_arch_context_override): New.  Architecture context inferred
	from mapping symbols with ISA string.
	(dis_arch_context_current): New.  A pointer to either
	dis_arch_context_default or dis_arch_context_override.
	(riscv_rps_dis): Add summary.  Use initial values from the initial
	value of dis_arch_context_current - dis_arch_context_default.
	(from_last_map_symbol): Make it file scope to decide whether we
	should revert the architecture to the default in
	riscv_get_map_state function.
	(set_riscv_current_dis_arch_context): New function to update
	riscv_rps_dis and dis_arch_context_current.
	(set_riscv_dis_arch_context): New function to update the
	architecture for the given context.
	(update_riscv_dis_xlen): Consider dis_arch_context_current->xlen
	when guessing correct XLEN.
	(is_arch_changed): New.  Set to true if the architecture is
	changed.
	(init_riscv_dis_state_for_arch): New function to track whether
	the architecture string is changed.
	(init_riscv_dis_state_for_arch_and_options): Keep track of the
	architecture string change and update XLEN if it has changed.
	(update_riscv_dis_arch): New function to set both the architecture
	and the context.  Call initialization functions if needed.
	(riscv_get_map_state): Add update argument.  Keep track of the
	mapping symbols with ISA string and update the architecture and
	the context if required.
	(riscv_search_mapping_symbol): Move from_last_map_symbol to
	file scope.  Call riscv_get_map_state function with architecture
	and context updates enabled.
	(riscv_data_length): Call riscv_get_map_state function with
	architecture and context updates disabled.
	(riscv_get_disassembler): Add an error handling on Tag_RISCV_arch.
	Call update_riscv_dis_arch function to update the architecture
	and the context.

Co-developed-by: Nelson Chu <nelson@rivosinc.com>
---
 opcodes/riscv-dis.c | 174 ++++++++++++++++++++++++++++++++++++++------
 1 file changed, 152 insertions(+), 22 deletions(-)

diff --git a/opcodes/riscv-dis.c b/opcodes/riscv-dis.c
index 174b6e180543..d48448bb3597 100644
--- a/opcodes/riscv-dis.c
+++ b/opcodes/riscv-dis.c
@@ -32,6 +32,9 @@
 #include <stdint.h>
 #include <ctype.h>
 
+/* Default architecture string (if not available).  */
+static const char *const initial_default_arch = "rv64gc";
+
 /* Current XLEN for the disassembler.  */
 static unsigned xlen = 0;
 
@@ -48,14 +51,36 @@ static enum riscv_spec_class default_isa_spec = ISA_SPEC_CLASS_DRAFT - 1;
    (as specified by the ELF attributes or the `priv-spec' option).  */
 static enum riscv_spec_class default_priv_spec = PRIV_SPEC_CLASS_NONE;
 
-static riscv_subset_list_t riscv_subsets;
+/* RISC-V disassembler architecture context type.  */
+typedef struct
+{
+  const char *arch_str;
+  const char *default_arch;
+  riscv_subset_list_t subsets;
+  unsigned xlen;
+  bool no_xlen_if_default;
+} riscv_dis_arch_context_t;
+
+/* Context: default (either initial_default_arch or ELF attribute).  */
+static riscv_dis_arch_context_t dis_arch_context_default
+    = { NULL, initial_default_arch, {}, 0, true };
+
+/* Context: override (mapping symbols with ISA string). */
+static riscv_dis_arch_context_t dis_arch_context_override
+    = { NULL, NULL, {}, 0, false };
+
+/* Pointer to the current disassembler architecture context.  */
+static riscv_dis_arch_context_t *dis_arch_context_current
+    = &dis_arch_context_default;
+
+/* RISC-V ISA string parser structure (current).  */
 static riscv_parse_subset_t riscv_rps_dis =
 {
-  &riscv_subsets,	/* subset_list.  */
-  opcodes_error_handler,/* error_handler.  */
-  &xlen,		/* xlen.  */
-  &default_isa_spec,	/* isa_spec.  */
-  false,		/* check_unknown_prefixed_ext.  */
+  &(dis_arch_context_default.subsets),	/* subset_list.  */
+  opcodes_error_handler,		/* error_handler.  */
+  &(dis_arch_context_default.xlen),	/* xlen.  */
+  &default_isa_spec,			/* isa_spec.  */
+  false,				/* check_unknown_prefixed_ext.  */
 };
 
 /* Private data structure for the RISC-V disassembler.  */
@@ -71,6 +96,7 @@ struct riscv_private_data
 /* Used for mapping symbols.  */
 static int last_map_symbol = -1;
 static bfd_vma last_stop_offset = 0;
+static bool from_last_map_symbol = false;
 
 /* Register names as used by the disassembler.  */
 static const char * const *riscv_gpr_names;
@@ -83,6 +109,59 @@ static bool no_aliases = false;
 static bool is_numeric = false;
 \f
 
+/* Set current disassembler context (dis_arch_context_current).
+   Return true if successfully updated.  */
+
+static bool
+set_riscv_current_dis_arch_context (riscv_dis_arch_context_t* context)
+{
+  if (context == dis_arch_context_current)
+    return false;
+  dis_arch_context_current = context;
+  riscv_rps_dis.subset_list = &(context->subsets);
+  riscv_rps_dis.xlen = &(context->xlen);
+  return true;
+}
+
+/* Update riscv_dis_arch_context_t by an ISA string.
+   Return true if the architecture is updated by arch.  */
+
+static bool
+set_riscv_dis_arch_context (riscv_dis_arch_context_t *context,
+			    const char *arch)
+{
+  /* Check whether the architecture is changed and
+     return false if the architecture will not be changed.  */
+  if (context->arch_str)
+    {
+      if (context->default_arch && arch == context->default_arch)
+	return false;
+      if (strcmp (context->arch_str, arch) == 0)
+	return false;
+    }
+  /* Update architecture string.  */
+  if (context->arch_str != context->default_arch)
+    free ((void *) context->arch_str);
+  context->arch_str = (arch != context->default_arch)
+			    ? xstrdup (arch)
+			    : context->default_arch;
+  /* Update other contents (subset list and XLEN).  */
+  riscv_subset_list_t *prev_subsets = riscv_rps_dis.subset_list;
+  unsigned *prev_xlen = riscv_rps_dis.xlen;
+  riscv_rps_dis.subset_list = &(context->subsets);
+  riscv_rps_dis.xlen = &(context->xlen);
+  context->xlen = 0;
+  riscv_release_subset_list (&context->subsets);
+  riscv_parse_subset (&riscv_rps_dis, context->arch_str);
+  riscv_rps_dis.subset_list = prev_subsets;
+  riscv_rps_dis.xlen = prev_xlen;
+  /* Special handling on the default architecture.  */
+  if (context->no_xlen_if_default && arch == context->default_arch)
+    context->xlen = 0;
+  return true;
+}
+\f
+
 /* Guess and update current XLEN.  */
 
 static void
@@ -95,9 +174,13 @@ update_riscv_dis_xlen (struct disassemble_info *info)
        This is only effective if XLEN-specific BFD machine architecture is
        chosen.  If XLEN-neutral (like riscv), BFD machine architecture is
        ignored on XLEN selection.
-     2. ELF class in dummy ELF header.  */
+     2. Non-default RISC-V architecture string set by either an ELF
+	attribute or a mapping symbol with ISA string.
+     3. ELF class in dummy ELF header.  */
   if (xlen_by_mach != 0)
     xlen = xlen_by_mach;
+  else if (dis_arch_context_current->xlen != 0)
+    xlen = dis_arch_context_current->xlen;
   else if (xlen_by_elf != 0)
     xlen = xlen_by_elf;
   else if (info != NULL && info->section != NULL)
@@ -107,11 +190,24 @@ update_riscv_dis_xlen (struct disassemble_info *info)
     }
 }
 
+/* Initialization (for arch).  */
+
+static bool is_arch_changed = false;
+
+static void
+init_riscv_dis_state_for_arch (void)
+{
+  is_arch_changed = true;
+}
+
 /* Initialization (for arch and options).  */
 
 static void
 init_riscv_dis_state_for_arch_and_options (void)
 {
+  /* If the architecture string is changed, update XLEN.  */
+  if (is_arch_changed)
+    update_riscv_dis_xlen (NULL);
   /* Set GPR register names to disassemble.  */
   riscv_gpr_names = is_numeric ? riscv_gpr_names_numeric : riscv_gpr_names_abi;
   /* Set FPR register names to disassemble.  */
@@ -119,6 +215,25 @@ init_riscv_dis_state_for_arch_and_options (void)
       = !riscv_subset_supports (&riscv_rps_dis, "zfinx")
 	    ? (is_numeric ? riscv_fpr_names_numeric : riscv_fpr_names_abi)
 	    : riscv_gpr_names;
+  /* Save previous options and mark them "unchanged".  */
+  is_arch_changed = false;
+}
+
+/* Update architecture for disassembler with its context.
+   Call initialization functions if either:
+   -  the architecture for current context is changed or
+   -  context is updated to a new one.  */
+
+static void
+update_riscv_dis_arch (riscv_dis_arch_context_t *context, const char *arch)
+{
+  if ((set_riscv_dis_arch_context (context, arch)
+       && dis_arch_context_current == context)
+      || set_riscv_current_dis_arch_context (context))
+    {
+      init_riscv_dis_state_for_arch ();
+      init_riscv_dis_state_for_arch_and_options ();
+    }
 }
 \f
 
@@ -874,7 +989,8 @@ riscv_get_map_state_by_name (const char *name, const char** arch)
 static bool
 riscv_get_map_state (int n,
 		     enum riscv_seg_mstate *state,
-		     struct disassemble_info *info)
+		     struct disassemble_info *info,
+		     bool update)
 {
   const char *name, *arch = NULL;
 
@@ -888,12 +1004,25 @@ riscv_get_map_state (int n,
   if (newstate == MAP_NONE)
     return false;
   *state = newstate;
-  if (arch)
-    {
-      riscv_release_subset_list (&riscv_subsets);
-      riscv_parse_subset (&riscv_rps_dis, arch);
-      init_riscv_dis_state_for_arch_and_options ();
-    }
+  if (newstate == MAP_INSN && update)
+  {
+    if (arch)
+      {
+	/* Override the architecture.  */
+	update_riscv_dis_arch (&dis_arch_context_override, arch);
+      }
+    else if (!from_last_map_symbol
+	     && set_riscv_current_dis_arch_context (&dis_arch_context_default))
+      {
+	/* Revert to the default architecture and call init functions if:
+	   - there's no ISA string in the mapping symbol,
+	   - mapping symbol is not reused and
+	   - current disassembler context is changed to the default one.
+	   This is a shortcut path to avoid full update_riscv_dis_arch.  */
+	init_riscv_dis_state_for_arch ();
+	init_riscv_dis_state_for_arch_and_options ();
+      }
+  }
   return true;
 }
 
@@ -905,7 +1034,6 @@ riscv_search_mapping_symbol (bfd_vma memaddr,
 			     struct disassemble_info *info)
 {
   enum riscv_seg_mstate mstate;
-  bool from_last_map_symbol;
   bool found = false;
   int symbol = -1;
   int n;
@@ -945,7 +1073,7 @@ riscv_search_mapping_symbol (bfd_vma memaddr,
       /* We have searched all possible symbols in the range.  */
       if (addr > memaddr)
 	break;
-      if (riscv_get_map_state (n, &mstate, info))
+      if (riscv_get_map_state (n, &mstate, info, true))
 	{
 	  symbol = n;
 	  found = true;
@@ -972,7 +1100,7 @@ riscv_search_mapping_symbol (bfd_vma memaddr,
 	  if (addr < (info->section ? info->section->vma : 0))
 	    break;
 	  /* Stop searching once we find the closed mapping symbol.  */
-	  if (riscv_get_map_state (n, &mstate, info))
+	  if (riscv_get_map_state (n, &mstate, info, true))
 	    {
 	      symbol = n;
 	      found = true;
@@ -1008,7 +1136,7 @@ riscv_data_length (bfd_vma memaddr,
 	{
 	  bfd_vma addr = bfd_asymbol_value (info->symtab[n]);
 	  if (addr > memaddr
-	      && riscv_get_map_state (n, &m, info))
+	      && riscv_get_map_state (n, &m, info, false))
 	    {
 	      if (addr - memaddr < length)
 		length = addr - memaddr;
@@ -1134,7 +1262,7 @@ print_insn_riscv (bfd_vma memaddr, struct disassemble_info *info)
 disassembler_ftype
 riscv_get_disassembler (bfd *abfd)
 {
-  const char *default_arch = "rv64gc";
+  const char *default_arch = initial_default_arch;
 
   if (abfd && bfd_get_flavour (abfd) == bfd_target_elf_flavour)
     {
@@ -1150,12 +1278,14 @@ riscv_get_disassembler (bfd *abfd)
 						  attr[Tag_c].i,
 						  &default_priv_spec);
 	  default_arch = attr[Tag_RISCV_arch].s;
+	  /* For ELF files with (somehow) no architecture string
+	     in the attributes, use the default value.  */
+	  if (!default_arch)
+	    default_arch = initial_default_arch;
 	}
     }
 
-  riscv_release_subset_list (&riscv_subsets);
-  riscv_parse_subset (&riscv_rps_dis, default_arch);
-  init_riscv_dis_state_for_arch_and_options ();
+  update_riscv_dis_arch (&dis_arch_context_default, default_arch);
   return print_insn_riscv;
 }
 
-- 
2.38.1

[PATCH v2 11/11] RISC-V: Move disassembler private data initialization

[Tsukasa OI]

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

Because disassemble_info.private_data could be used not only by
riscv_disassemble_insn, this commit splits the initialization of the
private data to a separate function.

This commit now allows storing mapping symbol and/or section-related
information to riscv_private_data.

In performance perspective, it also has a penalty.  However, it can be
easily paid back by other optimizations and it makes implementing
some optimizations easier.

opcodes/ChangeLog:

	* riscv-dis.c (init_riscv_dis_private_data): New.
	(riscv_disassemble_insn): Move private data initialization to
	init_riscv_dis_private_data.
	(print_insn_riscv): Start initializing the private data
	instead of instruction only riscv_disassemble_insn function.
---
 opcodes/riscv-dis.c | 51 +++++++++++++++++++++++++--------------------
 1 file changed, 28 insertions(+), 23 deletions(-)

diff --git a/opcodes/riscv-dis.c b/opcodes/riscv-dis.c
index d48448bb3597..b9309c092b17 100644
--- a/opcodes/riscv-dis.c
+++ b/opcodes/riscv-dis.c
@@ -219,6 +219,29 @@ init_riscv_dis_state_for_arch_and_options (void)
   is_arch_changed = false;
 }
 
+/* Initialize private data of the disassemble_info.  */
+
+static void
+init_riscv_dis_private_data (struct disassemble_info *info)
+{
+  struct riscv_private_data *pd;
+
+  pd = info->private_data = xcalloc (1, sizeof (struct riscv_private_data));
+  pd->gp = 0;
+  pd->print_addr = 0;
+  for (int i = 0; i < (int)ARRAY_SIZE (pd->hi_addr); i++)
+    pd->hi_addr[i] = -1;
+  pd->to_print_addr = false;
+  pd->has_gp = false;
+
+  for (int i = 0; i < info->symtab_size; i++)
+    if (strcmp (bfd_asymbol_name (info->symtab[i]), RISCV_GP_SYMBOL) == 0)
+      {
+	pd->gp = bfd_asymbol_value (info->symtab[i]);
+	pd->has_gp = true;
+      }
+}
+
 /* Update architecture for disassembler with its context.
    Call initialization functions if either:
    -  the architecture for current context is changed or
@@ -809,7 +832,7 @@ riscv_disassemble_insn (bfd_vma memaddr,
   const struct riscv_opcode *op, *matched_op;
   static bool init = false;
   static const struct riscv_opcode *riscv_hash[OP_MASK_OP + 1];
-  struct riscv_private_data *pd;
+  struct riscv_private_data *pd = info->private_data;
   int insnlen;
 
 #define OP_HASH_IDX(i) ((i) & (riscv_insn_length (i) == 2 ? 0x3 : OP_MASK_OP))
@@ -824,28 +847,6 @@ riscv_disassemble_insn (bfd_vma memaddr,
       init = true;
     }
 
-  if (info->private_data == NULL)
-    {
-      int i;
-
-      pd = info->private_data = xcalloc (1, sizeof (struct riscv_private_data));
-      pd->gp = 0;
-      pd->print_addr = 0;
-      for (i = 0; i < (int)ARRAY_SIZE (pd->hi_addr); i++)
-	pd->hi_addr[i] = -1;
-      pd->to_print_addr = false;
-      pd->has_gp = false;
-
-      for (i = 0; i < info->symtab_size; i++)
-	if (strcmp (bfd_asymbol_name (info->symtab[i]), RISCV_GP_SYMBOL) == 0)
-	  {
-	    pd->gp = bfd_asymbol_value (info->symtab[i]);
-	    pd->has_gp = true;
-	  }
-    }
-  else
-    pd = info->private_data;
-
   insnlen = riscv_insn_length (word);
 
   /* RISC-V instructions are always little-endian.  */
@@ -1219,6 +1220,10 @@ print_insn_riscv (bfd_vma memaddr, struct disassemble_info *info)
   int (*riscv_disassembler) (bfd_vma, insn_t, const bfd_byte *,
 			     struct disassemble_info *);
 
+  /* Initialize the private data.  */
+  if (info->private_data == NULL)
+    init_riscv_dis_private_data (info);
+
   /* Guess and update XLEN if we haven't determined it yet.  */
   if (xlen == 0)
     update_riscv_dis_xlen (info);
-- 
2.38.1