* [RFA] Fix splay tree KEY leak detected in GDB test gdb.base/macscp.exp
@ 2019-01-26 22:34 Philippe Waroquiers
2019-02-03 17:45 ` Kevin Buettner
0 siblings, 1 reply; 7+ messages in thread
From: Philippe Waroquiers @ 2019-01-26 22:34 UTC (permalink / raw)
To: gdb-patches; +Cc: Philippe Waroquiers
When a node is removed from a splay tree, the splay tree was
not using the function splay_tree_delete_key_fn to release the key.
This was causing a leak, fixed by Tom Tromey.
This patch fixes another key leak, that happens when a key equal to
a key already present is inserted. In such a case, we have to release
the old KEY.
Note that this is based on the assumption that the caller always
allocates a new KEY when doing an insert.
Also, clarify the documentation about when the release functions are
called.
include/ChangeLog
2019-01-26 Philippe Waroquiers <philippe.waroquiers@skynet.be>
* splay-tree.h (splay_tree_delete_key_fn): Update comment.
(splay_tree_delete_value_fn): Likewise.
libiberty/ChangeLog
2019-01-26 Philippe Waroquiers <philippe.waroquiers@skynet.be>
* splay-tree.c (splay_tree_insert): Also release old KEY in case
of insertion of a key equal to an already present key.
(splay_tree_new_typed_alloc): Update comment.
---
include/splay-tree.h | 11 +++++++++--
libiberty/splay-tree.c | 13 ++++++++++---
2 files changed, 19 insertions(+), 5 deletions(-)
diff --git a/include/splay-tree.h b/include/splay-tree.h
index 0d26272943..da533dec18 100644
--- a/include/splay-tree.h
+++ b/include/splay-tree.h
@@ -58,11 +58,18 @@ typedef struct splay_tree_node_s *splay_tree_node;
typedef int (*splay_tree_compare_fn) (splay_tree_key, splay_tree_key);
/* The type of a function used to deallocate any resources associated
- with the key. */
+ with the key. If you provide this function, the splay tree
+ will take the ownership of the memory of the splay_tree_key arg
+ of splay_tree_insert. This function is called to release the keys
+ present in the tree when calling splay_tree_delete or splay_tree_remove.
+ If splay_tree_insert is called with a key equal to a key already
+ present in the tree, the old key and old value will be released. */
typedef void (*splay_tree_delete_key_fn) (splay_tree_key);
/* The type of a function used to deallocate any resources associated
- with the value. */
+ with the value. If you provide this function, the memory of the
+ splay_tree_value arg of splay_tree_insert is managed similarly to
+ the splay_tree_key memory: see splay_tree_delete_key_fn. */
typedef void (*splay_tree_delete_value_fn) (splay_tree_value);
/* The type of a function used to iterate over the tree. */
diff --git a/libiberty/splay-tree.c b/libiberty/splay-tree.c
index 21d23c38df..4bbb39a62c 100644
--- a/libiberty/splay-tree.c
+++ b/libiberty/splay-tree.c
@@ -318,7 +318,11 @@ different types need to be allocated with different allocators.
The splay tree will use @var{compare_fn} to compare nodes,
@var{delete_key_fn} to deallocate keys, and @var{delete_value_fn} to
-deallocate values.
+deallocate values. Keys and values will be deallocated when the
+tree is deleted using splay_tree_delete or when a node is removed
+using splay_tree_remove. splay_tree_insert will release the previously
+inserted key and value using @var{delete_key_fn} and @var{delete_value_fn}
+if the inserted key is already found in the tree.
@end deftypefn
@@ -372,10 +376,13 @@ splay_tree_insert (splay_tree sp, splay_tree_key key, splay_tree_value value)
if (sp->root && comparison == 0)
{
- /* If the root of the tree already has the indicated KEY, just
- replace the value with VALUE. */
+ /* If the root of the tree already has the indicated KEY, delete
+ the old key and old value, and replace them with KEY and VALUE. */
+ if (sp->delete_key)
+ (*sp->delete_key) (sp->root->key);
if (sp->delete_value)
(*sp->delete_value)(sp->root->value);
+ sp->root->key = key;
sp->root->value = value;
}
else
--
2.20.1
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFA] Fix splay tree KEY leak detected in GDB test gdb.base/macscp.exp
2019-01-26 22:34 [RFA] Fix splay tree KEY leak detected in GDB test gdb.base/macscp.exp Philippe Waroquiers
@ 2019-02-03 17:45 ` Kevin Buettner
2019-02-03 17:58 ` Philippe Waroquiers
0 siblings, 1 reply; 7+ messages in thread
From: Kevin Buettner @ 2019-02-03 17:45 UTC (permalink / raw)
To: Philippe Waroquiers; +Cc: gdb-patches
On Sat, 26 Jan 2019 23:34:35 +0100
Philippe Waroquiers <philippe.waroquiers@skynet.be> wrote:
> include/ChangeLog
> 2019-01-26 Philippe Waroquiers <philippe.waroquiers@skynet.be>
>
> * splay-tree.h (splay_tree_delete_key_fn): Update comment.
> (splay_tree_delete_value_fn): Likewise.
>
> libiberty/ChangeLog
> 2019-01-26 Philippe Waroquiers <philippe.waroquiers@skynet.be>
>
> * splay-tree.c (splay_tree_insert): Also release old KEY in case
> of insertion of a key equal to an already present key.
> (splay_tree_new_typed_alloc): Update comment.
These changes look reasonable to me, BUT...
I think you'll need to get approval for this patch from the gcc
maintainers. The libiberty README file indicates that fixes should be
sent to gcc-patches@gcc.gnu.org.
Kevin
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFA] Fix splay tree KEY leak detected in GDB test gdb.base/macscp.exp
2019-02-03 17:45 ` Kevin Buettner
@ 2019-02-03 17:58 ` Philippe Waroquiers
2019-02-05 13:48 ` Tom Tromey
0 siblings, 1 reply; 7+ messages in thread
From: Philippe Waroquiers @ 2019-02-03 17:58 UTC (permalink / raw)
To: Kevin Buettner; +Cc: gdb-patches
Thanks for looking at this.
The below change was discussed with Tom on irc.
Tom pushed a first fix in the splay tree, so it would be easier
for me if he (or someone else) could also push this one.
Thanks
Philippe
On Sun, 2019-02-03 at 10:45 -0700, Kevin Buettner wrote:
> On Sat, 26 Jan 2019 23:34:35 +0100
> Philippe Waroquiers <philippe.waroquiers@skynet.be> wrote:
>
> > include/ChangeLog
> > 2019-01-26 Philippe Waroquiers <philippe.waroquiers@skynet.be>
> >
> > * splay-tree.h (splay_tree_delete_key_fn): Update comment.
> > (splay_tree_delete_value_fn): Likewise.
> >
> > libiberty/ChangeLog
> > 2019-01-26 Philippe Waroquiers <philippe.waroquiers@skynet.be>
> >
> > * splay-tree.c (splay_tree_insert): Also release old KEY in case
> > of insertion of a key equal to an already present key.
> > (splay_tree_new_typed_alloc): Update comment.
>
> These changes look reasonable to me, BUT...
>
> I think you'll need to get approval for this patch from the gcc
> maintainers. The libiberty README file indicates that fixes should be
> sent to gcc-patches@gcc.gnu.org.
>
> Kevin
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFA] Fix splay tree KEY leak detected in GDB test gdb.base/macscp.exp
2019-02-03 17:58 ` Philippe Waroquiers
@ 2019-02-05 13:48 ` Tom Tromey
2019-02-05 19:55 ` Philippe Waroquiers
0 siblings, 1 reply; 7+ messages in thread
From: Tom Tromey @ 2019-02-05 13:48 UTC (permalink / raw)
To: Philippe Waroquiers; +Cc: Kevin Buettner, gdb-patches
>>>>> "Philippe" == Philippe Waroquiers <philippe.waroquiers@skynet.be> writes:
Philippe> The below change was discussed with Tom on irc.
Philippe> Tom pushed a first fix in the splay tree, so it would be easier
Philippe> for me if he (or someone else) could also push this one.
Did you send them to gcc-patches?
If so and they were approved, let me know and I will commit them
(though maybe not until Monday, depending).
If not, let me know, and I can forward the note, etc.
thanks,
Tom
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFA] Fix splay tree KEY leak detected in GDB test gdb.base/macscp.exp
2019-02-05 13:48 ` Tom Tromey
@ 2019-02-05 19:55 ` Philippe Waroquiers
2019-02-06 14:23 ` Tom Tromey
2019-02-12 13:13 ` Tom Tromey
0 siblings, 2 replies; 7+ messages in thread
From: Philippe Waroquiers @ 2019-02-05 19:55 UTC (permalink / raw)
To: Tom Tromey; +Cc: Kevin Buettner, gdb-patches
On Tue, 2019-02-05 at 06:47 -0700, Tom Tromey wrote:
> > > > > > "Philippe" == Philippe Waroquiers <philippe.waroquiers@skynet.be> writes:
>
> Philippe> The below change was discussed with Tom on irc.
> Philippe> Tom pushed a first fix in the splay tree, so it would be easier
> Philippe> for me if he (or someone else) could also push this one.
>
> Did you send them to gcc-patches?
> If so and they were approved, let me know and I will commit them
> (though maybe not until Monday, depending).
> If not, let me know, and I can forward the note, etc.
No, I did not send it.
It would be really nice if you could take that in charge.
Also, there is a leak related to macro definition
(but not in splay tree), for which there is a patch series:
[RFAv3 0/2] Fix leaks in macro definition
https://sourceware.org/ml/gdb-patches/2019-01/msg00567.html
This series is still to be reviewed.
Thanks
Philippe
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFA] Fix splay tree KEY leak detected in GDB test gdb.base/macscp.exp
2019-02-05 19:55 ` Philippe Waroquiers
@ 2019-02-06 14:23 ` Tom Tromey
2019-02-12 13:13 ` Tom Tromey
1 sibling, 0 replies; 7+ messages in thread
From: Tom Tromey @ 2019-02-06 14:23 UTC (permalink / raw)
To: Philippe Waroquiers; +Cc: Tom Tromey, Kevin Buettner, gdb-patches
>>>>> "Philippe" == Philippe Waroquiers <philippe.waroquiers@skynet.be> writes:
Philippe> No, I did not send it.
Philippe> It would be really nice if you could take that in charge.
I forwarded it:
https://gcc.gnu.org/ml/gcc-patches/2019-02/msg00323.html
Tom
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFA] Fix splay tree KEY leak detected in GDB test gdb.base/macscp.exp
2019-02-05 19:55 ` Philippe Waroquiers
2019-02-06 14:23 ` Tom Tromey
@ 2019-02-12 13:13 ` Tom Tromey
1 sibling, 0 replies; 7+ messages in thread
From: Tom Tromey @ 2019-02-12 13:13 UTC (permalink / raw)
To: Philippe Waroquiers; +Cc: Tom Tromey, Kevin Buettner, gdb-patches
>>>>> "Philippe" == Philippe Waroquiers <philippe.waroquiers@skynet.be> writes:
>> Did you send them to gcc-patches?
>> If so and they were approved, let me know and I will commit them
>> (though maybe not until Monday, depending).
>> If not, let me know, and I can forward the note, etc.
Philippe> No, I did not send it.
Philippe> It would be really nice if you could take that in charge.
I've checked this in to gcc and binutils-gdb now.
Thanks for writing this.
Tom
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-02-12 13:13 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-26 22:34 [RFA] Fix splay tree KEY leak detected in GDB test gdb.base/macscp.exp Philippe Waroquiers
2019-02-03 17:45 ` Kevin Buettner
2019-02-03 17:58 ` Philippe Waroquiers
2019-02-05 13:48 ` Tom Tromey
2019-02-05 19:55 ` Philippe Waroquiers
2019-02-06 14:23 ` Tom Tromey
2019-02-12 13:13 ` Tom Tromey
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).