5:03 a.m.
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);
}
}
...
}
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 ())
>>> + {