[PATCH 0/6] Remove some uses of xfree

[PATCH 0/6] Remove some uses of xfree

[Tom Tromey]

I found a few spots using manual memory management that could easily
be changed to use unique_xmalloc_ptr or std::string.  I split this
into separate patches to make each change simpler to understand.

Regression tested on x86-64 Fedora 34.

Tom

[PATCH 1/6] Use unique_xmalloc_ptr in solib_catchpoint

[Tom Tromey]

This changes struct solib_catchpoint to use a unique_xmalloc_ptr,
removing a bit of manual memory management.
---
 gdb/breakpoint.c | 19 +++++++------------
 1 file changed, 7 insertions(+), 12 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index e742a1eccfe..7a9541836c9 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -7973,22 +7973,15 @@ static struct breakpoint_ops catch_vfork_breakpoint_ops;
 
 struct solib_catchpoint : public breakpoint
 {
-  ~solib_catchpoint () override;
-
   /* True for "catch load", false for "catch unload".  */
   bool is_load;
 
   /* Regular expression to match, if any.  COMPILED is only valid when
      REGEX is non-NULL.  */
-  char *regex;
+  gdb::unique_xmalloc_ptr<char> regex;
   std::unique_ptr<compiled_regex> compiled;
 };
 
-solib_catchpoint::~solib_catchpoint ()
-{
-  xfree (this->regex);
-}
-
 static int
 insert_catch_solib (struct bp_location *ignore)
 {
@@ -8104,14 +8097,16 @@ print_one_catch_solib (struct breakpoint *b, struct bp_location **locs)
   if (self->is_load)
     {
       if (self->regex)
-	msg = string_printf (_("load of library matching %s"), self->regex);
+	msg = string_printf (_("load of library matching %s"),
+			     self->regex.get ());
       else
 	msg = _("load of library");
     }
   else
     {
       if (self->regex)
-	msg = string_printf (_("unload of library matching %s"), self->regex);
+	msg = string_printf (_("unload of library matching %s"),
+			     self->regex.get ());
       else
 	msg = _("unload of library");
     }
@@ -8139,7 +8134,7 @@ print_recreate_catch_solib (struct breakpoint *b, struct ui_file *fp)
 		      b->disposition == disp_del ? "tcatch" : "catch",
 		      self->is_load ? "load" : "unload");
   if (self->regex)
-    fprintf_unfiltered (fp, " %s", self->regex);
+    fprintf_unfiltered (fp, " %s", self->regex.get ());
   fprintf_unfiltered (fp, "\n");
 }
 
@@ -8162,7 +8157,7 @@ add_solib_catchpoint (const char *arg, bool is_load, bool is_temp, bool enabled)
     {
       c->compiled.reset (new compiled_regex (arg, REG_NOSUB,
 					     _("Invalid regexp")));
-      c->regex = xstrdup (arg);
+      c->regex = make_unique_xstrdup (arg);
     }
 
   c->is_load = is_load;
-- 
2.31.1

[PATCH 2/6] Use unique_xmalloc_ptr in exec_catchpoint

[Tom Tromey]

