[7/9] Change solib-aix.c to use type-safe registry

Message ID 20190710153947.25721-8-tromey@adacore.com
State New, archived
Headers

Commit Message

Tom Tromey July 10, 2019, 3:39 p.m. UTC
  This changes solib-aix.c to use the type-safe registry, and removes a
use of VEC in the process.

gdb/ChangeLog
2019-07-10  Tom Tromey  <tromey@adacore.com>

	* solib-aix.c (library_list_type): New typedef.
	(lm_info_aix_p): Remove typedef.  Don't define VEC.
	(struct solib_aix_inferior_data) <library_list>: Change type.
	(solib_aix_inferior_data_handle): Change type.
	(get_solib_aix_inferior_data): Update.
	(solib_aix_free_library_list): Remove.
	(library_list_start_library): Update.
	(solib_aix_parse_libraries, solib_aix_get_library_list): Change
	return type.
	(solib_aix_get_library_list)
	(solib_aix_solib_create_inferior_hook, solib_aix_current_sos)
	(solib_aix_normal_stop_observer, _initialize_solib_aix): Update.
---
 gdb/ChangeLog   |  15 ++++++
 gdb/solib-aix.c | 139 ++++++++++++++++--------------------------------
 2 files changed, 61 insertions(+), 93 deletions(-)
  

Comments

Simon Marchi July 10, 2019, 6:55 p.m. UTC | #1
On 2019-07-10 11:39 a.m., Tom Tromey wrote:
> This changes solib-aix.c to use the type-safe registry, and removes a
> use of VEC in the process.
> 
> gdb/ChangeLog
> 2019-07-10  Tom Tromey  <tromey@adacore.com>
> 
> 	* solib-aix.c (library_list_type): New typedef.
> 	(lm_info_aix_p): Remove typedef.  Don't define VEC.
> 	(struct solib_aix_inferior_data) <library_list>: Change type.
> 	(solib_aix_inferior_data_handle): Change type.
> 	(get_solib_aix_inferior_data): Update.
> 	(solib_aix_free_library_list): Remove.
> 	(library_list_start_library): Update.
> 	(solib_aix_parse_libraries, solib_aix_get_library_list): Change
> 	return type.
> 	(solib_aix_get_library_list)
> 	(solib_aix_solib_create_inferior_hook, solib_aix_current_sos)
> 	(solib_aix_normal_stop_observer, _initialize_solib_aix): Update.
> ---
>  gdb/ChangeLog   |  15 ++++++
>  gdb/solib-aix.c | 139 ++++++++++++++++--------------------------------
>  2 files changed, 61 insertions(+), 93 deletions(-)
> 
> diff --git a/gdb/solib-aix.c b/gdb/solib-aix.c
> index bf2f30d01cd..8f0a3ca9231 100644
> --- a/gdb/solib-aix.c
> +++ b/gdb/solib-aix.c
> @@ -59,14 +59,13 @@ struct lm_info_aix : public lm_info_base
>    ULONGEST data_size = 0;
>  };
>  
> -typedef lm_info_aix *lm_info_aix_p;
> -DEF_VEC_P(lm_info_aix_p);
> +typedef gdb::optional<std::vector<lm_info_aix>> library_list_type;

Hmm, I find that hiding the gdb::optional behind a typedef can be a bit
confusing.  It's easy to forget that it's an optional if it's not "in your
face".  I think it would help to be explicit and spell out gdb::optional
where needed.  If you want to keep the typedef, I would suggest defining it
as:

  typedef std::vector<lm_info_aix> library_list_type;

and using gdb::optional<library_list_type> in the code.  Or even
gdb::optional<std::vector<lm_info_aix>> is not too long.

