[PATCH 0/3] Refactor find_file_and_directory

[PATCH 0/3] Refactor find_file_and_directory

[Tom Tromey]

This short series refactors find_file_and_directory to get it into
shape for the new DWARF indexer.  This work makes it simpler to fix
the one lingering fission bug on that branch.

Regression tested on x86-64 Fedora 34.  I also ran a few tests using
fission and fission-dwp ... and btw it turns out that the new
include-main.exp fails with these on master.

Let me know what you think.

Tom

[PATCH 1/3] Remove Irix case from find_file_and_directory

[Tom Tromey]

find_file_and_directory has a special case for the Irix 6.2 compiler.
Since this is long obsolete, this patch removes it.
---
 gdb/dwarf2/read.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index 57538fc0adf..3cf0c9ea2a8 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -10509,15 +10509,6 @@ find_file_and_directory (struct die_info *die, struct dwarf2_cu *cu)
 	res.comp_dir
 	  = cu->per_objfile->objfile->intern (comp_dir_storage.c_str ());
     }
-  if (res.comp_dir != NULL)
-    {
-      /* Irix 6.2 native cc prepends <machine>.: to the compilation
-	 directory, get rid of it.  */
-      const char *cp = strchr (res.comp_dir, ':');
-
-      if (cp && cp != res.comp_dir && cp[-1] == '.' && cp[1] == '/')
-	res.comp_dir = cp + 1;
-    }
 
   if (res.name == NULL)
     res.name = "<unknown>";
-- 
2.31.1

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

[Tom Tromey]

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_)
+  {
+  }
+
+  /* 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.  */
+  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

[PATCH 3/3] Cache the result of find_file_and_directory

[Tom Tromey]

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.
---
 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

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

[Lancelot SIX]

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

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

[Tom Tromey]

>> +struct file_and_directory
>> +{
>> +  file_and_directory (const char *name_, const char *dir_)
>> +    : name (name_),
>> +      comp_dir (dir_)

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

Yeah, I did that in the wrong patch.
I've fixed it locally now.

>> +  /* The filename.  This is never NULL.  */

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

Thanks, I made this change as well.

Tom

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

[Joel Brobecker]

Hi Tom,

In addition to the comments already made about the name of the class
members, I was wondering why you were adding the following include
in this patch, since I didn't see any use of the file_and_directory.

> 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"

Looking at the next patch, I think perhaps this include should be moved
over to patch #3?

-- 
Joel

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

[Joel Brobecker]

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

Re: [PATCH 1/3] Remove Irix case from find_file_and_directory

[Joel Brobecker]

> find_file_and_directory has a special case for the Irix 6.2 compiler.
> Since this is long obsolete, this patch removes it.

Looks good to me. Thanks Tom!

> ---
>  gdb/dwarf2/read.c | 9 ---------
>  1 file changed, 9 deletions(-)
> 
> diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
> index 57538fc0adf..3cf0c9ea2a8 100644
> --- a/gdb/dwarf2/read.c
> +++ b/gdb/dwarf2/read.c
> @@ -10509,15 +10509,6 @@ find_file_and_directory (struct die_info *die, struct dwarf2_cu *cu)
>  	res.comp_dir
>  	  = cu->per_objfile->objfile->intern (comp_dir_storage.c_str ());
>      }
> -  if (res.comp_dir != NULL)
> -    {
> -      /* Irix 6.2 native cc prepends <machine>.: to the compilation
> -	 directory, get rid of it.  */
> -      const char *cp = strchr (res.comp_dir, ':');
> -
> -      if (cp && cp != res.comp_dir && cp[-1] == '.' && cp[1] == '/')
> -	res.comp_dir = cp + 1;
> -    }
>  
>    if (res.name == NULL)
>      res.name = "<unknown>";
> -- 
> 2.31.1
> 

-- 
Joel

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

[Tom Tromey]

>> 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"

Joel> Looking at the next patch, I think perhaps this include should be moved
Joel> over to patch #3?

I made this change.

Tom

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

[Tom Tromey]

>> 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.

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

Thanks, I'm going to check these in now.

Tom

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

[Simon Marchi]

On 2021-12-04 13:22, Tom Tromey wrote:
>>> 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.
> 
> Joel> With the renaming of the class members moved to patch #2 (I made the
> Joel> assumption that all the changes in file-and-dir.h are about that, at
> Joel> least from what I could see), this patch looks good to me.
> 
> Thanks, I'm going to check these in now.
> 
> Tom
> 

Hi Tom,

I see these failures on Ubuntu 20.04, when building with
AddressSanitizer, starting with this patch.

    gdb.dwarf2/imported-unit.exp - ptype main
    gdb.dwarf2/imported-unit-abstract-const-value.exp - p aaa
    gdb.dwarf2/imported-unit-bp-c.exp - break /tmp/imported-unit-bp-alt.c:19
    gdb.dwarf2/inlined_subroutine-inheritance.exp - setting breakpoint at bytes_repeat (eof)

Simon

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

[Tom Tromey]

Simon> I see these failures on Ubuntu 20.04, when building with
Simon> AddressSanitizer, starting with this patch.

Sorry about that.  I'll fix it soon.  Or you can back it out if you
prefer.

Tom

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

[Simon Marchi]

