[RFA] Fix leak in the dwarf reader

[RFA] Fix leak in the dwarf reader

[Philippe Waroquiers]

Valgrind reports a leak in the dwarf reader (see details below).
The function dw2_get_file_names_reader is interning in the per_objfile
all the file names it finds, except the name of 'fnd file name and directory'.
Instead, it was xstrdup-ing the name.
Fix the leaks by also interning the name.

This was validated running the tests natively, and under valgrind.
Leaks have decreased as mentionned below.
Valgrind detected no error such as double free or use after free.

Stack trace of the leak:
==4113266== 490,735 bytes in 17,500 blocks are definitely lost in loss record 7,061 of 7,074
==4113266==    at 0x483979B: malloc (vg_replace_malloc.c:393)
==4113266==    by 0x25A454: xmalloc (alloc.c:57)
==4113266==    by 0x7D1E1E: xstrdup (xstrdup.c:34)
==4113266==    by 0x39D141: dw2_get_file_names_reader (read.c:2825)
==4113266==    by 0x39D141: dw2_get_file_names(dwarf2_per_cu_data*, dwarf2_per_objfile*) (read.c:2851)
==4113266==    by 0x39DD6C: dw_expand_symtabs_matching_file_matcher(dwarf2_per_objfile*, gdb::function_view<bool (char const*, bool)>) (read.c:4149)
==4113266==    by 0x3BC8B5: cooked_index_functions::expand_symtabs_matching(objfile*, gdb::function_view<bool (char const*, bool)>, lookup_name_info const*, gdb::function_view<bool (char const*)>, gdb::function_view<bool (compunit_symtab*)>, enum_flags<block_search_flag_values>, domain_enum, search_domain) (read.c:18688)
==4113266==    by 0x5DD1EA: objfile::map_symtabs_matching_filename(char const*, char const*, gdb::function_view<bool (symtab*)>) (symfile-debug.c:207)
==4113266==    by 0x5F04CC: iterate_over_symtabs(char const*, gdb::function_view<bool (symtab*)>) (symtab.c:633)
==4113266==    by 0x477EE3: collect_symtabs_from_filename(char const*, program_space*) (linespec.c:3712)
==4113266==    by 0x477FC1: symtabs_from_filename(char const*, program_space*) (linespec.c:3726)
==4113266==    by 0x47A9B8: convert_explicit_location_spec_to_linespec(linespec_state*, linespec*, char const*, char const*, symbol_name_match_type, char const*, line_offset) (linespec.c:2329)
==4113266==    by 0x47E86E: convert_explicit_location_spec_to_sals (linespec.c:2388)
==4113266==    by 0x47E86E: location_spec_to_sals(linespec_parser*, location_spec const*) (linespec.c:3104)
==4113266==    by 0x47EDAC: decode_line_full(location_spec*, int, program_space*, symtab*, int, linespec_result*, char const*, char const*) (linespec.c:3149)
...

Without the fix, the top 10 leaks are:
./gdb/testsuite/outputs/gdb.base/condbreak-bad/gdb.log:345:==3213924==    definitely lost: 130,937 bytes in 5,409 blocks
./gdb/testsuite/outputs/gdb.base/hbreak2/gdb.log:619:==3758919==    definitely lost: 173,323 bytes in 7,204 blocks
./gdb/testsuite/outputs/gdb.mi/mi-var-cp/gdb.log:1320:==4152873==    definitely lost: 172,826 bytes in 7,207 blocks
./gdb/testsuite/outputs/gdb.base/advance-until-multiple-locations/gdb.log:398:==2992643==    definitely lost: 172,965 bytes in 7,211 blocks
./gdb/testsuite/outputs/gdb.mi/mi-var-cmd/gdb.log:2302:==4159476==    definitely lost: 173,129 bytes in 7,211 blocks
./gdb/testsuite/outputs/gdb.cp/gdb2384/gdb.log:222:==3811851==    definitely lost: 218,106 bytes in 7,761 blocks
./gdb/testsuite/outputs/gdb.cp/mb-templates/gdb.log:310:==3787344==    definitely lost: 290,311 bytes in 10,340 blocks
./gdb/testsuite/outputs/gdb.mi/mi-var-rtti/gdb.log:2568:==4158350==    definitely lost: 435,427 bytes in 15,507 blocks
./gdb/testsuite/outputs/gdb.mi/mi-catch-cpp-exceptions/gdb.log:1704:==4119722==    definitely lost: 435,405 bytes in 15,510 blocks
./gdb/testsuite/outputs/gdb.mi/mi-vla-fortran/gdb.log:768:==4113266==    definitely lost: 508,585 bytes in 18,109 blocks