> @@ -137,30 +125,30 @@ library_list_start_library (struct gdb_xml_parser *parser,
>  			    void *user_data,
>  			    std::vector<gdb_xml_value> &attributes)
>  {
> -  VEC (lm_info_aix_p) **list = (VEC (lm_info_aix_p) **) user_data;
> -  lm_info_aix *item = new lm_info_aix;
> +  std::vector<lm_info_aix> *list = (std::vector<lm_info_aix> *) user_data;
> +  lm_info_aix item;
>    struct gdb_xml_value *attr;
>  
>    attr = xml_find_attribute (attributes, "name");
> -  item->filename = xstrdup ((const char *) attr->value.get ());
> +  item.filename = (const char *) attr->value.get ();
>  
>    attr = xml_find_attribute (attributes, "member");
>    if (attr != NULL)
> -    item->member_name = xstrdup ((const char *) attr->value.get ());
> +    item.member_name = (const char *) attr->value.get ();

It seems like that fixes a mem leak, we were xstrdup-ing into an std::string.

> @@ -237,26 +206,18 @@ static const struct gdb_xml_element library_list_elements[] =
>  /* Parse LIBRARY, a string containing the loader info in XML format,
>     and return an lm_info_aix_p vector.

The part about "lm_info_aix_p" should be updated.

>  
> -   Return NULL if the parsing failed.  */
> +   Return false if the parsing failed.  */

That comment change (Return false) doesn't look in sync with the code (the
function doesn't return a bool).

>  
> -static VEC (lm_info_aix_p) *
> +static library_list_type
>  solib_aix_parse_libraries (const char *library)
>  {
> -  VEC (lm_info_aix_p) *result = NULL;
> -  auto cleanup = make_scope_exit ([&] ()
> -    {
> -      solib_aix_free_library_list (&result);
> -    });
> +  std::vector<lm_info_aix> result;
>  
>    if (gdb_xml_parse_quick (_("aix library list"), "library-list-aix.dtd",
> -                           library_list_elements, library, &result) == 0)
> -    {
> -      /* Parsed successfully, keep the result.  */
> -      cleanup.release ();
> -      return result;
> -    }
> +			   library_list_elements, library, &result) == 0)
> +    return library_list_type (std::move (result));

I think you could do "return result;" directly and it will do The Right Thing.

