| Message ID | 20181124115237.8943-1-philippe.waroquiers@skynet.be |
|---|---|
| State | New, archived |
| Headers |
Received: (qmail 79903 invoked by alias); 24 Nov 2018 11:52:49 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <gdb-patches.sourceware.org> List-Unsubscribe: <mailto:gdb-patches-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:gdb-patches-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 79890 invoked by uid 89); 24 Nov 2018 11:52:48 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-27.6 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_LOW, SPF_PASS autolearn=ham version=3.3.2 spammy=transfer, HContent-Transfer-Encoding:8bit X-HELO: mailsec112.isp.belgacom.be Received: from mailsec112.isp.belgacom.be (HELO mailsec112.isp.belgacom.be) (195.238.20.108) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Sat, 24 Nov 2018 11:52:45 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=skynet.be; i=@skynet.be; q=dns/txt; s=securemail; t=1543060366; x=1574596366; h=from:to:cc:subject:date:message-id:mime-version: content-transfer-encoding; bh=y+ub4/JQSjUzQD2oDZ4GCYHS2e9iAlysxpiAfdoDofo=; b=a9AWhsp96UnIP4sOadoYJlT8ggcghArbWx1qFj+CZ1EXGr+2xIhqw7Lw luGOVG8TrU8SsEP3y7G+melJGSZbsA==; Received: from 184.205-67-87.adsl-dyn.isp.belgacom.be (HELO md.home) ([87.67.205.184]) by relay.skynet.be with ESMTP/TLS/DHE-RSA-AES128-GCM-SHA256; 24 Nov 2018 12:52:43 +0100 From: Philippe Waroquiers <philippe.waroquiers@skynet.be> To: gdb-patches@sourceware.org Cc: Philippe Waroquiers <philippe.waroquiers@skynet.be> Subject: [PUSHED/OBVIOUS] Re-fix leak in source.c (open_source_file). Date: Sat, 24 Nov 2018 12:52:37 +0100 Message-Id: <20181124115237.8943-1-philippe.waroquiers@skynet.be> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-IsSubscribed: yes |
Commit Message
Philippe Waroquiers
Nov. 24, 2018, 11:52 a.m. UTC
Leak fixed in '8e6a5953e1d Fix 4K leak in open_source_file' has been partially undone by '2179fbc36d23 Return scoped_fd from open_source_file'. Re-add the transfer of current s->fullname to the unique_xmalloc_ptr fullname given to find_and_open_source. --- gdb/ChangeLog | 8 ++++++++ gdb/source.c | 2 +- 2 files changed, 9 insertions(+), 1 deletion(-)
Comments
>>>>> "Philippe" == Philippe Waroquiers <philippe.waroquiers@skynet.be> writes:
Philippe> Leak fixed in '8e6a5953e1d Fix 4K leak in open_source_file' has been partially
Philippe> undone by '2179fbc36d23 Return scoped_fd from open_source_file'. Re-add the
Philippe> transfer of current s->fullname to the unique_xmalloc_ptr fullname given to
Philippe> find_and_open_source.
Sorry about that, and thank you for fixing it.
Tom
On Thu, 2018-11-29 at 11:12 -0700, Tom Tromey wrote: > > > > > > "Philippe" == Philippe Waroquiers <philippe.waroquiers@skynet.be> writes: > > Philippe> Leak fixed in '8e6a5953e1d Fix 4K leak in open_source_file' has been partially > Philippe> undone by '2179fbc36d23 Return scoped_fd from open_source_file'. Re-add the > Philippe> transfer of current s->fullname to the unique_xmalloc_ptr fullname given to > Philippe> find_and_open_source. > > Sorry about that, and thank you for fixing it. No problem. Note that after having run all GDB tests under valgrind, I still see some 4K leaks in open_source_file appearing (I suspect) when GDB reloads an executable that has changed. Philippe
>>>>> "Philippe" == Philippe Waroquiers <philippe.waroquiers@skynet.be> writes:
Philippe> Note that after having run all GDB tests under valgrind, I still
Philippe> see some 4K leaks in open_source_file appearing (I suspect)
Philippe> when GDB reloads an executable that has changed.
What are the leaks? I wonder if the "fullname"s in the psymtabs aren't
being freed.
I have been wondering lately if this field should be pulled out into
some separate hash table. This would save a little memory if there are
file names, which seems likely.
Tom
On Thu, 2018-11-29 at 16:29 -0700, Tom Tromey wrote: > > > > > > "Philippe" == Philippe Waroquiers <philippe.waroquiers@skynet.be> writes: > > Philippe> Note that after having run all GDB tests under valgrind, I still > Philippe> see some 4K leaks in open_source_file appearing (I suspect) > Philippe> when GDB reloads an executable that has changed. > > What are the leaks? I wonder if the "fullname"s in the psymtabs aren't > being freed. > > I have been wondering lately if this field should be pulled out into > some separate hash table. This would save a little memory if there are > file names, which seems likely. Yes, the 4K leak is the fullname (but fullname is freed now in all tests except in the 4 below tests, which all have something to do with reloading an executable/reload symbols ... Note gdb.ada/exec_changed also has a bad behaviour under valgrind: ==28802== Stack overflow in thread #1: can't grow stack to 0x1ffe801000 ==28802== ==28802== Process terminating with default action of signal 11 (SIGSEGV) ==28802== Access not within mapped region at address 0x1FFE801FE8 ==28802== Stack overflow in thread #1: can't grow stack to 0x1ffe801000 ==28802== at 0x5BF7DC8: __find_specmb (printf-parse.h:108) ==28802== by 0x5BF7DC8: vfprintf (vfprintf.c:1312) Not yet analysed ... grep -n -ie 'open_source_file' $(ls -rt $(find . -name '*valgrind.log')) ./testsuite/outputs/gdb.ada/exec_changed/valgrind.log:291:==28802== by 0x60ABFC: open_source_file(symtab*) (source.c:1074) ./testsuite/outputs/gdb.ada/exec_changed/valgrind.log:329: fun:_Z16open_source_fileP6symtab ./testsuite/outputs/gdb.base/chng-syms/valgrind.log:69:==12362== by 0x60ABFC: open_source_file(symtab*) (source.c:1074) ./testsuite/outputs/gdb.base/chng-syms/valgrind.log:98: fun:_Z16open_source_fileP6symtab ./testsuite/outputs/gdb.base/reread/valgrind.log:1885:==2245== by 0x60ABFC: open_source_file(symtab*) (source.c:1074) ./testsuite/outputs/gdb.base/reread/valgrind.log:1914: fun:_Z16open_source_fileP6symtab ./testsuite/outputs/gdb.dwarf2/gdb-index/valgrind.log:19:==13830== by 0x60ABFC: open_source_file(symtab*) (source.c:1074) ./testsuite/outputs/gdb.dwarf2/gdb-index/valgrind.log:57: fun:_Z16open_source_fileP6symtab chng-syms full output (other tests contains similar leaks) is: ==12362== Memcheck, a memory error detector ==12362== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al. ==12362== Using Valgrind-3.15.0.GIT and LibVEX; rerun with -h for copyright info ==12362== Command: /bd/home/philippe/gdb/git/build_obvious/gdb/gdb -nw -nx -data-directory /bd/home/philippe/gdb/git/build_obvious/gdb/testsuite/../data-directory ==12362== Parent PID: 12361 ==12362== ==12362== ==12362== HEAP SUMMARY: ==12362== in use at exit: 4,043,008 bytes in 4,040 blocks ==12362== total heap usage: 79,418 allocs, 75,378 frees, 99,134,863 bytes allocated ==12362== ==12362== VALGRIND_GDB_ERROR_BEGIN ==12362== 88 bytes in 1 blocks are definitely lost in loss record 829 of 3,086 ==12362== at 0x4C2E273: realloc (vg_replace_malloc.c:826) ==12362== by 0x41B58C: xrealloc (common-utils.c:62) ==12362== by 0x60AF20: find_source_lines(symtab*, int) (source.c:1201) ==12362== by 0x60B41B: print_source_lines_base(symtab*, int, int, enum_flags<print_source_lines_flag>) (source.c:1347) ==12362== by 0x616AAF: print_frame_info(frame_info*, int, print_what, int, int) (stack.c:911) ==12362== by 0x616C15: print_stack_frame(frame_info*, int, print_what, int) (stack.c:181) ==12362== by 0x51235E: print_stop_location (infrun.c:7987) ==12362== by 0x51235E: print_stop_event(ui_out*) (infrun.c:8004) ==12362== by 0x40DC9D: cli_on_normal_stop(bpstats*, int) (cli-interp.c:145) ==12362== by 0x512A09: operator() (functional:2127) ==12362== by 0x512A09: notify (observable.h:106) ==12362== by 0x512A09: normal_stop() (infrun.c:8277) ==12362== by 0x515FB0: fetch_inferior_event(void*) (infrun.c:3907) ==12362== by 0x4B388C: gdb_wait_for_event(int) (event-loop.c:859) ==12362== by 0x4B3996: gdb_do_one_event() [clone .part.4] (event-loop.c:322) ==12362== by 0x4B3B54: gdb_do_one_event (common-exceptions.h:219) ==12362== by 0x4B3B54: start_event_loop() (event-loop.c:371) ==12362== by 0x551BD7: captured_command_loop() (main.c:330) ==12362== by 0x552BCC: captured_main (main.c:1177) ==12362== by 0x552BCC: gdb_main(captured_main_args*) (main.c:1193) ==12362== by 0x29B317: main (gdb.c:32) ==12362== ==12362== VALGRIND_GDB_ERROR_END { <insert_a_suppression_name_here> Memcheck:Leak match-leak-kinds: definite fun:realloc fun:xrealloc fun:_Z17find_source_linesP6symtabi fun:_ZL23print_source_lines_baseP6symtabii10enum_flagsI23print_source_lines_flagE fun:_Z16print_frame_infoP10frame_infoi10print_whatii fun:_Z17print_stack_frameP10frame_infoi10print_whati fun:print_stop_location fun:_Z16print_stop_eventP6ui_out fun:_ZL18cli_on_normal_stopP7bpstatsi fun:operator() fun:notify fun:_Z11normal_stopv fun:_Z20fetch_inferior_eventPv fun:_ZL18gdb_wait_for_eventi fun:_Z16gdb_do_one_eventv.part.4 fun:gdb_do_one_event fun:_Z16start_event_loopv fun:_ZL21captured_command_loopv fun:captured_main fun:_Z8gdb_mainP18captured_main_args fun:main } ==12362== VALGRIND_GDB_ERROR_BEGIN ==12362== 4,096 bytes in 1 blocks are definitely lost in loss record 2,999 of 3,086 ==12362== at 0x4C2BE2D: malloc (vg_replace_malloc.c:299) ==12362== by 0x5BF0987: realpath@@GLIBC_2.3 (canonicalize.c:78) ==12362== by 0x41F643: gdb_realpath(char const*) (pathstuff.c:72) ==12362== by 0x60A98F: find_and_open_source(char const*, char const*, std::unique_ptr<char, gdb::xfree_deleter<char> >*) (source.c:995) ==12362== by 0x60ABFC: open_source_file(symtab*) (source.c:1074) ==12362== by 0x60B028: print_source_lines_base(symtab*, int, int, enum_flags<print_source_lines_flag>) (source.c:1283) ==12362== by 0x616AAF: print_frame_info(frame_info*, int, print_what, int, int) (stack.c:911) ==12362== by 0x616C15: print_stack_frame(frame_info*, int, print_what, int) (stack.c:181) ==12362== by 0x51235E: print_stop_location (infrun.c:7987) ==12362== by 0x51235E: print_stop_event(ui_out*) (infrun.c:8004) ==12362== by 0x40DC9D: cli_on_normal_stop(bpstats*, int) (cli-interp.c:145) ==12362== by 0x512A09: operator() (functional:2127) ==12362== by 0x512A09: notify (observable.h:106) ==12362== by 0x512A09: normal_stop() (infrun.c:8277) ==12362== by 0x515FB0: fetch_inferior_event(void*) (infrun.c:3907) ==12362== by 0x4B388C: gdb_wait_for_event(int) (event-loop.c:859) ==12362== by 0x4B3996: gdb_do_one_event() [clone .part.4] (event-loop.c:322) ==12362== by 0x4B3B54: gdb_do_one_event (common-exceptions.h:219) ==12362== by 0x4B3B54: start_event_loop() (event-loop.c:371) ==12362== by 0x551BD7: captured_command_loop() (main.c:330) ==12362== by 0x552BCC: captured_main (main.c:1177) ==12362== by 0x552BCC: gdb_main(captured_main_args*) (main.c:1193) ==12362== by 0x29B317: main (gdb.c:32) ==12362== ==12362== VALGRIND_GDB_ERROR_END { <insert_a_suppression_name_here> Memcheck:Leak match-leak-kinds: definite fun:malloc fun:realpath@@GLIBC_2.3 fun:_Z12gdb_realpathPKc fun:_Z20find_and_open_sourcePKcS0_PSt10unique_ptrIcN3gdb13xfree_deleterIcEEE fun:_Z16open_source_fileP6symtab fun:_ZL23print_source_lines_baseP6symtabii10enum_flagsI23print_source_lines_flagE fun:_Z16print_frame_infoP10frame_infoi10print_whatii fun:_Z17print_stack_frameP10frame_infoi10print_whati fun:print_stop_location fun:_Z16print_stop_eventP6ui_out fun:_ZL18cli_on_normal_stopP7bpstatsi fun:operator() fun:notify fun:_Z11normal_stopv fun:_Z20fetch_inferior_eventPv fun:_ZL18gdb_wait_for_eventi fun:_Z16gdb_do_one_eventv.part.4 fun:gdb_do_one_event fun:_Z16start_event_loopv fun:_ZL21captured_command_loopv fun:captured_main fun:_Z8gdb_mainP18captured_main_args fun:main } ==12362== LEAK SUMMARY: ==12362== definitely lost: 4,184 bytes in 2 blocks ==12362== indirectly lost: 0 bytes in 0 blocks ==12362== possibly lost: 143,232 bytes in 164 blocks ==12362== still reachable: 3,895,464 bytes in 3,867 blocks ==12362== suppressed: 128 bytes in 7 blocks ==12362== Reachable blocks (those to which a pointer was found) are not shown. ==12362== To see them, rerun with: --leak-check=full --show-leak-kinds=all ==12362== ==12362== For counts of detected and suppressed errors, rerun with: -v ==12362== ERROR SUMMARY: 67 errors from 67 contexts (suppressed: 1476 from 235)
>>>>> "Philippe" == Philippe Waroquiers <philippe.waroquiers@skynet.be> writes: >> What are the leaks? I wonder if the "fullname"s in the psymtabs aren't >> being freed. Philippe> Yes, the 4K leak is the fullname (but fullname is freed now in all tests Philippe> except in the 4 below tests, which all have something to do with reloading Philippe> an executable/reload symbols ... I thought I sent a reply to this yesterday but I don't see it now. My first guess is that reread_symbols isn't calling forget_cached_source_info_for_objfile. Ages ago Jan had a patch to make reread_symbols just do a delete+new for the objfile, rather than trying to do a brain transplant on the object. That still seems like a good idea, for this sort of reason. However, once my psymtab series lands, it seems nicer to just move this logic into the psymtab holder object. This should fix the bug as well. Tom
diff --git a/gdb/ChangeLog b/gdb/ChangeLog index 90e8d7a5c6..2515a4d0b8 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,3 +1,11 @@ +2018-11-24 Philippe Waroquiers <philippe.waroquiers@skynet.be> + + * source.c (open_source_file): Leak fixed in '8e6a5953e1d Fix 4K + leak in open_source_file' has been partially undone by '2179fbc36d23 + Return scoped_fd from open_source_file'. Re-add the transfer of + current s->fullname to the unique_xmalloc_ptr fullname given + to find_and_open_source. + 2018-11-23 Pedro Alves <palves@redhat.com> * gdbthread.h (enum thread_state): Move comments here. diff --git a/gdb/source.c b/gdb/source.c index b38eed5be6..e295fbf49e 100644 --- a/gdb/source.c +++ b/gdb/source.c @@ -1068,7 +1068,7 @@ open_source_file (struct symtab *s) if (!s) return scoped_fd (-1); - gdb::unique_xmalloc_ptr<char> fullname; + gdb::unique_xmalloc_ptr<char> fullname (s->fullname); s->fullname = NULL; scoped_fd fd = find_and_open_source (s->filename, SYMTAB_DIRNAME (s), &fullname);