diff mbox series

[8/8] gdb/dwarf: store dwo_file_up in dwo_file_set

Message ID 20250305050834.2223161-8-simon.marchi@polymtl.ca
State New
Headers
Series [1/8] gdb/dwarf: remove unnecessary local variable in dw2_get_file_names_reader |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 success Build passed
linaro-tcwg-bot/tcwg_gdb_build--master-arm success Build passed
linaro-tcwg-bot/tcwg_gdb_check--master-arm success Test passed
linaro-tcwg-bot/tcwg_gdb_check--master-aarch64 success Test passed

Commit Message

Simon Marchi March 5, 2025, 5:06 a.m. UTC 
  From: Simon Marchi <simon.marchi@polymtl.ca>

Heap-allocated dwo_file objects, stored in dwarf2_per_bfd::dwo_files,
are never freed.  They are created in one of the
create_dwo_unit_in_dwp_* or lookup_dwo_cutu functions.  I confirmed this
by running:

  $ make check TESTS="gdb.cp/anon-ns.exp" RUNTESTFLAGS="--target_board=fission-dwp"
  $ ./gdb -q -nx --data-directory=data-directory testsuite/outputs/gdb.cp/anon-ns/anon-ns -ex "p main" -ex "file" -batch

... and checking the ASan leak report.  I also debugged this invocation
of GDB, placed a breakpoint on ~dwo_file, and didn't see any hit.

Change the dwo_file set to hold dwo_file_up objects.  When the
dwarf2_per_bfd object gets destroyed, dwo_file objects will
automatically get destroyed.  With this change, I see the related leaks
disappear in the ASan leak report, and my ~dwo_file breakpoint gets hit
when debugging GDB.

Change-Id: Icb38539c3f9e553f3625c625a00fc63dd6e9f3c5
---
 gdb/dwarf2/read.c | 176 +++++++++++++++++++++++-----------------------
 gdb/dwarf2/read.h |  13 ++--
 2 files changed, 97 insertions(+), 92 deletions(-)

Comments

Tom Tromey March 5, 2025, 4:52 p.m. UTC | #1 
>>>>> "Simon" == simon marchi <simon.marchi@polymtl.ca> writes:

Simon> Change the dwo_file set to hold dwo_file_up objects.  When the
Simon> dwarf2_per_bfd object gets destroyed, dwo_file objects will
Simon> automatically get destroyed.  With this change, I see the related leaks
Simon> disappear in the ASan leak report, and my ~dwo_file breakpoint gets hit
Simon> when debugging GDB.

Thanks for finding & fixing this.
Approved-By: Tom Tromey <tom@tromey.com>

Tom
diff mbox series

Patch

diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index 10d983da50cb..1d2e1a0440eb 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -416,8 +416,6 @@  struct dwo_file
   dwo_unit_set tus;
 };
 
-using dwo_file_up = std::unique_ptr<dwo_file>;
-
 /* See dwarf2/read.h.  */
 
 std::size_t
@@ -434,7 +432,7 @@  dwo_file_hash::operator() (const dwo_file_search &search) const noexcept
 /* See dwarf2/read.h.  */
 
 std::size_t
-dwo_file_hash::operator() (const dwo_file *file) const noexcept
+dwo_file_hash::operator() (const dwo_file_up &file) const noexcept
 {
   return (*this) ({ file->dwo_name.c_str (), file->comp_dir });
 }
