From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 90392 invoked by alias); 15 Jan 2019 16:50:36 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 90381 invoked by uid 89); 15 Jan 2019 16:50:35 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.2 spammy= X-HELO: smtp.polymtl.ca Received: from smtp.polymtl.ca (HELO smtp.polymtl.ca) (132.207.4.11) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 15 Jan 2019 16:50:33 +0000 Received: from simark.ca (simark.ca [158.69.221.121]) (authenticated bits=0) by smtp.polymtl.ca (8.14.7/8.14.7) with ESMTP id x0FGoPa9004515 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT) for ; Tue, 15 Jan 2019 11:50:31 -0500 Received: by simark.ca (Postfix, from userid 112) id A55A21E7BB; Tue, 15 Jan 2019 11:50:25 -0500 (EST) Received: from simark.ca (localhost [127.0.0.1]) by simark.ca (Postfix) with ESMTP id F0FCE1E4C2; Tue, 15 Jan 2019 11:50:23 -0500 (EST) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7bit Date: Tue, 15 Jan 2019 16:50:00 -0000 From: Simon Marchi To: Philippe Waroquiers Cc: gdb-patches@sourceware.org Subject: Re: [RFA] Fix leaks in macro definitions. In-Reply-To: <20190115055611.17967-1-philippe.waroquiers@skynet.be> References: <20190115055611.17967-1-philippe.waroquiers@skynet.be> Message-ID: <63b6768fbedbe46b7a46f0d29f7b178c@polymtl.ca> X-Sender: simon.marchi@polymtl.ca User-Agent: Roundcube Webmail/1.3.6 X-IsSubscribed: yes X-SW-Source: 2019-01/txt/msg00347.txt.bz2 On 2019-01-15 00:56, Philippe Waroquiers wrote: > Valgrind detects leaks like the following (gdb.base/macsp.exp). > This patch fixes the 3 leaks. > Tested on debian/amd64, natively and under valgrind. > > ==22285== 64 (48 direct, 16 indirect) bytes in 1 blocks are definitely > lost in loss record 737 of 3,377 > ==22285== at 0x4C2BE6D: malloc (vg_replace_malloc.c:309) > ==22285== by 0x4049E7: xmalloc (common-utils.c:44) > ==22285== by 0x533A20: new_macro_key(macro_table*, char const*, > macro_source_file*, int) (macrotab.c:355) > ==22285== by 0x53438B: macro_define_function(macro_source_file*, > int, char const*, int, char const**, char const*) (macrotab.c:822) > ==22285== by 0x52F945: macro_define_command(char const*, int) > (macrocmd.c:409) > ... > ==22285== 128 (96 direct, 32 indirect) bytes in 2 blocks are > definitely lost in loss record 1,083 of 3,377 > ==22285== at 0x4C2BE6D: malloc (vg_replace_malloc.c:309) > ==22285== by 0x4049E7: xmalloc (common-utils.c:44) > ==22285== by 0x533A20: new_macro_key(macro_table*, char const*, > macro_source_file*, int) (macrotab.c:355) > ==22285== by 0x534277: > macro_define_object_internal(macro_source_file*, int, char const*, > char const*, macro_special_kind) (macrotab.c:776) > ==22285== by 0x52F7E0: macro_define_command(char const*, int) > (macrocmd.c:414) > ... > ==22285== 177 bytes in 19 blocks are definitely lost in loss record > 1,193 of 3,377 > ==22285== at 0x4C2BE6D: malloc (vg_replace_malloc.c:309) > ==22285== by 0x4049E7: xmalloc (common-utils.c:44) > ==22285== by 0x52F5BD: extract_identifier(char const**, int) > (macrocmd.c:316) > ==22285== by 0x52F77D: macro_define_command(char const*, int) > (macrocmd.c:355) > > gdb/ChangeLog > 2019-01-15 Philippe Waroquiers > > * macrocmd.c (macro_define_command): Use a unique_xmalloc_ptr > to maintain name ownership. > * macrotab.c (macro_define_internal): New function that > factorizes macro_define_object_internal and macro_define_function > code. If the newly inserted node has replaced a existing node, > call macro_tree_delete_key to free the unused just allocated key. > (macro_define_object_internal): Use macro_define_internal. > (macro_define_function): Likewise. > (macro_undef): Free the key of the removed node. > --- > gdb/macrocmd.c | 10 +++---- > gdb/macrotab.c | 75 +++++++++++++++++++++++++++++--------------------- > 2 files changed, 48 insertions(+), 37 deletions(-) > > diff --git a/gdb/macrocmd.c b/gdb/macrocmd.c > index 706a8353e2..c1c42fa910 100644 > --- a/gdb/macrocmd.c > +++ b/gdb/macrocmd.c > @@ -346,14 +346,13 @@ static void > macro_define_command (const char *exp, int from_tty) > { > temporary_macro_definition new_macro; > - char *name = NULL; > > if (!exp) > error (_("usage: macro define NAME[(ARGUMENT-LIST)] > [REPLACEMENT-LIST]")); > > skip_ws (&exp); > - name = extract_identifier (&exp, 0); > - if (! name) > + gdb::unique_xmalloc_ptr name (extract_identifier (&exp, 0)); > + if (name.get () == NULL) Nit, you don't have to use ".get ()" here. I think extract_identifier should return a gdb::unique_xmalloc_ptr. This call site lower: argv[new_macro.argc] = extract_identifier (&exp, 1); can then use ".release ()". > @@ -774,8 +780,28 @@ macro_define_object_internal (struct > macro_source_file *source, int line, > return; > > k = new_macro_key (t, name, source, line); > - d = new_macro_definition (t, macro_object_like, kind, 0, > replacement); > - splay_tree_insert (t->definitions, (splay_tree_key) k, > (splay_tree_value) d); > + d = new_macro_definition (t, kind, argc, argv, replacement); > + s = splay_tree_insert (t->definitions, (splay_tree_key) k, > (splay_tree_value) d); > + if ((struct macro_key *) s->key != k) > + { > + /* If the node inserted is not using k, it means we have a > redefinition. > + We must free the newly allocated k, as it will not be stored in > + the splay tree. */ > + macro_tree_delete_key (k); > + } Would you consider that a bug in the splay tree implementation? Should it use the new key and free the old one with the key deallocation function? > @@ -841,8 +850,10 @@ macro_undef (struct macro_source_file *source, int > line, > arguments like '-DFOO -UFOO -DFOO=2'. */ > if (source == key->start_file > && line == key->start_line) > - splay_tree_remove (source->table->definitions, n->key); > - > + { > + splay_tree_remove (source->table->definitions, n->key); > + macro_tree_delete_key (key); > + } > else > { > /* This function is the only place a macro's end-of-scope Same here, should it be the splay tree code that deletes the key? Simon