1:52 a.m.
There is an issue that, for kernel modules, "dis -rl" fails to display
module's code line number data after execute "bt" cmd in crash.
Without the patch:
crsah> mod -S
crash> bt
PID: 1500 TASK: ff2bd8b093524000 CPU: 16 COMMAND: "lpfc_worker_0"
#0 [ff2c9f725c39f9e0] machine_kexec at ffffffff8e0686d3
...snip...
#7 [ff2c9f725c39fc00] page_fault at ffffffff8ea0114e
[exception RIP: lpfc_nlp_get+210]
RIP: ffffffffc0f60f82 RSP: ff2c9f725c39fcb0 RFLAGS: 00010046
RAX: 0000000000000046 RBX: ff2bd8d8ac056000 RCX: 0000000000fffffc
RDX: 0000000000000000 RSI: 0000000000000046 RDI: 0000000000000046
RBP: ff2bd8d8ac056090 R8: 0000000000000000 R9: 0000000000000000
R10: ff2bd90d1f8701c0 R11: 0000000000000001 R12: ff2bd93320482ae0
R13: ff2bd93051a80524 R14: ff2bd93051a80000 R15: ff2bd9332079fc00
ORIG_RAX: ffffffffffffffff CS: 0010 SS: 0018
#8 [ff2c9f725c39fcc0] __lpfc_sli_release_iocbq_s4 at ffffffffc0f2f425 [lpfc]
...snip...
crash> dis -rl ffffffffc0f60f82
0xffffffffc0f60eb0 <lpfc_nlp_get>: nopl 0x0(%rax,%rax,1) [FTRACE NOP]
0xffffffffc0f60eb5 <lpfc_nlp_get+5>: push %rbp
0xffffffffc0f60eb6 <lpfc_nlp_get+6>: push %rbx
0xffffffffc0f60eb7 <lpfc_nlp_get+7>: test %rdi,%rdi
With the patch:
crash> mod -S
crash> bt
PID: 1500 TASK: ff2bd8b093524000 CPU: 16 COMMAND: "lpfc_worker_0"
#0 [ff2c9f725c39f9e0] machine_kexec at ffffffff8e0686d3
...snip...
#7 [ff2c9f725c39fc00] page_fault at ffffffff8ea0114e
[exception RIP: lpfc_nlp_get+210]
RIP: ffffffffc0f60f82 RSP: ff2c9f725c39fcb0 RFLAGS: 00010046
RAX: 0000000000000046 RBX: ff2bd8d8ac056000 RCX: 0000000000fffffc
RDX: 0000000000000000 RSI: 0000000000000046 RDI: 0000000000000046
RBP: ff2bd8d8ac056090 R8: 0000000000000000 R9: 0000000000000000
R10: ff2bd90d1f8701c0 R11: 0000000000000001 R12: ff2bd93320482ae0
R13: ff2bd93051a80524 R14: ff2bd93051a80000 R15: ff2bd9332079fc00
ORIG_RAX: ffffffffffffffff CS: 0010 SS: 0018
#8 [ff2c9f725c39fcc0] __lpfc_sli_release_iocbq_s4 at ffffffffc0f2f425 [lpfc]
...snip...
crash> dis -rl ffffffffc0f60f82
/usr/src/debug/kernel-4.18.0-425.13.1.el8_7/linux-4.18.0-425.13.1.el8_7.x86_64/drivers/scsi/lpfc/lpfc_hbadisc.c:
6756
0xffffffffc0f60eb0 <lpfc_nlp_get>: nopl 0x0(%rax,%rax,1) [FTRACE NOP]
/usr/src/debug/kernel-4.18.0-425.13.1.el8_7/linux-4.18.0-425.13.1.el8_7.x86_64/drivers/scsi/lpfc/lpfc_hbadisc.c:
6759
0xffffffffc0f60eb5 <lpfc_nlp_get+5>: push %rbp
The root cause is, after kernel module been loaded by command mod, the symtable
is not expanded in gdb side. crash command bt or dis will trigger such an
expansion. However the symtable expansion is different for the 2 commands:
The stack trace of "dis -rl" for symtable expanding:
#0 0x00000000008d8d9f in add_compunit_symtab_to_objfile (cu=cu@entry=0xe6a77a0) at
symfile.c:2914
#1 0x00000000006d3293 in buildsym_compunit::end_symtab_with_blockvector
(this=<optimized out>, static_block=static_block@entry=0xfbe4b60, section=1,
expandable=expandable@entry=0) at buildsym.c:1072
#2 0x00000000006d336a in buildsym_compunit::end_symtab_from_static_block
(this=<optimized out>, static_block=static_block@entry=0xfbe4b60,
section=<optimized out>, expandable=expandable@entry=0) at buildsym.c:1106
#3 0x000000000077e8e9 in process_full_comp_unit (pretend_language=<optimized
out>, cu=0x8ee4c60) at /usr/include/c++/8/bits/unique_ptr.h:716
#4 process_queue (per_objfile=0xc54c870) at dwarf2/read.c:9220
#5 dw2_do_instantiate_symtab (per_cu=<optimized out>, per_objfile=0xc54c870,
skip_partial=<optimized out>) at dwarf2/read.c:2448
#6 0x000000000077ed67 in dw2_instantiate_symtab (per_cu=0xdd0a320,
per_objfile=0xc54c870, skip_partial=<optimized out>) at dwarf2/read.c:2472
#7 0x000000000077f75e in dw2_expand_all_symtabs (objfile=<optimized out>) at
dwarf2/read.c:3768
#8 0x00000000008f254d in gdb_get_line_number (req=0x7fffffffb1f0) at symtab.c:7112
#9 0x00000000008f22af in gdb_command_funnel_1 (req=0x7fffffffb1f0) at symtab.c:7023
#10 0x00000000008f2003 in gdb_command_funnel (req=0x7fffffffb1f0) at symtab.c:6965
#11 0x00000000005b7f02 in gdb_interface (req=req@entry=0x7fffffffb1f0) at
gdb_interface.c:409
#12 0x00000000005f5bd8 in get_line_number (addr=18446744072651935408,
buf=buf@entry=0x7fffffffd460 "", reserved=reserved@entry=0) at symbols.c:4440
#13 0x000000000059e574 in cmd_dis () at kernel.c:2143
The stack trace of "bt" for symtable expanding:
#0 0x00000000008d8d9f in add_compunit_symtab_to_objfile (cu=cu@entry=0x1ad15630) at
symfile.c:2914
#1 0x00000000006d3293 in buildsym_compunit::end_symtab_with_blockvector
(this=<optimized out>, static_block=static_block@entry=0x1db0be30, section=1,
expandable=expandable@entry=0) at buildsym.c:1072
#2 0x00000000006d336a in buildsym_compunit::end_symtab_from_static_block
(this=<optimized out>, static_block=static_block@entry=0x1db0be30,
section=<optimized out>, expandable=expandable@entry=0) at buildsym.c:1106
#3 0x000000000077e8e9 in process_full_comp_unit (pretend_language=<optimized
out>, cu=0x7465240) at /usr/include/c++/8/bits/unique_ptr.h:716
#4 process_queue (per_objfile=0xc113810) at dwarf2/read.c:9220
#5 dw2_do_instantiate_symtab (per_cu=<optimized out>, per_objfile=0xc113810,
skip_partial=<optimized out>) at dwarf2/read.c:2448
#6 0x000000000077ed67 in dw2_instantiate_symtab (per_cu=0xdd069d0,
per_objfile=0xc113810, skip_partial=<optimized out>) at dwarf2/read.c:2472
#7 0x000000000077f8ed in dw2_lookup_symbol (objfile=<optimized out>,
block_index=STATIC_BLOCK, name=0x7fffffffc890 "cpumask_t", domain=STRUCT_DOMAIN)
at dwarf2/read.c:3669
#8 0x00000000008e6d03 in lookup_symbol_via_quick_fns (objfile=0xdd277a0,
block_index=STATIC_BLOCK, name=0x7fffffffc890 "cpumask_t", domain=STRUCT_DOMAIN)
at symtab.c:2392
#9 0x00000000008e7153 in lookup_symbol_in_objfile (objfile=0xdd277a0,
block_index=STATIC_BLOCK, name=0x7fffffffc890 "cpumask_t", domain=STRUCT_DOMAIN)
at symtab.c:2541
#10 0x00000000008e73c6 in lookup_symbol_global_or_static_iterator_cb (objfile=0xdd277a0,
cb_data=0x7fffffffc470) at symtab.c:2615
#11 0x00000000008b99c4 in svr4_iterate_over_objfiles_in_search_order
(gdbarch=<optimized out>, cb=0x8e7342
<lookup_symbol_global_or_static_iterator_cb(objfile*, void*)>,
cb_data=0x7fffffffc470, current_objfile=0x0) at solib-svr4.c:3248
#12 0x00000000008e754e in lookup_global_or_static_symbol (name=0x7fffffffc890
"cpumask_t", block_index=STATIC_BLOCK, objfile=0x0, domain=STRUCT_DOMAIN) at
symtab.c:2660
#13 0x00000000008e75da in lookup_static_symbol (name=0x7fffffffc890
"cpumask_t", domain=STRUCT_DOMAIN) at symtab.c:2678
#14 0x00000000008e632c in lookup_symbol_aux (name=0x7fffffffc890 "cpumask_t",
match_type=symbol_name_match_type::FULL, block=0x0, domain=STRUCT_DOMAIN,
language=language_c, is_a_field_of_this=0x0) at symtab.c:2122
#15 0x00000000008e5a7a in lookup_symbol_in_language (name=0x7fffffffc890
"cpumask_t", block=0x0, domain=STRUCT_DOMAIN, lang=language_c,
is_a_field_of_this=0x0) at symtab.c:1889
#16 0x00000000008e5b30 in lookup_symbol (name=0x7fffffffc890 "cpumask_t",
block=0x0, domain=STRUCT_DOMAIN, is_a_field_of_this=0x0) at symtab.c:1915
#17 0x00000000008f2a4a in gdb_get_datatype (req=0x7fffffffc730) at symtab.c:7229
#18 0x00000000008f22c0 in gdb_command_funnel_1 (req=0x7fffffffc730) at symtab.c:7027
#19 0x00000000008f2003 in gdb_command_funnel (req=0x7fffffffc730) at symtab.c:6965
#20 0x00000000005b7f02 in gdb_interface (req=req@entry=0x7fffffffc730) at
gdb_interface.c:409
#21 0x00000000005f8a9f in datatype_info (name=name@entry=0xa8454d "cpumask_t",
member=member@entry=0x0, dm=dm@entry=0xfffffffffffffffc) at symbols.c:5715
#22 0x0000000000599947 in cpu_map_size (type=<optimized out>) at kernel.c:913
#23 0x00000000005a975d in get_cpus_online () at kernel.c:9556
#24 0x0000000000637a8b in diskdump_get_prstatus_percpu (cpu=16) at diskdump.c:2277
#25 0x000000000062f0e4 in get_netdump_regs_x86_64 (bt=0x7fffffffd950,
ripp=0x7fffffffd130, rspp=0x7fffffffd138) at netdump.c:3471
#26 0x000000000059fe68 in back_trace (bt=bt@entry=0x7fffffffd950) at kernel.c:3092
#27 0x00000000005ab1cb in cmd_bt () at kernel.c:2859
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.
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 ())
+ {
--
2.40.1
12:16 a.m.
New subject: [PATCH v2] symbols: expand all kernel module symtable if not all expanded previously
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,
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 ())
> + {
12:30 a.m.
New subject: [PATCH v2] symbols: expand all kernel module symtable if not all expanded previously
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 ())
> + {
8:10 p.m.
New subject: [PATCH v2] symbols: expand all kernel module symtable if not all expanded previously
Lianbo,
My apologies, I mixed up your ack for the other one with for this one...
Please check this later just in case.
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 ())
>>> + {
5:03 a.m.
New subject: [PATCH v2] symbols: expand all kernel module symtable if not all expanded previously
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 ())
>>> + {
5:31 a.m.
New subject: [PATCH v2] symbols: expand all kernel module symtable if not all expanded previously
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 ())
> >>> + {
7:07 a.m.
New subject: [PATCH v2] symbols: expand all kernel module symtable if not all expanded previously
On Wed, Nov 29, 2023 at 7:32 PM Tao Liu <ltao(a)redhat.com> wrote:
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
Thank you for the try, Tao.
performance loss. Objfile will always expand_all_symtabs(), no
matter
if the symbols have been expanded or not. The previous faulty
It's strange, I guess that it should be invoked only when a module address
line number can not be found, that is to say: the find_pc_line() returned a
Null 'sal.symtab'.
I just tried it as below:
crash> dis -rl ffffffffc0f60f82 | head
gdb_get_line_number 7101
/usr/src/debug/kernel-4.18.0-425.13.1.el8_7/linux-4.18.0-425.13.1.el8_7.x86_64/drivers/scsi/lpfc/lpfc_hbadisc.c:
6756
0xffffffffc0f60eb0 <lpfc_nlp_get>: nopl 0x0(%rax,%rax,1) [FTRACE NOP]
/usr/src/debug/kernel-4.18.0-425.13.1.el8_7/linux-4.18.0-425.13.1.el8_7.x86_64/drivers/scsi/lpfc/lpfc_hbadisc.c:
6759
...
crash> dis -rl ffffffffc0457000
/usr/src/debug/kernel-4.18.0-425.13.1.el8_7/linux-4.18.0-425.13.1.el8_7.x86_64/drivers/video/fbdev/core/syscopyarea.c:
316
0xffffffffc0457000 <sys_copyarea>: nopl 0x0(%rax,%rax,1) [FTRACE NOP]
crash> dis -rl ffffffffc03ee000
/usr/src/debug/kernel-4.18.0-425.13.1.el8_7/linux-4.18.0-425.13.1.el8_7.x86_64/block/t10-pi.c:
258
0xffffffffc03ee000 <t10_pi_type3_prepare>: nopl 0x0(%rax,%rax,1)
[FTRACE NOP]
crash>
And I did not see the debuginfo again(only one time):
gdb_get_line_number 7101
Did I miss anything else?
Thanks
Lianbo
"!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 ())
>> >>> + {
9:31 a.m.
New subject: [PATCH v2] symbols: expand all kernel module symtable if not all expanded previously
Hi Lijiang,
On Wed, Nov 29, 2023 at 9:07 PM lijiang <lijiang(a)redhat.com> wrote:
On Wed, Nov 29, 2023 at 7:32 PM Tao Liu <ltao(a)redhat.com> wrote:
>
> 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
Thank you for the try, Tao.
>
> performance loss. Objfile will always expand_all_symtabs(), no matter
> if the symbols have been expanded or not. The previous faulty
It's strange, I guess that it should be invoked only when a module address line
number can not be found, that is to say: the find_pc_line() returned a Null
'sal.symtab'.
I just tried it as below:
crash> dis -rl ffffffffc0f60f82 | head
gdb_get_line_number 7101
/usr/src/debug/kernel-4.18.0-425.13.1.el8_7/linux-4.18.0-425.13.1.el8_7.x86_64/drivers/scsi/lpfc/lpfc_hbadisc.c:
6756
0xffffffffc0f60eb0 <lpfc_nlp_get>: nopl 0x0(%rax,%rax,1) [FTRACE NOP]
/usr/src/debug/kernel-4.18.0-425.13.1.el8_7/linux-4.18.0-425.13.1.el8_7.x86_64/drivers/scsi/lpfc/lpfc_hbadisc.c:
6759
...
crash> dis -rl ffffffffc0457000
/usr/src/debug/kernel-4.18.0-425.13.1.el8_7/linux-4.18.0-425.13.1.el8_7.x86_64/drivers/video/fbdev/core/syscopyarea.c:
316
0xffffffffc0457000 <sys_copyarea>: nopl 0x0(%rax,%rax,1) [FTRACE NOP]
crash> dis -rl ffffffffc03ee000
/usr/src/debug/kernel-4.18.0-425.13.1.el8_7/linux-4.18.0-425.13.1.el8_7.x86_64/block/t10-pi.c:
258
0xffffffffc03ee000 <t10_pi_type3_prepare>: nopl 0x0(%rax,%rax,1) [FTRACE
NOP]
crash>
And I did not see the debuginfo again(only one time):
gdb_get_line_number 7101
Did I miss anything else?
I cannot reproduce the performance issue, not sure why...
The problem I encountered previously, is the first find_pc_line()
always return sal.symtab==NULL, so if
"!objfile_has_full_symbols(objfile)" check removed,
expand_all_symtabs() will be called multiple times, which will
decrease the overall performance. So I drafted the patch by using a
objfile->all_symtabs_expanded flag instead. And I have tried to debug
into find_pc_line() to find out the reason, however I failed.
Since I cannot reproduce the performance issue, then I think we can
use the removing "!objfile_has_full_symbols(objfile)" check, it is
more simple.
Thanks,
Tao Liu
Thanks
Lianbo
>
> "!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 ())
> >> >>> + {
>
1:56 a.m.
New subject: [PATCH v2] symbols: expand all kernel module symtable if not all expanded previously
On 2023/11/30 0:31, Tao Liu wrote:
> And I did not see the debuginfo again(only one time):
> gdb_get_line_number 7101
>
> Did I miss anything else?
Is there no case where sal.symtab == NULL after expansion?
I don't think of, but e.g. a wrong address is requested.
personally I'm ok with the current patch for fail-safe, quite a simple one.
also reverting is troublesome... this is my fault though.
Thanks,
Kazu
>>
> I cannot reproduce the performance issue, not sure why...
>
> The problem I encountered previously, is the first find_pc_line()
> always return sal.symtab==NULL, so if
> "!objfile_has_full_symbols(objfile)" check removed,
> expand_all_symtabs() will be called multiple times, which will
> decrease the overall performance. So I drafted the patch by using a
> objfile->all_symtabs_expanded flag instead. And I have tried to debug
> into find_pc_line() to find out the reason, however I failed.
>
> Since I cannot reproduce the performance issue, then I think we can
> use the removing "!objfile_has_full_symbols(objfile)" check, it is
> more simple.
>
> Thanks,
> Tao Liu
>
2:52 a.m.
New subject: [PATCH v2] symbols: expand all kernel module symtable if not all expanded previously
On Thu, Nov 30, 2023 at 3:56 PM HAGIO KAZUHITO(萩尾 一仁) <k-hagio-ab(a)nec.com>
wrote:
On 2023/11/30 0:31, Tao Liu wrote:
>> And I did not see the debuginfo again(only one time):
>> gdb_get_line_number 7101
>>
>> Did I miss anything else?
Is there no case where sal.symtab == NULL after expansion?
I don't think of, but e.g. a wrong address is requested.
personally I'm ok with the current patch for fail-safe, quite a simple one.
also reverting is troublesome... this is my fault though.
No worries, Kazu. If any risks are found in the future, we can try to fix
them.
Let's leave it there for now. Just an idea, no other specific concerns.
Thanks
Lianbo
Thanks,
Kazu
>>
> I cannot reproduce the performance issue, not sure why...
>
> The problem I encountered previously, is the first find_pc_line()
> always return sal.symtab==NULL, so if
> "!objfile_has_full_symbols(objfile)" check removed,
> expand_all_symtabs() will be called multiple times, which will
> decrease the overall performance. So I drafted the patch by using a
> objfile->all_symtabs_expanded flag instead. And I have tried to debug
> into find_pc_line() to find out the reason, however I failed.
>
> Since I cannot reproduce the performance issue, then I think we can
> use the removing "!objfile_has_full_symbols(objfile)" check, it is
> more simple.
>
> Thanks,
> Tao Liu
>
8:04 p.m.
New subject: [PATCH v2] symbols: expand all kernel module symtable if not all expanded previously
On 2023/11/30 17:52, lijiang wrote:
On Thu, Nov 30, 2023 at 3:56 PM HAGIO KAZUHITO(萩尾 一仁)
<k-hagio-ab(a)nec.com>
wrote:
> On 2023/11/30 0:31, Tao Liu wrote:
>
>>> And I did not see the debuginfo again(only one time):
>>> gdb_get_line_number 7101
>>>
>>> Did I miss anything else?
>
> Is there no case where sal.symtab == NULL after expansion?
> I don't think of, but e.g. a wrong address is requested.
>
> personally I'm ok with the current patch for fail-safe, quite a simple one.
>
> also reverting is troublesome... this is my fault though.
>
>
No worries, Kazu. If any risks are found in the future, we can try to fix
them.
Let's leave it there for now. Just an idea, no other specific concerns.
I see, thanks.
Kazu
361
days inactive
375
days old
devel@lists.crash-utility.osci.io
10 comments
3 participants
participants (3)
-
HAGIO KAZUHITO(萩尾 一仁) -
lijiang -
Tao Liu