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

[Bruno Larsen <blarsen@redhat.com>]

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

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?

> Change-Id: Ic6e44c407539d92b7863d7ba82405ade17f384ad
> ---
>   gdb/dwarf2/line-header.c | 47 ++++++++++++----------------------------
>   gdb/dwarf2/line-header.h | 10 ++++++---
>   gdb/dwarf2/macro.c       | 16 +++++++++++++-
>   3 files changed, 36 insertions(+), 37 deletions(-)
> 
> 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;



>   }
>   
>   static void
> diff --git a/gdb/dwarf2/line-header.h b/gdb/dwarf2/line-header.h
> index 53db3a1d5aa8..0fe539b0c427 100644
> --- a/gdb/dwarf2/line-header.h
> +++ b/gdb/dwarf2/line-header.h
> @@ -36,9 +36,10 @@ struct file_entry
>   {
>     file_entry () = default;
>   
> -  file_entry (const char *name_, dir_index d_index_,
> +  file_entry (const char *name_, file_name_index index_, dir_index d_index_,
>   	      unsigned int mod_time_, unsigned int length_)
>       : name (name_),
> +      index (index_),
>         d_index (d_index_),
>         mod_time (mod_time_),
>         length (length_)
> @@ -52,6 +53,9 @@ struct file_entry
>        owned by debug_line_buffer.  */
>     const char *name {};
>   
> +  /* The index of this file in the file table.  */
> +  file_name_index index {};
> +
>     /* The directory index (1-based).  */
>     dir_index d_index {};
>   
> @@ -168,8 +172,8 @@ struct line_header
>     const gdb_byte *statement_program_start {}, *statement_program_end {};
>   
>     /* Return file name relative to the compilation directory of file
> -     number FILE in this object's file name table.  */
> -  std::string file_file_name (int file) const;
> +     FE in this object's file name table.  */
> +  std::string file_file_name (const file_entry &fe) const;
>   
>     /* Return the compilation directory of the compilation unit in the context of
>        which this line header is read.  Return nullptr if non applicable.  */
> diff --git a/gdb/dwarf2/macro.c b/gdb/dwarf2/macro.c
> index 99c3653a2c3a..38c0fdfec738 100644
> --- a/gdb/dwarf2/macro.c
> +++ b/gdb/dwarf2/macro.c
> @@ -52,7 +52,21 @@ macro_start_file (buildsym_compunit *builder,
>   		  const struct line_header *lh)
>   {
>     /* File name relative to the compilation directory of this source file.  */
> -  std::string file_name = lh->file_file_name (file);
> +  const file_entry *fe = lh->file_name_at (file);
> +  std::string file_name;
> +
> +  if (fe != nullptr)
> +    file_name = lh->file_file_name (*fe);
> +  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);
> +
> +      file_name = string_printf ("<bad macro file number %d>", file);
> +    }
>   
>     if (! current_file)
>       {

Cheers!
Bruno Larsen