>  
> -  return NULL;
> +  return {};
>  }
>  
>  #endif /* HAVE_LIBEXPAT */
> @@ -271,14 +232,14 @@ solib_aix_parse_libraries (const char *library)
>     is not NULL, then print a warning including WARNING_MSG and
>     a description of the error.  */
>  
> -static VEC (lm_info_aix_p) *
> +static library_list_type &
>  solib_aix_get_library_list (struct inferior *inf, const char *warning_msg)
>  {

The comment above says this function can return NULL, which can't happen if
returning a reference.

> @@ -530,29 +485,29 @@ static struct so_list *
>  solib_aix_current_sos (void)
>  {
>    struct so_list *start = NULL, *last = NULL;
> -  VEC (lm_info_aix_p) *library_list;
> -  lm_info_aix *info;
>    int ix;
>  
> -  library_list = solib_aix_get_library_list (current_inferior (), NULL);
> -  if (library_list == NULL)
> +  library_list_type &library_list
> +    = solib_aix_get_library_list (current_inferior (), NULL);
> +  if (!library_list.has_value ())
>      return NULL;
>  
>    /* Build a struct so_list for each entry on the list.
>       We skip the first entry, since this is the entry corresponding
>       to the main executable, not a shared library.  */
> -  for (ix = 1; VEC_iterate (lm_info_aix_p, library_list, ix, info); ix++)
> +  for (ix = 1; ix < library_list->size (); ix++)

This could be:

  for (lm_info_aix &info : *library_list)

Simon
  
Tom Tromey July 10, 2019, 8:33 p.m. UTC | #2
>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

>> +typedef gdb::optional<std::vector<lm_info_aix>> library_list_type;

Simon> Hmm, I find that hiding the gdb::optional behind a typedef can be a bit
Simon> confusing.  It's easy to forget that it's an optional if it's not "in your
Simon> face".  I think it would help to be explicit and spell out gdb::optional
Simon> where needed.

Makes sense, I've removed the typedef.

I made the other changes you mentioned, except...

>> -  for (ix = 1; VEC_iterate (lm_info_aix_p, library_list, ix, info); ix++)
>> +  for (ix = 1; ix < library_list->size (); ix++)

Simon> This could be:

Simon>   for (lm_info_aix &info : *library_list)

This isn't quite the same, because the current loop starts at index 1,
not 0.  So, I didn't make this change.

Tom
  
Simon Marchi July 10, 2019, 8:38 p.m. UTC | #3
On 2019-07-10 4:33 p.m., Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:
> 
>>> +typedef gdb::optional<std::vector<lm_info_aix>> library_list_type;
> 
> Simon> Hmm, I find that hiding the gdb::optional behind a typedef can be a bit
> Simon> confusing.  It's easy to forget that it's an optional if it's not "in your
> Simon> face".  I think it would help to be explicit and spell out gdb::optional
> Simon> where needed.
> 
> Makes sense, I've removed the typedef.
> 
> I made the other changes you mentioned, except...
> 
>>> -  for (ix = 1; VEC_iterate (lm_info_aix_p, library_list, ix, info); ix++)
>>> +  for (ix = 1; ix < library_list->size (); ix++)
> 
> Simon> This could be:
> 
> Simon>   for (lm_info_aix &info : *library_list)
> 
> This isn't quite the same, because the current loop starts at index 1,
> not 0.  So, I didn't make this change.

Ah good point, you are right, sorry about that!
  

Patch

diff --git a/gdb/solib-aix.c b/gdb/solib-aix.c
index bf2f30d01cd..8f0a3ca9231 100644
--- a/gdb/solib-aix.c
+++ b/gdb/solib-aix.c
@@ -59,14 +59,13 @@  struct lm_info_aix : public lm_info_base
   ULONGEST data_size = 0;
 };
 
-typedef lm_info_aix *lm_info_aix_p;
-DEF_VEC_P(lm_info_aix_p);
+typedef gdb::optional<std::vector<lm_info_aix>> library_list_type;
 
 /* This module's per-inferior data.  */
 
 struct solib_aix_inferior_data
 {
-  /* The list of shared libraries.  NULL if not computed yet.
+  /* The list of shared libraries.
 
      Note that the first element of this list is always the main
      executable, which is not technically a shared library.  But
@@ -74,11 +73,11 @@  struct solib_aix_inferior_data
      the same principles applied to shared libraries also apply
      to the main executable.  So it's simpler to keep it as part
      of this list.  */
-  VEC (lm_info_aix_p) *library_list;
+  library_list_type library_list;
 };
 
 /* Key to our per-inferior data.  */
-static const struct inferior_data *solib_aix_inferior_data_handle;
+static inferior_key<solib_aix_inferior_data> solib_aix_inferior_data_handle;
 
 /* Return this module's data for the given inferior.
    If none is found, add a zero'ed one now.  */
@@ -88,13 +87,9 @@  get_solib_aix_inferior_data (struct inferior *inf)
 {
   struct solib_aix_inferior_data *data;
 
-  data = ((struct solib_aix_inferior_data *)
-	  inferior_data (inf, solib_aix_inferior_data_handle));
+  data = solib_aix_inferior_data_handle.get (inf);
   if (data == NULL)
-    {
-      data = XCNEW (struct solib_aix_inferior_data);
-      set_inferior_data (inf, solib_aix_inferior_data_handle, data);
-    }
+    data = solib_aix_inferior_data_handle.emplace (inf);
 
   return data;
 }
@@ -103,7 +98,7 @@  get_solib_aix_inferior_data (struct inferior *inf)
 
 /* Dummy implementation if XML support is not compiled in.  */
 
-static VEC (lm_info_aix_p) *
+static library_list_type
 solib_aix_parse_libraries (const char *library)
 {
   static int have_warned;
@@ -115,14 +110,7 @@  solib_aix_parse_libraries (const char *library)
                  "at compile time"));
     }
 
-  return NULL;
-}
-
-/* Dummy implementation if XML support is not compiled in.  */
-
-static void
-solib_aix_free_library_list (void *p)
-{
+  return {};
 }
 
 #else /* HAVE_LIBEXPAT */
@@ -137,30 +125,30 @@  library_list_start_library (struct gdb_xml_parser *parser,
 			    void *user_data,
 			    std::vector<gdb_xml_value> &attributes)
 {
-  VEC (lm_info_aix_p) **list = (VEC (lm_info_aix_p) **) user_data;
-  lm_info_aix *item = new lm_info_aix;
+  std::vector<lm_info_aix> *list = (std::vector<lm_info_aix> *) user_data;
+  lm_info_aix item;
   struct gdb_xml_value *attr;
 
   attr = xml_find_attribute (attributes, "name");
-  item->filename = xstrdup ((const char *) attr->value.get ());
+  item.filename = (const char *) attr->value.get ();
 
   attr = xml_find_attribute (attributes, "member");
   if (attr != NULL)
-    item->member_name = xstrdup ((const char *) attr->value.get ());
+    item.member_name = (const char *) attr->value.get ();
 
   attr = xml_find_attribute (attributes, "text_addr");
-  item->text_addr = * (ULONGEST *) attr->value.get ();
+  item.text_addr = * (ULONGEST *) attr->value.get ();
 
   attr = xml_find_attribute (attributes, "text_size");
-  item->text_size = * (ULONGEST *) attr->value.get ();
+  item.text_size = * (ULONGEST *) attr->value.get ();
 
   attr = xml_find_attribute (attributes, "data_addr");
-  item->data_addr = * (ULONGEST *) attr->value.get ();
+  item.data_addr = * (ULONGEST *) attr->value.get ();
 
   attr = xml_find_attribute (attributes, "data_size");
-  item->data_size = * (ULONGEST *) attr->value.get ();
+  item.data_size = * (ULONGEST *) attr->value.get ();
 
-  VEC_safe_push (lm_info_aix_p, *list, item);
+  list->push_back (std::move (item));
 }
 
 /* Handle the start of a <library-list-aix> element.  */
