Hi Lijiang,

On Wed, Nov 29, 2023 at 7:04 PM lijiang <lijiang@redhat.com> wrote:
>
> On Wed, Nov 29, 2023 at 10:10 AM HAGIO KAZUHITO(萩尾　一仁) <k-hagio-ab@nec.com> 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

performance loss. Objfile will always expand_all_symtabs(), no matter
if the symbols have been expanded or not. The previous faulty

"!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@nec.com> 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@nec.com>
>> >>
>> >> (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@redhat.com>
>> >>> ---
>> >>> 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 ())
>> >>> +         {