[Crash-utility] Re: [PATCH v2] symbols: expand all kernel module symtable if not all expanded previously

On Wed, Nov 29, 2023 at 7:32 PM Tao Liu <ltao(a)redhat.com&gt; wrote: > > Hi Lijiang, > > On Wed, Nov 29, 2023 at 7:04 PM lijiang <lijiang(a)redhat.com&gt; wrote: > > > > On Wed, Nov 29, 2023 at 10:10 AM HAGIO KAZUHITO(萩尾　一仁) <k-hagio-ab(a)nec.com&gt; wrote: > >> > >> Lianbo, > >> > >> My apologies, I mixed up your ack for the other one with for this one... > > > > > > No worries, Kazu. > > > >> > >> > >> Please check this later just in case. > >> > > > > For the v2, I have a comment: > > > > In the get_line_number(), if the variable 'lm' and ' lm->loaded_objfile' are valid, it may trigger the expand_all_symtabs() calling in the gdb_get_line_number(), why doesn't it work well there? As Tao mentioned, the objfile_has_full_symbols() may fail. > > > > Given that, I would recommend using the existed mechanism to fix this issue, the gdb_get_line_number() is added by crash-utility, so we just remove the check objfile_has_full_symbols() from the if-block, that should be good, because it always try to find a module address line number first, if not found, the expand_all_symtabs() will be invoked, and we don't need to worry about expanding all symbol tables every time. > > > > In addition, an advantage is that it can avoid changing the actual gdb code. > > > > Please refer to the following code sections: > > --- > > char * > > get_line_number(ulong addr, char *buf, int reserved) > > { > > ... > > if (module_symbol(addr, NULL, &lm, NULL, 0)) { > > if (!(lm->mod_flags & MOD_LOAD_SYMS)) > > return(buf); > > } > > ... > > if (!strlen(buf)) { > > req = &request; > > BZERO(req, sizeof(struct gnu_request)); > > req->command = GNU_GET_LINE_NUMBER; > > req->addr = addr; > > req->buf = buf; > > if (lm && lm->loaded_objfile) > > req->lm = lm; > > if ((sp = value_search(addr, NULL))) > > req->name = sp->name; > > gdb_interface(req); > > } > > ... > > return(buf); > > } > > > > --- > > static void > > gdb_get_line_number(struct gnu_request *req) > > { > > struct symtab_and_line sal; > > struct objfile *objfile; > > CORE_ADDR pc; > > > > #define LASTCHAR(s) (s[strlen(s)-1]) > > > > /* > > * Prime the addrmap pump. > > */ > > pc = req->addr; > > > > sal = find_pc_line(pc, 0); > > > > if (!sal.symtab) { > > /* > > * If a module address line number can't be found, it's typically > > * due to its addrmap still containing offset values because its > > * objfile doesn't have full symbols loaded. > > */ > > if (req->lm) { > > objfile = req->lm->loaded_objfile; > > if (!objfile_has_full_symbols(objfile) && objfile->sf) { > > objfile->sf->qf->expand_all_symtabs(objfile); > > sal = find_pc_line(pc, 0); > > } > > } > > ... > > } > > If I understand you correctly, you mean just remove > "!objfile_has_full_symbols(objfile)" from the if block, so the code > will be like the following right? > > if (req->lm) { > objfile = req->lm->loaded_objfile; > if (objfile->sf) { > objfile->sf->qf->expand_all_symtabs(objfile); > sal = find_pc_line(pc, 0); > } > } > > Yes, it can give the correct result, but this will have a great > performance decrease. I have tried this way but gave up due to Thank you for the try, Tao. > > performance loss. Objfile will always expand_all_symtabs(), no matter > if the symbols have been expanded or not. The previous faulty It's strange, I guess that it should be invoked only when a module address line number can not be found, that is to say: the find_pc_line() returned a Null 'sal.symtab'. I just tried it as below: crash> dis -rl ffffffffc0f60f82 | head gdb_get_line_number 7101 /usr/src/debug/kernel-4.18.0-425.13.1.el8_7/linux-4.18.0-425.13.1.el8_7.x86_64/drivers/scsi/lpfc/lpfc_hbadisc.c: 6756 0xffffffffc0f60eb0 <lpfc_nlp_get>: nopl 0x0(%rax,%rax,1) [FTRACE NOP] /usr/src/debug/kernel-4.18.0-425.13.1.el8_7/linux-4.18.0-425.13.1.el8_7.x86_64/drivers/scsi/lpfc/lpfc_hbadisc.c: 6759 ... crash> dis -rl ffffffffc0457000 /usr/src/debug/kernel-4.18.0-425.13.1.el8_7/linux-4.18.0-425.13.1.el8_7.x86_64/drivers/video/fbdev/core/syscopyarea.c: 316 0xffffffffc0457000 <sys_copyarea>: nopl 0x0(%rax,%rax,1) [FTRACE NOP] crash> dis -rl ffffffffc03ee000 /usr/src/debug/kernel-4.18.0-425.13.1.el8_7/linux-4.18.0-425.13.1.el8_7.x86_64/block/t10-pi.c: 258 0xffffffffc03ee000 <t10_pi_type3_prepare>: nopl 0x0(%rax,%rax,1) [FTRACE NOP] crash> And I did not see the debuginfo again(only one time): gdb_get_line_number 7101 Did I miss anything else?

Thanks Lianbo > > "!objfile_has_full_symbols(objfile)" check is to prevent such > re-expanding. > > Thanks, > Tao Liu > > > > > Just an idea, any comments? > > > > Thanks > > Lianbo > > > > > >> https://github.com/crash-utility/crash/commit/f2ee6fa6c841ddc37ba665909da... > >> > >> Thanks, > >> Kazu > >> > >> On 2023/11/20 15:30, Tao Liu wrote: > >> > Hi Kazu, > >> > > >> > On Mon, Nov 20, 2023 at 2:16 PM HAGIO KAZUHITO(萩尾　一仁) > >> > <k-hagio-ab(a)nec.com&gt; wrote: > >> >> > >> >> On 2023/11/17 16:52, Tao Liu wrote: > >> >> > >> >>> For the stacktrace of "dis -rl", it calls dw2_expand_all_symtabs() to expand > >> >>> all symtable of the objfile, or "*.ko.debug" in our case. However for > >> >>> the stacktrace of "bt", it doesn't expand all, but only a subset of symtable > >> >>> which is enough to find a symbol by dw2_lookup_symbol(). As a result, the > >> >>> objfile->compunit_symtabs, which is the head of a single linked list of > >> >>> struct compunit_symtab, is not NULL but didn't contain all symtables. It > >> >>> will not be reinitialized in gdb_get_line_number() by "dis -rl" because > >> >>> !objfile_has_full_symbols(objfile) check will fail, so it cannot display > >> >>> the proper code line number data. > >> >>> > >> >>> Since objfile_has_full_symbols(objfile) check cannot ensure all symbols > >> >>> been expanded, this patch add a new member as a flag for struct objfile > >> >>> to record if all symbols have been expanded. The flag will be set only ofter > >> >>> expand_all_symtabs been called. > >> >> > >> >> Thank you for the v2. > >> >> Great, I can't think of any better way than this for now. > >> >> > >> >> Acked-by: Kazuhito Hagio <k-hagio-ab(a)nec.com&gt; > >> >> > >> >> (maybe I will reduce the commit log a bit when merging.) > >> >> > >> > Thanks a lot for the patch review. Please go ahead if the commit log > >> > is too long. > >> > > >> > Thanks, > >> > Tao Liu > >> > > >> >> Thanks, > >> >> Kazu > >> >> > >> >>> > >> >>> Signed-off-by: Tao Liu <ltao(a)redhat.com&gt; > >> >>> --- > >> >>> v1 -> v2: new method for kernel module symtabs expansion. > >> >>> v1: expand all kernel modules symtabs when loading by mod -s/-S > >> >>> v2: record if a specific kernel module's symtab been all expanded, > >> >>> and skip re-expansion if true. > >> >>> --- > >> >>> gdb-10.2.patch | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++ > >> >>> 1 file changed, 50 insertions(+) > >> >>> > >> >>> diff --git a/gdb-10.2.patch b/gdb-10.2.patch > >> >>> index d81030d..2f7d585 100644 > >> >>> --- a/gdb-10.2.patch > >> >>> +++ b/gdb-10.2.patch > >> >>> @@ -3187,3 +3187,53 @@ exit 0 > >> >>> result = stringtab + symbol_entry->_n._n_n._n_offset; > >> >>> } > >> >>> else > >> >>> +--- gdb-10.2/gdb/objfiles.h.orig > >> >>> ++++ gdb-10.2/gdb/objfiles.h > >> >>> +@@ -712,6 +712,8 @@ struct objfile > >> >>> + next time. If an objfile does not have the symbols, it will > >> >>> + never have them. */ > >> >>> + bool skip_jit_symbol_lookup = false; > >> >>> ++ > >> >>> ++ bool all_symtabs_expanded = false; > >> >>> + }; > >> >>> + > >> >>> + /* A deleter for objfile. */ > >> >>> +--- gdb-10.2/gdb/symfile.c.orig > >> >>> ++++ gdb-10.2/gdb/symfile.c > >> >>> +@@ -1133,8 +1133,10 @@ symbol_file_add_with_addrs (bfd *abfd, const char *name, > >> >>> + printf_filtered (_("Expanding full symbols from %ps...\n"), > >> >>> + styled_string (file_name_style.style (), name)); > >> >>> + > >> >>> +- if (objfile->sf) > >> >>> ++ if (objfile->sf) { > >> >>> + objfile->sf->qf->expand_all_symtabs (objfile); > >> >>> ++ objfile->all_symtabs_expanded = true; > >> >>> ++ } > >> >>> + } > >> >>> + > >> >>> + /* Note that we only print a message if we have no symbols and have > >> >>> +--- gdb-10.2/gdb/symtab.c.orig > >> >>> ++++ gdb-10.2/gdb/symtab.c > >> >>> +@@ -7097,8 +7097,9 @@ gdb_get_line_number(struct gnu_request *req) > >> >>> + */ > >> >>> + if (req->lm) { > >> >>> + objfile = req->lm->loaded_objfile; > >> >>> +- if (!objfile_has_full_symbols(objfile) && objfile->sf) { > >> >>> ++ if (!objfile->all_symtabs_expanded && objfile->sf) { > >> >>> + objfile->sf->qf->expand_all_symtabs(objfile); > >> >>> ++ objfile->all_symtabs_expanded = true; > >> >>> + sal = find_pc_line(pc, 0); > >> >>> + } > >> >>> + } > >> >>> +@@ -7761,8 +7765,10 @@ iterate_datatypes (struct gnu_request *req) > >> >>> + { > >> >>> + for (objfile *objfile : current_program_space->objfiles ()) > >> >>> + { > >> >>> +- if (objfile->sf) > >> >>> ++ if (objfile->sf) { > >> >>> + objfile->sf->qf->expand_all_symtabs(objfile); > >> >>> ++ objfile->all_symtabs_expanded = true; > >> >>> ++ } > >> >>> + > >> >>> + for (compunit_symtab *cust : objfile->compunits ()) > >> >>> + { >