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

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

[Joseph Faulls]

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

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!

---
 opcodes/riscv-dis.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/opcodes/riscv-dis.c b/opcodes/riscv-dis.c
index 684098d386a..24286702dd3 100644
--- a/opcodes/riscv-dis.c
+++ b/opcodes/riscv-dis.c
@@ -1076,11 +1076,12 @@ 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)
+  /* Start scanning from wherever we finished last time, or the start
+     of the function.  */
+  if (from_last_map_symbol)
     n = last_map_symbol;
+  else
+    n = info->symtab_pos + 1;

   /* Find the suitable mapping symbol to dump.  */
   for (; n < info->symtab_size; n++)
--
2.34.1

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

[Joseph Faulls]

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

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!

---
 opcodes/riscv-dis.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/opcodes/riscv-dis.c b/opcodes/riscv-dis.c
index 684098d386a..24286702dd3 100644
--- a/opcodes/riscv-dis.c
+++ b/opcodes/riscv-dis.c
@@ -1076,11 +1076,12 @@ 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)
+  /* Start scanning from wherever we finished last time, or the start
+     of the function.  */
+  if (from_last_map_symbol)
     n = last_map_symbol;
+  else
+    n = info->symtab_pos + 1;

   /* Find the suitable mapping symbol to dump.  */
   for (; n < info->symtab_size; n++)
--
2.34.1

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

[Palmer Dabbelt]

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.

So I think we just need to figure out exactly what's going on here, it's 
pretty early on my end and Nelson is probably asleep...

>
> ---
>  opcodes/riscv-dis.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/opcodes/riscv-dis.c b/opcodes/riscv-dis.c
> index 684098d386a..24286702dd3 100644
> --- a/opcodes/riscv-dis.c
> +++ b/opcodes/riscv-dis.c
> @@ -1076,11 +1076,12 @@ 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)
> +  /* Start scanning from wherever we finished last time, or the start
> +     of the function.  */
> +  if (from_last_map_symbol)
>      n = last_map_symbol;
> +  else
> +    n = info->symtab_pos + 1;
>
>    /* Find the suitable mapping symbol to dump.  */
>    for (; n < info->symtab_size; n++)

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

[Nelson Chu]

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

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,
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.
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

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

[Joseph Faulls]

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

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<mailto:palmer@dabbelt.com>> wrote:
On Fri, 03 May 2024 05:56:47 PDT (-0700), Joseph.Faulls@imgtec.com<mailto: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<mailto:binutils@sourceware.org> <binutils@sourceware.org<mailto:binutils@sourceware.org>>
> Cc: nelson@rivosinc.com<mailto:nelson@rivosinc.com> <nelson@rivosinc.com<mailto: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

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

[Nelson Chu]

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