public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
* Synthetic symbol leak in in elf_x86_64_get_synthetic_symtab and elf_read_minimal_symbols
@ 2017-08-07 15:19 Alex Lindsay
  2017-08-11  9:27 ` Yao Qi
  0 siblings, 1 reply; 11+ messages in thread
From: Alex Lindsay @ 2017-08-07 15:19 UTC (permalink / raw)
  To: gdb-patches

Detected this leak with valgrind memcheck:

==21225== 463 (336 direct, 127 indirect) bytes in 1 blocks are 
definitely lost in loss record 10,770 of 10,949^M
==21225==    at 0x4C2DB8F: malloc (in 
/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)^M
==21225==    by 0x6C6DA2: bfd_malloc (libbfd.c:193)^M
==21225==    by 0x6C6F4D: bfd_zmalloc (libbfd.c:278)^M
==21225==    by 0x6D252E: elf_x86_64_get_synthetic_symtab 
(elf64-x86-64.c:6846)^M
==21225==    by 0x4B397A: elf_read_minimal_symbols (elfread.c:1124)^M
==21225==    by 0x4B397A: elf_symfile_read(objfile*, 
enum_flags<symfile_add_flag>) (elfread.c:1182)^M
==21225==    by 0x63AC94: read_symbols(objfile*, 
enum_flags<symfile_add_flag>) (symfile.c:861)^M
==21225==    by 0x63A773: syms_from_objfile_1 (symfile.c:1062)

We perform a couple of  dynamic allocations in 
elf_x86_64_get_synthetic_symtab (elf64-x86-64.c):

   s = *ret = (asymbol *) bfd_zmalloc (size);

   names = (char *) bfd_malloc (size);

that appear to never get freed. My patch for this:

diff --git a/gdb/elfread.c b/gdb/elfread.c
index ece704ca7c..5ed8a6f957 100644
--- a/gdb/elfread.c
+++ b/gdb/elfread.c
@@ -1144,6 +1144,9 @@ elf_read_minimal_symbols (struct objfile *objfile, 
int symfile_flags,

    if (symtab_create_debug)
      fprintf_unfiltered (gdb_stdlog, "Done reading minimal symbols.\n");
+  if (synthcount > 0)
+    xfree ((char *) synthsyms->name);
+  xfree (synthsyms);
  }

  /* Scan and build partial symbols for a symbol file.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Synthetic symbol leak in in elf_x86_64_get_synthetic_symtab and elf_read_minimal_symbols
  2017-08-07 15:19 Synthetic symbol leak in in elf_x86_64_get_synthetic_symtab and elf_read_minimal_symbols Alex Lindsay
@ 2017-08-11  9:27 ` Yao Qi
  2017-08-11 15:07   ` Alex Lindsay
  2017-08-11 15:31   ` H.J. Lu
  0 siblings, 2 replies; 11+ messages in thread
From: Yao Qi @ 2017-08-11  9:27 UTC (permalink / raw)
  To: Alex Lindsay; +Cc: gdb-patches

On 17-08-07 10:19:15, Alex Lindsay wrote:
> 
> We perform a couple of  dynamic allocations in
> elf_x86_64_get_synthetic_symtab (elf64-x86-64.c):
> 
>   s = *ret = (asymbol *) bfd_zmalloc (size);
> 
>   names = (char *) bfd_malloc (size);
> 
> that appear to never get freed. My patch for this:

Good catch!  It is more complicated that other bfd targets allocate
memory for asymbol in a different way as if asymbol.name is defined
as an zero-length array, so memory allocated for both asymbol and .name
in one bfd_malloc call, like,

	  sym = *r->sym_ptr_ptr;
	  if (!sym_exists_at (syms, opdsymend, symcount,
			      sym->section->id, sym->value + r->addend))
	    {
	      ++count;
	      size += sizeof (asymbol);
	      size += strlen (syms[i]->name) + 2;
	    }
	}

      if (size == 0)
	goto done;
      s = *ret = bfd_malloc (size);