@@ -443,7 +441,7 @@  dwo_file_hash::operator() (const dwo_file *file) const noexcept
 
 bool
 dwo_file_eq::operator() (const dwo_file_search &search,
-			 const dwo_file *dwo_file) const noexcept
+			 const dwo_file_up &dwo_file) const noexcept
 {
   if (search.dwo_name != dwo_file->dwo_name)
     return false;
@@ -457,7 +455,8 @@  dwo_file_eq::operator() (const dwo_file_search &search,
 /* See dwarf2/read.h.  */
 
 bool
-dwo_file_eq::operator() (const dwo_file *a, const dwo_file *b) const noexcept
+dwo_file_eq::operator() (const dwo_file_up &a,
+			 const dwo_file_up &b) const noexcept
 { return (*this) ({ a->dwo_name.c_str (), a->comp_dir }, b); }
 
 /* These sections are what may appear in a DWP file.  */
@@ -4051,7 +4050,7 @@  process_skeletonless_type_units (dwarf2_per_objfile *per_objfile,
 {
   /* Skeletonless TUs in DWP files without .gdb_index is not supported yet.  */
   if (get_dwp_file (per_objfile) == nullptr)
-    for (dwo_file *file : per_objfile->per_bfd->dwo_files)
+    for (const dwo_file_up &file : per_objfile->per_bfd->dwo_files)
       for (dwo_unit *unit : file->tus)
 	process_skeletonless_type_unit (unit, per_objfile, storage);
 }
@@ -6879,7 +6878,7 @@  lookup_dwo_file (dwarf2_per_bfd *per_bfd, const char *dwo_name,
 {
   auto it = per_bfd->dwo_files.find (dwo_file_search {dwo_name, comp_dir});
 
-  return it != per_bfd->dwo_files.end () ? *it : nullptr;
+  return it != per_bfd->dwo_files.end () ? it->get() : nullptr;
 }
 
 /* die_reader_func for create_dwo_cu.  */
@@ -7550,18 +7549,18 @@  create_dwo_unit_in_dwp_v1 (dwarf2_per_bfd *per_bfd,
       dwarf_read_debug_printf ("Creating virtual DWO: %s",
 			       virtual_dwo_name.c_str ());
 
-      dwo_file = new struct dwo_file;
-      dwo_file->dwo_name = std::move (virtual_dwo_name);
-      dwo_file->comp_dir = comp_dir;
-      dwo_file->sections.abbrev = sections.abbrev;
-      dwo_file->sections.line = sections.line;
-      dwo_file->sections.loc = sections.loc;
-      dwo_file->sections.macinfo = sections.macinfo;
-      dwo_file->sections.macro = sections.macro;
-      dwo_file->sections.str_offsets = sections.str_offsets;
+      dwo_file_up new_dwo_file = std::make_unique<struct dwo_file> ();
+      new_dwo_file->dwo_name = std::move (virtual_dwo_name);
+      new_dwo_file->comp_dir = comp_dir;
+      new_dwo_file->sections.abbrev = sections.abbrev;
+      new_dwo_file->sections.line = sections.line;
+      new_dwo_file->sections.loc = sections.loc;
+      new_dwo_file->sections.macinfo = sections.macinfo;
+      new_dwo_file->sections.macro = sections.macro;
+      new_dwo_file->sections.str_offsets = sections.str_offsets;
 
       /* The "str" section is global to the entire DWP file.  */
-      dwo_file->sections.str = dwp_file->sections.str;
+      new_dwo_file->sections.str = dwp_file->sections.str;
 
       /* The info or types section is assigned below to dwo_unit,
 	 there's no need to record it in dwo_file.
@@ -7570,8 +7569,10 @@  create_dwo_unit_in_dwp_v1 (dwarf2_per_bfd *per_bfd,
 	 types we'll grow the vector and eventually have to reallocate space
 	 for it, invalidating all copies of pointers into the previous
 	 contents.  */
-      auto [it, inserted] = per_bfd->dwo_files.emplace (dwo_file);
+      auto [it, inserted]
+	= per_bfd->dwo_files.emplace (std::move (new_dwo_file));
       gdb_assert (inserted);
+      dwo_file = it->get ();
     }
   else
     dwarf_read_debug_printf ("Using existing virtual DWO: %s",
@@ -7736,36 +7737,35 @@  create_dwo_unit_in_dwp_v2 (dwarf2_per_bfd *per_bfd,
       dwarf_read_debug_printf ("Creating virtual DWO: %s",
 			       virtual_dwo_name.c_str ());
 
-      dwo_file = new struct dwo_file;
-      dwo_file->dwo_name = std::move (virtual_dwo_name);
-      dwo_file->comp_dir = comp_dir;
-      dwo_file->sections.abbrev =
-	create_dwp_v2_or_v5_section (per_bfd, &dwp_file->sections.abbrev,
-				     sections.abbrev_offset,
-				     sections.abbrev_size);
-      dwo_file->sections.line =
-	create_dwp_v2_or_v5_section (per_bfd, &dwp_file->sections.line,
-				     sections.line_offset,
-				     sections.line_size);
-      dwo_file->sections.loc =
-	create_dwp_v2_or_v5_section (per_bfd, &dwp_file->sections.loc,
-				     sections.loc_offset, sections.loc_size);
-      dwo_file->sections.macinfo =
-	create_dwp_v2_or_v5_section (per_bfd, &dwp_file->sections.macinfo,
-				     sections.macinfo_offset,
-				     sections.macinfo_size);
-      dwo_file->sections.macro =
-	create_dwp_v2_or_v5_section (per_bfd, &dwp_file->sections.macro,
-				     sections.macro_offset,
-				     sections.macro_size);
-      dwo_file->sections.str_offsets =
-	create_dwp_v2_or_v5_section (per_bfd,
-				     &dwp_file->sections.str_offsets,
-				     sections.str_offsets_offset,
-				     sections.str_offsets_size);
+      dwo_file_up new_dwo_file = std::make_unique<struct dwo_file> ();
+      new_dwo_file->dwo_name = std::move (virtual_dwo_name);
+      new_dwo_file->comp_dir = comp_dir;
+      new_dwo_file->sections.abbrev
+	= create_dwp_v2_or_v5_section (per_bfd, &dwp_file->sections.abbrev,
+				       sections.abbrev_offset,
+				       sections.abbrev_size);
+      new_dwo_file->sections.line
+	= create_dwp_v2_or_v5_section (per_bfd, &dwp_file->sections.line,
+				       sections.line_offset,
+				       sections.line_size);
+      new_dwo_file->sections.loc
+	= create_dwp_v2_or_v5_section (per_bfd, &dwp_file->sections.loc,
+				       sections.loc_offset, sections.loc_size);
+      new_dwo_file->sections.macinfo
+	= create_dwp_v2_or_v5_section (per_bfd, &dwp_file->sections.macinfo,
+				       sections.macinfo_offset,
+				       sections.macinfo_size);
+      new_dwo_file->sections.macro
+	= create_dwp_v2_or_v5_section (per_bfd, &dwp_file->sections.macro,
+				       sections.macro_offset,
+				       sections.macro_size);
+      new_dwo_file->sections.str_offsets
+	= create_dwp_v2_or_v5_section (per_bfd, &dwp_file->sections.str_offsets,
+				       sections.str_offsets_offset,
+				       sections.str_offsets_size);
 
       /* The "str" section is global to the entire DWP file.  */
-      dwo_file->sections.str = dwp_file->sections.str;
+      new_dwo_file->sections.str = dwp_file->sections.str;
 
       /* The info or types section is assigned below to dwo_unit,
 	 there's no need to record it in dwo_file.
@@ -7774,8 +7774,10 @@  create_dwo_unit_in_dwp_v2 (dwarf2_per_bfd *per_bfd,
 	 types we'll grow the vector and eventually have to reallocate space
 	 for it, invalidating all copies of pointers into the previous
 	 contents.  */
-      auto [it, inserted] = per_bfd->dwo_files.emplace (dwo_file);
+      auto [it, inserted]
+	= per_bfd->dwo_files.emplace (std::move (new_dwo_file));
       gdb_assert (inserted);
+      dwo_file = it->get ();
     }
   else
     dwarf_read_debug_printf ("Using existing virtual DWO: %s",
@@ -7904,41 +7906,36 @@  create_dwo_unit_in_dwp_v5 (dwarf2_per_bfd *per_bfd,
       dwarf_read_debug_printf ("Creating virtual DWO: %s",
 			       virtual_dwo_name.c_str ());
 
-      dwo_file = new struct dwo_file;
-      dwo_file->dwo_name = std::move (virtual_dwo_name);
-      dwo_file->comp_dir = comp_dir;
-      dwo_file->sections.abbrev =
-	create_dwp_v2_or_v5_section (per_bfd,
-				     &dwp_file->sections.abbrev,
-				     sections.abbrev_offset,
-				     sections.abbrev_size);
-      dwo_file->sections.line =
-	create_dwp_v2_or_v5_section (per_bfd,
-				     &dwp_file->sections.line,
-				     sections.line_offset, sections.line_size);
-      dwo_file->sections.macro =
-	create_dwp_v2_or_v5_section (per_bfd,
-				     &dwp_file->sections.macro,
-				     sections.macro_offset,
-				     sections.macro_size);
-      dwo_file->sections.loclists =
-	create_dwp_v2_or_v5_section (per_bfd,
-				     &dwp_file->sections.loclists,
-				     sections.loclists_offset,
-				     sections.loclists_size);
-      dwo_file->sections.rnglists =
-	create_dwp_v2_or_v5_section (per_bfd,
-				     &dwp_file->sections.rnglists,
-				     sections.rnglists_offset,
-				     sections.rnglists_size);
-      dwo_file->sections.str_offsets =
-	create_dwp_v2_or_v5_section (per_bfd,
-				     &dwp_file->sections.str_offsets,
-				     sections.str_offsets_offset,
-				     sections.str_offsets_size);
+      dwo_file_up new_dwo_file = std::make_unique<struct dwo_file> ();
+      new_dwo_file->dwo_name = std::move (virtual_dwo_name);
+      new_dwo_file->comp_dir = comp_dir;
+      new_dwo_file->sections.abbrev
+	= create_dwp_v2_or_v5_section (per_bfd, &dwp_file->sections.abbrev,
+				       sections.abbrev_offset,
+				       sections.abbrev_size);
+      new_dwo_file->sections.line
+	= create_dwp_v2_or_v5_section (per_bfd, &dwp_file->sections.line,
+				       sections.line_offset,
+				       sections.line_size);
+      new_dwo_file->sections.macro
+	= create_dwp_v2_or_v5_section (per_bfd, &dwp_file->sections.macro,
+				       sections.macro_offset,
+				       sections.macro_size);
+      new_dwo_file->sections.loclists
+	= create_dwp_v2_or_v5_section (per_bfd, &dwp_file->sections.loclists,
+				       sections.loclists_offset,
+				       sections.loclists_size);
+      new_dwo_file->sections.rnglists
+	= create_dwp_v2_or_v5_section (per_bfd, &dwp_file->sections.rnglists,
+				       sections.rnglists_offset,
+				       sections.rnglists_size);
+      new_dwo_file->sections.str_offsets
+	= create_dwp_v2_or_v5_section (per_bfd, &dwp_file->sections.str_offsets,
+				       sections.str_offsets_offset,
+				       sections.str_offsets_size);
 
       /* The "str" section is global to the entire DWP file.  */
-      dwo_file->sections.str = dwp_file->sections.str;
+      new_dwo_file->sections.str = dwp_file->sections.str;
 
       /* The info or types section is assigned below to dwo_unit,
 	 there's no need to record it in dwo_file.
@@ -7947,8 +7944,10 @@  create_dwo_unit_in_dwp_v5 (dwarf2_per_bfd *per_bfd,
 	 types we'll grow the vector and eventually have to reallocate space
 	 for it, invalidating all copies of pointers into the previous
 	 contents.  */
-      auto [it, inserted] = per_bfd->dwo_files.emplace (dwo_file);
+      auto [it, inserted]
+	= per_bfd->dwo_files.emplace (std::move (new_dwo_file));
       gdb_assert (inserted);
+      dwo_file = it->get ();
     }
   else
     dwarf_read_debug_printf ("Using existing virtual DWO: %s",
@@ -8200,7 +8199,7 @@  dwarf2_locate_dwo_sections (struct objfile *objfile, bfd *abfd,
    by PER_CU.  This is for the non-DWP case.
    The result is NULL if DWO_NAME can't be found.  */
 
-static struct dwo_file *
+static dwo_file_up
 open_and_init_dwo_file (dwarf2_cu *cu, const char *dwo_name,
 			const char *comp_dir)
 {
@@ -8215,7 +8214,7 @@  open_and_init_dwo_file (dwarf2_cu *cu, const char *dwo_name,
       return NULL;
     }
 
-  dwo_file_up dwo_file (new struct dwo_file);
+  dwo_file_up dwo_file = std::make_unique<struct dwo_file> ();
   dwo_file->dwo_name = dwo_name;
   dwo_file->comp_dir = comp_dir;
   dwo_file->dbfd = std::move (dbfd);
@@ -8237,7 +8236,7 @@  open_and_init_dwo_file (dwarf2_cu *cu, const char *dwo_name,
 
   bfd_cache_close (dwo_file->dbfd.get ());
 
-  return dwo_file.release ();
+  return dwo_file;
 }
 
 /* This function is mapped across the sections and remembers the offset and
@@ -8573,12 +8572,15 @@  lookup_dwo_cutu (dwarf2_cu *cu, const char *dwo_name, const char *comp_dir,
 	  /* Read in the file and build a table of the CUs/TUs it contains.
 
 	     NOTE: This will be nullptr if unable to open the file.  */
-	  dwo_file = open_and_init_dwo_file (cu, dwo_name, comp_dir);
+	  dwo_file_up new_dwo_file
+	    = open_and_init_dwo_file (cu, dwo_name, comp_dir);
 
-	  if (dwo_file != nullptr)
+	  if (new_dwo_file != nullptr)
 	    {
-	      auto [it, inserted] = per_bfd->dwo_files.emplace (dwo_file);
+	      auto [it, inserted]
+		= per_bfd->dwo_files.emplace (std::move (new_dwo_file));
 	      gdb_assert (inserted);
+	      dwo_file = (*it).get ();
 	    }
 	}
 
diff --git a/gdb/dwarf2/read.h b/gdb/dwarf2/read.h
index 164f9ae11409..44747bf7ccdd 100644
--- a/gdb/dwarf2/read.h
+++ b/gdb/dwarf2/read.h
@@ -445,6 +445,8 @@  using signatured_type_set
 
 struct dwo_file;
 
+using dwo_file_up = std::unique_ptr<dwo_file>;
+
 /* This is used when looking up entries in a dwo_file_set.  */
 
 struct dwo_file_search
@@ -464,7 +466,7 @@  struct dwo_file_hash
   using is_transparent = void;
 
   std::size_t operator() (const dwo_file_search &search) const noexcept;
-  std::size_t operator() (const dwo_file *file) const noexcept;
+  std::size_t operator() (const dwo_file_up &file) const noexcept;
 };
 
 /* Equal function for dwo_file objects, using their dwo_name and comp_dir as
@@ -475,13 +477,14 @@  struct dwo_file_eq
   using is_transparent = void;
 
   bool operator() (const dwo_file_search &search,
-		   const dwo_file *dwo_file) const noexcept;
-  bool operator() (const dwo_file *a, const dwo_file *b) const noexcept;
+		   const dwo_file_up &dwo_file) const noexcept;
+  bool operator() (const dwo_file_up &a, const dwo_file_up &b) const noexcept;
 };
 
 /* Set of dwo_file objects, using their dwo_name and comp_dir as identity.  */
 
-using dwo_file_set = gdb::unordered_set<dwo_file *, dwo_file_hash, dwo_file_eq>;
+using dwo_file_up_set
+  = gdb::unordered_set<dwo_file_up, dwo_file_hash, dwo_file_eq>;
 
 struct dwp_file;
 
@@ -635,7 +638,7 @@  struct dwarf2_per_bfd
   struct tu_stats tu_stats;
 
   /* Set of dwo_file objects.  */
-  dwo_file_set dwo_files;
+  dwo_file_up_set dwo_files;
 
   /* True if we've checked for whether there is a DWP file.  */
   bool dwp_checked = false;