diff mbox

C++ify osdata

Message ID 19f399ce-dfb9-15fd-41ed-2a4576a438f6@ericsson.com
State New, archived
Headers

Commit Message

Simon Marchi Nov. 23, 2017, 6:39 p.m. UTC 
  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(-)

Comments

Pedro Alves Nov. 23, 2017, 10:46 p.m. UTC | #1 
On 11/23/2017 06:39 PM, Simon Marchi wrote:
> On 2017-11-23 07:54 AM, Pedro Alves wrote:

>> Isn't this leaking the heap-allocated vectors?
>>
>> Why make it a map of pointers, actually?  Why not:
>>

> Oops, thanks for noticing and for the suggestion.  I don't know what I was thinking.
> What about this patch?

LGTM.

Thanks,
Pedro Alves
Simon Marchi Nov. 24, 2017, 2:58 a.m. UTC | #2 
On 2017-11-23 17:46, Pedro Alves wrote:
> On 11/23/2017 06:39 PM, Simon Marchi wrote:
>> On 2017-11-23 07:54 AM, Pedro Alves wrote:
> 
>>> Isn't this leaking the heap-allocated vectors?
>>> 
>>> Why make it a map of pointers, actually?  Why not:
>>> 
> 
>> Oops, thanks for noticing and for the suggestion.  I don't know what I 
>> was thinking.
>> What about this patch?
> 
> LGTM.
> 
> Thanks,
> Pedro Alves

Thanks, pushed.

Simon
diff mbox

Patch

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");