or

  size = count * sizeof (asymbol);
  p = relplt->relocation;
  for (i = 0; i < count; i++, p += elf32_arm_size_info.int_rels_per_ext_rel)
    {
      size += strlen ((*p->sym_ptr_ptr)->name) + sizeof ("@plt");
      if (p->addend != 0)
	size += sizeof ("+0x") - 1 + 8;
    }

  s = *ret = (asymbol *) bfd_malloc (size);

> 
> diff --git a/gdb/elfread.c b/gdb/elfread.c
> index ece704ca7c..5ed8a6f957 100644
> --- a/gdb/elfread.c
> +++ b/gdb/elfread.c
> @@ -1144,6 +1144,9 @@ elf_read_minimal_symbols (struct objfile *objfile, int
> symfile_flags,
> 
>    if (symtab_create_debug)
>      fprintf_unfiltered (gdb_stdlog, "Done reading minimal symbols.\n");
> +  if (synthcount > 0)
> +    xfree ((char *) synthsyms->name);

We can't do this for some bfd targets.

> +  xfree (synthsyms);

We can only safely do this, but .name is leaked for x86_64.  Other
tools using bfd, like objdump, nm, and gprof may have this issue too.
I'll ask binutils people on asymbol allocation and de-allocation.

-- 
Yao (齐尧)

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Synthetic symbol leak in in elf_x86_64_get_synthetic_symtab and elf_read_minimal_symbols
  2017-08-11  9:27 ` Yao Qi
@ 2017-08-11 15:07   ` Alex Lindsay
  2017-08-11 15:31   ` H.J. Lu
  1 sibling, 0 replies; 11+ messages in thread
From: Alex Lindsay @ 2017-08-11 15:07 UTC (permalink / raw)
  To: Yao Qi; +Cc: gdb-patches

Thanks for looking into it!

On 08/11/2017 04:27 AM, Yao Qi wrote:
> On 17-08-07 10:19:15, Alex Lindsay wrote:
>> We perform a couple of  dynamic allocations in
>> elf_x86_64_get_synthetic_symtab (elf64-x86-64.c):
>>
>>    s = *ret = (asymbol *) bfd_zmalloc (size);
>>
>>    names = (char *) bfd_malloc (size);
>>
>> that appear to never get freed. My patch for this:
> Good catch!  It is more complicated that other bfd targets allocate
> memory for asymbol in a different way as if asymbol.name is defined
> as an zero-length array, so memory allocated for both asymbol and .name
> in one bfd_malloc call, like,
>
> 	  sym = *r->sym_ptr_ptr;
> 	  if (!sym_exists_at (syms, opdsymend, symcount,
> 			      sym->section->id, sym->value + r->addend))
> 	    {
> 	      ++count;
> 	      size += sizeof (asymbol);
> 	      size += strlen (syms[i]->name) + 2;
> 	    }
> 	}
>
>        if (size == 0)
> 	goto done;
>        s = *ret = bfd_malloc (size);
>
> or
>
>    size = count * sizeof (asymbol);
>    p = relplt->relocation;
>    for (i = 0; i < count; i++, p += elf32_arm_size_info.int_rels_per_ext_rel)
>      {
>        size += strlen ((*p->sym_ptr_ptr)->name) + sizeof ("@plt");
>        if (p->addend != 0)
> 	size += sizeof ("+0x") - 1 + 8;
>      }
>
>    s = *ret = (asymbol *) bfd_malloc (size);
>
>> diff --git a/gdb/elfread.c b/gdb/elfread.c
>> index ece704ca7c..5ed8a6f957 100644
>> --- a/gdb/elfread.c
>> +++ b/gdb/elfread.c
>> @@ -1144,6 +1144,9 @@ elf_read_minimal_symbols (struct objfile *objfile, int
>> symfile_flags,
>>
>>     if (symtab_create_debug)
>>       fprintf_unfiltered (gdb_stdlog, "Done reading minimal symbols.\n");
>> +  if (synthcount > 0)
>> +    xfree ((char *) synthsyms->name);
> We can't do this for some bfd targets.
>
>> +  xfree (synthsyms);
> We can only safely do this, but .name is leaked for x86_64.  Other
> tools using bfd, like objdump, nm, and gprof may have this issue too.
> I'll ask binutils people on asymbol allocation and de-allocation.
>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Synthetic symbol leak in in elf_x86_64_get_synthetic_symtab and elf_read_minimal_symbols
  2017-08-11  9:27 ` Yao Qi
  2017-08-11 15:07   ` Alex Lindsay
@ 2017-08-11 15:31   ` H.J. Lu
  2017-08-11 15:46     ` Yao Qi
  1 sibling, 1 reply; 11+ messages in thread
From: H.J. Lu @ 2017-08-11 15:31 UTC (permalink / raw)
  To: Yao Qi; +Cc: Alex Lindsay, GDB

On Fri, Aug 11, 2017 at 2:27 AM, Yao Qi <qiyaoltc@gmail.com> wrote:
> On 17-08-07 10:19:15, Alex Lindsay wrote:
>>
>> We perform a couple of  dynamic allocations in
>> elf_x86_64_get_synthetic_symtab (elf64-x86-64.c):
>>
>>   s = *ret = (asymbol *) bfd_zmalloc (size);
>>
>>   names = (char *) bfd_malloc (size);
>>
>> that appear to never get freed. My patch for this:
>
> Good catch!  It is more complicated that other bfd targets allocate
> memory for asymbol in a different way as if asymbol.name is defined
> as an zero-length array, so memory allocated for both asymbol and .name
> in one bfd_malloc call, like,
>
>           sym = *r->sym_ptr_ptr;
>           if (!sym_exists_at (syms, opdsymend, symcount,
>                               sym->section->id, sym->value + r->addend))
>             {
>               ++count;
>               size += sizeof (asymbol);
>               size += strlen (syms[i]->name) + 2;
>             }
>         }
>
>       if (size == 0)
>         goto done;
>       s = *ret = bfd_malloc (size);
>
> or
>
>   size = count * sizeof (asymbol);
>   p = relplt->relocation;
>   for (i = 0; i < count; i++, p += elf32_arm_size_info.int_rels_per_ext_rel)
>     {
>       size += strlen ((*p->sym_ptr_ptr)->name) + sizeof ("@plt");
>       if (p->addend != 0)
>         size += sizeof ("+0x") - 1 + 8;
>     }
>
>   s = *ret = (asymbol *) bfd_malloc (size);
>
>>
>> diff --git a/gdb/elfread.c b/gdb/elfread.c
>> index ece704ca7c..5ed8a6f957 100644
>> --- a/gdb/elfread.c
>> +++ b/gdb/elfread.c
>> @@ -1144,6 +1144,9 @@ elf_read_minimal_symbols (struct objfile *objfile, int
>> symfile_flags,
>>
>>    if (symtab_create_debug)
>>      fprintf_unfiltered (gdb_stdlog, "Done reading minimal symbols.\n");
>> +  if (synthcount > 0)
>> +    xfree ((char *) synthsyms->name);
>
> We can't do this for some bfd targets.
>
>> +  xfree (synthsyms);
>
> We can only safely do this, but .name is leaked for x86_64.  Other
> tools using bfd, like objdump, nm, and gprof may have this issue too.
> I'll ask binutils people on asymbol allocation and de-allocation.
>

This is:

https://sourceware.org/bugzilla/show_bug.cgi?id=21943

i386 and x86-64 get_synthetic_symtab don't know if @plt should
be added to symbol name for a PLT entry.  The first pass checks
if @plt is needed and extra space is allocated in the second pass.
We can assume @plt is needed and waste some space if it isn't.

-- 
H.J.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Synthetic symbol leak in in elf_x86_64_get_synthetic_symtab and elf_read_minimal_symbols
  2017-08-11 15:31   ` H.J. Lu
@ 2017-08-11 15:46     ` Yao Qi
  2017-08-11 16:44       ` H.J. Lu
  0 siblings, 1 reply; 11+ messages in thread
From: Yao Qi @ 2017-08-11 15:46 UTC (permalink / raw)
  To: H.J. Lu; +Cc: Alex Lindsay, GDB

On 17-08-11 08:30:21, H.J. Lu wrote:
> >
> > We can only safely do this, but .name is leaked for x86_64.  Other
> > tools using bfd, like objdump, nm, and gprof may have this issue too.
> > I'll ask binutils people on asymbol allocation and de-allocation.
> >
> 
> This is:
> 
> https://sourceware.org/bugzilla/show_bug.cgi?id=21943
> 

I opened it :)

