Patchwork [1/3] gdbserver: Use std::list for all_dlls

login
register
mail settings
Submitter Simon Marchi
Date Oct. 9, 2017, 2:30 p.m.
Message ID <20171009143036.10215-2-simon.marchi@polymtl.ca>
Download mbox | patch
Permalink /patch/23410/
State New
Headers show

Comments

Simon Marchi - Oct. 9, 2017, 2:30 p.m.
As a small step towards removing inferior_list/inferior_list_entry, this
patch replaces the usage of inferior_list for the list of dlls by an
std::list.

I am able to build gdbserver with mingw on Linux, but I am not able to
test this on a Windows machine (the only platform that uses this code).

gdb/gdbserver/ChangeLog:

	* dll.h: Include <list>.
	(struct dll_info) <entry>: Remove field.
	(all_dlls): Change type to std::list<dll_info *>.
	* dll.c: Include <algorithm>.
	(get_dll): Remove macro.
	(all_dlls): Change type to std::list<dll_info *>.
	(free_one_dll): Change parameter type to dll_info *.
	(match_dll): Likewise.
	(loaded_dll): Adjust to removal of entry field and all_dlls type
	change.
	(unloaded_dll): Adjust to all_dlls type change, use
	std::find_if.
	(clear_dlls): Adjust to all_dlls type change.
	* server.c (emit_dll_description): Remove.
	(handle_qxfer_libraries): Adjust to all_dlls type change,
	integrate emit_dll_description's functionality.
---
 gdb/gdbserver/dll.c    | 36 ++++++++++++++++++------------------
 gdb/gdbserver/dll.h    |  8 +++-----
 gdb/gdbserver/server.c | 20 ++++----------------
 3 files changed, 25 insertions(+), 39 deletions(-)
Pedro Alves - Oct. 9, 2017, 2:44 p.m.
On 10/09/2017 03:30 PM, Simon Marchi wrote:

> diff --git a/gdb/gdbserver/dll.h b/gdb/gdbserver/dll.h
> index 39e5eb0653..52f924bc85 100644
> --- a/gdb/gdbserver/dll.h
> +++ b/gdb/gdbserver/dll.h
> @@ -18,17 +18,15 @@
>  #ifndef DLL_H
>  #define DLL_H
>  
> +#include <list>
> +
>  struct dll_info
>  {
> -  /* This must appear first.  See inferiors.h.
> -     The list iterator functions assume it.  */
> -  struct inferior_list_entry entry;
> -
>    char *name;
>    CORE_ADDR base_addr;
>  };
>  
> -extern struct inferior_list all_dlls;
> +extern std::list<dll_info *> all_dlls;

Is there a reason for making this a list of dll_info pointers
instead of a list of dll_info objects?  If you make this a list of
objects, then each list node + dll_info is allocated in one go,
very much like the current code.  With a list of pointers, you
have an extra allocation/indirection for each dll_info (one for
node + pointer, another for the dll_info pointee).

Thanks,
Pedro Alves
Simon Marchi - Oct. 9, 2017, 2:49 p.m.
On 2017-10-09 10:44 AM, Pedro Alves wrote:
> On 10/09/2017 03:30 PM, Simon Marchi wrote:
> 
>> diff --git a/gdb/gdbserver/dll.h b/gdb/gdbserver/dll.h
>> index 39e5eb0653..52f924bc85 100644
>> --- a/gdb/gdbserver/dll.h
>> +++ b/gdb/gdbserver/dll.h
>> @@ -18,17 +18,15 @@
>>  #ifndef DLL_H
>>  #define DLL_H
>>  
>> +#include <list>
>> +
>>  struct dll_info
>>  {
>> -  /* This must appear first.  See inferiors.h.
>> -     The list iterator functions assume it.  */
>> -  struct inferior_list_entry entry;
>> -
>>    char *name;
>>    CORE_ADDR base_addr;
>>  };
>>  
>> -extern struct inferior_list all_dlls;
>> +extern std::list<dll_info *> all_dlls;
> 
> Is there a reason for making this a list of dll_info pointers
> instead of a list of dll_info objects?  If you make this a list of
> objects, then each list node + dll_info is allocated in one go,
> very much like the current code.  With a list of pointers, you
> have an extra allocation/indirection for each dll_info (one for
> node + pointer, another for the dll_info pointee).

No reason, except that I thought this was a reasonnable intermediary step.
I'll try doing as you say, it's probably not too long.

Simon

Patch

diff --git a/gdb/gdbserver/dll.c b/gdb/gdbserver/dll.c
index c74601a5f4..ec672be797 100644
--- a/gdb/gdbserver/dll.c
+++ b/gdb/gdbserver/dll.c
@@ -18,20 +18,20 @@ 
 #include "server.h"
 #include "dll.h"
 
-#define get_dll(inf) ((struct dll_info *)(inf))
+#include <algorithm>
 
 /* An "unspecified" CORE_ADDR, for match_dll.  */
 #define UNSPECIFIED_CORE_ADDR (~(CORE_ADDR) 0)
 
-struct inferior_list all_dlls;
+std::list<dll_info *> all_dlls;
 int dlls_changed;
 
 static void
