Thanks, committed ;) Nelson On Tue, May 14, 2024 at 5:10 PM Joseph Faulls 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 > *Sent:* Tuesday, May 14, 2024 1:34 AM > *To:* Palmer Dabbelt > *Cc:* Joseph Faulls ; 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] > , > 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 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 > > Cc: 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] > . > 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 >