diff mbox series

[RFA] Fix leak in the dwarf reader

Message ID 20221127125413.2769629-1-philippe.waroquiers@skynet.be
State Committed
Commit 9d69cd24db44f99d96c9b2aa1db771228c4ff118
Headers
Series [RFA] Fix leak in the dwarf reader |

Commit Message

Philippe Waroquiers Nov. 27, 2022, 12:54 p.m. UTC 
  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(-)

Comments

Simon Marchi Nov. 27, 2022, 6:48 p.m. UTC | #1 
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
Philippe Waroquiers Nov. 27, 2022, 8:19 p.m. UTC | #2 
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.
diff mbox series

Patch

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