RISC-V: Search for mapping symbols from the last one found
Checks
Commit Message
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(-)
--
2.34.1
Comments
Ping. I know it's a small patch, but it sped up disassembly of my use case from 5 minutes to 5 seconds.
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++)
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
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
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
>
@@ -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++)