Re: [PATCH] Add "set debug minsyms" command

Re: [PATCH] Add "set debug minsyms" command

[John Baldwin]

On 12/21/18 1:47 PM, Simon Marchi wrote:
> While discussing this issue:
> 
>   https://sourceware.org/ml/gdb-patches/2018-12/threads.html#00082
> 
> I added a printf to be able to quickly see all minimal symbols recorded
> by GDB.  I thought it would be useful to have it built-in, for the
> future.
> 
> The output isn't particularly pretty.  I found it more readable when making
> sure the fields were vertically aligned, which results in a lot of space wasted
> in the "type" column (the width is based on the length of the longest
> enumerator):
> 
>   minsym: recording minsym type: mst_data               addr: 0x00000000004047c0  section: 2      name: __rt_psrelocs_end
>   minsym: recording minsym type: mst_text               addr: 0x0000000000402b88  section: 0      name: exit
> 
> But since this is just debugging output, I think it doesn't really
> matter.
> 
> Also, I didn't use paddress to print the address, because:
> 
> 1. There is no gdbarch handy at this point
> 2. The address may not actually be an address, but any numerical value.
>    Printing with paddress could change how it's displayed (e.g. mask
>    certain bits) and could be misleading.  I think it's better to print
>    the actual raw value saved in the minimal symbol.

2) seems compelling to me.
 
