Re: [PATCH][gdb] Print user/includes fields for maint commands

[Tom de Vries <tdevries@suse.de>]

On 24-03-2020 23:41, Simon Marchi wrote:
> On 2020-03-24 6:28 p.m., Tom de Vries wrote:
>> On 24-03-2020 18:04, Simon Marchi wrote:
>>
>> Hi,
>>
>> thanks for the review, I've followed up on all the comments.
>>
>> Updated patch attached below.
>>
>> Thanks,
>> - Tom
>>
>>
>> 0001-gdb-Print-user-includes-fields-for-maint-commands.patch
>>
>> [gdb] Print user/includes fields for maint commands
>>
>> The type struct compunit_symtab contains two fields (disregarding field next)
>> that express relations with other compunit_symtabs: user and includes.
>>
>> These fields are currently not printed with "maint info symtabs" and
>> "maint print symbols".
>>
>> Fix this such that for "maint info symtabs" we print:
>> ...
>>    { ((struct compunit_symtab *) 0x23e8450)
>>      debugformat DWARF 2
>>      producer (null)
>>      dirname (null)
>>      blockvector ((struct blockvector *) 0x23e8590)
>> +    user ((struct compunit_symtab *) 0x2336280)
>> +    ( includes
>> +      ((struct compunit_symtab *) 0x23e85e0)
>> +      ((struct compunit_symtab *) 0x23e8960)
>> +    )
>>          { symtab <unknown> ((struct symtab *) 0x23e85b0)
>>            fullname (null)
>>            linetable ((struct linetable *) 0x0)
>>          }
>>    }
>> ...
>>
>> And for "maint print symbols" we print:
>> ...
>> -Symtab for file <unknown>
>> +Symtab for file <unknown> at 0x23e85b0
>>  Read from object file /data/gdb_versions/devel/a.out (0x233ccf0)
>>  Language: c
>>
>>  Blockvector:
>>
>>  block #000, object at 0x23e8530, 0 syms/buckets in 0x0..0x0
>>    block #001, object at 0x23e84d0 under 0x23e8530, 0 syms/buckets in 0x0..0x0
>>
>> +Compunit user: 0x2336300
>> +Compunit include: 0x23e8900
>> +Compunit include: 0x23dd970
>> ...
>> Note: for user and includes we don't list the actual compunit_symtab address,
>> but instead the corresponding symtab address, which allows us to find that
>> symtab elsewhere in the output (given that we also now print the address of
>> symtabs).
>>
>> gdb/ChangeLog:
>>
>> 2020-03-24  Tom de Vries  <tdevries@suse.de>
>>
>> 	* symtab.h (is_main_symtab_of_compunit_symtab): New function.
>> 	* symmisc.c (dump_symtab_1): Print user and includes fields.
>> 	(maintenance_info_symtabs): Same.
>>
>> ---
>>  gdb/symmisc.c | 52 +++++++++++++++++++++++++++++++++++++++++++++++++---
>>  gdb/symtab.h  |  7 +++++++
>>  2 files changed, 56 insertions(+), 3 deletions(-)
>>
>> diff --git a/gdb/symmisc.c b/gdb/symmisc.c
>> index 3df526bddb..6ee1b5edfd 100644
>> --- a/gdb/symmisc.c
>> +++ b/gdb/symmisc.c
>> @@ -279,8 +279,10 @@ dump_symtab_1 (struct symtab *symtab, struct ui_file *outfile)
>>    const struct block *b;
>>    int depth;
>>  
>> -  fprintf_filtered (outfile, "\nSymtab for file %s\n",
>> -		    symtab_to_filename_for_display (symtab));
>> +  fprintf_filtered (outfile, "\nSymtab for file %s at %s\n",
>> +		    symtab_to_filename_for_display (symtab),
>> +		    host_address_to_string (symtab));
>> +
>>    if (SYMTAB_DIRNAME (symtab) != NULL)
>>      fprintf_filtered (outfile, "Compilation directory is %s\n",
>>  		      SYMTAB_DIRNAME (symtab));
>> @@ -308,7 +310,7 @@ dump_symtab_1 (struct symtab *symtab, struct ui_file *outfile)
>>      }
>>    /* Now print the block info, but only for compunit symtabs since we will
>>       print lots of duplicate info otherwise.  */
>> -  if (symtab == COMPUNIT_FILETABS (SYMTAB_COMPUNIT (symtab)))
>> +  if (is_main_symtab_of_compunit_symtab (symtab))
>>      {
>>        fprintf_filtered (outfile, "\nBlockvector:\n\n");
>>        bv = SYMTAB_BLOCKVECTOR (symtab);
>> @@ -371,6 +373,28 @@ dump_symtab_1 (struct symtab *symtab, struct ui_file *outfile)
>>  			"\nBlockvector same as owning compunit: %s\n\n",
>>  			compunit_filename);
>>      }
>> +
>> +  if (is_main_symtab_of_compunit_symtab (symtab))
>> +    {
> 
> Hmm, if this is the exact same condition as the previous `if`, what's the point of
> having a separate `if`?
> 

The first if-else covers printing the block info.

If we'd merge the if-else and the following if, we'd have:
- the true clause related to block info
- the code related to user/includes
- the false clause related to block info
which I find confusing.

> Now that I read this code a bit more, I realize that it's written in this way because
> it dates from the time where there wasn't a distinction between compunit_symtabs and
> symtabs.  Nowadays, it would probably be preferable to print the compunit_symtab
> information at some level higher, in maintenance_print_symbols.  If you want to take the
> time to do things right, you can try do to that, otherwise I'm also fine with this
> patch (one the question above about the `if` is answered).
> 

Perhaps later, right now I don't feel knowledgeable enough in this area
to do refactoring.

Thanks,
- Tom