Re: [PATCH] RISC-V: Search for mapping symbols from the last one found

[Nelson Chu <nelson@rivosinc.com>]

[-- Attachment #1: Type: text/plain, Size: 6419 bytes --]

Thanks, committed ;)

Nelson

On Tue, May 14, 2024 at 5:10 PM Joseph Faulls <Joseph.Faulls@imgtec.com>
wrote:

> Thanks, Nelson. Updated the patch:
>
> With previous behaviour, multiple mapping symbols within the same
> function would result in all the mapping symbols being searched.
> This could slow down disassembly dramatically.
>
> Multiple mapping symbols within a function can be a result of encoding
> instructions as data, like sometimes seen in random instruction
> generators.
>
> opcodes/ChangeLog:
>
>         * riscv-dis.c (riscv_search_mapping_symbol): Use last mapping
>           symbol if it exists.
> ---
>  opcodes/riscv-dis.c | 12 ++++--------
>  1 file changed, 4 insertions(+), 8 deletions(-)
>
> diff --git a/opcodes/riscv-dis.c b/opcodes/riscv-dis.c
> index 684098d386a..e6596c47423 100644
> --- a/opcodes/riscv-dis.c
> +++ b/opcodes/riscv-dis.c
> @@ -1076,11 +1076,9 @@ riscv_search_mapping_symbol (bfd_vma memaddr,
>    from_last_map_symbol = (last_map_symbol >= 0
>                           && info->stop_offset == last_stop_offset);
>
> -  /* Start scanning at the start of the function, or wherever
> -     we finished last time.  */
> -  n = info->symtab_pos + 1;
> -  if (from_last_map_symbol && n >= last_map_symbol)
> -    n = last_map_symbol;
> +  /* Start scanning from wherever we finished last time, or the start
> +     of the function.  */
> +  n = from_last_map_symbol ? last_map_symbol : info->symtab_pos + 1;
>
>    /* Find the suitable mapping symbol to dump.  */
>    for (; n < info->symtab_size; n++)
> @@ -1105,9 +1103,7 @@ riscv_search_mapping_symbol (bfd_vma memaddr,
>       can pick up a text mapping symbol of a preceeding section.  */
>    if (!found)
>      {
> -      n = info->symtab_pos;
> -      if (from_last_map_symbol && n >= last_map_symbol)
> -       n = last_map_symbol;
> +      n = from_last_map_symbol ? last_map_symbol : info->symtab_pos;
>
>        for (; n >= 0; n--)
>         {
> --
> 2.34.1
>
> ------------------------------
> *From:* Nelson Chu <nelson@rivosinc.com>
> *Sent:* Tuesday, May 14, 2024 1:34 AM
> *To:* Palmer Dabbelt <palmer@dabbelt.com>
> *Cc:* Joseph Faulls <Joseph.Faulls@imgtec.com>; binutils@sourceware.org <
> binutils@sourceware.org>
> *Subject:* Re: [PATCH] RISC-V: Search for mapping symbols from the last
> one found
>
>
> There is a similar (n >= last_map_symbol) check when we need to backtrace
> searching, https://github.com/bminor/binutils-gdb/blob/master/opcodes/riscv-dis.c#L1109
> [github.com]
> <https://urldefense.com/v3/__https://github.com/bminor/binutils-gdb/blob/master/opcodes/riscv-dis.c*L1109__;Iw!!KCwjcDI!zCUUWandbHQVnj1F9uYJb9d3R0tZc6qdNw08xyHSliol7nDMdHOVDm7pp_3zEykv3p4RP46uhAiFnpVQcFWu$>,
> should we also need to fix this?
>
> diff --git a/opcodes/riscv-dis.c b/opcodes/riscv-dis.c
>
> index 684098d386a..e6596c47423 100644
>
> --- a/opcodes/riscv-dis.c
>
> +++ b/opcodes/riscv-dis.c
>
> @@ -1076,11 +1076,9 @@ riscv_search_mapping_symbol (bfd_vma memaddr,
>
>    from_last_map_symbol = (last_map_symbol >= 0
>                           && info->stop_offset == last_stop_offset);
>
> -  /* Start scanning at the start of the function, or wherever
>
> -     we finished last time.  */
>
> -  n = info->symtab_pos + 1;
>
> -  if (from_last_map_symbol && n >= last_map_symbol)
>
> -    n = last_map_symbol;
>
> +  /* Start scanning from wherever we finished last time, or the start
>
> +     of the function.  */
>
> +  n = from_last_map_symbol ? last_map_symbol : info->symtab_pos + 1;
>
>
>    /* Find the suitable mapping symbol to dump.  */
>    for (; n < info->symtab_size; n++)
> @@ -1105,9 +1103,7 @@ riscv_search_mapping_symbol (bfd_vma memaddr,
>
>       can pick up a text mapping symbol of a preceeding section.  */
>    if (!found)
>      {
> -      n = info->symtab_pos;
>
> -      if (from_last_map_symbol && n >= last_map_symbol)
>
> -       n = last_map_symbol;
>
> +      n = from_last_map_symbol ? last_map_symbol : info->symtab_pos;
>
>
>        for (; n >= 0; n--)
>
> On Mon, May 6, 2024 at 11:12 PM Palmer Dabbelt <palmer@dabbelt.com> wrote:
>
> On Fri, 03 May 2024 05:56:47 PDT (-0700), Joseph.Faulls@imgtec.com wrote:
> > Ping. I know it's a small patch, but it sped up disassembly of my use
> case from 5 minutes to 5 seconds.
> > ________________________________
> > From: Joseph Faulls
> > Sent: Thursday, April 18, 2024 11:48 AM
> > To: binutils@sourceware.org <binutils@sourceware.org>
> > Cc: nelson@rivosinc.com <nelson@rivosinc.com>
> > Subject: [PATCH] RISC-V: Search for mapping symbols from the last one
> found
> >
> > With previous behaviour, multiple mapping symbols within the same
> > function would result in all the mapping symbols being searched.
> > This could slow down disassembly dramatically.
> >
> > Multiple mapping symbols within a function can be a result of encoding
> > instructions as data, like sometimes seen in random instruction
> > generators.
> >
> > opcodes/ChangeLog:
> >
> >         * riscv-dis.c (riscv_search_mapping_symbol): Use last mapping
> >           symbol if it exists.
> >
> > Note for reviewers:
> > I don't see why the previous check 'n >= last_map_symbol exists'. Why do
> we force starting from the start of the function if the last mapping symbol
> was found after it? If I'm missing something, please let me know!
>
> Sorry for being slow here.  Nelson and I were talking a bit right before
> the weekend and it looks like this was just a bug, but it's very similar
> to what the Arm code does.
>
>
> I guess that is because we ported the code from aarch64 at the beginning,
> and then kept their code to fix the stuff probably this one, https://github.com/bminor/binutils-gdb/commit/51457761649bab6868343b3da2258d73a62901f7
> [github.com]
> <https://urldefense.com/v3/__https://github.com/bminor/binutils-gdb/commit/51457761649bab6868343b3da2258d73a62901f7__;!!KCwjcDI!zCUUWandbHQVnj1F9uYJb9d3R0tZc6qdNw08xyHSliol7nDMdHOVDm7pp_3zEykv3p4RP46uhAiFnpPZmLn7$>.
> But even applying Joseph's patch, the testcase from the above commit also
> looks good, and I also passed the riscv-gnu-toolchain for the above changes
> (two places use n >= last_map_symbol).  So it seems no reason to keep the
> (n >= last_map_symbol) checks for now.
>
> Nelson
>