[PATCH 1/4] gdb: remove target description macros

[PATCH 1/4] gdb: remove target description macros

[Simon Marchi]

In my opinion, the target_desc_fetched, current_target_desc and
target_description_filename macros in target-descriptions.c are not very
useful.  I don't think it's useful to hide that they operate on the
current inferior, as everything currently works under the assumption
that the various tdesc commands operate on the current inferior, and I
don't see that changing in the foreseeable future.

This change also avoids having multiple unnecessary calls to
current_inferior and get_tdesc_info per function.

gdb/ChangeLog:

	* target-descriptions.c (struct target_desc_info) <tdesc>:
	Adjust doc.
	(target_desc_fetched): Remove.
	(current_target_desc): Remove.
	(target_description_filename): Remove.
	(target_find_description): Adjust.
	(target_clear_description): Adjust.
	(target_current_description): Adjust.
	(set_tdesc_filename_cmd): Adjust.
	(show_tdesc_filename_cmd): Adjust.
	(unset_tdesc_filename_cmd): Adjust.
	(maint_print_c_tdesc_cmd): Adjust.
	(maint_print_xml_tdesc_cmd): Adjust.

Change-Id: Ibfb581490e949c16d59924e2cac633ede5c26c5b
---
 gdb/target-descriptions.c | 75 +++++++++++++++++++--------------------
 1 file changed, 37 insertions(+), 38 deletions(-)

diff --git a/gdb/target-descriptions.c b/gdb/target-descriptions.c
index c0e798a3530a..fd9e5a9d115d 100644
--- a/gdb/target-descriptions.c
+++ b/gdb/target-descriptions.c
@@ -450,7 +450,7 @@ struct target_desc_info
 
   /* The description fetched from the target, or NULL if the target
      did not supply any description.  Only valid when
-     target_desc_fetched is set.  Only the description initialization
+     FETCHED is set.  Only the description initialization
      code should access this; normally, the description should be
      accessed through the gdbarch object.  */
 
@@ -511,15 +511,6 @@ target_desc_info_free (struct target_desc_info *tdesc_info)
     }
 }
 
-/* Convenience helper macros.  */
-
-#define target_desc_fetched \
-  get_tdesc_info (current_inferior ())->fetched
-#define current_target_desc \
-  get_tdesc_info (current_inferior ())->tdesc
-#define target_description_filename \
-  get_tdesc_info (current_inferior ())->filename
-
 /* The string manipulated by the "set tdesc filename ..." command.  */
 
 static char *tdesc_filename_cmd_string;
