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
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@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
