diff mbox series

[RFC,1/1] gdb/riscv: Cache per-BFD disassembler

Message ID 760e2c443b1244b2a0a64f7f865e905b9ef90a72.1665287884.git.research_trasio@irq.a4lg.com
State Committed
Headers
Series gdb/riscv: Cache per-BFD disassembler |

Commit Message

Tsukasa OI Oct. 9, 2022, 3:59 a.m. UTC 
  On RISC-V, calling the disassembler function (libopcodes) is not a small
cost.  This is because riscv_get_disassembler function sets the default
architecture from given BFD's .riscv.attributes section.  However, by
default, GDB calls this function for every instruction.

This commit replaces RISC-V's disassembler function and stop calling
riscv_get_disassembler function if current BFD has not changed.

It expects around 30-80% improvements on disassembling relatively large
chunk of RISC-V code but most of them will be obscured by a RISC-V
disassembler optimization the author is currently working on.  Still, 3-5%
of performance improvements will remain (due to reduced BFD section reads).
---
 gdb/riscv-tdep.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

Comments

Tom Tromey Oct. 14, 2022, 6:39 p.m. UTC | #1 
>>>>> Tsukasa OI via Gdb-patches <gdb-patches@sourceware.org> writes:

> +static int
> +riscv_print_insn (bfd_vma memaddr, disassemble_info *info)
> +{
> +  static disassembler_ftype disassemble_fn = NULL;
> +  static bfd *abfd = NULL;

This seems mildly dangerous, in that a BFD could be destroyed, then a
new one created with the same address.

It's better to cache things using the registry system.
See gdb/registry.h.  You can look for "registry<bfd>" for examples.

Tom
Tsukasa OI Oct. 16, 2022, 3:31 p.m. UTC | #2 
On 2022/10/15 3:39, Tom Tromey wrote:
>>>>>> Tsukasa OI via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
>> +static int
>> +riscv_print_insn (bfd_vma memaddr, disassemble_info *info)
>> +{
>> +  static disassembler_ftype disassemble_fn = NULL;
>> +  static bfd *abfd = NULL;
> 
> This seems mildly dangerous, in that a BFD could be destroyed, then a
> new one created with the same address.
> 
> It's better to cache things using the registry system.
> See gdb/registry.h.  You can look for "registry<bfd>" for examples.
> 
> Tom
> 

Thanks for letting me know.

Because my opcodes-side optimization on RISC-V will hide most of the
performance improvements made by that proposed patch, I will need to
redo the benchmark after I write a patch to use registry system.  If
registry cost is too high, I will have to scrap this idea instead of
using the registry system.

Thanks,
Tsukasa
diff mbox series

Patch

diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c
index feca17d9141..ed68a52df4c 100644
--- a/gdb/riscv-tdep.c
+++ b/gdb/riscv-tdep.c
@@ -55,6 +55,7 @@ 
 #include "cli/cli-decode.h"
 #include "observable.h"
 #include "prologue-value.h"
+#include "progspace.h"
 #include "arch/riscv.h"
 #include "riscv-ravenscar-thread.h"
 
@@ -1308,6 +1309,27 @@  riscv_print_one_register_info (struct gdbarch *gdbarch,
   gdb_printf (file, "\n");
 }
 
+/* Calling disassembler function on RISC-V is not fast as others.
+   We cache the disassembler function as long as current BFD is the same.  */
+
+static int
+riscv_print_insn (bfd_vma memaddr, disassemble_info *info)
+{
+  static disassembler_ftype disassemble_fn = NULL;
+  static bfd *abfd = NULL;
+  bfd *ebfd = current_program_space->exec_bfd ();
+
+  if (disassemble_fn == NULL || abfd != ebfd)
+    {
+      disassemble_fn = disassembler (
+	  info->arch, info->endian == BFD_ENDIAN_BIG, info->mach, ebfd);
+      abfd = ebfd;
+    }
+
+  gdb_assert (disassemble_fn != NULL);
+  return (*disassemble_fn) (memaddr, info);
+}
+
 /* Return true if REGNUM is a valid CSR register.  The CSR register space
    is sparsely populated, so not every number is a named CSR.  */
 
@@ -3926,6 +3948,7 @@  riscv_gdbarch_init (struct gdbarch_info info,
   set_gdbarch_pc_regnum (gdbarch, RISCV_PC_REGNUM);
 
   set_gdbarch_print_registers_info (gdbarch, riscv_print_registers_info);
+  set_gdbarch_print_insn (gdbarch, riscv_print_insn);
 
   set_tdesc_pseudo_register_name (gdbarch, riscv_pseudo_register_name);
   set_tdesc_pseudo_register_type (gdbarch, riscv_pseudo_register_type);