diff mbox

[PUSHED/OBVIOUS] Re-fix leak in source.c (open_source_file).

Message ID 20181124115237.8943-1-philippe.waroquiers@skynet.be
State New
Headers show

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

Tom Tromey Nov. 29, 2018, 6:12 p.m. UTC | #1
>>>>> "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
Philippe Waroquiers Nov. 29, 2018, 11:14 p.m. UTC | #2
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
Tom Tromey Nov. 29, 2018, 11:29 p.m. UTC | #3
>>>>> "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
Philippe Waroquiers Nov. 29, 2018, 11:45 p.m. UTC | #4
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)
Tom Tromey Nov. 30, 2018, 4:39 p.m. UTC | #5
>>>>> "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 mbox

Patch

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);