Hi Lijiang,
On Wed, Nov 29, 2023 at 9:07 PM lijiang <lijiang@xxxxxxxxxx> wrote:
>
> On Wed, Nov 29, 2023 at 7:32 PM Tao Liu <ltao@xxxxxxxxxx> wrote:
>>
>> Hi Lijiang,
>>
>> On Wed, Nov 29, 2023 at 7:04 PM lijiang <lijiang@xxxxxxxxxx> wrote:
>> >
>> > On Wed, Nov 29, 2023 at 10:10 AM HAGIO KAZUHITO(萩尾 一仁) <k-hagio-ab@xxxxxxx> 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?
>
I cannot reproduce the performance issue, not sure why...
The problem I encountered previously, is the first find_pc_line()
always return sal.symtab==NULL, so if
"!objfile_has_full_symbols(objfile)" check removed,
expand_all_symtabs() will be called multiple times, which will
decrease the overall performance. So I drafted the patch by using a
objfile->all_symtabs_expanded flag instead. And I have tried to debug
into find_pc_line() to find out the reason, however I failed.
Since I cannot reproduce the performance issue, then I think we can
use the removing "!objfile_has_full_symbols(objfile)" check, it is
more simple.
Thanks,
Tao Liu
> 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/f2ee6fa6c841ddc37ba665909dafbc7294c34d64
>> >>
>> >> 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@xxxxxxx> 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@xxxxxxx>
>> >> >>
>> >> >> (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@xxxxxxxxxx>
>> >> >>> ---
>> >> >>> 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 ())
>> >> >>> + {
>>
--
Crash-utility mailing list -- devel@xxxxxxxxxxxxxxxxxxxxxxxxxxx
To unsubscribe send an email to devel-leave@xxxxxxxxxxxxxxxxxxxxxxxxxxx
https://${domain_name}/admin/lists/devel.lists.crash-utility.osci.io/
Contribution Guidelines: https://github.com/crash-utility/crash/wiki