> i386 and x86-64 get_synthetic_symtab don't know if @plt should
> be added to symbol name for a PLT entry.  The first pass checks
> if @plt is needed and extra space is allocated in the second pass.
> We can assume @plt is needed and waste some space if it isn't.

Do you plan to fix it?

-- 
Yao (齐尧)

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Synthetic symbol leak in in elf_x86_64_get_synthetic_symtab and elf_read_minimal_symbols
  2017-08-11 15:46     ` Yao Qi
@ 2017-08-11 16:44       ` H.J. Lu
  2017-08-11 21:20         ` Alex Lindsay
  0 siblings, 1 reply; 11+ messages in thread
From: H.J. Lu @ 2017-08-11 16:44 UTC (permalink / raw)
  To: Yao Qi; +Cc: Alex Lindsay, GDB

On Fri, Aug 11, 2017 at 8:45 AM, Yao Qi <qiyaoltc@gmail.com> wrote:
> On 17-08-11 08:30:21, H.J. Lu wrote:
>> >
>> > We can only safely do this, but .name is leaked for x86_64.  Other
>> > tools using bfd, like objdump, nm, and gprof may have this issue too.
>> > I'll ask binutils people on asymbol allocation and de-allocation.
>> >
>>
>> This is:
>>
>> https://sourceware.org/bugzilla/show_bug.cgi?id=21943
>>
>
> I opened it :)
>
>> i386 and x86-64 get_synthetic_symtab don't know if @plt should
>> be added to symbol name for a PLT entry.  The first pass checks
>> if @plt is needed and extra space is allocated in the second pass.
>> We can assume @plt is needed and waste some space if it isn't.
>
> Do you plan to fix it?
>

Done.

-- 
H.J.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Synthetic symbol leak in in elf_x86_64_get_synthetic_symtab and elf_read_minimal_symbols
  2017-08-11 16:44       ` H.J. Lu