@@ -530,11 +521,13 @@ static char *tdesc_filename_cmd_string;
 void
 target_find_description (void)
 {
+  target_desc_info *tdesc_info = get_tdesc_info (current_inferior ());
+
   /* If we've already fetched a description from the target, don't do
      it again.  This allows a target to fetch the description early,
      during its to_open or to_create_inferior, if it needs extra
      information about the target to initialize.  */
-  if (target_desc_fetched)
+  if (tdesc_info->fetched)
     return;
 
   /* The current architecture should not have any target description
@@ -544,31 +537,29 @@ target_find_description (void)
 
   /* First try to fetch an XML description from the user-specified
      file.  */
-  current_target_desc = NULL;
-  if (target_description_filename != NULL
-      && *target_description_filename != '\0')
-    current_target_desc
-      = file_read_description_xml (target_description_filename);
+  tdesc_info->tdesc = nullptr;
+  if (tdesc_info->filename != nullptr && *tdesc_info->filename != '\0')
+    tdesc_info->tdesc= file_read_description_xml (tdesc_info->filename);
 
   /* Next try to read the description from the current target using
      target objects.  */
-  if (current_target_desc == NULL)
-    current_target_desc = target_read_description_xml
+  if (tdesc_info->tdesc == nullptr)
+    tdesc_info->tdesc = target_read_description_xml
       (current_inferior ()->top_target ());
 
   /* If that failed try a target-specific hook.  */
-  if (current_target_desc == NULL)
-    current_target_desc = target_read_description
+  if (tdesc_info->tdesc == nullptr)
+    tdesc_info->tdesc = target_read_description
       (current_inferior ()->top_target ());
 
   /* If a non-NULL description was returned, then update the current
      architecture.  */
-  if (current_target_desc)
+  if (tdesc_info->tdesc != nullptr)
     {
       struct gdbarch_info info;
 
       gdbarch_info_init (&info);
-      info.target_desc = current_target_desc;
+      info.target_desc = tdesc_info->tdesc;
       if (!gdbarch_update_p (info))
 	warning (_("Architecture rejected target-supplied description"));
       else
@@ -577,7 +568,7 @@ target_find_description (void)
 
 	  data = ((struct tdesc_arch_data *)
 		  gdbarch_data (target_gdbarch (), tdesc_data));
-	  if (tdesc_has_registers (current_target_desc)
+	  if (tdesc_has_registers (tdesc_info->tdesc)
 	      && data->arch_regs.empty ())
 	    warning (_("Target-supplied registers are not supported "
 		       "by the current architecture"));
@@ -586,7 +577,7 @@ target_find_description (void)
 
   /* Now that we know this description is usable, record that we
      fetched it.  */
-  target_desc_fetched = 1;
+  tdesc_info->fetched = 1;
 }
 
 /* Discard any description fetched from the current target, and switch
@@ -595,14 +586,15 @@ target_find_description (void)
 void
 target_clear_description (void)
 {
-  struct gdbarch_info info;
+  target_desc_info *tdesc_info = get_tdesc_info (current_inferior ());
 
-  if (!target_desc_fetched)
+  if (!tdesc_info->fetched)
     return;
 
-  target_desc_fetched = 0;
-  current_target_desc = NULL;
+  tdesc_info->fetched = 0;
+  tdesc_info->tdesc = nullptr;
 
+  gdbarch_info info;
   gdbarch_info_init (&info);
   if (!gdbarch_update_p (info))
     internal_error (__FILE__, __LINE__,
@@ -616,8 +608,10 @@ target_clear_description (void)
 const struct target_desc *
 target_current_description (void)
 {
-  if (target_desc_fetched)
-    return current_target_desc;
+  target_desc_info *tdesc_info = get_tdesc_info (current_inferior ());
+
+  if (tdesc_info->fetched)
+    return tdesc_info->tdesc;
 
   return NULL;
 }
@@ -1298,8 +1292,10 @@ static void
 set_tdesc_filename_cmd (const char *args, int from_tty,
 			struct cmd_list_element *c)
 {
-  xfree (target_description_filename);
-  target_description_filename = xstrdup (tdesc_filename_cmd_string);
+  target_desc_info *tdesc_info = get_tdesc_info (current_inferior ());
+
+  xfree (tdesc_info->filename);
+  tdesc_info->filename = xstrdup (tdesc_filename_cmd_string);
 
   target_clear_description ();
   target_find_description ();
@@ -1310,7 +1306,7 @@ show_tdesc_filename_cmd (struct ui_file *file, int from_tty,
 			 struct cmd_list_element *c,
 			 const char *value)
 {
-  value = target_description_filename;
+  value = get_tdesc_info (current_inferior ())->filename;
 
   if (value != NULL && *value != '\0')
     printf_filtered (_("The target description will be read from \"%s\".\n"),
@@ -1323,8 +1319,10 @@ show_tdesc_filename_cmd (struct ui_file *file, int from_tty,
 static void
 unset_tdesc_filename_cmd (const char *args, int from_tty)
 {
-  xfree (target_description_filename);
-  target_description_filename = NULL;
+  target_desc_info *tdesc_info = get_tdesc_info (current_inferior ());
+
+  xfree (tdesc_info->filename);
+  tdesc_info->filename = nullptr;
   target_clear_description ();
   target_find_description ();
 }
@@ -1778,8 +1776,9 @@ maint_print_c_tdesc_cmd (const char *args, int from_tty)
 	 architecture's.  This lets a GDB for one architecture generate C
 	 for another architecture's description, even though the gdbarch
 	 initialization code will reject the new description.  */
-      tdesc = current_target_desc;
-      filename = target_description_filename;
+      target_desc_info *tdesc_info = get_tdesc_info (current_inferior ());
+      tdesc = tdesc_info->tdesc;
+      filename = tdesc_info->filename;
     }
   else
     {
@@ -1850,7 +1849,7 @@ maint_print_xml_tdesc_cmd (const char *args, int from_tty)
 	 architecture's.  This lets a GDB for one architecture generate XML
 	 for another architecture's description, even though the gdbarch
 	 initialization code will reject the new description.  */
-      tdesc = current_target_desc;
+      tdesc = get_tdesc_info (current_inferior ())->tdesc;
     }
   else
     {
-- 
2.30.1

[PATCH 2/4] gdb: change target_desc_info::fetched to bool

[Simon Marchi]

gdb/ChangeLog:

	* target-descriptions.c (struct target_desc_info) <fetched>:
	bool.
	(target_find_description): Adjust.
	(target_clear_description): Adjust.

Change-Id: Ib69e097b38cf270e674f1249105d535a312954e1
---
 gdb/target-descriptions.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/gdb/target-descriptions.c b/gdb/target-descriptions.c
index fd9e5a9d115d..aee1478301b6 100644
--- a/gdb/target-descriptions.c
+++ b/gdb/target-descriptions.c
@@ -446,7 +446,7 @@ struct target_desc_info
   /* A flag indicating that a description has already been fetched
      from the target, so it should not be queried again.  */
 
-  int fetched;
+  bool fetched;
 
   /* The description fetched from the target, or NULL if the target
      did not supply any description.  Only valid when
@@ -577,7 +577,7 @@ target_find_description (void)
 
   /* Now that we know this description is usable, record that we
      fetched it.  */
-  tdesc_info->fetched = 1;
+  tdesc_info->fetched = true;
 }
 
 /* Discard any description fetched from the current target, and switch
@@ -591,7 +591,7 @@ target_clear_description (void)
   if (!tdesc_info->fetched)
     return;
 
-  tdesc_info->fetched = 0;
+  tdesc_info->fetched = false;
   tdesc_info->tdesc = nullptr;
 
   gdbarch_info info;
-- 
2.30.1

Re: [PATCH 2/4] gdb: change target_desc_info::fetched to bool

[Andrew Burgess]

* Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> [2021-05-06 12:49:55 -0400]:

> gdb/ChangeLog:
> 
> 	* target-descriptions.c (struct target_desc_info) <fetched>:
> 	bool.
> 	(target_find_description): Adjust.
> 	(target_clear_description): Adjust.

LGTM.

Thanks,
Andrew


> 
> Change-Id: Ib69e097b38cf270e674f1249105d535a312954e1
> ---
>  gdb/target-descriptions.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/gdb/target-descriptions.c b/gdb/target-descriptions.c
> index fd9e5a9d115d..aee1478301b6 100644
> --- a/gdb/target-descriptions.c
> +++ b/gdb/target-descriptions.c
> @@ -446,7 +446,7 @@ struct target_desc_info
>    /* A flag indicating that a description has already been fetched
>       from the target, so it should not be queried again.  */
>  
> -  int fetched;
> +  bool fetched;
>  
>    /* The description fetched from the target, or NULL if the target
>       did not supply any description.  Only valid when
> @@ -577,7 +577,7 @@ target_find_description (void)
>  
>    /* Now that we know this description is usable, record that we
>       fetched it.  */
> -  tdesc_info->fetched = 1;
> +  tdesc_info->fetched = true;
>  }
>  
>  /* Discard any description fetched from the current target, and switch
> @@ -591,7 +591,7 @@ target_clear_description (void)
>    if (!tdesc_info->fetched)
>      return;
>  
> -  tdesc_info->fetched = 0;
> +  tdesc_info->fetched = false;
>    tdesc_info->tdesc = nullptr;
>  
>    gdbarch_info info;
> -- 
> 2.30.1
> 

[PATCH 3/4] gdb: (de-)allocate target_desc_info with new/delete

[Simon Marchi]

In preparation for using non-POD types in the struct.

gdb/ChangeLog:

	* target-descriptions.c (struct target_desc_info): Initialize
	fields.
	(get_tdesc_info): Use new.
	(target_desc_info_free): Use delete.

Change-Id: I10fdaeeae7cdbd7930ae7adeeb13f7f363c67c7a
---
 gdb/target-descriptions.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/gdb/target-descriptions.c b/gdb/target-descriptions.c
index aee1478301b6..341020fed762 100644
--- a/gdb/target-descriptions.c
+++ b/gdb/target-descriptions.c
@@ -446,7 +446,7 @@ struct target_desc_info
   /* A flag indicating that a description has already been fetched
      from the target, so it should not be queried again.  */
 
