This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

[PATCH] Plug line_header leaks (Re: Synthetic symbol leak in in elf_x86_64_get_synthetic_symtab and elf_read_minimal_symbols)


On 08/17/2017 06:42 PM, Pedro Alves wrote:
> On 08/17/2017 01:31 PM, Philippe Waroquiers wrote:
> 
>> My knowledge of c++ is close to 0, so I cannot help much
>> to find the source of the leak.
>> I am wondering however who owns the memory allocated
>> at dwarf2read.c:9362 :
>>   line_header_up lh = dwarf_decode_line_header (line_offset, cu);
>> when the logic goes later on to line 9389
>>         gdb_assert (die->tag != DW_TAG_partial_unit);
>> (for info: in the c version 7.11, this assert was followed by
>>       make_cleanup (free_cu_line_header, cu);
>> )
> 
> That does look like the reason for the leak.  I'm taking a look.

Here's what I pushed.

>From 4c8aa72d0eb714a91ca2e47b816d0b4a0cb27843 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Thu, 17 Aug 2017 22:53:53 +0100
Subject: [PATCH] Plug line_header leaks

This plugs a couple leaks introduced by commit fff8551cf549
("dwarf2read.c: Some C++fycation, use std::vector, std::unique_ptr").

The first problem is that nothing owns the temporary line_header that
handle_DW_AT_stmt_list creates in some cases.  Before the commit
mentioned above, the temporary line_header case used to have:

  make_cleanup (free_cu_line_header, cu);

and that cleanup was assumed to be run by process_die, after
handle_DW_AT_stmt_list returns and before child DIEs were processed.

The second problem is found in setup_type_unit_groups: that also used
to have a similar make_cleanup call, and ended up with a similar leak
after the commit mentioned above.

Fix both cases by recording in dwarf2_cu whether a line header is
owned by the cu/die, and have process_die explicitly free the
line_header if so, making use of a new RAII object that also replaces
the reset_die_in_process cleanup, while at it.

Thanks to Philippe Waroquiers for noticing the leak and pointing in
the right direction.

gdb/ChangeLog:
2017-08-17  Pedro Alves  <palves@redhat.com>

	* dwarf2read.c (struct dwarf2_cu) <line_header_die_owner>: New
	field.
	(reset_die_in_process): Delete, replaced by ...
	(process_die_scope): ... this new class.  Make it responsible for
	freeing cu->line_header too.
	(process_die): Use process_die_scope.
	(handle_DW_AT_stmt_list): Record the line header's owner CU/DIE in
	cu->line_header_die_owner.  Don't release the line header if it's
	owned by the CU.
	(setup_type_unit_groups): Make the CU/DIE own the line header.
	Don't release the line header here.
---
 gdb/ChangeLog    | 14 +++++++++
 gdb/dwarf2read.c | 89 +++++++++++++++++++++++++++++++++++---------------------
 2 files changed, 70 insertions(+), 33 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index d2c194e..50b7237 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,17 @@
+2017-08-17  Pedro Alves  <palves@redhat.com>
+
+	* dwarf2read.c (struct dwarf2_cu) <line_header_die_owner>: New
+	field.
+	(reset_die_in_process): Delete, replaced by ...
+	(process_die_scope): ... this new class.  Make it responsible for
+	freeing cu->line_header too.
+	(process_die): Use process_die_scope.
+	(handle_DW_AT_stmt_list): Record the line header's owner CU/DIE in
+	cu->line_header_die_owner.  Don't release the line header if it's
+	owned by the CU.
+	(setup_type_unit_groups): Make the CU/DIE own the line header.
+	Don't release the line header here.
+
 2017-08-17  Alex Lindsay  <alexlindsay239@gmail.com>  (tiny change)
 
 	* elfread.c (elf_read_minimal_symbols): xfree synthsyms.
diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index 4f2fdce..0e28144 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -546,6 +546,12 @@ struct dwarf2_cu
 
   /* Header data from the line table, during full symbol processing.  */
   struct line_header *line_header;
+  /* Non-NULL if LINE_HEADER is owned by this DWARF_CU.  Otherwise,
+     it's owned by dwarf2_per_objfile::line_header_hash.  If non-NULL,
+     this is the DW_TAG_compile_unit die for this CU.  We'll hold on
+     to the line header as long as this DIE is being processed.  See
+     process_die_scope.  */
+  die_info *line_header_die_owner;
 
   /* A list of methods which need to have physnames computed
      after all type information has been read.  */
@@ -8471,28 +8477,44 @@ process_imported_unit_die (struct die_info *die, struct dwarf2_cu *cu)
     }
 }
 
