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)
{
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 (!(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);
}
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);
}
}
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/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
