Re: [PATCH v3 4/7] gdb/dwarf: pass a file_entry to line_header::file_file_name

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

On 5/3/22 16:12, Bruno Larsen wrote:
> Hi Simon!
> On 4/28/22 00:35, Simon Marchi via Gdb-patches wrote:
>> In the following patch, there will be some callers of file_file_name
>> that will already have access to the file_entry object for which they
>> want the file name.  It would be inefficient to have them pass an index,
>> only for line_header::file_file_name to re-lookup the same file_entry
>> object.  Change line_header::file_file_name to accept a file_entry
>> object reference, instead of an index to look up.
>>
>> I think this change makes sense in any case.  Callers that have an index
>> can first obtain a file_entry using line_header::file_name_at or
>> line_header::file_names.
>>
>> When passing a file_entry object, we can assume that the file_entry's
>> index is valid, unlike when passing an index.  So, push the special case
>> about an invalid index to the sole current caller of file_file_name,
>> macro_start_file.  I think that error belongs there anyway, since it
>> specifically talks about "bad file number in macro information".
>>
>> This requires recording the file index in the file_entry structure, so
>> add that.
>>
>
> Thanks for looking at this! We definitely need some movement in the
> file side of things. The general direction looks good, but I have some
> questions

Thanks for the review.  Sorry for the delay, I'm just coming back to
this patch series.

> Related to this patch, do you feel like it could be worth trying to
> centralize files into a single class? Joining file_entry,
> file_and_directory, and the information contained in symtabs into a
> single class (maybe file_info?) that will always handle names and
> paths the same way?

I don't know, I don't immediately see how that could be done, and it
depends what you mean by "handle names and paths".

>> diff --git a/gdb/dwarf2/line-header.c b/gdb/dwarf2/line-header.c
>> index 13379851b9b6..33af77d3ecf3 100644
>> --- a/gdb/dwarf2/line-header.c
>> +++ b/gdb/dwarf2/line-header.c
>> @@ -48,47 +48,28 @@ line_header::add_file_name (const char *name,
>>                   unsigned int mod_time,
>>                   unsigned int length)
>>   {
>> +  file_name_index index
>> +    = version >= 5 ? file_names_size (): file_names_size () + 1;
>> +
>>     if (dwarf_line_debug >= 2)
>> -    {
>> -      size_t new_size;
>> -      if (version >= 5)
>> -    new_size = file_names_size ();
>> -      else
>> -    new_size = file_names_size () + 1;
>> -      gdb_printf (gdb_stdlog, "Adding file %zu: %s\n",
>> -          new_size, name);
>> -    }
>> -  m_file_names.emplace_back (name, d_index, mod_time, length);
>> +    gdb_printf (gdb_stdlog, "Adding file %d: %s\n", index, name);
>> +
>> +  m_file_names.emplace_back (name, index, d_index, mod_time, length);
>>   }
>>     std::string
>> -line_header::file_file_name (int file) const
>> +line_header::file_file_name (const file_entry &fe) const
>>   {
>> -  /* Is the file number a valid index into the line header's file name
>> -     table?  Remember that file numbers start with one, not zero.  */
>> -  if (is_valid_file_index (file))
>> -    {
>> -      const file_entry *fe = file_name_at (file);
>> +  gdb_assert (is_valid_file_index (fe.index));
>>   -      if (!IS_ABSOLUTE_PATH (fe->name))
>> -    {
>> -      const char *dir = fe->include_dir (this);
>> -      if (dir != NULL)
>> -        return path_join (dir, fe->name);
>> -    }
>> +  if (IS_ABSOLUTE_PATH (fe.name))
>> +    return fe.name;
>>   -      return fe->name;
>> -    }
>> -  else
>> -    {
>> -      /* The compiler produced a bogus file number.  We can at least
>> -     record the macro definitions made in the file, even if we
>> -     won't be able to find the file by name.  */
>> -      complaint (_("bad file number in macro information (%d)"),
>> -         file);
>> +  const char *dir = fe.include_dir (this);
>> +  if (dir != nullptr)
>> +    return path_join (dir, fe.name);
>>   -      return string_printf ("<bad macro file number %d>", file);
>> -    }
>> +  return fe.name;
>
> I'm a bit confused about this last return. Why have a shortcut for
> absolute paths, then attempting to get the include dir, then returning
> only the name if that fails, instead of simplifying to something like:
>
> if (!IS_ABSOLUTE_PATH (fe.name)
>   {
>     const char *dir = fe.include_dir (this);
>     if (dir != nullptr)
>       return path_join (dir, fe.name);
>   }
> return fe.name;

Hmm, matter of style I guess.  I'm a big fan of early returns, I see it
as filtering out cases, and I find it lightens the cognitive load a bit.
When I see:

  if (IS_ABSOLUTE_PATH (fe.name))
    return fe.name;

I understand immediately that if fe.name is absolute, we return it as
is, and the rest of the code only deals with relative paths.  So my
brain can stop thinking about absolute paths for the rest of the
function.

Perhaps it would make more sense to swap the last two cases though, like
this:


  if (IS_ABSOLUTE_PATH (fe.name))
    return fe.name;

  const char *dir = fe.include_dir (this);
  if (dir == nullptr)
    return fe.name;

  return path_join (dir, fe.name);

Since the "dir == nullptr" condition filters out the unusual / error
case of not having a directory.  I'll do this change.

Simon