Re: [PATCH 2/2] Free multidicts from blockvector

Simon Marchi
2025-10-23T18:32:21Z

On 10/23/25 1:17 PM, Tom Tromey wrote:
> Currently, nothing in the tree ever calls mdict_free. However, code
> does heap-allocate some multidicts. A simple way to see this is to
> use valgrind, run "gdb -readnow" on the executable created by
> gdb.dwarf2/struct-with-sig.exp, and then use "file" to clear the
> objfile list. This yields:
>
> ==1522843== 144 (16 direct, 128 indirect) bytes in 1 blocks are definitely lost in loss record 905 of 3,005
> ==1522843== at 0x4843866: malloc (vg_replace_malloc.c:446)
> ==1522843== by 0x48E397: xmalloc (alloc.c:52)
> ==1522843== by 0x59DE66: multidictionary* xnew<multidictionary>() (poison.h:102)
> ==1522843== by 0x59CFF4: mdict_create_hashed_expandable(language) (dictionary.c:965)
> ==1522843== by 0x50A269: buildsym_compunit::finish_block_internal(symbol*, pending**, pending_block*, dynamic_prop const*, unsigned long, unsigned long, int, int) (buildsym.c:221)
> ==1522843== by 0x50AE04: buildsym_compunit::end_compunit_symtab_get_static_block(unsigned long, int, int) (buildsym.c:818)
> ==1522843== by 0x50C4CF: buildsym_compunit::end_expandable_symtab(unsigned long) (buildsym.c:1037)
> ==1522843== by 0x61DBC6: process_full_type_unit (read.c:4970)
>
> This patch fixes the leaks by calling mdict_free when a blockvector is
> destroyed.
> ---
> gdb/block.c | 6 ++++++
> gdb/block.h | 2 ++
> 2 files changed, 8 insertions(+)
>
> diff --git a/gdb/block.c b/gdb/block.c
> index 02e6d0e4be2bab2c1b3750a2f396e4bc97b7bc5d..2d8d40ec4eb94a1c891a3f8b48972eff3050e66d 100644
> --- a/gdb/block.c
> +++ b/gdb/block.c
> @@ -842,6 +842,12 @@ blockvector::append_block (struct block *block)
> m_blocks.push_back (block);
> }
>
> +blockvector::~blockvector ()
> +{
> + for (struct block *bl : m_blocks)
> + mdict_free (bl->multidict ());
> +}

Ideally it would be struct block's just to call that. You could add a
destructor to struct block and call it explicitly here. It would be the
same result, but would be more correct encapsulation-wise. But I guess
this is fine too for the time being.

> +
> /* Implement 'maint info blocks' command. If passed an argument then
> print a list of all blocks at the given address. With no arguments
> then list all blocks at the current address of the current inferior. */
> diff --git a/gdb/block.h b/gdb/block.h
> index 61070619ca7daa7b3310a9c5cd8d6ca86c044ada..98ce5a3839dbb8419c9e3a918d27b3b58485fea4 100644
> --- a/gdb/block.h
> +++ b/gdb/block.h
> @@ -424,6 +424,8 @@ struct blockvector
> : m_blocks (nblocks, nullptr)
> {}
>
> + ~blockvector ();

Since there is a user-defined destructor, I would add a
DISABLE_COPY_AND_ASSIGN. Otherwise, LGTM.

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

Simon