-  bool fetched;
+  bool fetched = false;
 
   /* The description fetched from the target, or NULL if the target
      did not supply any description.  Only valid when
@@ -454,12 +454,12 @@ struct target_desc_info
      code should access this; normally, the description should be
      accessed through the gdbarch object.  */
 
-  const struct target_desc *tdesc;
+  const struct target_desc *tdesc = nullptr;
 
   /* The filename to read a target description from, as set by "set
      tdesc filename ..."  */
 
-  char *filename;
+  char *filename = nullptr;
 };
 
 /* Get the inferior INF's target description info, allocating one on
@@ -469,7 +469,8 @@ static struct target_desc_info *
 get_tdesc_info (struct inferior *inf)
 {
   if (inf->tdesc_info == NULL)
-    inf->tdesc_info = XCNEW (struct target_desc_info);
+    inf->tdesc_info = new target_desc_info;
+
   return inf->tdesc_info;
 }
 
@@ -507,7 +508,7 @@ target_desc_info_free (struct target_desc_info *tdesc_info)
   if (tdesc_info != NULL)
     {
       xfree (tdesc_info->filename);
-      xfree (tdesc_info);
+      delete tdesc_info;
     }
 }
 
-- 
2.30.1

Re: [PATCH 3/4] gdb: (de-)allocate target_desc_info with new/delete

[Andrew Burgess]

* Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> [2021-05-06 12:49:56 -0400]:

> In preparation for using non-POD types in the struct.
> 
> gdb/ChangeLog:
> 
> 	* target-descriptions.c (struct target_desc_info): Initialize
> 	fields.
> 	(get_tdesc_info): Use new.
> 	(target_desc_info_free): Use delete.

LGTM.

Thanks,
Andrew


> 
> Change-Id: I10fdaeeae7cdbd7930ae7adeeb13f7f363c67c7a
> ---
>  gdb/target-descriptions.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/gdb/target-descriptions.c b/gdb/target-descriptions.c
> index aee1478301b6..341020fed762 100644
> --- a/gdb/target-descriptions.c
> +++ b/gdb/target-descriptions.c
> @@ -446,7 +446,7 @@ struct target_desc_info
>    /* A flag indicating that a description has already been fetched
>       from the target, so it should not be queried again.  */
>  
> -  bool fetched;
> +  bool fetched = false;
>  
>    /* The description fetched from the target, or NULL if the target
>       did not supply any description.  Only valid when
> @@ -454,12 +454,12 @@ struct target_desc_info
>       code should access this; normally, the description should be
>       accessed through the gdbarch object.  */
>  
> -  const struct target_desc *tdesc;
> +  const struct target_desc *tdesc = nullptr;
>  
>    /* The filename to read a target description from, as set by "set
>       tdesc filename ..."  */
>  
> -  char *filename;
> +  char *filename = nullptr;
>  };
>  
>  /* Get the inferior INF's target description info, allocating one on
> @@ -469,7 +469,8 @@ static struct target_desc_info *
>  get_tdesc_info (struct inferior *inf)
>  {
>    if (inf->tdesc_info == NULL)
> -    inf->tdesc_info = XCNEW (struct target_desc_info);
> +    inf->tdesc_info = new target_desc_info;
> +
>    return inf->tdesc_info;
>  }
>  
> @@ -507,7 +508,7 @@ target_desc_info_free (struct target_desc_info *tdesc_info)
>    if (tdesc_info != NULL)
>      {
>        xfree (tdesc_info->filename);
> -      xfree (tdesc_info);
> +      delete tdesc_info;
>      }
>  }
>  
> -- 
> 2.30.1
> 

[PATCH 4/4] gdb: make target_desc_info::filename an std::string

[Simon Marchi]

To make the management of memory automatic.

As to why I chose to make this an std::string and not an
std::unique_xmalloc_ptr<char>: some parts of the code consider both a
NULL value and an empty string value to mean "no filename".
target_desc_info_from_user_p, however, doesn't check for a non-NULL but
empty string value.  So it seems like having two ways of denoting "no
filename" can lead to these kinds of inconsistencies.  Using
std::string, "no filename" is only represented by an empty value.

As a bonus, using an std::string lets us copy target_desc_info objects
using the default assignment operator.

gdb/ChangeLog:

	* target-descriptions.c (struct target_desc_info) <filename>:
	Make std::string.
	(copy_inferior_target_desc_info): Adjust.
	(target_desc_info_free): Adjust.
	(target_find_description): Adjust.
	(set_tdesc_filename_cmd): Adjust.
	(show_tdesc_filename_cmd): Adjust.
	(unset_tdesc_filename_cmd): Adjust.
	(maint_print_c_tdesc_cmd): Adjust.

Change-Id: I4e3a6ad8ccda2b88c202471d4f54249753cad127
---
 gdb/target-descriptions.c | 34 ++++++++++++++--------------------
 1 file changed, 14 insertions(+), 20 deletions(-)

diff --git a/gdb/target-descriptions.c b/gdb/target-descriptions.c
index 341020fed762..053864dec58b 100644
--- a/gdb/target-descriptions.c
+++ b/gdb/target-descriptions.c
@@ -456,10 +456,12 @@ struct target_desc_info
 
   const struct target_desc *tdesc = nullptr;
 
-  /* The filename to read a target description from, as set by "set
-     tdesc filename ..."  */
+  /* If not empty, the filename to read a target description from, as set by
+     "set tdesc filename ...".
 
-  char *filename = nullptr;
+     If empty, there is not filename specified by the user.  */
+
+  std::string filename;
 };
 
 /* Get the inferior INF's target description info, allocating one on
@@ -484,7 +486,7 @@ static struct gdbarch_data *tdesc_data;
 int
 target_desc_info_from_user_p (struct target_desc_info *info)
 {
-  return info != NULL && info->filename != NULL;
+  return info != nullptr && !info->filename.empty ();
 }
 
 /* See target-descriptions.h.  */