@ 2017-08-11 21:20         ` Alex Lindsay
  2017-08-17 11:00           ` Yao Qi
  0 siblings, 1 reply; 11+ messages in thread
From: Alex Lindsay @ 2017-08-11 21:20 UTC (permalink / raw)
  To: H.J. Lu, Yao Qi; +Cc: GDB

I can verify that the objdump example is fixed in HEAD, but I still get 
this leak with `valgrind --leak-check=full --show-leak-kinds=definite 
gdb ./hello`:

==18127== 300,438 bytes in 5 blocks are definitely lost in loss record 
11,404 of 11,407
==18127==    at 0x4C2DE31: malloc (vg_replace_malloc.c:299)
==18127==    by 0x62F747: bfd_malloc (libbfd.c:193)
==18127==    by 0x62F941: bfd_zmalloc (libbfd.c:278)
==18127==    by 0x649E14: elf_x86_64_get_synthetic_symtab 
(elf64-x86-64.c:6835)
==18127==    by 0x2F1B9E: elf_read_minimal_symbols(objfile*, int, 
elfinfo const*) (elfread.c:1124)
==18127==    by 0x2F1D7C: elf_symfile_read(objfile*, 
enum_flags<symfile_add_flag>) (elfread.c:1182)
==18127==    by 0x563738: read_symbols(objfile*, 
enum_flags<symfile_add_flag>) (symfile.c:861)
==18127==    by 0x563E55: syms_from_objfile_1(objfile*, 
section_addr_info*, enum_flags<symfile_add_flag>) (symfile.c:1062)
==18127==    by 0x563EAC: syms_from_objfile(objfile*, 
section_addr_info*, enum_flags<symfile_add_flag>) (symfile.c:1078)
==18127==    by 0x5641F7: symbol_file_add_with_addrs(bfd*, char const*, 
enum_flags<symfile_add_flag>, section_addr_info*, 
enum_flags<objfile_flag>, objfile*) (symfile.c:1177)
==18127==    by 0x5644C1: symbol_file_add_from_bfd(bfd*, char const*, 
enum_flags<symfile_add_flag>, section_addr_info*, 
enum_flags<objfile_flag>, objfile*) (symfile.c:1268)
==18127==    by 0x547B32: solib_read_symbols(so_list*, 
enum_flags<symfile_add_flag>) (solib.c:707)

