[PATCH] RISC-V: Search for mapping symbols from the last one found
Joseph Faulls
Joseph.Faulls@imgtec.com
Tue May 14 09:10:28 GMT 2024
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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://sourceware.org/pipermail/binutils/attachments/20240514/9887cd05/attachment-0001.htm>
More information about the Binutils
mailing list