@@ -495,9 +497,7 @@ copy_inferior_target_desc_info (struct inferior *destinf, struct inferior *srcin
   struct target_desc_info *src = get_tdesc_info (srcinf);
   struct target_desc_info *dest = get_tdesc_info (destinf);
 
-  dest->fetched = src->fetched;
-  dest->tdesc = src->tdesc;
-  dest->filename = src->filename != NULL ? xstrdup (src->filename) : NULL;
+  *dest = *src;
 }
 
 /* See target-descriptions.h.  */
@@ -505,11 +505,7 @@ copy_inferior_target_desc_info (struct inferior *destinf, struct inferior *srcin
 void
 target_desc_info_free (struct target_desc_info *tdesc_info)
 {
-  if (tdesc_info != NULL)
-    {
-      xfree (tdesc_info->filename);
-      delete tdesc_info;
-    }
+  delete tdesc_info;
 }
 
 /* The string manipulated by the "set tdesc filename ..." command.  */
@@ -539,8 +535,8 @@ target_find_description (void)
   /* First try to fetch an XML description from the user-specified
      file.  */
   tdesc_info->tdesc = nullptr;
-  if (tdesc_info->filename != nullptr && *tdesc_info->filename != '\0')
-    tdesc_info->tdesc= file_read_description_xml (tdesc_info->filename);
+  if (!tdesc_info->filename.empty ())
+    tdesc_info->tdesc = file_read_description_xml (tdesc_info->filename.data ());
 
   /* Next try to read the description from the current target using
      target objects.  */
@@ -1295,8 +1291,7 @@ set_tdesc_filename_cmd (const char *args, int from_tty,
 {
   target_desc_info *tdesc_info = get_tdesc_info (current_inferior ());
 
-  xfree (tdesc_info->filename);
-  tdesc_info->filename = xstrdup (tdesc_filename_cmd_string);
+  tdesc_info->filename = tdesc_filename_cmd_string;
 
   target_clear_description ();
   target_find_description ();
@@ -1307,7 +1302,7 @@ show_tdesc_filename_cmd (struct ui_file *file, int from_tty,
 			 struct cmd_list_element *c,
 			 const char *value)
 {
-  value = get_tdesc_info (current_inferior ())->filename;
+  value = get_tdesc_info (current_inferior ())->filename.data ();
 
   if (value != NULL && *value != '\0')
     printf_filtered (_("The target description will be read from \"%s\".\n"),
@@ -1322,8 +1317,7 @@ unset_tdesc_filename_cmd (const char *args, int from_tty)
 {
   target_desc_info *tdesc_info = get_tdesc_info (current_inferior ());
 
-  xfree (tdesc_info->filename);
-  tdesc_info->filename = nullptr;
+  tdesc_info->filename.clear ();
   target_clear_description ();
   target_find_description ();
 }
@@ -1779,7 +1773,7 @@ maint_print_c_tdesc_cmd (const char *args, int from_tty)
 	 initialization code will reject the new description.  */
       target_desc_info *tdesc_info = get_tdesc_info (current_inferior ());
       tdesc = tdesc_info->tdesc;
-      filename = tdesc_info->filename;
+      filename = tdesc_info->filename.data ();
     }
   else
     {
-- 
2.30.1

Re: [PATCH 4/4] gdb: make target_desc_info::filename an std::string

[Andrew Burgess]

* Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> [2021-05-06 12:49:57 -0400]:

> To make the management of memory automatic.
> 
> As to why I chose to make this an std::string and not an
> std::unique_xmalloc_ptr<char>: some parts of the code consider both a
> NULL value and an empty string value to mean "no filename".
> target_desc_info_from_user_p, however, doesn't check for a non-NULL but
> empty string value.  So it seems like having two ways of denoting "no
> filename" can lead to these kinds of inconsistencies.  Using
> std::string, "no filename" is only represented by an empty value.
> 
> As a bonus, using an std::string lets us copy target_desc_info objects
> using the default assignment operator.
> 
> gdb/ChangeLog:
> 
> 	* target-descriptions.c (struct target_desc_info) <filename>:
> 	Make std::string.
> 	(copy_inferior_target_desc_info): Adjust.
> 	(target_desc_info_free): Adjust.
> 	(target_find_description): Adjust.
> 	(set_tdesc_filename_cmd): Adjust.
> 	(show_tdesc_filename_cmd): Adjust.
> 	(unset_tdesc_filename_cmd): Adjust.
> 	(maint_print_c_tdesc_cmd): Adjust.

LGTM.

Thanks,
Andrew

> 
> Change-Id: I4e3a6ad8ccda2b88c202471d4f54249753cad127
> ---
>  gdb/target-descriptions.c | 34 ++++++++++++++--------------------
>  1 file changed, 14 insertions(+), 20 deletions(-)
> 
> diff --git a/gdb/target-descriptions.c b/gdb/target-descriptions.c
> index 341020fed762..053864dec58b 100644
> --- a/gdb/target-descriptions.c
> +++ b/gdb/target-descriptions.c
> @@ -456,10 +456,12 @@ struct target_desc_info
>  
>    const struct target_desc *tdesc = nullptr;
>  
> -  /* The filename to read a target description from, as set by "set
> -     tdesc filename ..."  */
> +  /* If not empty, the filename to read a target description from, as set by
> +     "set tdesc filename ...".
>  
> -  char *filename = nullptr;
> +     If empty, there is not filename specified by the user.  */
> +
> +  std::string filename;
>  };
>  
>  /* Get the inferior INF's target description info, allocating one on
> @@ -484,7 +486,7 @@ static struct gdbarch_data *tdesc_data;
>  int
>  target_desc_info_from_user_p (struct target_desc_info *info)
>  {
> -  return info != NULL && info->filename != NULL;
> +  return info != nullptr && !info->filename.empty ();
>  }
>  
>  /* See target-descriptions.h.  */
> @@ -495,9 +497,7 @@ copy_inferior_target_desc_info (struct inferior *destinf, struct inferior *srcin
>    struct target_desc_info *src = get_tdesc_info (srcinf);
>    struct target_desc_info *dest = get_tdesc_info (destinf);
>  
> -  dest->fetched = src->fetched;
> -  dest->tdesc = src->tdesc;
> -  dest->filename = src->filename != NULL ? xstrdup (src->filename) : NULL;
> +  *dest = *src;
>  }
>  
>  /* See target-descriptions.h.  */
> @@ -505,11 +505,7 @@ copy_inferior_target_desc_info (struct inferior *destinf, struct inferior *srcin
>  void
>  target_desc_info_free (struct target_desc_info *tdesc_info)
>  {
> -  if (tdesc_info != NULL)
> -    {
> -      xfree (tdesc_info->filename);
> -      delete tdesc_info;
> -    }
> +  delete tdesc_info;
>  }
>  
>  /* The string manipulated by the "set tdesc filename ..." command.  */
> @@ -539,8 +535,8 @@ target_find_description (void)
>    /* First try to fetch an XML description from the user-specified
>       file.  */
>    tdesc_info->tdesc = nullptr;
> -  if (tdesc_info->filename != nullptr && *tdesc_info->filename != '\0')
> -    tdesc_info->tdesc= file_read_description_xml (tdesc_info->filename);
> +  if (!tdesc_info->filename.empty ())
> +    tdesc_info->tdesc = file_read_description_xml (tdesc_info->filename.data ());
>  
>    /* Next try to read the description from the current target using
>       target objects.  */
> @@ -1295,8 +1291,7 @@ set_tdesc_filename_cmd (const char *args, int from_tty,
>  {
>    target_desc_info *tdesc_info = get_tdesc_info (current_inferior ());
>  
> -  xfree (tdesc_info->filename);
> -  tdesc_info->filename = xstrdup (tdesc_filename_cmd_string);
> +  tdesc_info->filename = tdesc_filename_cmd_string;
>  
>    target_clear_description ();
>    target_find_description ();
> @@ -1307,7 +1302,7 @@ show_tdesc_filename_cmd (struct ui_file *file, int from_tty,
>  			 struct cmd_list_element *c,
>  			 const char *value)
>  {
> -  value = get_tdesc_info (current_inferior ())->filename;
> +  value = get_tdesc_info (current_inferior ())->filename.data ();
>  
>    if (value != NULL && *value != '\0')
>      printf_filtered (_("The target description will be read from \"%s\".\n"),
> @@ -1322,8 +1317,7 @@ unset_tdesc_filename_cmd (const char *args, int from_tty)
>  {
>    target_desc_info *tdesc_info = get_tdesc_info (current_inferior ());
>  
> -  xfree (tdesc_info->filename);
> -  tdesc_info->filename = nullptr;
> +  tdesc_info->filename.clear ();
>    target_clear_description ();
>    target_find_description ();
>  }
> @@ -1779,7 +1773,7 @@ maint_print_c_tdesc_cmd (const char *args, int from_tty)
>  	 initialization code will reject the new description.  */
>        target_desc_info *tdesc_info = get_tdesc_info (current_inferior ());
>        tdesc = tdesc_info->tdesc;
> -      filename = tdesc_info->filename;
> +      filename = tdesc_info->filename.data ();
>      }
>    else
>      {
> -- 
> 2.30.1
> 

Re: [PATCH 4/4] gdb: make target_desc_info::filename an std::string

[Simon Marchi]

On 2021-05-07 6:22 a.m., Andrew Burgess wrote:
> * Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> [2021-05-06 12:49:57 -0400]:
> 
>> To make the management of memory automatic.
>>
>> As to why I chose to make this an std::string and not an
>> std::unique_xmalloc_ptr<char>: some parts of the code consider both a
>> NULL value and an empty string value to mean "no filename".
>> target_desc_info_from_user_p, however, doesn't check for a non-NULL but
>> empty string value.  So it seems like having two ways of denoting "no
>> filename" can lead to these kinds of inconsistencies.  Using
>> std::string, "no filename" is only represented by an empty value.
>>
>> As a bonus, using an std::string lets us copy target_desc_info objects
>> using the default assignment operator.
>>
>> gdb/ChangeLog:
>>
>> 	* target-descriptions.c (struct target_desc_info) <filename>:
>> 	Make std::string.
>> 	(copy_inferior_target_desc_info): Adjust.
>> 	(target_desc_info_free): Adjust.
>> 	(target_find_description): Adjust.
>> 	(set_tdesc_filename_cmd): Adjust.
>> 	(show_tdesc_filename_cmd): Adjust.
>> 	(unset_tdesc_filename_cmd): Adjust.
>> 	(maint_print_c_tdesc_cmd): Adjust.
> 
> LGTM.
> 
> Thanks,
> Andrew

Thanks, all pushed.

Simon

Re: [PATCH 1/4] gdb: remove target description macros

[Andrew Burgess]

* Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> [2021-05-06 12:49:54 -0400]:

> In my opinion, the target_desc_fetched, current_target_desc and
> target_description_filename macros in target-descriptions.c are not very
> useful.  I don't think it's useful to hide that they operate on the
> current inferior, as everything currently works under the assumption
> that the various tdesc commands operate on the current inferior, and I
> don't see that changing in the foreseeable future.
> 
> This change also avoids having multiple unnecessary calls to
> current_inferior and get_tdesc_info per function.

Awesome!  One minor nit inline below.

> 
> gdb/ChangeLog:
> 
> 	* target-descriptions.c (struct target_desc_info) <tdesc>:
> 	Adjust doc.
> 	(target_desc_fetched): Remove.
> 	(current_target_desc): Remove.
> 	(target_description_filename): Remove.
> 	(target_find_description): Adjust.
> 	(target_clear_description): Adjust.
> 	(target_current_description): Adjust.
> 	(set_tdesc_filename_cmd): Adjust.
> 	(show_tdesc_filename_cmd): Adjust.
> 	(unset_tdesc_filename_cmd): Adjust.
> 	(maint_print_c_tdesc_cmd): Adjust.
> 	(maint_print_xml_tdesc_cmd): Adjust.
> 
> Change-Id: Ibfb581490e949c16d59924e2cac633ede5c26c5b
> ---
>  gdb/target-descriptions.c | 75 +++++++++++++++++++--------------------
>  1 file changed, 37 insertions(+), 38 deletions(-)
> 
> diff --git a/gdb/target-descriptions.c b/gdb/target-descriptions.c
> index c0e798a3530a..fd9e5a9d115d 100644
> --- a/gdb/target-descriptions.c
> +++ b/gdb/target-descriptions.c
> @@ -450,7 +450,7 @@ struct target_desc_info
>  
>    /* The description fetched from the target, or NULL if the target
>       did not supply any description.  Only valid when
> -     target_desc_fetched is set.  Only the description initialization
> +     FETCHED is set.  Only the description initialization
>       code should access this; normally, the description should be
>       accessed through the gdbarch object.  */
>  
> @@ -511,15 +511,6 @@ target_desc_info_free (struct target_desc_info *tdesc_info)
>      }
>  }
>  
> -/* Convenience helper macros.  */
> -
> -#define target_desc_fetched \
> -  get_tdesc_info (current_inferior ())->fetched
> -#define current_target_desc \
> -  get_tdesc_info (current_inferior ())->tdesc
> -#define target_description_filename \
> -  get_tdesc_info (current_inferior ())->filename
> -
>  /* The string manipulated by the "set tdesc filename ..." command.  */
>  
>  static char *tdesc_filename_cmd_string;
> @@ -530,11 +521,13 @@ static char *tdesc_filename_cmd_string;
>  void
>  target_find_description (void)
>  {
> +  target_desc_info *tdesc_info = get_tdesc_info (current_inferior ());
> +
>    /* If we've already fetched a description from the target, don't do
>       it again.  This allows a target to fetch the description early,
>       during its to_open or to_create_inferior, if it needs extra
>       information about the target to initialize.  */
> -  if (target_desc_fetched)
> +  if (tdesc_info->fetched)
>      return;
>  
>    /* The current architecture should not have any target description
> @@ -544,31 +537,29 @@ target_find_description (void)
>  
>    /* First try to fetch an XML description from the user-specified
>       file.  */
> -  current_target_desc = NULL;
> -  if (target_description_filename != NULL
> -      && *target_description_filename != '\0')
> -    current_target_desc
> -      = file_read_description_xml (target_description_filename);
> +  tdesc_info->tdesc = nullptr;
> +  if (tdesc_info->filename != nullptr && *tdesc_info->filename != '\0')
> +    tdesc_info->tdesc= file_read_description_xml (tdesc_info->filename);

Missed a space before '=' here.

Otherwise, looks great.

Thanks,
Andrew

>  
>    /* Next try to read the description from the current target using
>       target objects.  */
> -  if (current_target_desc == NULL)
> -    current_target_desc = target_read_description_xml
> +  if (tdesc_info->tdesc == nullptr)
> +    tdesc_info->tdesc = target_read_description_xml
>        (current_inferior ()->top_target ());
>  
>    /* If that failed try a target-specific hook.  */
> -  if (current_target_desc == NULL)
> -    current_target_desc = target_read_description
> +  if (tdesc_info->tdesc == nullptr)
> +    tdesc_info->tdesc = target_read_description
>        (current_inferior ()->top_target ());
>  
>    /* If a non-NULL description was returned, then update the current
>       architecture.  */
> -  if (current_target_desc)
> +  if (tdesc_info->tdesc != nullptr)
>      {
>        struct gdbarch_info info;
>  
>        gdbarch_info_init (&info);
> -      info.target_desc = current_target_desc;
> +      info.target_desc = tdesc_info->tdesc;
>        if (!gdbarch_update_p (info))
>  	warning (_("Architecture rejected target-supplied description"));
>        else
> @@ -577,7 +568,7 @@ target_find_description (void)
>  
>  	  data = ((struct tdesc_arch_data *)
>  		  gdbarch_data (target_gdbarch (), tdesc_data));
> -	  if (tdesc_has_registers (current_target_desc)
> +	  if (tdesc_has_registers (tdesc_info->tdesc)
>  	      && data->arch_regs.empty ())
>  	    warning (_("Target-supplied registers are not supported "
>  		       "by the current architecture"));
> @@ -586,7 +577,7 @@ target_find_description (void)
>  
>    /* Now that we know this description is usable, record that we
>       fetched it.  */
> -  target_desc_fetched = 1;
> +  tdesc_info->fetched = 1;
>  }
>  
>  /* Discard any description fetched from the current target, and switch
> @@ -595,14 +586,15 @@ target_find_description (void)
>  void
>  target_clear_description (void)
>  {
> -  struct gdbarch_info info;
> +  target_desc_info *tdesc_info = get_tdesc_info (current_inferior ());
>  
> -  if (!target_desc_fetched)
> +  if (!tdesc_info->fetched)
>      return;
>  
> -  target_desc_fetched = 0;
> -  current_target_desc = NULL;
> +  tdesc_info->fetched = 0;
> +  tdesc_info->tdesc = nullptr;
>  
> +  gdbarch_info info;
>    gdbarch_info_init (&info);
>    if (!gdbarch_update_p (info))
>      internal_error (__FILE__, __LINE__,
> @@ -616,8 +608,10 @@ target_clear_description (void)
>  const struct target_desc *
>  target_current_description (void)
>  {
> -  if (target_desc_fetched)
> -    return current_target_desc;
> +  target_desc_info *tdesc_info = get_tdesc_info (current_inferior ());
> +
> +  if (tdesc_info->fetched)
> +    return tdesc_info->tdesc;
>  
>    return NULL;
>  }
> @@ -1298,8 +1292,10 @@ static void
>  set_tdesc_filename_cmd (const char *args, int from_tty,
>  			struct cmd_list_element *c)
>  {
> -  xfree (target_description_filename);
> -  target_description_filename = xstrdup (tdesc_filename_cmd_string);
> +  target_desc_info *tdesc_info = get_tdesc_info (current_inferior ());
> +
> +  xfree (tdesc_info->filename);
> +  tdesc_info->filename = xstrdup (tdesc_filename_cmd_string);
>  
>    target_clear_description ();
>    target_find_description ();
> @@ -1310,7 +1306,7 @@ show_tdesc_filename_cmd (struct ui_file *file, int from_tty,
>  			 struct cmd_list_element *c,
>  			 const char *value)
>  {
> -  value = target_description_filename;
> +  value = get_tdesc_info (current_inferior ())->filename;
>  
>    if (value != NULL && *value != '\0')
>      printf_filtered (_("The target description will be read from \"%s\".\n"),
> @@ -1323,8 +1319,10 @@ show_tdesc_filename_cmd (struct ui_file *file, int from_tty,
>  static void
>  unset_tdesc_filename_cmd (const char *args, int from_tty)
>  {
> -  xfree (target_description_filename);
> -  target_description_filename = NULL;
> +  target_desc_info *tdesc_info = get_tdesc_info (current_inferior ());
> +
> +  xfree (tdesc_info->filename);
> +  tdesc_info->filename = nullptr;
>    target_clear_description ();
>    target_find_description ();
>  }
> @@ -1778,8 +1776,9 @@ maint_print_c_tdesc_cmd (const char *args, int from_tty)
>  	 architecture's.  This lets a GDB for one architecture generate C
>  	 for another architecture's description, even though the gdbarch
>  	 initialization code will reject the new description.  */
> -      tdesc = current_target_desc;
> -      filename = target_description_filename;
> +      target_desc_info *tdesc_info = get_tdesc_info (current_inferior ());
> +      tdesc = tdesc_info->tdesc;
> +      filename = tdesc_info->filename;
>      }
>    else
>      {
> @@ -1850,7 +1849,7 @@ maint_print_xml_tdesc_cmd (const char *args, int from_tty)
>  	 architecture's.  This lets a GDB for one architecture generate XML
>  	 for another architecture's description, even though the gdbarch
>  	 initialization code will reject the new description.  */
> -      tdesc = current_target_desc;
> +      tdesc = get_tdesc_info (current_inferior ())->tdesc;
>      }
>    else
>      {
> -- 
> 2.30.1
> 

Re: [PATCH 1/4] gdb: remove target description macros

[Simon Marchi]

On 2021-05-07 6:17 a.m., Andrew Burgess wrote:
>> +  if (tdesc_info->filename != nullptr && *tdesc_info->filename != '\0')
>> +    tdesc_info->tdesc= file_read_description_xml (tdesc_info->filename);
> Missed a space before '=' here.
> 
> Otherwise, looks great.
> 
> Thanks,
> Andrew
> 

Fixed, thanks!

Simon