5:31 a.m.
Hi Lijiang,
On Wed, Nov 29, 2023 at 7:04 PM lijiang <lijiang(a)redhat.com> wrote:
On Wed, Nov 29, 2023 at 10:10 AM HAGIO KAZUHITO(萩尾 一仁) <k-hagio-ab(a)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/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> 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>
> >>
> >> (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>
> >>> ---
> >>> 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 ())
> >>> + {