This changes struct exec_catchpoint to use a unique_xmalloc_ptr,
removing a bit of manual memory management.
---
 gdb/breakpoint.c | 19 +++++--------------
 1 file changed, 5 insertions(+), 14 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 7a9541836c9..8016dd3b83f 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -8250,21 +8250,12 @@ create_fork_vfork_event_catchpoint (struct gdbarch *gdbarch,
 
 struct exec_catchpoint : public breakpoint
 {
-  ~exec_catchpoint () override;
-
   /* Filename of a program whose exec triggered this catchpoint.
      This field is only valid immediately after this catchpoint has
      triggered.  */
-  char *exec_pathname;
+  gdb::unique_xmalloc_ptr<char> exec_pathname;
 };
 
-/* Exec catchpoint destructor.  */
-
-exec_catchpoint::~exec_catchpoint ()
-{
-  xfree (this->exec_pathname);
-}
-
 static int
 insert_catch_exec (struct bp_location *bl)
 {
@@ -8287,7 +8278,7 @@ breakpoint_hit_catch_exec (const struct bp_location *bl,
   if (ws->kind != TARGET_WAITKIND_EXECD)
     return 0;
 
-  c->exec_pathname = xstrdup (ws->value.execd_pathname);
+  c->exec_pathname = make_unique_xstrdup (ws->value.execd_pathname);
   return 1;
 }
 
@@ -8311,7 +8302,7 @@ print_it_catch_exec (bpstat bs)
     }
   uiout->field_signed ("bkptno", b->number);
   uiout->text (" (exec'd ");
-  uiout->field_string ("new-exec", c->exec_pathname);
+  uiout->field_string ("new-exec", c->exec_pathname.get ());
   uiout->text ("), ");
 
   return PRINT_SRC_AND_LOC;
@@ -8336,7 +8327,7 @@ print_one_catch_exec (struct breakpoint *b, struct bp_location **last_loc)
   if (c->exec_pathname != NULL)
     {
       uiout->text (", program \"");
-      uiout->field_string ("what", c->exec_pathname);
+      uiout->field_string ("what", c->exec_pathname.get ());
       uiout->text ("\" ");
     }
 
@@ -11324,7 +11315,7 @@ catch_exec_command_1 (const char *arg, int from_tty,
   std::unique_ptr<exec_catchpoint> c (new exec_catchpoint ());
   init_catchpoint (c.get (), gdbarch, temp, cond_string,
 		   &catch_exec_breakpoint_ops);
-  c->exec_pathname = NULL;
+  c->exec_pathname.reset ();
 
   install_breakpoint (0, std::move (c), 1);
 }
-- 
2.31.1

[PATCH 3/6] Use unique_xmalloc_ptr in watchpoint

[Tom Tromey]

This changes struct watchpoint to use unique_xmalloc_ptr in a couple
of places, removing a bit of manual memory management.
---
 gdb/breakpoint.c           | 33 ++++++++++++++-------------------
 gdb/breakpoint.h           |  6 ++----
 gdb/guile/scm-breakpoint.c |  2 +-
 gdb/python/py-breakpoint.c |  2 +-
 4 files changed, 18 insertions(+), 25 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 8016dd3b83f..477c03b7de3 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -1844,7 +1844,9 @@ update_watchpoint (struct watchpoint *b, int reparse)
       const char *s;
 
       b->exp.reset ();
-      s = b->exp_string_reparse ? b->exp_string_reparse : b->exp_string;
+      s = (b->exp_string_reparse
+	   ? b->exp_string_reparse.get ()
+	   : b->exp_string.get ());
       b->exp = parse_exp_1 (&s, 0, b->exp_valid_block, 0);
       /* If the meaning of expression itself changed, the old value is
 	 no longer relevant.  We don't want to report a watchpoint hit
@@ -6131,7 +6133,7 @@ print_one_breakpoint_location (struct breakpoint *b,
 	  if (opts.addressprint)
 	    uiout->field_skip ("addr");
 	  annotate_field (5);
-	  uiout->field_string ("what", w->exp_string);
+	  uiout->field_string ("what", w->exp_string.get ());
 	}
       else if (!is_catchpoint (b) || is_exception_catchpoint (b)
 	       || is_ada_exception_catchpoint (b))
@@ -6358,7 +6360,7 @@ print_one_breakpoint_location (struct breakpoint *b,
 	{
 	  struct watchpoint *w = (struct watchpoint *) b;
 
-	  uiout->field_string ("original-location", w->exp_string);
+	  uiout->field_string ("original-location", w->exp_string.get ());
 	}
       else if (b->location != NULL
 	       && event_location_to_string (b->location.get ()) != NULL)
@@ -10053,14 +10055,6 @@ watchpoint_exp_is_const (const struct expression *exp)
   return exp->op->constant_p ();
 }
 
-/* Watchpoint destructor.  */
-
-watchpoint::~watchpoint ()
-{
-  xfree (this->exp_string);
-  xfree (this->exp_string_reparse);
-}
-
 /* Implement the "re_set" breakpoint_ops method for watchpoints.  */
 
 static void
@@ -10295,7 +10289,7 @@ print_mention_watchpoint (struct breakpoint *b)
   ui_out_emit_tuple tuple_emitter (uiout, tuple_name);
   uiout->field_signed ("number", b->number);
   uiout->text (": ");
-  uiout->field_string ("exp", w->exp_string);
+  uiout->field_string ("exp", w->exp_string.get ());
 }
 
 /* Implement the "print_recreate" breakpoint_ops method for
@@ -10323,7 +10317,7 @@ print_recreate_watchpoint (struct breakpoint *b, struct ui_file *fp)
 		      _("Invalid watchpoint type."));
     }
 
-  fprintf_unfiltered (fp, " %s", w->exp_string);
+  fprintf_unfiltered (fp, " %s", w->exp_string.get ());
   print_recreate_thread (b, fp);
 }
 
@@ -10488,7 +10482,7 @@ print_mention_masked_watchpoint (struct breakpoint *b)
   ui_out_emit_tuple tuple_emitter (uiout, tuple_name);
   uiout->field_signed ("number", b->number);
   uiout->text (": ");
-  uiout->field_string ("exp", w->exp_string);
+  uiout->field_string ("exp", w->exp_string.get ());
 }
 
 /* Implement the "print_recreate" breakpoint_ops method for
@@ -10515,7 +10509,7 @@ print_recreate_masked_watchpoint (struct breakpoint *b, struct ui_file *fp)
 		      _("Invalid hardware watchpoint type."));
     }
 
-  fprintf_unfiltered (fp, " %s mask 0x%s", w->exp_string,
+  fprintf_unfiltered (fp, " %s mask 0x%s", w->exp_string.get (),
 		      phex (w->hw_wp_mask, sizeof (CORE_ADDR)));
   print_recreate_thread (b, fp);
 }
@@ -10794,13 +10788,14 @@ watch_command_1 (const char *arg, int accessflag, int from_tty,
       CORE_ADDR addr = value_as_address (val.get ());
 
       w->exp_string_reparse
-	= current_language->watch_location_expression (t, addr).release ();
+	= current_language->watch_location_expression (t, addr);
 
-      w->exp_string = xstrprintf ("-location %.*s",
-				  (int) (exp_end - exp_start), exp_start);
+      w->exp_string.reset (xstrprintf ("-location %.*s",
+				       (int) (exp_end - exp_start),
+				       exp_start));
     }
   else
-    w->exp_string = savestring (exp_start, exp_end - exp_start);
+    w->exp_string.reset (savestring (exp_start, exp_end - exp_start));
 
   if (use_mask)
     {
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index 2b15622f98d..ad64f8320e9 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -831,13 +831,11 @@ struct breakpoint
 
 struct watchpoint : public breakpoint
 {
-  ~watchpoint () override;
-
   /* String form of exp to use for displaying to the user (malloc'd),
      or NULL if none.  */
