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

[Philippe Waroquiers]

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

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 90e8d7a5c6..2515a4d0b8 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,11 @@
[phone] 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 <***@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);

[Tom Tromey]

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]

Post by Tom Tromey
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]

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]

Post by Tom Tromey
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]

Post by Tom Tromey
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