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