-  char *exp_string;
+  gdb::unique_xmalloc_ptr<char> exp_string;
   /* String form to use for reparsing of EXP (malloc'd) or NULL.  */
-  char *exp_string_reparse;
+  gdb::unique_xmalloc_ptr<char> exp_string_reparse;
 
   /* The expression we are watching, or NULL if not a watchpoint.  */
   expression_up exp;
diff --git a/gdb/guile/scm-breakpoint.c b/gdb/guile/scm-breakpoint.c
index 67484e440f5..f48671f0ea9 100644
--- a/gdb/guile/scm-breakpoint.c
+++ b/gdb/guile/scm-breakpoint.c
@@ -883,7 +883,7 @@ gdbscm_breakpoint_expression (SCM self)
 
   wp = (struct watchpoint *) bp_smob->bp;
 
-  const char *str = wp->exp_string;
+  const char *str = wp->exp_string.get ();
   if (! str)
     str = "";
 
diff --git a/gdb/python/py-breakpoint.c b/gdb/python/py-breakpoint.c
index edff03282a1..7ec73af016b 100644
--- a/gdb/python/py-breakpoint.c
+++ b/gdb/python/py-breakpoint.c
@@ -433,7 +433,7 @@ bppy_get_expression (PyObject *self, void *closure)
 
   wp = (struct watchpoint *) obj->bp;
 
-  str = wp->exp_string;
+  str = wp->exp_string.get ();
   if (! str)
     str = "";
 
-- 
2.31.1

[PATCH 4/6] Use unique_xmalloc_ptr in bp_location

[Tom Tromey]

This changes struct bp_location to use a unique_xmalloc_ptr, removing
a bit of manual memory management.
---
 gdb/breakpoint.c | 12 ++++--------
 gdb/breakpoint.h |  4 ++--
 2 files changed, 6 insertions(+), 10 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 477c03b7de3..5c3a6c6e4aa 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -7192,7 +7192,7 @@ set_breakpoint_location_function (struct bp_location *loc)
 	find_pc_partial_function (loc->address, &function_name, NULL, NULL);
 
       if (function_name)
-	loc->function_name = xstrdup (function_name);
+	loc->function_name = make_unique_xstrdup (function_name);
     }
 }
 