> gdb/ChangeLog:
> 
> 	* minsyms.c: Include cli/cli-cmds.h.
> 	(debug_minsyms): New.
> 	(mst_str): New.
> 	(minimal_symbol_reader::record_full): Add debug output.
> 	(_initialize_minsyms): New.
> ---
>  gdb/auto-load.c |  2 ++
>  gdb/elfread.c   |  2 ++
>  gdb/minsyms.c   | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 52 insertions(+)
> 
> diff --git a/gdb/auto-load.c b/gdb/auto-load.c
> index 33d282afe83..e35fd29426b 100644
> --- a/gdb/auto-load.c
> +++ b/gdb/auto-load.c
> @@ -1175,6 +1175,8 @@ load_auto_scripts_for_objfile (struct objfile *objfile)
>  static void
>  auto_load_new_objfile (struct objfile *objfile)
>  {
> +  std::vector <int> c;
> +  c.clear();
>    if (!objfile)
>      {
>        /* OBJFILE is NULL when loading a new "main" symbol-file.  */

This seems spurious (also not in ChangeLog)?

> diff --git a/gdb/elfread.c b/gdb/elfread.c
> index 71e6fcca6ec..359089b166c 100644
> --- a/gdb/elfread.c
> +++ b/gdb/elfread.c
> @@ -249,6 +249,8 @@ elf_symtab_read (minimal_symbol_reader &reader,
>  	  continue;
>  	}
>  
> +      printf(" --- %s\n", sym->name);
> +
>        /* Skip "special" symbols, e.g. ARM mapping symbols.  These are
>  	 symbols which do not correspond to objects in the symbol table,
>  	 but have some other target-specific meaning.  */

Likewise.

> diff --git a/gdb/minsyms.c b/gdb/minsyms.c
> index 0f854422e0f..98ce969eed0 100644
> --- a/gdb/minsyms.c
> +++ b/gdb/minsyms.c
> @@ -1112,6 +1143,11 @@ minimal_symbol_reader::record_full (const char *name, int name_len,
>    if (ms_type == mst_file_text && startswith (name, "__gnu_compiled"))
>      return (NULL);
>  
> +  if (debug_minsyms)
> +    printf_unfiltered
> +      ("minsym: recording minsym type: %-21s  addr: 0x%016llx  section: %-5d  name: %s\n",
> +       mst_str (ms_type), (long long) address, section, name);

Maybe plongest() instead of %llx?  Or does plongest not do the leading 0 fill
you want?

-- 
John Baldwin

                                                                            

Re: [PATCH] Add "set debug minsyms" command

[Simon Marchi]

On 2018-12-21 4:59 p.m., John Baldwin wrote:
>> diff --git a/gdb/auto-load.c b/gdb/auto-load.c
>> index 33d282afe83..e35fd29426b 100644
>> --- a/gdb/auto-load.c
>> +++ b/gdb/auto-load.c
>> @@ -1175,6 +1175,8 @@ load_auto_scripts_for_objfile (struct objfile *objfile)
>>  static void
>>  auto_load_new_objfile (struct objfile *objfile)
>>  {
>> +  std::vector <int> c;
>> +  c.clear();
>>    if (!objfile)
>>      {
>>        /* OBJFILE is NULL when loading a new "main" symbol-file.  */
> 
> This seems spurious (also not in ChangeLog)?
> 
>> diff --git a/gdb/elfread.c b/gdb/elfread.c
>> index 71e6fcca6ec..359089b166c 100644
>> --- a/gdb/elfread.c
>> +++ b/gdb/elfread.c
>> @@ -249,6 +249,8 @@ elf_symtab_read (minimal_symbol_reader &reader,
>>  	  continue;
>>  	}
>>  
>> +      printf(" --- %s\n", sym->name);
>> +
>>        /* Skip "special" symbols, e.g. ARM mapping symbols.  These are
>>  	 symbols which do not correspond to objects in the symbol table,
>>  	 but have some other target-specific meaning.  */
> 
> Likewise.

Woops, I meant to remove those changes, looks like I amended instead by mistake.

>> diff --git a/gdb/minsyms.c b/gdb/minsyms.c
>> index 0f854422e0f..98ce969eed0 100644
>> --- a/gdb/minsyms.c
>> +++ b/gdb/minsyms.c
>> @@ -1112,6 +1143,11 @@ minimal_symbol_reader::record_full (const char *name, int name_len,
>>    if (ms_type == mst_file_text && startswith (name, "__gnu_compiled"))
>>      return (NULL);
>>  
>> +  if (debug_minsyms)
>> +    printf_unfiltered
>> +      ("minsym: recording minsym type: %-21s  addr: 0x%016llx  section: %-5d  name: %s\n",
>> +       mst_str (ms_type), (long long) address, section, name);
> 
> Maybe plongest() instead of %llx?  Or does plongest not do the leading 0 fill
> you want?
I don't really mind about the leading 0s, it's just that having fixed width, right aligned
and padded with spaces would give something like "0x       1234".  But I ended up using
hex_string.

Here's v2:


From 388db2a2b0299ac769943f7840c3cd4e01a216ac Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@ericsson.com>
Date: Thu, 20 Dec 2018 11:43:52 -0500
Subject: [PATCH] Add debug output for recorded minsyms

While discussing this issue:

  https://sourceware.org/ml/gdb-patches/2018-12/threads.html#00082

I added a printf to be able to quickly see all minimal symbols recorded
by GDB.  I thought it would be useful to have it built-in, for the
future.

  Recording minsym:  mst_data                         0x400780    15  _IO_stdin_used
  Recording minsym:  mst_text                         0x400700    13  __libc_csu_init
  Recording minsym:  mst_bss                          0x601058    25  _end

New since v1:

- Remove unrelated changes
- Don't print field names
- Use hex_string to print addresses
- Don't add a new "set debug" knob, use "set debug symtab-create" instead.
  Output this when the value is >= 2, since there can be quite a lot of
  symbols.

gdb/ChangeLog:

	* minsyms.c (mst_str): New.
	(minimal_symbol_reader::record_full): Add debug output.
---
 gdb/minsyms.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/gdb/minsyms.c b/gdb/minsyms.c
index 0f854422e0f..71d630374ac 100644
--- a/gdb/minsyms.c
+++ b/gdb/minsyms.c
@@ -1080,6 +1080,33 @@ minimal_symbol_reader::record (const char *name, CORE_ADDR address,
   record_with_info (name, address, ms_type, section);
 }

+/* Convert an enumerator of type minimal_symbol_type to its string
+   representation.  */
+
+static const char *
+mst_str (minimal_symbol_type t)
+{
+#define MST_TO_STR(x) case x: return #x;
+  switch (t)
+  {
+    MST_TO_STR (mst_unknown);
+    MST_TO_STR (mst_text);
+    MST_TO_STR (mst_text_gnu_ifunc);
+    MST_TO_STR (mst_slot_got_plt);
+    MST_TO_STR (mst_data);
+    MST_TO_STR (mst_bss);
+    MST_TO_STR (mst_abs);
+    MST_TO_STR (mst_solib_trampoline);
+    MST_TO_STR (mst_file_text);
+    MST_TO_STR (mst_file_data);
+    MST_TO_STR (mst_file_bss);
+
+    default:
+      return "mst_???";
+  }
+#undef MST_TO_STR
+}
+
 /* See minsyms.h.  */

 struct minimal_symbol *
@@ -1112,6 +1139,10 @@ minimal_symbol_reader::record_full (const char *name, int name_len,
   if (ms_type == mst_file_text && startswith (name, "__gnu_compiled"))
     return (NULL);

+  if (symtab_create_debug >= 2)
+    printf_unfiltered ("Recording minsym:  %-21s  %18s  %4d  %s\n",
+		       mst_str (ms_type), hex_string (address), section, name);
+
   if (m_msym_bunch_index == BUNCH_SIZE)
     {
       newobj = XCNEW (struct msym_bunch);
-- 
2.20.1

Re: [PATCH] Add "set debug minsyms" command

[Simon Marchi]

On 2018-12-21 19:17, John Baldwin wrote:
> Looks good to me.

Thanks, I pushed it.

Simon

Re: [PATCH] Add "set debug minsyms" command

[Eli Zaretskii]

> Date: Fri, 21 Dec 2018 21:20:44 -0500
> From: Simon Marchi <simon.marchi@polymtl.ca>
> Cc: Simon Marchi <simon.marchi@ericsson.com>, gdb-patches@sourceware.org
> 
> On 2018-12-21 19:17, John Baldwin wrote:
> > Looks good to me.
> 
> Thanks, I pushed it.

Thanks, but please also update the manual and NEWS.

Re: [PATCH] Add "set debug minsyms" command

[Simon Marchi]

On 2018-12-22 02:28, Eli Zaretskii wrote:
>> Date: Fri, 21 Dec 2018 21:20:44 -0500
>> From: Simon Marchi <simon.marchi@polymtl.ca>
>> Cc: Simon Marchi <simon.marchi@ericsson.com>, 
>> gdb-patches@sourceware.org
>> 
>> On 2018-12-21 19:17, John Baldwin wrote:
>> > Looks good to me.
>> 
>> Thanks, I pushed it.
> 
> Thanks, but please also update the manual and NEWS.

It was indeed missing from the original patch, but I ended up not adding 
a new command, so I don't think this is needed.

Re: [PATCH] Add "set debug minsyms" command

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:

Simon> It was indeed missing from the original patch, but I ended up not
Simon> adding a new command, so I don't think this is needed.

The manual documents the values for "set debug symtab-create".  I think
a patch is indeed needed since you added a new value.

Also I was wondering why not just use "maint print msymbols".

thanks,
Tom

Re: [PATCH] Add "set debug minsyms" command

[Simon Marchi]

On 2018-12-24 15:51, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:
> 
> Simon> It was indeed missing from the original patch, but I ended up 
> not
> Simon> adding a new command, so I don't think this is needed.
> 
> The manual documents the values for "set debug symtab-create".  I think
> a patch is indeed needed since you added a new value.

Actually that value is already used and documented:

Turns on or off display of debugging messages related to symbol table 
creation. The default is 0 (off). A value of 1 provides basic 
information. A value greater than 1 provides more verbose information.

> Also I was wondering why not just use "maint print msymbols".

Because I didn't know about it :).  Do you think the new debugging 
output is redundant and should be removed?  It's the same data (minimal 
symbols), but in a different order.  With "set debug symtab-create", 
they are shown in the order they are provided by the symbol reader (same 
order as in the file usually), whereas "maint print msymbols" shows them 
as saved in GDB's memory, sorted by address.  I think that the "set 
debug symtab-create" may be useful when debugging the symbol reader, 
whereas "maint print msymbols" may be useful when debugging lookups.

Simon

Re: [PATCH] Add "set debug minsyms" command

[Eli Zaretskii]

> Date: Wed, 26 Dec 2018 11:43:18 -0500
> From: Simon Marchi <simon.marchi@polymtl.ca>
> Cc: Eli Zaretskii <eliz@gnu.org>, jhb@freebsd.org, gdb-patches@sourceware.org
> 
> > The manual documents the values for "set debug symtab-create".  I think
> > a patch is indeed needed since you added a new value.
> 
> Actually that value is already used and documented:
> 
> Turns on or off display of debugging messages related to symbol table 
> creation. The default is 0 (off). A value of 1 provides basic 
> information. A value greater than 1 provides more verbose information.
> 
> > Also I was wondering why not just use "maint print msymbols".
> 
> Because I didn't know about it :).  Do you think the new debugging 
> output is redundant and should be removed?  It's the same data (minimal 
> symbols), but in a different order.  With "set debug symtab-create", 
> they are shown in the order they are provided by the symbol reader (same 
> order as in the file usually), whereas "maint print msymbols" shows them 
> as saved in GDB's memory, sorted by address.  I think that the "set 
> debug symtab-create" may be useful when debugging the symbol reader, 
> whereas "maint print msymbols" may be useful when debugging lookups.

Btw, when I "set debug symtab-create 1" (or even a very large value),
I see no symbols, just the files from which those symbols are loaded.
Where's the code that shows the symbols?  Could it be that this
happens only on ELF platforms?

Re: [PATCH] Add "set debug minsyms" command

[Simon Marchi]

On 2018-12-26 12:19 p.m., Eli Zaretskii wrote:
> Btw, when I "set debug symtab-create 1" (or even a very large value),
> I see no symbols, just the files from which those symbols are loaded.
> Where's the code that shows the symbols?  Could it be that this
> happens only on ELF platforms?

I use "set debug symtab-create 2" and see lines like:

  Recording minsym:  mst_text                         0x403578     0  EnterCriticalSection

The code that prints this is in minsyms.c line 1143, function
minimal_symbol_reader::record_full [1].  It should be common to all binary formats.

Maybe you are based on an older commit?  This was added last week.

Simon

[1] https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blob;f=gdb/minsyms.c;h=e0f3122a974f5547f732ffd952ee0b56ae8189fb;hb=HEAD#l1142

Re: [PATCH] Add "set debug minsyms" command

[Eli Zaretskii]

> Cc: tom@tromey.com, jhb@freebsd.org, gdb-patches@sourceware.org
> From: Simon Marchi <simon.marchi@polymtl.ca>
> Date: Wed, 26 Dec 2018 12:24:11 -0500
> 
> Maybe you are based on an older commit?  This was added last week.

That explains everything.

Re: [PATCH] Add "set debug minsyms" command

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:

>> Also I was wondering why not just use "maint print msymbols".

Simon> Because I didn't know about it :).  Do you think the new debugging
Simon> output is redundant and should be removed?

It doesn't matter much to me either way.  I tend not to use these,
though I'm not entirely sure why.

Tom