<div dir="ltr"><div dir="ltr">There is a similar (n >= last_map_symbol) check when we need to backtrace searching, <a href="https://github.com/bminor/binutils-gdb/blob/master/opcodes/riscv-dis.c#L1109">https://github.com/bminor/binutils-gdb/blob/master/opcodes/riscv-dis.c#L1109</a>, should we also need to fix this?</div><div dir="ltr"><div><br><div>diff --git a/opcodes/riscv-dis.c b/opcodes/riscv-dis.c <br>index 684098d386a..e6596c47423 100644 <br>--- a/opcodes/riscv-dis.c <br>+++ b/opcodes/riscv-dis.c <br>@@ -1076,11 +1076,9 @@ riscv_search_mapping_symbol (bfd_vma memaddr, <br> from_last_map_symbol = (last_map_symbol >= 0<br> && info->stop_offset == last_stop_offset);<br><br>- /* Start scanning at the start of the function, or wherever <br>- we finished last time. */ <br>- n = info->symtab_pos + 1; <br>- if (from_last_map_symbol && n >= last_map_symbol) <br>- n = last_map_symbol; <br>+ /* Start scanning from wherever we finished last time, or the start <br>+ of the function. */ <br>+ n = from_last_map_symbol ? last_map_symbol : info->symtab_pos + 1; <br><br> /* Find the suitable mapping symbol to dump. */<br> for (; n < info->symtab_size; n++)<br>@@ -1105,9 +1103,7 @@ riscv_search_mapping_symbol (bfd_vma memaddr, <br> can pick up a text mapping symbol of a preceeding section. */<br> if (!found)<br> {<br>- n = info->symtab_pos; <br>- if (from_last_map_symbol && n >= last_map_symbol) <br>- n = last_map_symbol; <br>+ n = from_last_map_symbol ? last_map_symbol : info->symtab_pos; <br><br> for (; n >= 0; n--)<br></div></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, May 6, 2024 at 11:12 PM Palmer Dabbelt <<a href="mailto:palmer@dabbelt.com">palmer@dabbelt.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Fri, 03 May 2024 05:56:47 PDT (-0700), <a href="mailto:Joseph.Faulls@imgtec.com" target="_blank">Joseph.Faulls@imgtec.com</a> wrote:<br>
> Ping. I know it's a small patch, but it sped up disassembly of my use case from 5 minutes to 5 seconds.<br>
> ________________________________<br>
> From: Joseph Faulls<br>
> Sent: Thursday, April 18, 2024 11:48 AM<br>
> To: <a href="mailto:binutils@sourceware.org" target="_blank">binutils@sourceware.org</a> <<a href="mailto:binutils@sourceware.org" target="_blank">binutils@sourceware.org</a>><br>
> Cc: <a href="mailto:nelson@rivosinc.com" target="_blank">nelson@rivosinc.com</a> <<a href="mailto:nelson@rivosinc.com" target="_blank">nelson@rivosinc.com</a>><br>
> Subject: [PATCH] RISC-V: Search for mapping symbols from the last one found<br>
><br>
> With previous behaviour, multiple mapping symbols within the same<br>
> function would result in all the mapping symbols being searched.<br>
> This could slow down disassembly dramatically.<br>
><br>
> Multiple mapping symbols within a function can be a result of encoding<br>
> instructions as data, like sometimes seen in random instruction<br>
> generators.<br>
><br>
> opcodes/ChangeLog:<br>
><br>
> * riscv-dis.c (riscv_search_mapping_symbol): Use last mapping<br>
> symbol if it exists.<br>
><br>
> Note for reviewers:<br>
> 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!<br>
<br>
Sorry for being slow here. Nelson and I were talking a bit right before <br>
the weekend and it looks like this was just a bug, but it's very similar <br>
to what the Arm code does.<br></blockquote><div><br></div><div>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, <a href="https://github.com/bminor/binutils-gdb/commit/51457761649bab6868343b3da2258d73a62901f7">https://github.com/bminor/binutils-gdb/commit/51457761649bab6868343b3da2258d73a62901f7</a>. 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.</div><div><br></div><div>Nelson</div></div></div>