Re: [PATCH 2/3] Move file_and_directory to new file and C++-ize

[Lancelot SIX via Gdb-patches <gdb-patches@sourceware.org>]

Hi,

I have some remarks below.

On Mon, Nov 29, 2021 at 06:33:03PM -0700, Tom Tromey wrote:
> This moves file_and_directory to a new file, and then C++-izes it --
> replacing direct assignments with methods, and arranging for it to own
> any string that must be computed.  Finally, the CU's objfile will only
> be used on demand; this is an important property for the new DWARF
> indexer's parallel mode.
> ---
>  gdb/dwarf2/file-and-dir.h | 107 ++++++++++++++++++++++++++++++++++++++
>  gdb/dwarf2/read.c         |  66 ++++++++---------------
>  gdb/dwarf2/read.h         |   1 +
>  3 files changed, 129 insertions(+), 45 deletions(-)
>  create mode 100644 gdb/dwarf2/file-and-dir.h
> 
> diff --git a/gdb/dwarf2/file-and-dir.h b/gdb/dwarf2/file-and-dir.h
> new file mode 100644
> index 00000000000..64d5a08bf0c
> --- /dev/null
> +++ b/gdb/dwarf2/file-and-dir.h
> @@ -0,0 +1,107 @@
> +/* DWARF file and directory
> +
> +   Copyright (C) 1994-2021 Free Software Foundation, Inc.
> +
> +   Adapted by Gary Funck (gary@intrepid.com), Intrepid Technology,
> +   Inc.  with support from Florida State University (under contract
> +   with the Ada Joint Program Office), and Silicon Graphics, Inc.
> +   Initial contribution by Brent Benson, Harris Computer Systems, Inc.,
> +   based on Fred Fish's (Cygnus Support) implementation of DWARF 1
> +   support.
> +
> +   This file is part of GDB.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +#ifndef GDB_DWARF2_FILE_AND_DIR_H
> +#define GDB_DWARF2_FILE_AND_DIR_H
> +
> +#include "objfiles.h"
> +#include <string>
> +
> +/* The return type of find_file_and_directory.  Note, the enclosed
> +   string pointers are only valid while this object is valid.  */
> +
> +struct file_and_directory
> +{
> +  file_and_directory (const char *name_, const char *dir_)
> +    : name (name_),
> +      comp_dir (dir_)

The members are usually prefixed with 'm_'.  I think you change that in
then next patch, but I feel this change would be relevant in the
'c++ization' done by the current one.

> +  {
> +  }
> +
> +  /* Return true if the file name is unknown.  */
> +  bool is_unknown () const
> +  {
> +    return name == nullptr;
> +  }
> +
> +  /* Set the compilation directory.  */
> +  void set_comp_dir (std::string &&dir)
> +  {
> +    comp_dir_storage = std::move (dir);
> +    comp_dir = nullptr;
> +  }
> +
> +  /* Fetch the compilation directory.  This may return NULL in some
> +     circumstances.  Note that the return value here is not stable --
> +     it may change if this object is moved.  To get a stable pointer,
> +     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 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 ())
> +      {
> +	comp_dir = objfile->intern (comp_dir_storage);
> +	comp_dir_storage.clear ();
> +      }
> +    return comp_dir;
> +  }
> +
> +  /* Fetch the filename.  This never returns NULL.  */
> +  const char *get_name () const
> +  {
> +    return name == nullptr ? "<unknown>" : name;
> +  }
> +
> +  /* Set the filename.  */
> +  void set_name (const char *name_)
> +  {
> +    name = name_;
> +  }
> +
> +private:
> +
> +  /* The filename.  This is never NULL.  */

From how the class is implemented, it looks like this comment is not
accurate anymore and NAME can be NULL.

Best,
Lancelot.

> +  const char *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;
> +
> +  /* The compilation directory, if it needed to be allocated.  */
> +  std::string comp_dir_storage;
> +};
> +
> +#endif /* GDB_DWARF2_FILE_AND_DIR_H */
> diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
> index 3cf0c9ea2a8..5802fc0ef04 100644
> --- a/gdb/dwarf2/read.c
> +++ b/gdb/dwarf2/read.c
> @@ -1206,7 +1206,6 @@ static struct die_info *die_specification (struct die_info *die,
>  static line_header_up dwarf_decode_line_header (sect_offset sect_off,
>  						struct dwarf2_cu *cu);
>  
> -struct file_and_directory;
>  static void dwarf_decode_lines (struct line_header *,
>  				const file_and_directory &,
>  				struct dwarf2_cu *, dwarf2_psymtab *,
> @@ -1538,21 +1537,6 @@ dwarf2_per_cu_data_deleter::operator() (dwarf2_per_cu_data *data)
>      delete data;
>  }
>  
> -/* The return type of find_file_and_directory.  Note, the enclosed
> -   string pointers are only valid while this object is valid.  */
> -
> -struct file_and_directory
> -{
> -  /* The filename.  This is never NULL.  */
> -  const char *name;
> -
> -  /* The compilation directory.  NULL if not known.  If we needed to
> -     compute a new string, it will be stored in the per-BFD string
> -     bcache; otherwise, points directly to the DW_AT_comp_dir string
> -     attribute owned by the obstack that owns the DIE.  */
> -  const char *comp_dir;
> -};
> -
>  static file_and_directory find_file_and_directory (struct die_info *die,
>  						   struct dwarf2_cu *cu);
>  
> @@ -3024,7 +3008,7 @@ dw2_get_file_names_reader (const struct die_reader_specs *reader,
>    file_and_directory fnd = find_file_and_directory (comp_unit_die, cu);
>  
>    int offset = 0;
> -  if (strcmp (fnd.name, "<unknown>") != 0)
> +  if (fnd.is_unknown ())
>      ++offset;
>    else if (lh == nullptr)
>      return;
> @@ -3053,12 +3037,12 @@ dw2_get_file_names_reader (const struct die_reader_specs *reader,
>      }
>  
>    qfn->num_file_names = offset + include_names.size ();
> -  qfn->comp_dir = fnd.comp_dir;
> +  qfn->comp_dir = fnd.intern_comp_dir (per_objfile->objfile);
>    qfn->file_names =
>      XOBNEWVEC (&per_objfile->per_bfd->obstack, const char *,
>  	       qfn->num_file_names);
>    if (offset != 0)
> -    qfn->file_names[0] = xstrdup (fnd.name);
> +    qfn->file_names[0] = xstrdup (fnd.get_name ());
>  
>    if (!include_names.empty ())
>      memcpy (&qfn->file_names[offset], include_names.data (),
> @@ -7005,15 +6989,15 @@ process_psymtab_comp_unit_reader (const struct die_reader_specs *reader,
>    gdb::unique_xmalloc_ptr<char> debug_filename;
>    static const char artificial[] = "<artificial>";
>    file_and_directory fnd = find_file_and_directory (comp_unit_die, cu);
> -  if (strcmp (fnd.name, artificial) == 0)
> +  if (strcmp (fnd.get_name (), artificial) == 0)
>      {
>        debug_filename.reset (concat (artificial, "@",
>  				    sect_offset_str (per_cu->sect_off),
>  				    (char *) NULL));
> -      fnd.name = debug_filename.get ();
> +      fnd.set_name (debug_filename.get ());
>      }
>  
> -  pst = create_partial_symtab (per_cu, per_objfile, fnd.name);
> +  pst = create_partial_symtab (per_cu, per_objfile, fnd.get_name ());
>  
>    /* This must be done before calling dwarf2_build_include_psymtabs.  */
>    pst->dirname = dwarf2_string_attr (comp_unit_die, DW_AT_comp_dir, cu);
> @@ -10493,25 +10477,16 @@ producer_is_gcc_lt_4_3 (struct dwarf2_cu *cu)
>  static file_and_directory
>  find_file_and_directory (struct die_info *die, struct dwarf2_cu *cu)
>  {
> -  file_and_directory res;
> -
>    /* Find the filename.  Do not use dwarf2_name here, since the filename
>       is not a source language identifier.  */
> -  res.name = dwarf2_string_attr (die, DW_AT_name, cu);
> -  res.comp_dir = dwarf2_string_attr (die, DW_AT_comp_dir, cu);
> -
> -  if (res.comp_dir == NULL
> -      && producer_is_gcc_lt_4_3 (cu) && res.name != NULL
> -      && IS_ABSOLUTE_PATH (res.name))
> -    {
> -      std::string comp_dir_storage = ldirname (res.name);
> -      if (!comp_dir_storage.empty ())
> -	res.comp_dir
> -	  = cu->per_objfile->objfile->intern (comp_dir_storage.c_str ());
> -    }
> +  file_and_directory res (dwarf2_string_attr (die, DW_AT_name, cu),
> +			  dwarf2_string_attr (die, DW_AT_comp_dir, cu));
>  
> -  if (res.name == NULL)
> -    res.name = "<unknown>";
> +  if (res.get_comp_dir () == nullptr
> +      && producer_is_gcc_lt_4_3 (cu)
> +      && res.get_name () != nullptr
> +      && IS_ABSOLUTE_PATH (res.get_name ()))
> +    res.set_comp_dir (ldirname (res.get_name ()));
>  
>    return res;
>  }
> @@ -10643,7 +10618,7 @@ read_file_scope (struct die_info *die, struct dwarf2_cu *cu)
>  
>    file_and_directory fnd = find_file_and_directory (die, cu);
>  
> -  cu->start_symtab (fnd.name, fnd.comp_dir, lowpc);
> +  cu->start_symtab (fnd.get_name (), fnd.intern_comp_dir (objfile), lowpc);
>  
>    gdb_assert (per_objfile->sym_cu == nullptr);
>    scoped_restore restore_sym_cu
> @@ -20752,7 +20727,7 @@ compute_include_file_name (const struct line_header *lh, const file_entry &fe,
>  
>    gdb::unique_xmalloc_ptr<char> hold_compare;
>    if (!IS_ABSOLUTE_PATH (include_name)
> -      && (dir_name != NULL || cu_info.comp_dir != NULL))
> +      && (dir_name != nullptr || cu_info.get_comp_dir () != nullptr))
>      {
>        /* Avoid creating a duplicate name for CU_INFO.
>  	 We do this by comparing INCLUDE_NAME and CU_INFO.
> @@ -20782,19 +20757,20 @@ compute_include_file_name (const struct line_header *lh, const file_entry &fe,
>  	  include_name = name_holder->get ();
>  	  include_name_to_compare = include_name;
>  	}
> -      if (!IS_ABSOLUTE_PATH (include_name) && cu_info.comp_dir != nullptr)
> +      if (!IS_ABSOLUTE_PATH (include_name)
> +	  && cu_info.get_comp_dir () != nullptr)
>  	{
> -	  hold_compare.reset (concat (cu_info.comp_dir, SLASH_STRING,
> +	  hold_compare.reset (concat (cu_info.get_comp_dir (), SLASH_STRING,
>  				      include_name, (char *) NULL));
>  	  include_name_to_compare = hold_compare.get ();
>  	}
>      }
>  
>    gdb::unique_xmalloc_ptr<char> copied_name;
> -  const char *cu_filename = cu_info.name;
> -  if (!IS_ABSOLUTE_PATH (cu_filename) && cu_info.comp_dir != nullptr)
> +  const char *cu_filename = cu_info.get_name ();
> +  if (!IS_ABSOLUTE_PATH (cu_filename) && cu_info.get_comp_dir () != nullptr)
>      {
> -      copied_name.reset (concat (cu_info.comp_dir, SLASH_STRING,
> +      copied_name.reset (concat (cu_info.get_comp_dir (), SLASH_STRING,
>  				 cu_filename, (char *) NULL));
>        cu_filename = copied_name.get ();
>      }
> diff --git a/gdb/dwarf2/read.h b/gdb/dwarf2/read.h
> index fe34e3f95ae..0afcda1c3b0 100644
> --- a/gdb/dwarf2/read.h
> +++ b/gdb/dwarf2/read.h
> @@ -23,6 +23,7 @@
>  #include <queue>
>  #include <unordered_map>
>  #include "dwarf2/comp-unit-head.h"
> +#include "dwarf2/file-and-dir.h"
>  #include "dwarf2/index-cache.h"
>  #include "dwarf2/section.h"
>  #include "filename-seen-cache.h"
> -- 
> 2.31.1
> 

-- 
Lancelot SIX