[Crash-utility] [PATCH 1/2] Let gdb get kernel module symbols info from crash

[Tao Liu]

Hi Lijiang,

On Mon, Aug 29, 2022 at 8:36 PM lijiang <lijiang at redhat.com> wrote:
>
> Hi, Tao
> Thank you for the patch.
> On Wed, Aug 24, 2022 at 12:12 PM <crash-utility-request at redhat.com> wrote:
>>
>>
>> Date: Wed, 24 Aug 2022 12:10:33 +0800
>> From: Tao Liu <ltao at redhat.com>
>> To: crash-utility at redhat.com
>> Subject: [Crash-utility] [PATCH 1/2] Let gdb get kernel module symbols
>>         info from crash
>> Message-ID: <20220824041033.40559-1-ltao at redhat.com>
>> Content-Type: text/plain; charset="US-ASCII"; x-default=true
>>
>> Gdb will try to resolve an address to its corresponding symbol name such as
>> when printing a structure. It works fine for kernel symbols, because gdb can
>> find them through vmlinux. However as for kernel modules symbols, crash
>> resolves them by dig into "struct module", which gdb don't know. As a
>> results, gdb fails to translate an kernel modules address to it's symbolic
>> name. For example we can reproduce the issue as follows.
>>
>>     crash> timer
>>     ....
>>     4331308176       336  ffff94ea24240860  ffffffffc03762c0  <estimation_timer>
>>     ....
>>     crash> sym 0xffffffffc03762c0
>>     ffffffffc03762c0 (t) estimation_timer [ip_vs]
>>
>> Before patch:
>>     crash> timer_list ffff94ea24240860
>>     struct timer_list {
>>       ....
>>       function = 0xffffffffc03762c0,
>>       ....
>>     }
>>
>> After patch:
>>     crash> timer_list ffff94ea24240860
>>     struct timer_list {
>>       ....
>>       function = 0xffffffffc03762c0 <estimation_timer>,
>>       ....
>>     }
>>
>> In this patch, we add an interface for gdb, when gdb trying to build kernel
>> module's address symbolic, the info can be get from crash.
>>
>> Signed-off-by: Tao Liu <ltao at redhat.com>
>> ---
>>  defs.h          |  2 ++
>>  gdb-10.2.patch  | 35 +++++++++++++++++++++++++++++++++--
>>  gdb_interface.c | 12 ++++++++++++
>>  3 files changed, 47 insertions(+), 2 deletions(-)
>>
>> diff --git a/defs.h b/defs.h
>> index 9d6d891..afdcf6c 100644
>> --- a/defs.h
>> +++ b/defs.h
>> @@ -4874,6 +4874,7 @@ extern "C" int patch_kernel_symbol(struct gnu_request *);
>>  struct syment *symbol_search(char *);
>>  int gdb_line_number_callback(ulong, ulong, ulong);
>>  int gdb_print_callback(ulong);
>> +char *gdb_lookup_module_symbol(ulong, ulong *);
>>  extern "C" int same_file(char *, char *);
>>  #endif
>>
>> @@ -7284,6 +7285,7 @@ int gdb_pass_through(char *, FILE *, ulong);
>>  int gdb_readmem_callback(ulong, void *, int, int);
>>  int gdb_line_number_callback(ulong, ulong, ulong);
>>  int gdb_print_callback(ulong);
>> +char *gdb_lookup_module_symbol(ulong, ulong *);
>>  void gdb_error_hook(void);
>>  void restore_gdb_sanity(void);
>>  int is_gdb_command(int, ulong);
>> diff --git a/gdb-10.2.patch b/gdb-10.2.patch
>> index f0034ed..d08e9ab 100644
>> --- a/gdb-10.2.patch
>> +++ b/gdb-10.2.patch
>> @@ -399,7 +399,38 @@ exit 0
>>     if (build_address_symbolic (gdbarch, addr, do_demangle, false, &name,
>>                                 &offset, &filename, &line, &unmapped))
>>       return 0;
>> -@@ -1221,6 +1230,43 @@ print_command_1 (const char *args, int voidprint)
>> +@@ -567,6 +576,10 @@ print_address_symbolic (struct gdbarch *gdbarch, CORE_ADDR addr,
>> +
>> + /* See valprint.h.  */
>> +
>> ++#ifdef CRASH_MERGE
>> ++extern "C" char *gdb_lookup_module_symbol(unsigned long, unsigned long *);
>> ++#endif
>> ++
>> + int
>> + build_address_symbolic (struct gdbarch *gdbarch,
>> +                       CORE_ADDR addr,  /* IN */
>> +@@ -673,7 +686,19 @@ build_address_symbolic (struct gdbarch *gdbarch,
>> +       }
>> +     }
>> +   if (symbol == NULL && msymbol.minsym == NULL)
>> ++#ifdef CRASH_MERGE
>> ++  {
>> ++    char *name_ptr = gdb_lookup_module_symbol(addr, (unsigned long *)offset);
>> ++    if (name_ptr) {
>> ++      *name = name_ptr;
>> ++      return 0;
>> ++    } else {
>> ++      return 1;
>> ++    }
>> ++  }
>> ++#else
>> +     return 1;
>> ++#endif
>> +
>> +   /* If the nearest symbol is too far away, don't print anything symbolic.  */
>> +
>> +@@ -1221,6 +1246,43 @@ print_command_1 (const char *args, int voidprint)
>>       print_value (val, print_opts);
>>   }
>>
>
> Usually, the gdb patch needs to be put at the end of gdb-10.2.patch, which can help to
> apply your patch correctly. Could you please refer to the following commit?
> 4c85e982d25a ("gdb: fix for assigning NULL to std::string")
>
OK, it looks better, will update the v2 patch.

> In some commands, crash will trigger symbols loading via the value_search(), and also decide if the module symbols  need to be loaded there, but for this case, there seems to be no better solution.
>
Yes.

Thanks,
Tao Liu

> Thanks.
> Lianbo
>
>> @@ -443,7 +474,7 @@ exit 0
>>   /* See valprint.h.  */
>>
>>   void
>> -@@ -2855,6 +2901,12 @@ but no count or size letter (see \"x\" command)."),
>> +@@ -2855,6 +2917,12 @@ but no count or size letter (see \"x\" command)."),
>>     c = add_com ("print", class_vars, print_command, print_help.c_str ());
>>     set_cmd_completer_handle_brkchars (c, print_command_completer);
>>     add_com_alias ("p", "print", class_vars, 1);
>> diff --git a/gdb_interface.c b/gdb_interface.c
>> index 3a7fcc9..b14319c 100644
>> --- a/gdb_interface.c
>> +++ b/gdb_interface.c
>> @@ -935,6 +935,18 @@ gdb_print_callback(ulong addr)
>>                 return IS_KVADDR(addr);
>>  }
>>
>> +char *
>> +gdb_lookup_module_symbol(ulong addr, ulong *offset)
>> +{
>> +       struct syment *sp;
>> +
>> +       if ((sp = value_search_module(addr, offset))) {
>> +               return sp->name;
>> +       } else {
>> +               return NULL;
>> +       }
>> +}
>> +
>>  /*
>>   *  Used by gdb_interface() to catch gdb-related errors, if desired.
>>   */
>> --
>> 2.33.1
>>