On 2021-12-04 22:46, Tom Tromey wrote:
> Simon> I see these failures on Ubuntu 20.04, when building with
> Simon> AddressSanitizer, starting with this patch.
> 
> Sorry about that.  I'll fix it soon.  Or you can back it out if you
> prefer.
> 
> Tom
> 

I'm not personally affected so I do not mind, I can wait for the fix.

Thanks!

Simon

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

[Tom Tromey]

>> Sorry about that.  I'll fix it soon.  Or you can back it out if you
>> prefer.

Simon> I'm not personally affected so I do not mind, I can wait for the fix.

Here is what I am checking in.
I was able to reproduce the failure with valgrind, and confirmed that
this patch fixes it.

Tom

commit 33af066d07d495c81c7c102125aec8dbac62c27b
Author: Tom Tromey <tom@tromey.com>
Date:   Sun Dec 5 13:13:33 2021 -0700

    Preserve artificial CU name in process_psymtab_comp_unit_reader
    
    This fixes a use-after-free that Simon pointed out.
    process_psymtab_comp_unit_reader was allocating an artificial name for
    a CU, and then discarding it.  However, this name was preserved in the
    cached file_and_directory.  This patch arranges for the allocated name
    to be preserved there.

diff --git a/gdb/dwarf2/file-and-dir.h b/gdb/dwarf2/file-and-dir.h
index 1a9ccf35829..c56922ff90d 100644
--- a/gdb/dwarf2/file-and-dir.h
+++ b/gdb/dwarf2/file-and-dir.h
@@ -84,9 +84,10 @@ struct file_and_directory
   }
 
   /* Set the filename.  */
-  void set_name (const char *name)
+  void set_name (gdb::unique_xmalloc_ptr<char> name)
   {
-    m_name = name;
+    m_name_storage = std::move (name);
+    m_name = m_name_storage.get ();
   }
 
 private:
@@ -94,6 +95,9 @@ struct file_and_directory
   /* The filename.  */
   const char *m_name;
 
+  /* Storage for the filename, if needed.  */
+  gdb::unique_xmalloc_ptr<char> m_name_storage;
+
   /* 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
diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index ff5758eb0a4..f2d7da7de52 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -6986,15 +6986,15 @@ process_psymtab_comp_unit_reader (const struct die_reader_specs *reader,
   prepare_one_comp_unit (cu, comp_unit_die, pretend_language);
 
   /* 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);
   if (strcmp (fnd.get_name (), artificial) == 0)
     {
-      debug_filename.reset (concat (artificial, "@",
-				    sect_offset_str (per_cu->sect_off),
-				    (char *) NULL));
-      fnd.set_name (debug_filename.get ());
+      gdb::unique_xmalloc_ptr<char> debug_filename
+	(concat (artificial, "@",
+		 sect_offset_str (per_cu->sect_off),
+		 (char *) NULL));
+      fnd.set_name (std::move (debug_filename));
     }
 
   pst = create_partial_symtab (per_cu, per_objfile, fnd.get_name ());

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

[Simon Marchi]

On 2021-12-05 15:17, Tom Tromey wrote:
>>> Sorry about that.  I'll fix it soon.  Or you can back it out if you
>>> prefer.
> 
> Simon> I'm not personally affected so I do not mind, I can wait for the fix.
> 
> Here is what I am checking in.
> I was able to reproduce the failure with valgrind, and confirmed that
> this patch fixes it.
> 
> Tom

Thanks.  And while looking at that fix, I had this this question:
looking at find_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),
    			  dwarf2_string_attr (die, DW_AT_comp_dir, cu));

      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 ()));

      cu->per_cu->fnd.reset (new file_and_directory (std::move (res)));
      return *cu->per_cu->fnd;
    }

`dwarf2_string_attr (die, DW_AT_name, cu)` is used as the name in
`fnd`, and `fnd` is saved in the dwarf2_per_cu_data structure.  Is
`dwarf2_string_attr (die, DW_AT_name, cu)` value guaranteed to live at
least as long as the dwarf2_per_cu_data structure?  IIUC, the die_info
objects are tied to a dwarf2_cu.  And dwarf2_cus can be deleted
(by free_cached_comp_units).  I might be wrong, but I thought I'd
raise the point just in case.

Simon

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

[Tom Tromey]

Simon> Thanks.  And while looking at that fix, I had this this question:
Simon> looking at find_file_and_directory:

Simon>       file_and_directory res (dwarf2_string_attr (die, DW_AT_name, cu),
Simon>     			  dwarf2_string_attr (die, DW_AT_comp_dir, cu));

Simon> `dwarf2_string_attr (die, DW_AT_name, cu)` is used as the name in
Simon> `fnd`, and `fnd` is saved in the dwarf2_per_cu_data structure.  Is
Simon> `dwarf2_string_attr (die, DW_AT_name, cu)` value guaranteed to live at
Simon> least as long as the dwarf2_per_cu_data structure?

Yeah.  The string data is mapped either from the .debug_str (normally)
or .debug_info (for very short strings, sometimes) section.  The section
data is only unmapped when the per-BFD is destroyed, i.e., when the last
reference is removed.  (There are other cases here, the section data
might be allocated and freed per-objfile if it has relocations, but
again, it's long enough.)

There are cases where the string data is manipulated first -- C++ name
canonicalization (I can't think of another case but maybe there are
some).  However, when this is done, the resulting string is stored
somewhere with the appropriate lifetime, normally the per-BFD obstack.

Tom