-/* Reset the in_process bit of a die.  */
-
-static void
-reset_die_in_process (void *arg)
+/* RAII object that represents a process_die scope: i.e.,
+   starts/finishes processing a DIE.  */
+class process_die_scope
 {
-  struct die_info *die = (struct die_info *) arg;
+public:
+  process_die_scope (die_info *die, dwarf2_cu *cu)
+    : m_die (die), m_cu (cu)
+  {
+    /* We should only be processing DIEs not already in process.  */
+    gdb_assert (!m_die->in_process);
+    m_die->in_process = true;
+  }
 
-  die->in_process = 0;
-}
+  ~process_die_scope ()
+  {
+    m_die->in_process = false;
+
+    /* If we're done processing the DIE for the CU that owns the line
+       header, we don't need the line header anymore.  */
+    if (m_cu->line_header_die_owner == m_die)
+      {
+	delete m_cu->line_header;
+	m_cu->line_header = NULL;
+	m_cu->line_header_die_owner = NULL;
+      }
+  }
+
+private:
+  die_info *m_die;
+  dwarf2_cu *m_cu;
+};
 
 /* Process a die and its children.  */
 
 static void
 process_die (struct die_info *die, struct dwarf2_cu *cu)
 {
-  struct cleanup *in_process;
-
-  /* We should only be processing those not already in process.  */
-  gdb_assert (!die->in_process);
-
-  die->in_process = 1;
-  in_process = make_cleanup (reset_die_in_process,die);
+  process_die_scope scope (die, cu);
 
   switch (die->tag)
     {
@@ -8583,8 +8605,6 @@ process_die (struct die_info *die, struct dwarf2_cu *cu)
       new_symbol (die, NULL, cu);
       break;
     }
-
-  do_cleanups (in_process);
 }
 
 /* DWARF name computation.  */
@@ -9362,7 +9382,9 @@ handle_DW_AT_stmt_list (struct die_info *die, struct dwarf2_cu *cu,
   line_header_up lh = dwarf_decode_line_header (line_offset, cu);
   if (lh == NULL)
     return;
-  cu->line_header = lh.get ();
+
+  cu->line_header = lh.release ();
+  cu->line_header_die_owner = die;
 
   if (dwarf2_per_objfile->line_header_hash == NULL)
     slot = NULL;
@@ -9378,6 +9400,7 @@ handle_DW_AT_stmt_list (struct die_info *die, struct dwarf2_cu *cu,
       /* This newly decoded line number information unit will be owned
 	 by line_header_hash hash table.  */
       *slot = cu->line_header;
+      cu->line_header_die_owner = NULL;
     }
   else
     {
@@ -9392,7 +9415,6 @@ handle_DW_AT_stmt_list (struct die_info *die, struct dwarf2_cu *cu,
   dwarf_decode_lines (cu->line_header, comp_dir, cu, NULL, lowpc,
 		      decode_mapping);
 
-  lh.release ();
 }
 
 /* Process DW_TAG_compile_unit or DW_TAG_partial_unit.  */
@@ -9530,7 +9552,8 @@ setup_type_unit_groups (struct die_info *die, struct dwarf2_cu *cu)
       return;
     }
 
-  cu->line_header = lh.get ();
+  cu->line_header = lh.release ();
+  cu->line_header_die_owner = die;
 
   if (first_time)
     {
@@ -9541,21 +9564,23 @@ setup_type_unit_groups (struct die_info *die, struct dwarf2_cu *cu)
 	 process_full_type_unit still needs to know if this is the first
 	 time.  */
 
-      tu_group->num_symtabs = lh->file_names.size ();
-      tu_group->symtabs = XNEWVEC (struct symtab *, lh->file_names.size ());
+      tu_group->num_symtabs = cu->line_header->file_names.size ();
+      tu_group->symtabs = XNEWVEC (struct symtab *,
+				   cu->line_header->file_names.size ());
 
-      for (i = 0; i < lh->file_names.size (); ++i)
+      for (i = 0; i < cu->line_header->file_names.size (); ++i)
 	{
-	  file_entry &fe = lh->file_names[i];
+	  file_entry &fe = cu->line_header->file_names[i];
 
-	  dwarf2_start_subfile (fe.name, fe.include_dir (lh.get ()));
+	  dwarf2_start_subfile (fe.name, fe.include_dir (cu->line_header));
 
 	  if (current_subfile->symtab == NULL)
 	    {
-	      /* NOTE: start_subfile will recognize when it's been passed
-		 a file it has already seen.  So we can't assume there's a
-		 simple mapping from lh->file_names to subfiles, plus
-		 lh->file_names may contain dups.  */
+	      /* NOTE: start_subfile will recognize when it's been
+		 passed a file it has already seen.  So we can't
+		 assume there's a simple mapping from
+		 cu->line_header->file_names to subfiles, plus
+		 cu->line_header->file_names may contain dups.  */
 	      current_subfile->symtab
 		= allocate_symtab (cust, current_subfile->name);
 	    }
@@ -9568,16 +9593,14 @@ setup_type_unit_groups (struct die_info *die, struct dwarf2_cu *cu)
     {
       restart_symtab (tu_group->compunit_symtab, "", 0);
 
-      for (i = 0; i < lh->file_names.size (); ++i)
+      for (i = 0; i < cu->line_header->file_names.size (); ++i)
 	{
-	  struct file_entry *fe = &lh->file_names[i];
+	  file_entry &fe = cu->line_header->file_names[i];
 
-	  fe->symtab = tu_group->symtabs[i];
+	  fe.symtab = tu_group->symtabs[i];
 	}
     }
 
-  lh.release ();
-
   /* The main symtab is allocated last.  Type units don't have DW_AT_name
      so they don't have a "real" (so to speak) symtab anyway.
      There is later code that will assign the main symtab to all symbols
-- 
2.5.5



Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]