With the fix:
./gdb/testsuite/outputs/gdb.base/fork-running-state/gdb.log:1536:==2924193==    indirectly lost: 13,848 bytes in 98 blocks
./gdb/testsuite/outputs/gdb.base/fork-running-state/gdb.log:1675:==2928777==    indirectly lost: 13,848 bytes in 98 blocks
./gdb/testsuite/outputs/gdb.python/py-inferior-leak/gdb.log:4729:==3353335==    definitely lost: 3,360 bytes in 140 blocks
./gdb/testsuite/outputs/gdb.base/kill-detach-inferiors-cmd/gdb.log:210:==2746927==    indirectly lost: 13,246 bytes in 154 blocks
./gdb/testsuite/outputs/gdb.base/inferior-clone/gdb.log:179:==3034984==    indirectly lost: 12,921 bytes in 161 blocks
./gdb/testsuite/outputs/gdb.base/interrupt-daemon/gdb.log:209:==3006248==    indirectly lost: 20,683 bytes in 174 blocks
./gdb/testsuite/outputs/gdb.threads/watchpoint-fork/gdb.log:714:==3512403==    indirectly lost: 20,707 bytes in 175 blocks
./gdb/testsuite/outputs/gdb.threads/watchpoint-fork/gdb.log:962:==3514498==    indirectly lost: 20,851 bytes in 178 blocks
./gdb/testsuite/outputs/gdb.base/multi-forks/gdb.log:336:==2585839==    indirectly lost: 53,630 bytes in 386 blocks
./gdb/testsuite/outputs/gdb.base/multi-forks/gdb.log:1338:==2592417==    indirectly lost: 100,008 bytes in 1,154 blocks
---
 gdb/dwarf2/read.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index 53a6a8b5e6d..87c0914d839 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -2822,7 +2822,7 @@ dw2_get_file_names_reader (const struct die_reader_specs *reader,
     XOBNEWVEC (&per_objfile->per_bfd->obstack, const char *,
 	       qfn->num_file_names);
   if (offset != 0)
-    qfn->file_names[0] = xstrdup (fnd.get_name ());
+    qfn->file_names[0] = per_objfile->objfile->intern (fnd.get_name ());
 
   if (!include_names.empty ())
     memcpy (&qfn->file_names[offset], include_names.data (),
-- 
2.30.2

Re: [RFA] Fix leak in the dwarf reader

[Simon Marchi]

On 11/27/22 07:54, Philippe Waroquiers via Gdb-patches wrote:
> Valgrind reports a leak in the dwarf reader (see details below).
> The function dw2_get_file_names_reader is interning in the per_objfile
> all the file names it finds, except the name of 'fnd file name and directory'.
> Instead, it was xstrdup-ing the name.
> Fix the leaks by also interning the name.
> 
> This was validated running the tests natively, and under valgrind.
> Leaks have decreased as mentionned below.
> Valgrind detected no error such as double free or use after free.

I manually checked the uses of this field, it indeed doesn't appear to
be freed, so:`

Approved-By: Simon Marchi <simon.marchi@efficios.com>

> 
> Stack trace of the leak:
> ==4113266== 490,735 bytes in 17,500 blocks are definitely lost in loss record 7,061 of 7,074
> ==4113266==    at 0x483979B: malloc (vg_replace_malloc.c:393)
> ==4113266==    by 0x25A454: xmalloc (alloc.c:57)
> ==4113266==    by 0x7D1E1E: xstrdup (xstrdup.c:34)
> ==4113266==    by 0x39D141: dw2_get_file_names_reader (read.c:2825)
> ==4113266==    by 0x39D141: dw2_get_file_names(dwarf2_per_cu_data*, dwarf2_per_objfile*) (read.c:2851)
> ==4113266==    by 0x39DD6C: dw_expand_symtabs_matching_file_matcher(dwarf2_per_objfile*, gdb::function_view<bool (char const*, bool)>) (read.c:4149)
> ==4113266==    by 0x3BC8B5: cooked_index_functions::expand_symtabs_matching(objfile*, gdb::function_view<bool (char const*, bool)>, lookup_name_info const*, gdb::function_view<bool (char const*)>, gdb::function_view<bool (compunit_symtab*)>, enum_flags<block_search_flag_values>, domain_enum, search_domain) (read.c:18688)
> ==4113266==    by 0x5DD1EA: objfile::map_symtabs_matching_filename(char const*, char const*, gdb::function_view<bool (symtab*)>) (symfile-debug.c:207)
> ==4113266==    by 0x5F04CC: iterate_over_symtabs(char const*, gdb::function_view<bool (symtab*)>) (symtab.c:633)
> ==4113266==    by 0x477EE3: collect_symtabs_from_filename(char const*, program_space*) (linespec.c:3712)
> ==4113266==    by 0x477FC1: symtabs_from_filename(char const*, program_space*) (linespec.c:3726)
> ==4113266==    by 0x47A9B8: convert_explicit_location_spec_to_linespec(linespec_state*, linespec*, char const*, char const*, symbol_name_match_type, char const*, line_offset) (linespec.c:2329)
> ==4113266==    by 0x47E86E: convert_explicit_location_spec_to_sals (linespec.c:2388)
> ==4113266==    by 0x47E86E: location_spec_to_sals(linespec_parser*, location_spec const*) (linespec.c:3104)
> ==4113266==    by 0x47EDAC: decode_line_full(location_spec*, int, program_space*, symtab*, int, linespec_result*, char const*, char const*) (linespec.c:3149)
> ...
> 
> Without the fix, the top 10 leaks are:
> ./gdb/testsuite/outputs/gdb.base/condbreak-bad/gdb.log:345:==3213924==    definitely lost: 130,937 bytes in 5,409 blocks
> ./gdb/testsuite/outputs/gdb.base/hbreak2/gdb.log:619:==3758919==    definitely lost: 173,323 bytes in 7,204 blocks
> ./gdb/testsuite/outputs/gdb.mi/mi-var-cp/gdb.log:1320:==4152873==    definitely lost: 172,826 bytes in 7,207 blocks
> ./gdb/testsuite/outputs/gdb.base/advance-until-multiple-locations/gdb.log:398:==2992643==    definitely lost: 172,965 bytes in 7,211 blocks
> ./gdb/testsuite/outputs/gdb.mi/mi-var-cmd/gdb.log:2302:==4159476==    definitely lost: 173,129 bytes in 7,211 blocks
> ./gdb/testsuite/outputs/gdb.cp/gdb2384/gdb.log:222:==3811851==    definitely lost: 218,106 bytes in 7,761 blocks
> ./gdb/testsuite/outputs/gdb.cp/mb-templates/gdb.log:310:==3787344==    definitely lost: 290,311 bytes in 10,340 blocks
> ./gdb/testsuite/outputs/gdb.mi/mi-var-rtti/gdb.log:2568:==4158350==    definitely lost: 435,427 bytes in 15,507 blocks
> ./gdb/testsuite/outputs/gdb.mi/mi-catch-cpp-exceptions/gdb.log:1704:==4119722==    definitely lost: 435,405 bytes in 15,510 blocks
> ./gdb/testsuite/outputs/gdb.mi/mi-vla-fortran/gdb.log:768:==4113266==    definitely lost: 508,585 bytes in 18,109 blocks
> 
> With the fix:
> ./gdb/testsuite/outputs/gdb.base/fork-running-state/gdb.log:1536:==2924193==    indirectly lost: 13,848 bytes in 98 blocks
> ./gdb/testsuite/outputs/gdb.base/fork-running-state/gdb.log:1675:==2928777==    indirectly lost: 13,848 bytes in 98 blocks
> ./gdb/testsuite/outputs/gdb.python/py-inferior-leak/gdb.log:4729:==3353335==    definitely lost: 3,360 bytes in 140 blocks
> ./gdb/testsuite/outputs/gdb.base/kill-detach-inferiors-cmd/gdb.log:210:==2746927==    indirectly lost: 13,246 bytes in 154 blocks
> ./gdb/testsuite/outputs/gdb.base/inferior-clone/gdb.log:179:==3034984==    indirectly lost: 12,921 bytes in 161 blocks
> ./gdb/testsuite/outputs/gdb.base/interrupt-daemon/gdb.log:209:==3006248==    indirectly lost: 20,683 bytes in 174 blocks
> ./gdb/testsuite/outputs/gdb.threads/watchpoint-fork/gdb.log:714:==3512403==    indirectly lost: 20,707 bytes in 175 blocks
> ./gdb/testsuite/outputs/gdb.threads/watchpoint-fork/gdb.log:962:==3514498==    indirectly lost: 20,851 bytes in 178 blocks
> ./gdb/testsuite/outputs/gdb.base/multi-forks/gdb.log:336:==2585839==    indirectly lost: 53,630 bytes in 386 blocks
> ./gdb/testsuite/outputs/gdb.base/multi-forks/gdb.log:1338:==2592417==    indirectly lost: 100,008 bytes in 1,154 blocks

Thanks for checking this, I've been wanting to fix reported leaks for a
really long time, to make any new leak more obvious.  But it's always a
really low prio task.

Simon

Re: [RFA] Fix leak in the dwarf reader

[Philippe Waroquiers]

On Sun, 2022-11-27 at 13:48 -0500, Simon Marchi wrote:
> 
> On 11/27/22 07:54, Philippe Waroquiers via Gdb-patches wrote:
> > Valgrind reports a leak in the dwarf reader (see details below).
> > The function dw2_get_file_names_reader is interning in the per_objfile
> > all the file names it finds, except the name of 'fnd file name and directory'.
> > Instead, it was xstrdup-ing the name.
> > Fix the leaks by also interning the name.
> > 
> > This was validated running the tests natively, and under valgrind.
> > Leaks have decreased as mentionned below.
> > Valgrind detected no error such as double free or use after free.
> 
> I manually checked the uses of this field, it indeed doesn't appear to
> be freed, so:`
> 
> Approved-By: Simon Marchi <simon.marchi@efficios.com>
Thanks for the review, pushed with the approved tag.