On 08/11/2017 11:44 AM, H.J. Lu wrote:
> On Fri, Aug 11, 2017 at 8:45 AM, Yao Qi <qiyaoltc@gmail.com> wrote:
>> On 17-08-11 08:30:21, H.J. Lu wrote:
>>>> We can only safely do this, but .name is leaked for x86_64.  Other
>>>> tools using bfd, like objdump, nm, and gprof may have this issue too.
>>>> I'll ask binutils people on asymbol allocation and de-allocation.
>>>>
>>> This is:
>>>
>>> https://sourceware.org/bugzilla/show_bug.cgi?id=21943
>>>
>> I opened it :)
>>
>>> i386 and x86-64 get_synthetic_symtab don't know if @plt should
>>> be added to symbol name for a PLT entry.  The first pass checks
>>> if @plt is needed and extra space is allocated in the second pass.
>>> We can assume @plt is needed and waste some space if it isn't.
>> Do you plan to fix it?
>>
> Done.
>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Synthetic symbol leak in in elf_x86_64_get_synthetic_symtab and elf_read_minimal_symbols
  2017-08-11 21:20         ` Alex Lindsay
@ 2017-08-17 11:00           ` Yao Qi
  2017-08-17 12:31             ` Philippe Waroquiers
  0 siblings, 1 reply; 11+ messages in thread
From: Yao Qi @ 2017-08-17 11:00 UTC (permalink / raw)
  To: Alex Lindsay; +Cc: H.J. Lu, GDB

Alex Lindsay <alexlindsay239@gmail.com> writes:

> I can verify that the objdump example is fixed in HEAD, but I still
> get this leak with `valgrind --leak-check=full
> --show-leak-kinds=definite gdb ./hello`:

Yes, because your patch is not pushed in yet :)  I tweaked your patch a
little bit, and pushed it in.  Again, thanks for your contribution.  Do
you still see other memory leak issues after this fix?

-- 
Yao (齐尧)

From ba7139188c75a9c620cadea59158c5ffcab28acf Mon Sep 17 00:00:00 2001
From: Alex Lindsay <alexlindsay239@gmail.com>
Date: Thu, 17 Aug 2017 11:53:53 +0100
Subject: [PATCH] Synthetic symbol leak in elf_read_minimal_symbols

Detected this leak with valgrind memcheck:

==30840== 194 bytes in 1 blocks are definitely lost in loss record 9,138 of 10,922
==30840==    at 0x4C2DB8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==30840==    by 0x80DF82: bfd_malloc (libbfd.c:193)
==30840==    by 0x80E12D: bfd_zmalloc (libbfd.c:278)
==30840==    by 0x819E80: elf_x86_64_get_synthetic_symtab (elf64-x86-64.c:6835)
==30840==    by 0x4F7B01: elf_read_minimal_symbols(objfile*, int, elfinfo const*) (elfread.c:1124)
==30840==    by 0x4F7CE7: elf_symfile_read(objfile*, enum_flags<symfile_add_flag>) (elfread.c:1182)
==30840==    by 0x7557FC: read_symbols(objfile*, enum_flags<symfile_add_flag>) (symfile.c:861)
==30840==    by 0x755EE1: syms_from_objfile_1(objfile*, section_addr_info*, enum_flags<symfile_add_flag>) (symfile.c:1062)

We perform a dynamic allocation in
elf64-x86-64.c:elf_x86_64_get_synthetic_symtab

  s = *ret = (asymbol *) bfd_zmalloc (size);

that appear to never get freed.

gdb:

2017-08-17  Alex Lindsay  <alexlindsay239@gmail.com>

	* elfread.c (elf_read_minimal_symbols): xfree synthsyms.

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 10d63b0..d2c194e 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,7 @@
+2017-08-17  Alex Lindsay  <alexlindsay239@gmail.com>  (tiny change)
+
+	* elfread.c (elf_read_minimal_symbols): xfree synthsyms.
+
 2017-08-17  Ruslan Kabatsayev  <b7.10110111@gmail.com>
 
 	* NEWS: Mention new shortcuts for nexti and stepi in TUI
diff --git a/gdb/elfread.c b/gdb/elfread.c
index ece704c..a654661 100644
--- a/gdb/elfread.c
+++ b/gdb/elfread.c
@@ -1132,6 +1132,9 @@ elf_read_minimal_symbols (struct objfile *objfile, int symfile_flags,
 	synth_symbol_table[i] = synthsyms + i;
       elf_symtab_read (reader, objfile, ST_SYNTHETIC, synthcount,
 		       synth_symbol_table.get (), true);
+
+      xfree (synthsyms);
+      synthsyms = NULL;
     }
 
   /* Install any minimal symbols that have been collected as the current

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Synthetic symbol leak in in elf_x86_64_get_synthetic_symtab and elf_read_minimal_symbols
  2017-08-17 11:00           ` Yao Qi