-free_one_dll (struct inferior_list_entry *inf)
+free_one_dll (dll_info *dll)
 {
-  struct dll_info *dll = get_dll (inf);
   if (dll->name != NULL)
     free (dll->name);
+
   free (dll);
 }
 
@@ -39,11 +39,8 @@  free_one_dll (struct inferior_list_entry *inf)
    the key is ignored; so is an all-ones base address.  */
 
 static int
-match_dll (struct inferior_list_entry *inf, void *arg)
+match_dll (dll_info *iter, dll_info *key)
 {
-  struct dll_info *iter = (struct dll_info *) inf;
-  struct dll_info *key = (struct dll_info *) arg;
-
   if (key->base_addr != UNSPECIFIED_CORE_ADDR
       && iter->base_addr == key->base_addr)
     return 1;
@@ -62,12 +59,10 @@  loaded_dll (const char *name, CORE_ADDR base_addr)
 {
   struct dll_info *new_dll = XCNEW (struct dll_info);
 
-  new_dll->entry.id = minus_one_ptid;
-
   new_dll->name = xstrdup (name);
   new_dll->base_addr = base_addr;
 
-  add_inferior_to_list (&all_dlls, &new_dll->entry);
+  all_dlls.push_back (new_dll);
   dlls_changed = 1;
 }
 
@@ -76,16 +71,19 @@  loaded_dll (const char *name, CORE_ADDR base_addr)
 void
 unloaded_dll (const char *name, CORE_ADDR base_addr)
 {
-  struct dll_info *dll;
   struct dll_info key_dll;
 
   /* Be careful not to put the key DLL in any list.  */
   key_dll.name = (char *) name;
   key_dll.base_addr = base_addr;
 
-  dll = (struct dll_info *) find_inferior (&all_dlls, match_dll, &key_dll);
+  auto pred = [&key_dll] (dll_info *dll) {
+    return match_dll (dll, &key_dll);
+  };
 
-  if (dll == NULL)
+  auto dll = std::find_if (all_dlls.begin (), all_dlls.end (), pred);
+
+  if (dll == all_dlls.end ())
     /* For some inferiors we might get unloaded_dll events without having
        a corresponding loaded_dll.  In that case, the dll cannot be found
        in ALL_DLL, and there is nothing further for us to do.
@@ -99,8 +97,8 @@  unloaded_dll (const char *name, CORE_ADDR base_addr)
     {
       /* DLL has been found so remove the entry and free associated
          resources.  */
-      remove_inferior (&all_dlls, &dll->entry);
-      free_one_dll (&dll->entry);
+      free_one_dll (*dll);
+      all_dlls.erase (dll);
       dlls_changed = 1;
     }
 }
@@ -108,6 +106,8 @@  unloaded_dll (const char *name, CORE_ADDR base_addr)
 void
 clear_dlls (void)
 {
-  for_each_inferior (&all_dlls, free_one_dll);
-  clear_inferior_list (&all_dlls);
+  for (dll_info *dll : all_dlls)
+    free_one_dll (dll);
+
+  all_dlls.clear ();
 }
diff --git a/gdb/gdbserver/dll.h b/gdb/gdbserver/dll.h
index 39e5eb0653..52f924bc85 100644
--- a/gdb/gdbserver/dll.h
+++ b/gdb/gdbserver/dll.h
@@ -18,17 +18,15 @@ 
 #ifndef DLL_H
 #define DLL_H
 
+#include <list>
+
 struct dll_info
 {
-  /* This must appear first.  See inferiors.h.
-     The list iterator functions assume it.  */
-  struct inferior_list_entry entry;
-
   char *name;
   CORE_ADDR base_addr;
 };
 
-extern struct inferior_list all_dlls;
+extern std::list<dll_info *> all_dlls;
 extern int dlls_changed;
 
 extern void clear_dlls (void);
diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
index a95973547c..3d2c2ff5a0 100644
--- a/gdb/gdbserver/server.c
+++ b/gdb/gdbserver/server.c
@@ -1533,21 +1533,6 @@  handle_qxfer_features (const char *annex,
   return len;
 }
 
-/* Worker routine for handle_qxfer_libraries.
-   Emit the XML to describe the library in INF.  */
-
-static void
-emit_dll_description (struct inferior_list_entry *inf, void *arg)
-{
-  struct dll_info *dll = (struct dll_info *) inf;
-  std::string *document = (std::string *) arg;
-  std::string name = xml_escape_text (dll->name);
-
-  *document += string_printf
-    ("  <library name=\"%s\"><segment address=\"0x%lx\"/></library>\n",
-     name.c_str (), (long) dll->base_addr);
-}
-
 /* Handle qXfer:libraries:read.  */
 
 static int
@@ -1563,7 +1548,10 @@  handle_qxfer_libraries (const char *annex,
 
   std::string document = "<library-list version=\"1.0\">\n";
 
-  for_each_inferior_with_data (&all_dlls, emit_dll_description, &document);
+  for (const dll_info *dll : all_dlls)
+    document += string_printf
+      ("  <library name=\"%s\"><segment address=\"0x%lx\"/></library>\n",
+       dll->name, (long) dll->base_addr);
 
   document += "</library-list>\n";