Re: [PATCH 3/3] Cache the result of find_file_and_directory

[Joel Brobecker via Gdb-patches <gdb-patches@sourceware.org>]

Hi Tom,

> This changes the DWARF reader to cache the result of
> find_file_and_directory.  This is not especially important now, but it
> will help the new DWARF indexer.

With the renaming of the class members moved to patch #2 (I made the
assumption that all the changes in file-and-dir.h are about that, at
least from what I could see), this patch looks good to me.

> ---
>  gdb/dwarf2/file-and-dir.h | 38 +++++++++++++++++++-------------------
>  gdb/dwarf2/read.c         | 18 +++++++++++-------
>  gdb/dwarf2/read.h         |  6 ++++++
>  3 files changed, 36 insertions(+), 26 deletions(-)
> 
> diff --git a/gdb/dwarf2/file-and-dir.h b/gdb/dwarf2/file-and-dir.h
> index 64d5a08bf0c..b64d304964a 100644
> --- a/gdb/dwarf2/file-and-dir.h
> +++ b/gdb/dwarf2/file-and-dir.h
> @@ -35,23 +35,23 @@
>  
>  struct file_and_directory
>  {
> -  file_and_directory (const char *name_, const char *dir_)
> -    : name (name_),
> -      comp_dir (dir_)
> +  file_and_directory (const char *name, const char *dir)
> +    : m_name (name),
> +      m_comp_dir (dir)
>    {
>    }
>  
>    /* Return true if the file name is unknown.  */
>    bool is_unknown () const
>    {
> -    return name == nullptr;
> +    return m_name == nullptr;
>    }
>  
>    /* Set the compilation directory.  */
>    void set_comp_dir (std::string &&dir)
>    {
> -    comp_dir_storage = std::move (dir);
> -    comp_dir = nullptr;
> +    m_comp_dir_storage = std::move (dir);
> +    m_comp_dir = nullptr;
>    }
>  
>    /* Fetch the compilation directory.  This may return NULL in some
> @@ -60,48 +60,48 @@ struct file_and_directory
>       you should call intern_comp_dir.  */
>    const char *get_comp_dir () const
>    {
> -    if (!comp_dir_storage.empty ())
> -      return comp_dir_storage.c_str ();
> -    return comp_dir;
> +    if (!m_comp_dir_storage.empty ())
> +      return m_comp_dir_storage.c_str ();
> +    return m_comp_dir;
>    }
>  
>    /* If necessary, intern the compilation directory using OBJFILE's
>       string cache.  Returns the compilation directory.  */
>    const char *intern_comp_dir (struct objfile *objfile)
>    {
> -    if (!comp_dir_storage.empty ())
> +    if (!m_comp_dir_storage.empty ())
>        {
> -	comp_dir = objfile->intern (comp_dir_storage);
> -	comp_dir_storage.clear ();
> +	m_comp_dir = objfile->intern (m_comp_dir_storage);
> +	m_comp_dir_storage.clear ();
>        }
> -    return comp_dir;
> +    return m_comp_dir;
>    }
>  
>    /* Fetch the filename.  This never returns NULL.  */
>    const char *get_name () const
>    {
> -    return name == nullptr ? "<unknown>" : name;
> +    return m_name == nullptr ? "<unknown>" : m_name;
>    }
>  
>    /* Set the filename.  */
> -  void set_name (const char *name_)
> +  void set_name (const char *name)
>    {
> -    name = name_;
> +    m_name = name;
>    }
>  
>  private:
>  
>    /* The filename.  This is never NULL.  */
> -  const char *name;
> +  const char *m_name;
>  
>    /* The compilation directory.  NULL if not known.  If we needed to
>       compute a new string, it will be stored in the comp_dir_storage
>       member, and this will be NULL.  Otherwise, points directly to the
>       DW_AT_comp_dir string attribute.  */
> -  const char *comp_dir;
> +  const char *m_comp_dir;
>  
>    /* The compilation directory, if it needed to be allocated.  */
> -  std::string comp_dir_storage;
> +  std::string m_comp_dir_storage;
>  };
>  
>  #endif /* GDB_DWARF2_FILE_AND_DIR_H */
> diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
> index 5802fc0ef04..ff5758eb0a4 100644
> --- a/gdb/dwarf2/read.c
> +++ b/gdb/dwarf2/read.c
> @@ -1537,8 +1537,8 @@ dwarf2_per_cu_data_deleter::operator() (dwarf2_per_cu_data *data)
>      delete data;
>  }
>  
> -static file_and_directory find_file_and_directory (struct die_info *die,
> -						   struct dwarf2_cu *cu);
> +static file_and_directory &find_file_and_directory
> +     (struct die_info *die, struct dwarf2_cu *cu);
>  
>  static const char *compute_include_file_name
>       (const struct line_header *lh,
> @@ -3005,7 +3005,7 @@ dw2_get_file_names_reader (const struct die_reader_specs *reader,
>        lh = dwarf_decode_line_header (line_offset, cu);
>      }
>  
> -  file_and_directory fnd = find_file_and_directory (comp_unit_die, cu);
> +  file_and_directory &fnd = find_file_and_directory (comp_unit_die, cu);
>  
>    int offset = 0;
>    if (fnd.is_unknown ())
> @@ -6988,7 +6988,7 @@ process_psymtab_comp_unit_reader (const struct die_reader_specs *reader,
>    /* Allocate a new partial symbol table structure.  */
>    gdb::unique_xmalloc_ptr<char> debug_filename;
>    static const char artificial[] = "<artificial>";
> -  file_and_directory fnd = find_file_and_directory (comp_unit_die, cu);
> +  file_and_directory &fnd = find_file_and_directory (comp_unit_die, cu);
>    if (strcmp (fnd.get_name (), artificial) == 0)
>      {
>        debug_filename.reset (concat (artificial, "@",
> @@ -10474,9 +10474,12 @@ producer_is_gcc_lt_4_3 (struct dwarf2_cu *cu)
>    return cu->producer_is_gcc_lt_4_3;
>  }
>  
> -static file_and_directory
> +static file_and_directory &
>  find_file_and_directory (struct die_info *die, struct dwarf2_cu *cu)
>  {
> +  if (cu->per_cu->fnd != nullptr)
> +    return *cu->per_cu->fnd;
> +
>    /* Find the filename.  Do not use dwarf2_name here, since the filename
>       is not a source language identifier.  */
>    file_and_directory res (dwarf2_string_attr (die, DW_AT_name, cu),
> @@ -10488,7 +10491,8 @@ find_file_and_directory (struct die_info *die, struct dwarf2_cu *cu)
>        && IS_ABSOLUTE_PATH (res.get_name ()))
>      res.set_comp_dir (ldirname (res.get_name ()));
>  
> -  return res;
> +  cu->per_cu->fnd.reset (new file_and_directory (std::move (res)));
> +  return *cu->per_cu->fnd;
>  }
>  
>  /* Handle DW_AT_stmt_list for a compilation unit.
> @@ -10616,7 +10620,7 @@ read_file_scope (struct die_info *die, struct dwarf2_cu *cu)
>      lowpc = highpc;
>    lowpc = gdbarch_adjust_dwarf2_addr (gdbarch, lowpc + baseaddr);
>  
> -  file_and_directory fnd = find_file_and_directory (die, cu);
> +  file_and_directory &fnd = find_file_and_directory (die, cu);
>  
>    cu->start_symtab (fnd.get_name (), fnd.intern_comp_dir (objfile), lowpc);
>  
> diff --git a/gdb/dwarf2/read.h b/gdb/dwarf2/read.h
> index 0afcda1c3b0..2f849d5f9a1 100644
> --- a/gdb/dwarf2/read.h
> +++ b/gdb/dwarf2/read.h
> @@ -180,6 +180,12 @@ struct dwarf2_per_cu_data
>       should be private, but we can't make it private at the moment.  */
>    mutable comp_unit_head m_header {};
>  
> +  /* The file and directory for this CU.  This is cached so that we
> +     don't need to re-examine the DWO in some situations.  This may be
> +     nullptr, depending on the CU; for example a partial unit won't
> +     have one.  */
> +  std::unique_ptr<file_and_directory> fnd;
> +
>    /* When dwarf2_per_bfd::using_index is true, the 'quick' field
>       is active.  Otherwise, the 'psymtab' field is active.  */
>    union
> -- 
> 2.31.1
> 

-- 
Joel