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