Re: [PATCH v3 5/7] gdb: add "id" fields to identify symtabs and subfiles

[Simon Marchi <simon.marchi@polymtl.ca>]

On 4/28/22 19:53, Lancelot SIX wrote:
> Hi,
>
> Just some nits below.
>
> When applying the patch I have:
>
>     Applying: gdb: add "id" fields to identify symtabs and subfiles
>     .git/rebase-apply/patch:153: trailing whitespace.
>          It is used to look up existing subfiles in calls to start_subfile.  */
>     warning: 1 line adds whitespace errors.

Fixed.

>> Another this to consider is that while the main symtab's name (or
>
> s/this/thing/ ?

Fixed.

>> Changes in the core symtab files are:
>>
>>  - Add "name_for_id" and "filename_for_id" fields to "struct subfile"
>>    and "struct symta"b, next to existing "name" and "filename" fields.
>                       ^
> Two chars are swapped here.

Fixed.

>>  - Make buildsym_compunit::buildsym_compunit and
>>    buildsym_compunit::start_subfile accept a "name_for_id" parameter
>>    next to the existing "name" ones.
>>  - Make buildsym_compunit::start_subfile used "name_for_id" for looking
>>    up existing subfiles.  This is the key thing for making calls
>>    to start_subfile for the main source file look up the existing
>>    subfile successfully, and avoid relying on
>>    watch_main_source_file_lossage.
>>  - Make sal_macro_scope pass "filename_for_id", rather than "filename",
>>    to macro_lookup_inclusion.  This is the key thing to making the
>>    lookup work and macro printing work.
>>
>> Changes in the DWARF files are:
>>
>>  - Make line_header::file_file_name return the "most complete possible"
>>    name.  The only pre-existing user of this method is the macro code,
>>    to give the macro_source_file objects their name.  And we now want
>>    them to have this "most complete possible" name, which will match the
>>    corresponding symtab's "filename_for_id".
>>  - Make dwarf2_cu::start_compunit_symtab pass the "most complete
>>    possible" name for the main symtab's "filename_for_id".  In this
>>    context, where the info comes from the compilation unit's DW_AT_name
>>    / DW_AT_comp_dir, it means prepending DW_AT_comp_dir to DW_AT_name if
>>    DW_AT_name is not already absolute.
>>  - Change dwarf2_start_subfile to build a name_for_id for the subfile
>>    being started.  The simplest way is to re-use
>>    line_header::file_file_name, since the callers always have a
>>    file_entry handy.  This ensures that it will get the exact same path
>>    representation as the macro code does, for the same file (since it
>>    also uses line_header::file_file_name).
>>  - Update calls to allocate_symtab to pass the "name_for_id" from the
>>    subfile.
>>
>> The rest of the changes are to update the other symtab users (jit,
>> ctfread, mdebugread, xcoffread).  For those, the same value is passed
>> for the "id" as the for the filename, so they should keep the same
>> behavior they have today.
>
> I guess this is just personal preference, but I would have chosen to
> add overloads so those files do not have to change / bother about an
> argument which is meaningless for them.
>
> Something like:
>
>     // gdb/buildsym.h
>     struct buildsym_compunit
>     {
>       buildsym_compunit (struct objfile *objfile_, const char *name,
>                          const char *comp_dir_, enum language language_,
>                          CORE_ADDR last_addr)
>         : buildsym_compunit (objfile_, name, comp_dir_, name,
>                              language_, last_addr);
>       buildsym_compunit (struct objfile *objfile_, const char *name,
>                          const char *comp_dir_, const char *name_for_id,
>                          language language_, CORE_ADDR last_addr);
>
>       ...
>
>       void start_subfile (const char *name, const char *name_for_id);
>
>       void start_subfile (const char *name)
>       { start_subfile (name, name); }
>     };
>
>     // gdb/symfile.h
>     extern struct symtab *allocate_symtab
>       (struct compunit_symtab *cust, const char *filename, const char *id))
>       ATTRIBUTE_NONNULL (1);
>
>     static inline struct symtab *
>     allocate_symtab (struct compunit_symtab *cust, const char *filename)
>     {
>       return allocate_sumtab (cust, filename, filename);
>     }
>
> In the end it does not make much difference, your approach work also
> perfectly fine so feel free to keep it the way it is.

I did that.

>
> Otherwise, FWIW I find the approach to keep one string representation of
> the filename for display and one for some sort of canonicalization
> perfectly appropriate.

Ok, thanks for the review.

>> Similary, with DWARF 5:
>
> s/Similary/Similarly/

Fixed.

Simon