@ 2017-08-17 12:31             ` Philippe Waroquiers
  2017-08-17 17:42               ` Pedro Alves
  0 siblings, 1 reply; 11+ messages in thread
From: Philippe Waroquiers @ 2017-08-17 12:31 UTC (permalink / raw)
  To: Yao Qi; +Cc: Alex Lindsay, H.J. Lu, GDB

[-- Attachment #1: Type: text/plain, Size: 4329 bytes --]

On Thu, 2017-08-17 at 11:59 +0100, Yao Qi wrote:
> Alex Lindsay <alexlindsay239@gmail.com> writes:
> 
> > I can verify that the objdump example is fixed in HEAD, but I still
> > get this leak with `valgrind --leak-check=full
> > --show-leak-kinds=definite gdb ./hello`:
> 
> Yes, because your patch is not pushed in yet :)  I tweaked your patch a
> little bit, and pushed it in.  Again, thanks for your contribution.  Do
> you still see other memory leak issues after this fix?
> 
I still see significant leaks with 8.0.50.20170817-git
Below a mail I sent to Pedro about this (IIUC, he last modified
 dwarf_decode_line_header memory management, which is the
biggest leak I observed).

Attached the new leaks.

Philippe


-------------------
Hello Pedro,

Following some discussions on gdb patches, I wanted to run
the gdb test suite under Valgrind, just in case it would leak.
But before that, I did a small manual test, and with that,
Valgrind already reports a significant leak, see below.
So, it might be a little bit too early for me to
run the full test suite :).


The test consists in running a very small C executable,
that does a loop of malloc calls:
#include <stdlib.h>

static void test()
  {
    void* leak __attribute__((unused));
    int i;
    for (i = 0; i < 1000; i++)
       leak = (void*)malloc( 1 );
  }
  int main()
  {
    test();
    return 0;
  }


I step inside this small executable to execute a few
malloc calls, then do:
  thread apply all bt full
  info var .*
  info func .*
  x /s leak
  continue
  quit

The biggest leak is as below.
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);
)