@@ -180,25 +168,6 @@  library_list_start_list (struct gdb_xml_parser *parser,
                    version);
 }
 
-/* Discard the constructed library list.  */
-
-static void
-solib_aix_free_library_list (void *p)
-{
-  VEC (lm_info_aix_p) **result = (VEC (lm_info_aix_p) **) p;
-  lm_info_aix *info;
-  int ix;
-
-  if (solib_aix_debug)
-    fprintf_unfiltered (gdb_stdlog, "DEBUG: solib_aix_free_library_list\n");
-
-  for (ix = 0; VEC_iterate (lm_info_aix_p, *result, ix, info); ix++)
-    delete info;
-
-  VEC_free (lm_info_aix_p, *result);
-  *result = NULL;
-}
-
 /* The allowed elements and attributes for an AIX library list
    described in XML format.  The root element is a <library-list-aix>.  */
 
@@ -237,26 +206,18 @@  static const struct gdb_xml_element library_list_elements[] =
 /* Parse LIBRARY, a string containing the loader info in XML format,
    and return an lm_info_aix_p vector.
 
-   Return NULL if the parsing failed.  */
+   Return false if the parsing failed.  */
 
-static VEC (lm_info_aix_p) *
+static library_list_type
 solib_aix_parse_libraries (const char *library)
 {
-  VEC (lm_info_aix_p) *result = NULL;
-  auto cleanup = make_scope_exit ([&] ()
-    {
-      solib_aix_free_library_list (&result);
-    });
+  std::vector<lm_info_aix> result;
 
   if (gdb_xml_parse_quick (_("aix library list"), "library-list-aix.dtd",
-                           library_list_elements, library, &result) == 0)
-    {
-      /* Parsed successfully, keep the result.  */
-      cleanup.release ();
-      return result;
-    }
+			   library_list_elements, library, &result) == 0)
+    return library_list_type (std::move (result));
 
-  return NULL;
+  return {};
 }
 
 #endif /* HAVE_LIBEXPAT */
@@ -271,14 +232,14 @@  solib_aix_parse_libraries (const char *library)
    is not NULL, then print a warning including WARNING_MSG and
    a description of the error.  */
 
-static VEC (lm_info_aix_p) *
+static library_list_type &
 solib_aix_get_library_list (struct inferior *inf, const char *warning_msg)
 {
   struct solib_aix_inferior_data *data;
 
   /* If already computed, return the cached value.  */
   data = get_solib_aix_inferior_data (inf);
-  if (data->library_list != NULL)
+  if (data->library_list.has_value ())
     return data->library_list;
 
   gdb::optional<gdb::char_vector> library_document
@@ -288,7 +249,7 @@  solib_aix_get_library_list (struct inferior *inf, const char *warning_msg)
     {
       warning (_("%s (failed to read TARGET_OBJECT_LIBRARIES_AIX)"),
 	       warning_msg);
-      return NULL;
+      return data->library_list;
     }
 
   if (solib_aix_debug)
@@ -297,11 +258,8 @@  solib_aix_get_library_list (struct inferior *inf, const char *warning_msg)
 			library_document->data ());
 
   data->library_list = solib_aix_parse_libraries (library_document->data ());
-  if (data->library_list == NULL && warning_msg != NULL)
-    {
-      warning (_("%s (missing XML support?)"), warning_msg);
-      return NULL;
-    }
+  if (!data->library_list.has_value () && warning_msg != NULL)
+    warning (_("%s (missing XML support?)"), warning_msg);
 
   return data->library_list;
 }