@@ -12234,11 +12234,6 @@ say_where (struct breakpoint *b)
     }
 }
 
-bp_location::~bp_location ()
-{
-  xfree (function_name);
-}
-
 /* Destructor for the breakpoint base class.  */
 
 breakpoint::~breakpoint ()
@@ -13323,7 +13318,7 @@ ambiguous_names_p (struct bp_location *loc)
   for (l = loc; l != NULL; l = l->next)
     {
       const char **slot;
-      const char *name = l->function_name;
+      const char *name = l->function_name.get ();
 
       /* Allow for some names to be NULL, ignore them.  */
       if (name == NULL)
@@ -13642,7 +13637,8 @@ update_breakpoint_locations (struct breakpoint *b,
 	      {
 		for (bp_location *l : b->locations ())
 		  if (l->function_name
-		      && strcmp (e->function_name, l->function_name) == 0)
+		      && strcmp (e->function_name.get (),
+				 l->function_name.get ()) == 0)
 		    {
 		      l->enabled = e->enabled;
 		      l->disabled_by_cond = e->disabled_by_cond;
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index ad64f8320e9..304e2c1fca4 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -328,7 +328,7 @@ class bp_location : public refcounted_object
   /* Construct a bp_location with type TYPE.  */
   bp_location (breakpoint *owner, bp_loc_type type);
 
-  virtual ~bp_location ();
+  virtual ~bp_location () = default;
 
   /* Chain pointer to the next breakpoint location for
      the same parent breakpoint.  */
@@ -467,7 +467,7 @@ class bp_location : public refcounted_object
      with it.  */
   bound_probe probe {};
 
-  char *function_name = NULL;
+  gdb::unique_xmalloc_ptr<char> function_name;
 
   /* Details of the placed breakpoint, when inserted.  */
   bp_target_info target_info {};
-- 
2.31.1

[PATCH 5/6] Use unique_xmalloc_ptr in breakpoing

[Tom Tromey]

This changes struct breakpoint to use unique_xmalloc_ptr in a couple
of spots, removing a bit of manual memory management.
---
 gdb/breakpoint.c           | 109 ++++++++++++++++++-------------------
 gdb/breakpoint.h           |   6 +-
 gdb/guile/scm-breakpoint.c |   2 +-
 gdb/python/py-breakpoint.c |   2 +-
 gdb/remote.c               |   2 +-
 gdb/tracepoint.c           |   2 +-
 6 files changed, 60 insertions(+), 63 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 5c3a6c6e4aa..4a5b160f760 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -864,8 +864,7 @@ set_breakpoint_condition (struct breakpoint *b, const char *exp,
 {
   if (*exp == 0)
     {
-      xfree (b->cond_string);
-      b->cond_string = nullptr;
+      b->cond_string.reset ();
 
       if (is_watchpoint (b))
 	static_cast<watchpoint *> (b)->cond_exp.reset ();
@@ -946,8 +945,7 @@ set_breakpoint_condition (struct breakpoint *b, const char *exp,
 
       /* We know that the new condition parsed successfully.  The
 	 condition string of the breakpoint can be safely updated.  */
-      xfree (b->cond_string);
-      b->cond_string = xstrdup (exp);
+      b->cond_string = make_unique_xstrdup (exp);
       b->condition_not_parsed = 0;
     }
   mark_breakpoint_modified (b);
@@ -1862,7 +1860,7 @@ update_watchpoint (struct watchpoint *b, int reparse)
 	{
 	  b->cond_exp.reset ();
 
-	  s = b->cond_string;
+	  s = b->cond_string.get ();
 	  b->cond_exp = parse_exp_1 (&s, 0, b->cond_exp_valid_block, 0);
 	}
     }
@@ -2442,7 +2440,7 @@ build_target_command_list (struct bp_location *bl)
 		 need to parse the command to bytecodes again.  */
 	      loc->cmd_bytecode
 		= parse_cmd_to_aexpr (bl->address,
-				      loc->owner->extra_string);
+				      loc->owner->extra_string.get ());
 	    }
 
 	  /* If we have a NULL bytecode expression, it means something
@@ -5940,7 +5938,7 @@ print_breakpoint_location (struct breakpoint *b,
 	    uiout->text (",");
 	  else
 	    uiout->text (" ");
-	  uiout->text (b->extra_string);
+	  uiout->text (b->extra_string.get ());
 	}
     }
 
@@ -6222,7 +6220,7 @@ print_one_breakpoint_location (struct breakpoint *b,
 	uiout->text ("\ttrace only if ");
       else
 	uiout->text ("\tstop only if ");
-      uiout->field_string ("cond", b->cond_string);
+      uiout->field_string ("cond", b->cond_string.get ());
 
       /* Print whether the target is doing the breakpoint's condition
 	 evaluation.  If GDB is doing the evaluation, don't print anything.  */
@@ -8211,7 +8209,10 @@ init_catchpoint (struct breakpoint *b,
 
   init_raw_breakpoint (b, gdbarch, sal, bp_catchpoint, ops);
 
-  b->cond_string = (cond_string == NULL) ? NULL : xstrdup (cond_string);
+  if (cond_string == nullptr)
+    b->cond_string.reset ();
+  else
+    b->cond_string = make_unique_xstrdup (cond_string);
   b->disposition = temp ? disp_del : disp_donttouch;
 }
 
@@ -8726,7 +8727,7 @@ bp_loc_is_permanent (struct bp_location *loc)
 static void
 update_dprintf_command_list (struct breakpoint *b)
 {
-  char *dprintf_args = b->extra_string;
+  const char *dprintf_args = b->extra_string.get ();
   char *printf_line = NULL;
 
   if (!dprintf_args)
@@ -8851,8 +8852,8 @@ init_breakpoint_sal (struct breakpoint *b, struct gdbarch *gdbarch,
 	  b->thread = thread;
 	  b->task = task;
 
-	  b->cond_string = cond_string.release ();
-	  b->extra_string = extra_string.release ();
+	  b->cond_string = std::move (cond_string);
+	  b->extra_string = std::move (extra_string);
 	  b->ignore_count = ignore_count;
 	  b->enable_state = enabled ? bp_enabled : bp_disabled;
 	  b->disposition = disposition;
@@ -8919,7 +8920,7 @@ init_breakpoint_sal (struct breakpoint *b, struct gdbarch *gdbarch,
 	    error (_("Format string required"));
 	}
       else if (b->extra_string)
-	error (_("Garbage '%s' at end of command"), b->extra_string);
+	error (_("Garbage '%s' at end of command"), b->extra_string.get ());
     }
 
 
@@ -8929,8 +8930,8 @@ init_breakpoint_sal (struct breakpoint *b, struct gdbarch *gdbarch,
   for (bp_location *loc : b->locations ())
     {
       if (b->cond_string != nullptr)
-	set_breakpoint_location_condition (b->cond_string, loc, b->number,
-					   loc_num);
+	set_breakpoint_location_condition (b->cond_string.get (), loc,
+					   b->number, loc_num);
 
       ++loc_num;
     }
@@ -9155,13 +9156,14 @@ check_fast_tracepoint_sals (struct gdbarch *gdbarch,
 
 static void
 find_condition_and_thread (const char *tok, CORE_ADDR pc,
-			   char **cond_string, int *thread, int *task,
-			   char **rest)
+			   gdb::unique_xmalloc_ptr<char> *cond_string,
+			   int *thread, int *task,
+			   gdb::unique_xmalloc_ptr<char> *rest)
 {
-  *cond_string = NULL;
+  cond_string->reset ();
   *thread = -1;
   *task = 0;
-  *rest = NULL;
+  rest->reset ();
   bool force = false;
 
   while (tok && *tok)
@@ -9175,7 +9177,7 @@ find_condition_and_thread (const char *tok, CORE_ADDR pc,
 
       if ((*tok == '"' || *tok == ',') && rest)
 	{
-	  *rest = savestring (tok, strlen (tok));
+	  rest->reset (savestring (tok, strlen (tok)));
 	  return;
 	}
 
@@ -9198,7 +9200,7 @@ find_condition_and_thread (const char *tok, CORE_ADDR pc,
 		tok = tok + strlen (tok);
 	    }
 	  cond_end = tok;
-	  *cond_string = savestring (cond_start, cond_end - cond_start);
+	  cond_string->reset (savestring (cond_start, cond_end - cond_start));
 	}
       else if (toklen >= 1 && strncmp (tok, "-force-condition", toklen) == 0)
 	{
@@ -9231,7 +9233,7 @@ find_condition_and_thread (const char *tok, CORE_ADDR pc,
 	}
       else if (rest)
 	{
-	  *rest = savestring (tok, strlen (tok));
+	  rest->reset (savestring (tok, strlen (tok)));
 	  return;
 	}
       else
@@ -9246,16 +9248,18 @@ find_condition_and_thread (const char *tok, CORE_ADDR pc,
 
 static void
 find_condition_and_thread_for_sals (const std::vector<symtab_and_line> &sals,
-				    const char *input, char **cond_string,
-				    int *thread, int *task, char **rest)
+				    const char *input,
+				    gdb::unique_xmalloc_ptr<char> *cond_string,
+				    int *thread, int *task,
+				    gdb::unique_xmalloc_ptr<char> *rest)
 {
   int num_failures = 0;
   for (auto &sal : sals)
     {
-      char *cond = nullptr;
+      gdb::unique_xmalloc_ptr<char> cond;
       int thread_id = 0;
       int task_id = 0;
-      char *remaining = nullptr;
+      gdb::unique_xmalloc_ptr<char> remaining;
 
       /* Here we want to parse 'arg' to separate condition from thread
 	 number.  But because parsing happens in a context and the
@@ -9267,10 +9271,10 @@ find_condition_and_thread_for_sals (const std::vector<symtab_and_line> &sals,
 	{
 	  find_condition_and_thread (input, sal.pc, &cond, &thread_id,
 				     &task_id, &remaining);
-	  *cond_string = cond;
+	  *cond_string = std::move (cond);
 	  *thread = thread_id;
 	  *task = task_id;
-	  *rest = remaining;
+	  *rest = std::move (remaining);
 	  break;
 	}
       catch (const gdb_exception_error &e)
@@ -9441,15 +9445,15 @@ create_breakpoint (struct gdbarch *gdbarch,
 
       if (parse_extra)
 	{
-	  char *rest;
-	  char *cond;
+	  gdb::unique_xmalloc_ptr<char> rest;
+	  gdb::unique_xmalloc_ptr<char> cond;
 
 	  const linespec_sals &lsal = canonical.lsals[0];
 
 	  find_condition_and_thread_for_sals (lsal.sals, extra_string,
 					      &cond, &thread, &task, &rest);
-	  cond_string_copy.reset (cond);
-	  extra_string_copy.reset (rest);
+	  cond_string_copy = std::move (cond);
+	  extra_string_copy = std::move (rest);
 	}
       else
 	{
@@ -9512,12 +9516,16 @@ create_breakpoint (struct gdbarch *gdbarch,
       else
 	{
 	  /* Create a private copy of condition string.  */
-	  b->cond_string = cond_string != NULL ? xstrdup (cond_string) : NULL;
+	  b->cond_string.reset (cond_string != NULL
+				? xstrdup (cond_string)
+				: NULL);
 	  b->thread = thread;
 	}
 
       /* Create a private copy of any extra string.  */
-      b->extra_string = extra_string != NULL ? xstrdup (extra_string) : NULL;
+      b->extra_string.reset (extra_string != NULL
+			     ? xstrdup (extra_string)
+			     : NULL);
       b->ignore_count = ignore_count;
       b->disposition = tempflag ? disp_del : disp_donttouch;
       b->condition_not_parsed = 1;
@@ -10810,7 +10818,7 @@ watch_command_1 (const char *arg, int accessflag, int from_tty,
     }
 
   if (cond_start)
-    w->cond_string = savestring (cond_start, cond_end - cond_start);
+    w->cond_string.reset (savestring (cond_start, cond_end - cond_start));
   else
     w->cond_string = 0;
 
@@ -12186,13 +12194,13 @@ say_where (struct breakpoint *b)
 	{
 	  printf_filtered (_(" (%s,%s) pending."),
 			   event_location_to_string (b->location.get ()),
-			   b->extra_string);
+			   b->extra_string.get ());
 	}
       else
 	{
 	  printf_filtered (_(" (%s %s) pending."),
 			   event_location_to_string (b->location.get ()),
-			   b->extra_string);
+			   b->extra_string.get ());
 	}
     }
   else
@@ -12234,14 +12242,6 @@ say_where (struct breakpoint *b)
     }
 }
 
-/* Destructor for the breakpoint base class.  */
-
-breakpoint::~breakpoint ()
-{
-  xfree (this->cond_string);
-  xfree (this->extra_string);
-}
-
 /* See breakpoint.h.  */
 
 bp_location_range breakpoint::locations ()
@@ -12579,7 +12579,7 @@ bkpt_print_recreate (struct breakpoint *tp, struct ui_file *fp)
   /* Print out extra_string if this breakpoint is pending.  It might
      contain, for example, conditions that were set by the user.  */
   if (tp->loc == NULL && tp->extra_string != NULL)
-    fprintf_unfiltered (fp, " %s", tp->extra_string);
+    fprintf_unfiltered (fp, " %s", tp->extra_string.get ());
 
   print_recreate_thread (tp, fp);
 }
@@ -12988,7 +12988,7 @@ dprintf_print_recreate (struct breakpoint *tp, struct ui_file *fp)
 {
   fprintf_unfiltered (fp, "dprintf %s,%s",
 		      event_location_to_string (tp->location.get ()),
-		      tp->extra_string);
+		      tp->extra_string.get ());
   print_recreate_thread (tp, fp);
 }
 
@@ -13578,7 +13578,7 @@ update_breakpoint_locations (struct breakpoint *b,
 	{
 	  const char *s;
 
-	  s = b->cond_string;
+	  s = b->cond_string.get ();
 	  try
 	    {
 	      new_loc->cond = parse_exp_1 (&s, sal.pc,
@@ -13712,22 +13712,19 @@ location_to_sals (struct breakpoint *b, struct event_location *location,
 	resolve_sal_pc (&sal);
       if (b->condition_not_parsed && b->extra_string != NULL)
 	{
-	  char *cond_string, *extra_string;
+	  gdb::unique_xmalloc_ptr<char> cond_string, extra_string;
 	  int thread, task;
 
-	  find_condition_and_thread_for_sals (sals, b->extra_string,
+	  find_condition_and_thread_for_sals (sals, b->extra_string.get (),
 					      &cond_string, &thread,
 					      &task, &extra_string);
 	  gdb_assert (b->cond_string == NULL);
 	  if (cond_string)
-	    b->cond_string = cond_string;
+	    b->cond_string = std::move (cond_string);
 	  b->thread = thread;
 	  b->task = task;
 	  if (extra_string)
-	    {
-	      xfree (b->extra_string);
-	      b->extra_string = extra_string;
-	    }
+	    b->extra_string = std::move (extra_string);
 	  b->condition_not_parsed = 0;
 	}
 
@@ -15035,7 +15032,7 @@ save_breakpoints (const char *filename, int from_tty,
 	 instead.  */
 
       if (tp->cond_string)
-	fp.printf ("  condition $bpnum %s\n", tp->cond_string);
+	fp.printf ("  condition $bpnum %s\n", tp->cond_string.get ());
 
       if (tp->ignore_count)
 	fp.printf ("  ignore $bpnum %d\n", tp->ignore_count);
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index 304e2c1fca4..f19f11eb479 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -720,7 +720,7 @@ using bp_location_range = next_range<bp_location>;
 
 struct breakpoint
 {
-  virtual ~breakpoint ();
+  virtual ~breakpoint () = default;
 
   /* Return a range of this breakpoint's locations.  */
   bp_location_range locations ();
@@ -785,11 +785,11 @@ struct breakpoint
   int input_radix = 0;
   /* String form of the breakpoint condition (malloc'd), or NULL if
      there is no condition.  */
-  char *cond_string = NULL;
+  gdb::unique_xmalloc_ptr<char> cond_string;
 
   /* String form of extra parameters, or NULL if there are none.
      Malloc'd.  */
-  char *extra_string = NULL;
+  gdb::unique_xmalloc_ptr<char> extra_string;
 
   /* Holds the address of the related watchpoint_scope breakpoint when
      using watchpoints on local variables (might the concept of a
diff --git a/gdb/guile/scm-breakpoint.c b/gdb/guile/scm-breakpoint.c
index f48671f0ea9..ab1ddb1bae0 100644
--- a/gdb/guile/scm-breakpoint.c
+++ b/gdb/guile/scm-breakpoint.c
@@ -899,7 +899,7 @@ gdbscm_breakpoint_condition (SCM self)
     = bpscm_get_valid_breakpoint_smob_arg_unsafe (self, SCM_ARG1, FUNC_NAME);
   char *str;
 
-  str = bp_smob->bp->cond_string;
+  str = bp_smob->bp->cond_string.get ();
   if (! str)
     return SCM_BOOL_F;
 
diff --git a/gdb/python/py-breakpoint.c b/gdb/python/py-breakpoint.c
index 7ec73af016b..d99d9b18b49 100644
--- a/gdb/python/py-breakpoint.c
+++ b/gdb/python/py-breakpoint.c
@@ -449,7 +449,7 @@ bppy_get_condition (PyObject *self, void *closure)
 
   BPPY_REQUIRE_VALID (obj);
 
-  str = obj->bp->cond_string;
+  str = obj->bp->cond_string.get ();
   if (! str)
     Py_RETURN_NONE;
 
diff --git a/gdb/remote.c b/gdb/remote.c
index d5eb40ce578..7f530534fe6 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -13344,7 +13344,7 @@ remote_target::download_tracepoint (struct bp_location *loc)
 	    error ("%s", err_msg);
 
 	  encode_source_string (b->number, loc->address,
-				"cond", b->cond_string,
+				"cond", b->cond_string.get (),
 				buf.data () + strlen (buf.data ()),
 				buf.size () - strlen (buf.data ()));
 	  putpkt (buf.data ());
diff --git a/gdb/tracepoint.c b/gdb/tracepoint.c
index 3997d211182..df5013e45ae 100644
--- a/gdb/tracepoint.c
+++ b/gdb/tracepoint.c
@@ -3094,7 +3094,7 @@ find_matching_tracepoint_location (struct uploaded_tp *utp)
       if (b->type == utp->type
 	  && t->step_count == utp->step
 	  && t->pass_count == utp->pass
-	  && cond_string_is_same (t->cond_string,
+	  && cond_string_is_same (t->cond_string.get (),
 				  utp->cond_string.get ())
 	  /* FIXME also test actions.  */
 	  )
-- 
2.31.1

[PATCH 6/6] Use std::string in print_one_catch_syscall

[Tom Tromey]

This changes print_one_catch_syscall to use std::string, removing a
bit of manual memory management.
---
 gdb/break-catch-syscall.c | 22 +++++++++-------------
 1 file changed, 9 insertions(+), 13 deletions(-)

diff --git a/gdb/break-catch-syscall.c b/gdb/break-catch-syscall.c
index 32736f024ad..3d3b275c31e 100644
--- a/gdb/break-catch-syscall.c
+++ b/gdb/break-catch-syscall.c
@@ -250,28 +250,24 @@ print_one_catch_syscall (struct breakpoint *b,
 
   if (!c->syscalls_to_be_caught.empty ())
     {
-      char *text = xstrprintf ("%s", "");
+      std::string text;
 
+      bool first = true;
       for (int iter : c->syscalls_to_be_caught)
 	{
-	  char *previous_text = text;
 	  struct syscall s;
 	  get_syscall_by_number (gdbarch, iter, &s);
 
+	  if (!first)
+	    text += ", ";
+	  first = false;
+
 	  if (s.name != NULL)
-	    text = xstrprintf ("%s%s, ", text, s.name);
+	    text += s.name;
 	  else
-	    text = xstrprintf ("%s%d, ", text, iter);
-
-	  /* We have to xfree previous_text because xstrprintf dynamically
-	     allocates new space for text on every call.  */
-	  xfree (previous_text);
+	    text += std::to_string (iter);
 	}
-      /* Remove the last comma.  */
-      text[strlen (text) - 2] = '\0';
-      uiout->field_string ("what", text);
-      /* xfree last text.  */
-      xfree (text);
+      uiout->field_string ("what", text.c_str ());
     }
   else
     uiout->field_string ("what", "<any syscall>", metadata_style.style ());
-- 
2.31.1

Re: [PATCH 0/6] Remove some uses of xfree

[Bruno Larsen]

On 10/4/21 9:14 PM, Tom Tromey wrote:
> I found a few spots using manual memory management that could easily
> be changed to use unique_xmalloc_ptr or std::string.  I split this
> into separate patches to make each change simpler to understand.
> 
> Regression tested on x86-64 Fedora 34.
> 
> Tom
> 
> 

Tested this patch series on ppc64le RHEL 8.5, no regressions there, and aarch64 RHEL 8.5 has quite a few gdb.mi new failures, but those tests seem... unreliable at best. I'll dig a bit into them to see if they are racy, but in general looks like a good change with no errors introduced

-- 
Cheers!
Bruno Larsen

Re: [PATCH 0/6] Remove some uses of xfree

[Tom Tromey]

>>>>> "Tom" == Tom Tromey <tom@tromey.com> writes:

Tom> I found a few spots using manual memory management that could easily
Tom> be changed to use unique_xmalloc_ptr or std::string.  I split this
Tom> into separate patches to make each change simpler to understand.

Tom> Regression tested on x86-64 Fedora 34.

I'm checking this in now.

Tom