==31555== 1,102,606 (124,592 direct, 978,014 indirect) bytes in 1,198 blocks are definitely lost in loss record 12,252 of 12,262
==31555==    at 0x4C28215: operator new(unsigned long) (vg_replace_malloc.c:334)
==31555==    by 0x589266: dwarf_decode_line_header(sect_offset, dwarf2_cu*) (dwarf2read.c:17916)
==31555==    by 0x58BD4C: handle_DW_AT_stmt_list (dwarf2read.c:9362)
==31555==    by 0x58BD4C: read_file_scope (dwarf2read.c:9440)
==31555==    by 0x58BD4C: process_die(die_info*, dwarf2_cu*) (dwarf2read.c:8503)
==31555==    by 0x58FC97: process_full_comp_unit (dwarf2read.c:8289)
==31555==    by 0x58FC97: process_queue (dwarf2read.c:7823)
==31555==    by 0x58FC97: dw2_do_instantiate_symtab(dwarf2_per_cu_data*) (dwarf2read.c:2928)
==31555==    by 0x590D85: dwarf2_read_symtab(partial_symtab*, objfile*) (dwarf2read.c:7689)
==31555==    by 0x619E66: psymtab_to_symtab(objfile*, partial_symtab*) (psymtab.c:775)
==31555==    by 0x61BC8C: psym_expand_symtabs_matching(objfile*, gdb::function_view<bool (char const*, bool)>, gdb::function_view<bool (char const*)>, gdb::function_view<void (compunit_symtab*)>, search_domain) (psymtab.c:1431)
==31555==    by 0x64B7F5: expand_symtabs_matching(gdb::function_view<bool (char const*, bool)>, gdb::function_view<bool (char const*)>, gdb::function_view<void (compunit_symtab*)>, search_domain) (symfile.c:3834)
==31555==    by 0x654E07: search_symbols(char const*, search_domain, int, char const**, symbol_search**) (symtab.c:4307)
==31555==    by 0x655B9A: symtab_symbol_info(char*, search_domain, int) [clone .isra.57] (symtab.c:4557)
==31555==    by 0x493808: cmd_func(cmd_list_element*, char*, int) (cli-decode.c:1902)
==31555==    by 0x677AF5: execute_command(char*, int) (top.c:650)

(there are more than 350 definely or possibly loss records in total with this small test,
I am attaching the full leak report).

Thanks

Philippe

Side question: is there a systematic run of gdb test suite under Valgrind (or similar tool) ?
Once I (more or less) master what is happening, I might setup a night run of gdb test suite
under Valgrind, but that might only be useful if gdb is relatively 'memcheck leak/error' free.
As far as I can see, GDB 7.12 was quite 'leak-free' (the same test only leaked a few blocks)
so it seems we can have significant regressions in that area.


[-- Attachment #2: leaks.out.xz --]
[-- Type: application/x-xz, Size: 41540 bytes --]

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Synthetic symbol leak in in elf_x86_64_get_synthetic_symtab and elf_read_minimal_symbols
  2017-08-17 12:31             ` Philippe Waroquiers
@ 2017-08-17 17:42               ` Pedro Alves
  2017-08-17 22:32                 ` [PATCH] Plug line_header leaks (Re: Synthetic symbol leak in in elf_x86_64_get_synthetic_symtab and elf_read_minimal_symbols) Pedro Alves
  0 siblings, 1 reply; 11+ messages in thread
From: Pedro Alves @ 2017-08-17 17:42 UTC (permalink / raw)
  To: Philippe Waroquiers, Yao Qi; +Cc: Alex Lindsay, H.J. Lu, GDB

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.

Thanks,
Pedro Alves

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH] Plug line_header leaks (Re: Synthetic symbol leak in in elf_x86_64_get_synthetic_symtab and elf_read_minimal_symbols)
  2017-08-17 17:42               ` Pedro Alves
@ 2017-08-17 22:32                 ` Pedro Alves
  0 siblings, 0 replies; 11+ messages in thread
From: Pedro Alves @ 2017-08-17 22:32 UTC (permalink / raw)
  To: Philippe Waroquiers, Yao Qi; +Cc: Alex Lindsay, H.J. Lu, GDB

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);
 }
 \f
 /* 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


^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2017-08-17 22:32 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-08-07 15:19 Synthetic symbol leak in in elf_x86_64_get_synthetic_symtab and elf_read_minimal_symbols Alex Lindsay
2017-08-11  9:27 ` Yao Qi
2017-08-11 15:07   ` Alex Lindsay
2017-08-11 15:31   ` H.J. Lu
2017-08-11 15:46     ` Yao Qi
2017-08-11 16:44       ` H.J. Lu
2017-08-11 21:20         ` Alex Lindsay
2017-08-17 11:00           ` Yao Qi
2017-08-17 12:31             ` Philippe Waroquiers
2017-08-17 17:42               ` Pedro Alves
2017-08-17 22:32                 ` [PATCH] Plug line_header leaks (Re: Synthetic symbol leak in in elf_x86_64_get_synthetic_symtab and elf_read_minimal_symbols) Pedro Alves

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).