@@ -497,28 +455,25 @@  static void
 solib_aix_solib_create_inferior_hook (int from_tty)
 {
   const char *warning_msg = "unable to relocate main executable";
-  VEC (lm_info_aix_p) *library_list;
-  lm_info_aix *exec_info;
 
   /* We need to relocate the main executable...  */
 
-  library_list = solib_aix_get_library_list (current_inferior (),
-					     warning_msg);
-  if (library_list == NULL)
+  library_list_type &library_list
+    = solib_aix_get_library_list (current_inferior (), warning_msg);
+  if (!library_list.has_value ())
     return;  /* Warning already printed.  */
 
-  if (VEC_length (lm_info_aix_p, library_list) < 1)
+  if (library_list->empty ())
     {
       warning (_("unable to relocate main executable (no info from loader)"));
       return;
     }
 
-  exec_info = VEC_index (lm_info_aix_p, library_list, 0);
-
+  lm_info_aix &exec_info = (*library_list)[0];
   if (symfile_objfile != NULL)
     {
       gdb::unique_xmalloc_ptr<struct section_offsets> offsets
-	= solib_aix_get_section_offsets (symfile_objfile, exec_info);
+	= solib_aix_get_section_offsets (symfile_objfile, &exec_info);
 
       objfile_relocate (symfile_objfile, offsets.get ());
     }
@@ -530,29 +485,29 @@  static struct so_list *
 solib_aix_current_sos (void)
 {
   struct so_list *start = NULL, *last = NULL;
-  VEC (lm_info_aix_p) *library_list;
-  lm_info_aix *info;
   int ix;
 
-  library_list = solib_aix_get_library_list (current_inferior (), NULL);
-  if (library_list == NULL)
+  library_list_type &library_list
+    = solib_aix_get_library_list (current_inferior (), NULL);
+  if (!library_list.has_value ())
     return NULL;
 
   /* Build a struct so_list for each entry on the list.
      We skip the first entry, since this is the entry corresponding
      to the main executable, not a shared library.  */
-  for (ix = 1; VEC_iterate (lm_info_aix_p, library_list, ix, info); ix++)
+  for (ix = 1; ix < library_list->size (); ix++)
     {
       struct so_list *new_solib = XCNEW (struct so_list);
       std::string so_name;
 
-      if (info->member_name.empty ())
+      lm_info_aix &info = (*library_list)[ix];
+      if (info.member_name.empty ())
 	{
-	 /* INFO->FILENAME is probably not an archive, but rather
+	 /* INFO.FILENAME is probably not an archive, but rather
 	    a shared object.  Unusual, but it should be possible
 	    to link a program against a shared object directory,
 	    without having to put it in an archive first.  */
-	 so_name = info->filename;
+	 so_name = info.filename;
 	}
       else
 	{
@@ -560,15 +515,15 @@  solib_aix_current_sos (void)
 	    is a member of an archive.  Create a synthetic so_name
 	    that follows the same convention as AIX's ldd tool
 	    (Eg: "/lib/libc.a(shr.o)").  */
-	 so_name = string_printf ("%s(%s)", info->filename.c_str (),
-				  info->member_name.c_str ());
+	 so_name = string_printf ("%s(%s)", info.filename.c_str (),
+				  info.member_name.c_str ());
 	}
       strncpy (new_solib->so_original_name, so_name.c_str (),
 	       SO_NAME_MAX_PATH_SIZE - 1);
       new_solib->so_name[SO_NAME_MAX_PATH_SIZE - 1] = '\0';
       memcpy (new_solib->so_name, new_solib->so_original_name,
 	      SO_NAME_MAX_PATH_SIZE);
-      new_solib->lm_info = new lm_info_aix (*info);
+      new_solib->lm_info = new lm_info_aix (info);
 
       /* Add it to the list.  */
       if (!start)
@@ -758,7 +713,7 @@  solib_aix_normal_stop_observer (struct bpstats *unused_1, int unused_2)
   /* The inferior execution has been resumed, and it just stopped
      again.  This means that the list of shared libraries may have
      evolved.  Reset our cached value.  */
-  solib_aix_free_library_list (&data->library_list);
+  data->library_list.reset ();
 }
 
 /* Implements the "show debug aix-solib" command.  */
@@ -789,8 +744,6 @@  _initialize_solib_aix (void)
     = solib_aix_in_dynsym_resolve_code;
   solib_aix_so_ops.bfd_open = solib_aix_bfd_open;
 
-  solib_aix_inferior_data_handle = register_inferior_data ();
-
   gdb::observers::normal_stop.attach (solib_aix_normal_stop_observer);
 
   /* Debug this file's internals.  */