This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH] C++ify osdata
- From: Simon Marchi <simon dot marchi at ericsson dot com>
- To: Pedro Alves <palves at redhat dot com>, Simon Marchi <simon dot marchi at polymtl dot ca>, <gdb-patches at sourceware dot org>
- Date: Thu, 23 Nov 2017 13:39:34 -0500
- Subject: Re: [PATCH] C++ify osdata
- Authentication-results: sourceware.org; auth=none
- Authentication-results: spf=none (sender IP is ) smtp.mailfrom=simon dot marchi at ericsson dot com;
- References: <20171118233827.7418-1-simon.marchi@polymtl.ca> <9d0b9473-1fd5-8310-0cde-8233efed58bc@redhat.com>
- Spamdiagnosticmetadata: NSPM
- Spamdiagnosticoutput: 1:99
On 2017-11-23 07:54 AM, Pedro Alves wrote:
> Hi Simon,
>
> I think I spotted an issue here.
>
> On 11/18/2017 11:38 PM, Simon Marchi wrote:
>> + /* This keeps a map from integer (pid) to vector of struct osdata_item.
>> + The vector contains information about all threads for the given pid. */
>> + std::map<int, std::vector<osdata_item> *> tree_;
>
> Isn't this leaking the heap-allocated vectors?
>
> Why make it a map of pointers, actually? Why not:
>
> std::map<int, std::vector<osdata_item>>
>
> It's more efficient, and I think results in simpler code. You
> won't need the tree_.find() for example, can just do:
>
> tree_[pid_i].push_back (...);
>
> Thanks,
> Pedro Alves
>
Oops, thanks for noticing and for the suggestion. I don't know what I was thinking.
What about this patch?
>From 2a14038a76715c1bc84d8a533e17322521620202 Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@ericsson.com>
Date: Thu, 23 Nov 2017 13:21:35 -0500
Subject: [PATCH] Fix memory leak in list_available_thread_groups
Commit
C++ify osdata
479f8de1b3b7e69ca8d557bbe9d843c7d1bc89c5
introduced a memory leak. We allocate std::vectors and insert them in a
map, but never free them. Instead, the map value type can be
std::vector objects directly.
gdb/ChangeLog:
* mi/mi-main.c (list_available_thread_groups): Change map value
type to std::vector.
---
gdb/mi/mi-main.c | 22 ++++++----------------
1 file changed, 6 insertions(+), 16 deletions(-)
diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
index c7db478..3ca3500 100644
--- a/gdb/mi/mi-main.c
+++ b/gdb/mi/mi-main.c
@@ -716,7 +716,7 @@ list_available_thread_groups (const std::set<int> &ids, int recurse)
/* This keeps a map from integer (pid) to vector of struct osdata_item.
The vector contains information about all threads for the given pid. */
- std::map<int, std::vector<osdata_item> *> tree_;
+ std::map<int, std::vector<osdata_item>> tree;
/* get_osdata will throw if it cannot return data. */
std::unique_ptr<osdata> data = get_osdata ("processes");
@@ -729,18 +729,8 @@ list_available_thread_groups (const std::set<int> &ids, int recurse)
{
const std::string *pid = get_osdata_column (item, "pid");
int pid_i = strtoul (pid->c_str (), NULL, 0);
- std::vector<osdata_item> *vec;
- auto n = tree_.find (pid_i);
- if (n == tree_.end ())
- {
- vec = new std::vector<osdata_item>;
- tree_[pid_i] = vec;
- }
- else
- vec = n->second;
-
- vec->push_back (item);
+ tree[pid_i].push_back (item);
}
}
@@ -774,14 +764,14 @@ list_available_thread_groups (const std::set<int> &ids, int recurse)
if (recurse)
{
- auto n = tree_.find (pid_i);
- if (n != tree_.end ())
+ auto n = tree.find (pid_i);
+ if (n != tree.end ())
{
- std::vector<osdata_item> *children = n->second;
+ std::vector<osdata_item> &children = n->second;
ui_out_emit_list thread_list_emitter (uiout, "threads");
- for (const osdata_item &child : *children)
+ for (const osdata_item &child : children)
{
ui_out_emit_tuple tuple_emitter (uiout, NULL);
const std::string *tid = get_osdata_column (child, "tid");
--
2.7.4