[RFA 0/2] small clean ups in breakpoint allocation

[RFA 0/2] small clean ups in breakpoint allocation

[Tom Tromey]

This short series fixes up breakpoint allocation a bit.

The first patch fixes a real bug I happened across by accident -- a
new/free discrepancy.

The second patch aims to make these bugs much less likely in the
future.

Regression tested on the buildbot.

Tom

[RFA 1/2] Fix erroneous cleanup use in add_solib_catchpoint

[Tom Tromey]

I happened to notice that add_solib_catchpoint allocated the new
catchpoint with "new" but installed a cleanup using "xfree".  This
patch fixes the bug by changing the function to use std::unique_ptr
instead.

gdb/ChangeLog
2017-08-19  Tom Tromey  <tom@tromey.com>

	* breakpoint.c (add_solib_catchpoint): Use std::unique_ptr.
---
 gdb/ChangeLog    |  4 ++++
 gdb/breakpoint.c | 10 +++-------
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 7ad3f4c..339ab3a 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,7 @@
+2017-08-19  Tom Tromey  <tom@tromey.com>
+
+	* breakpoint.c (add_solib_catchpoint): Use std::unique_ptr.
+
 2017-08-18  Tom Tromey  <tom@tromey.com>
 	    Pedro Alves  <palves@redhat.com>
 
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index bc681cf..135741a 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -8464,16 +8464,13 @@ static struct breakpoint_ops catch_solib_breakpoint_ops;
 void
 add_solib_catchpoint (const char *arg, int is_load, int is_temp, int enabled)
 {
-  struct solib_catchpoint *c;
   struct gdbarch *gdbarch = get_current_arch ();
-  struct cleanup *cleanup;
 
   if (!arg)
     arg = "";
   arg = skip_spaces_const (arg);
 
-  c = new solib_catchpoint ();
-  cleanup = make_cleanup (xfree, c);
+  std::unique_ptr<solib_catchpoint> c (new solib_catchpoint ());
 
   if (*arg != '\0')
     {
@@ -8483,13 +8480,12 @@ add_solib_catchpoint (const char *arg, int is_load, int is_temp, int enabled)
     }
 
   c->is_load = is_load;
-  init_catchpoint (c, gdbarch, is_temp, NULL,
+  init_catchpoint (c.get (), gdbarch, is_temp, NULL,
 		   &catch_solib_breakpoint_ops);
 
   c->enable_state = enabled ? bp_enabled : bp_disabled;
 
-  discard_cleanups (cleanup);
-  install_breakpoint (0, c, 1);
+  install_breakpoint (0, c.release (), 1);
 }
 
 /* A helper function that does all the work for "catch load" and
-- 
2.9.4

[RFA 2/2] Change install_breakpoint to take a std::unique_ptr

[Tom Tromey]

This changes install_breakpoint to take a std::unique_ptr rvalue-ref
argument.  This makes it clear that install_breakpoint takes ownership
of the pointer, and prevents bugs like the one fixed by the previous
patch.

gdb/ChangeLog
2017-08-19  Tom Tromey  <tom@tromey.com>

	* breakpoint.h (install_breakpoint): Update.
	* breakpoint.c (add_solib_catchpoint): Update.
	(install_breakpoint): Change argument to a std::unique_ptr.
	(create_fork_vfork_event_catchpoint): Use std::unique_ptr.
	(create_breakpoint_sal, create_breakpoint): Update.
	(watch_command_1, catch_exec_command_1)
	(strace_marker_create_breakpoints_sal): Use std::unique_ptr.
	* break-catch-throw.c (handle_gnu_v3_exceptions): Use
	std::unique_ptr.
	* break-catch-syscall.c (create_syscall_event_catchpoint): Use
	std::unique_ptr.
	* break-catch-sig.c (create_signal_catchpoint): Use
	std::unique_ptr.
	* ada-lang.c (create_ada_exception_catchpoint): Use
	std::unique_ptr.
---
 gdb/ChangeLog             | 18 +++++++++++++++
 gdb/ada-lang.c            | 11 +++++-----
 gdb/break-catch-sig.c     |  7 +++---
 gdb/break-catch-syscall.c |  7 +++---
 gdb/break-catch-throw.c   |  3 +--
 gdb/breakpoint.c          | 56 +++++++++++++++++++----------------------------
 gdb/breakpoint.h          |  2 +-
 7 files changed, 54 insertions(+), 50 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 339ab3a..8f0a22f 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,23 @@
 2017-08-19  Tom Tromey  <tom@tromey.com>
 
+	* breakpoint.h (install_breakpoint): Update.
+	* breakpoint.c (add_solib_catchpoint): Update.
+	(install_breakpoint): Change argument to a std::unique_ptr.
+	(create_fork_vfork_event_catchpoint): Use std::unique_ptr.
+	(create_breakpoint_sal, create_breakpoint): Update.
+	(watch_command_1, catch_exec_command_1)
+	(strace_marker_create_breakpoints_sal): Use std::unique_ptr.
+	* break-catch-throw.c (handle_gnu_v3_exceptions): Use
+	std::unique_ptr.
+	* break-catch-syscall.c (create_syscall_event_catchpoint): Use
+	std::unique_ptr.
+	* break-catch-sig.c (create_signal_catchpoint): Use
+	std::unique_ptr.
+	* ada-lang.c (create_ada_exception_catchpoint): Use
+	std::unique_ptr.
+
+2017-08-19  Tom Tromey  <tom@tromey.com>
+
 	* breakpoint.c (add_solib_catchpoint): Use std::unique_ptr.
 
 2017-08-18  Tom Tromey  <tom@tromey.com>
diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
index 280247b..476f700 100644
--- a/gdb/ada-lang.c
+++ b/gdb/ada-lang.c
@@ -13010,20 +13010,19 @@ create_ada_exception_catchpoint (struct gdbarch *gdbarch,
 				 int disabled,
 				 int from_tty)
 {
-  struct ada_catchpoint *c;
   char *addr_string = NULL;
   const struct breakpoint_ops *ops = NULL;
   struct symtab_and_line sal
     = ada_exception_sal (ex_kind, excep_string, &addr_string, &ops);
 
-  c = new ada_catchpoint ();
-  init_ada_exception_breakpoint (c, gdbarch, sal, addr_string,
+  std::unique_ptr<ada_catchpoint> c (new ada_catchpoint ());
+  init_ada_exception_breakpoint (c.get (), gdbarch, sal, addr_string,
 				 ops, tempflag, disabled, from_tty);
   c->excep_string = excep_string;
-  create_excep_cond_exprs (c);
+  create_excep_cond_exprs (c.get ());
   if (cond_string != NULL)
-    set_breakpoint_condition (c, cond_string, from_tty);
-  install_breakpoint (0, c, 1);
+    set_breakpoint_condition (c.get (), cond_string, from_tty);
+  install_breakpoint (0, std::move (c), 1);
 }
 
 /* Implement the "catch exception" command.  */
diff --git a/gdb/break-catch-sig.c b/gdb/break-catch-sig.c
index 98888c9..9b8cf64 100644
--- a/gdb/break-catch-sig.c
+++ b/gdb/break-catch-sig.c
@@ -317,15 +317,14 @@ static void
 create_signal_catchpoint (int tempflag, std::vector<gdb_signal> &&filter,
 			  bool catch_all)
 {
-  struct signal_catchpoint *c;
   struct gdbarch *gdbarch = get_current_arch ();
 
-  c = new signal_catchpoint ();
-  init_catchpoint (c, gdbarch, tempflag, NULL, &signal_catchpoint_ops);
+  std::unique_ptr<signal_catchpoint> c (new signal_catchpoint ());
+  init_catchpoint (c.get (), gdbarch, tempflag, NULL, &signal_catchpoint_ops);
   c->signals_to_be_caught = std::move (filter);
   c->catch_all = catch_all;
 
-  install_breakpoint (0, c, 1);
+  install_breakpoint (0, std::move (c), 1);
 }
 
 
diff --git a/gdb/break-catch-syscall.c b/gdb/break-catch-syscall.c
index 701645e..1be29be 100644
--- a/gdb/break-catch-syscall.c
+++ b/gdb/break-catch-syscall.c
@@ -370,14 +370,13 @@ static void
 create_syscall_event_catchpoint (int tempflag, std::vector<int> &&filter,
                                  const struct breakpoint_ops *ops)
 {
-  struct syscall_catchpoint *c;
   struct gdbarch *gdbarch = get_current_arch ();
 
-  c = new syscall_catchpoint ();
-  init_catchpoint (c, gdbarch, tempflag, NULL, ops);
+  std::unique_ptr<syscall_catchpoint> c (new syscall_catchpoint ());
+  init_catchpoint (c.get (), gdbarch, tempflag, NULL, ops);
   c->syscalls_to_be_caught = std::move (filter);
 
-  install_breakpoint (0, c, 1);
+  install_breakpoint (0, std::move (c), 1);
 }
 
 /* Splits the argument using space as delimiter.  */
diff --git a/gdb/break-catch-throw.c b/gdb/break-catch-throw.c
index 5318e5f..b5322fc 100644
--- a/gdb/break-catch-throw.c
+++ b/gdb/break-catch-throw.c
@@ -387,8 +387,7 @@ handle_gnu_v3_exceptions (int tempflag, std::string &&except_rx,
 
   re_set_exception_catchpoint (cp.get ());
 
-  install_breakpoint (0, cp.get (), 1);
-  cp.release ();
+  install_breakpoint (0, std::move (cp), 1);
 }
 
 /* Look for an "if" token in *STRING.  The "if" token must be preceded
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 135741a..b0881ec 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -8485,7 +8485,7 @@ add_solib_catchpoint (const char *arg, int is_load, int is_temp, int enabled)
 
   c->enable_state = enabled ? bp_enabled : bp_disabled;
 
-  install_breakpoint (0, c.release (), 1);
+  install_breakpoint (0, std::move (c), 1);
 }
 
 /* A helper function that does all the work for "catch load" and
@@ -8540,8 +8540,10 @@ init_catchpoint (struct breakpoint *b,
 }
 
 void
-install_breakpoint (int internal, struct breakpoint *b, int update_gll)
+install_breakpoint (int internal, std::unique_ptr<breakpoint> &&arg, int update_gll)
 {
+  breakpoint *b = arg.release ();
+
   add_to_breakpoint_chain (b);
   set_breakpoint_number (internal, b);
   if (is_tracepoint (b))
@@ -8559,13 +8561,13 @@ create_fork_vfork_event_catchpoint (struct gdbarch *gdbarch,
 				    int tempflag, const char *cond_string,
                                     const struct breakpoint_ops *ops)
 {
-  struct fork_catchpoint *c = new fork_catchpoint ();
+  std::unique_ptr<fork_catchpoint> c (new fork_catchpoint ());
 
-  init_catchpoint (c, gdbarch, tempflag, cond_string, ops);
+  init_catchpoint (c.get (), gdbarch, tempflag, cond_string, ops);
 
   c->forked_inferior_pid = null_ptid;
 
-  install_breakpoint (0, c, 1);
+  install_breakpoint (0, std::move (c), 1);
 }
 
 /* Exec catchpoints.  */
@@ -9308,7 +9310,7 @@ create_breakpoint_sal (struct gdbarch *gdbarch,
 		       enabled, internal, flags,
 		       display_canonical);
 
-  install_breakpoint (internal, b.release (), 0);
+  install_breakpoint (internal, std::move (b), 0);
 }
 
 /* Add SALS.nelts breakpoints to the breakpoint table.  For each
@@ -9798,7 +9800,7 @@ create_breakpoint (struct gdbarch *gdbarch,
            && type_wanted != bp_hardware_breakpoint) || thread != -1)
 	b->pspace = current_program_space;
 
-      install_breakpoint (internal, b.release (), 0);
+      install_breakpoint (internal, std::move (b), 0);
     }
   
   if (VEC_length (linespec_sals, canonical.sals) > 1)
@@ -10961,7 +10963,6 @@ watch_command_1 (const char *arg, int accessflag, int from_tty,
      the hardware watchpoint.  */
   int use_mask = 0;
   CORE_ADDR mask = 0;
-  struct watchpoint *w;
   char *expression;
   struct cleanup *back_to;
 
@@ -11182,13 +11183,13 @@ watch_command_1 (const char *arg, int accessflag, int from_tty,
   else
     bp_type = bp_hardware_watchpoint;
 
-  w = new watchpoint ();
+  std::unique_ptr<watchpoint> w (new watchpoint ());
 
   if (use_mask)
-    init_raw_breakpoint_without_location (w, NULL, bp_type,
+    init_raw_breakpoint_without_location (w.get (), NULL, bp_type,
 					  &masked_watchpoint_breakpoint_ops);
   else
-    init_raw_breakpoint_without_location (w, NULL, bp_type,
+    init_raw_breakpoint_without_location (w.get (), NULL, bp_type,
 					  &watchpoint_breakpoint_ops);
   w->thread = thread;
   w->disposition = disp_donttouch;
@@ -11243,26 +11244,17 @@ watch_command_1 (const char *arg, int accessflag, int from_tty,
       /* The scope breakpoint is related to the watchpoint.  We will
 	 need to act on them together.  */
       w->related_breakpoint = scope_breakpoint;
-      scope_breakpoint->related_breakpoint = w;
+      scope_breakpoint->related_breakpoint = w.get ();
     }
 
   if (!just_location)
     value_free_to_mark (mark);
 
-  TRY
-    {
-      /* Finally update the new watchpoint.  This creates the locations
-	 that should be inserted.  */
-      update_watchpoint (w, 1);
-    }
-  CATCH (e, RETURN_MASK_ALL)
-    {
-      delete_breakpoint (w);
-      throw_exception (e);
-    }
-  END_CATCH
+  /* Finally update the new watchpoint.  This creates the locations
+     that should be inserted.  */
+  update_watchpoint (w.get (), 1);
 
-  install_breakpoint (internal, w, 1);
+  install_breakpoint (internal, std::move (w), 1);
   do_cleanups (back_to);
 }
 
@@ -11710,7 +11702,6 @@ catch_exec_command_1 (char *arg_entry, int from_tty,
 		      struct cmd_list_element *command)
 {
   const char *arg = arg_entry;
-  struct exec_catchpoint *c;
   struct gdbarch *gdbarch = get_current_arch ();
   int tempflag;
   const char *cond_string = NULL;
@@ -11731,12 +11722,12 @@ catch_exec_command_1 (char *arg_entry, int from_tty,
   if ((*arg != '\0') && !isspace (*arg))
     error (_("Junk at end of arguments."));
 
-  c = new exec_catchpoint ();
-  init_catchpoint (c, gdbarch, tempflag, cond_string,
+  std::unique_ptr<exec_catchpoint> c (new exec_catchpoint ());
+  init_catchpoint (c.get (), gdbarch, tempflag, cond_string,
 		   &catch_exec_breakpoint_ops);
   c->exec_pathname = NULL;
 
-  install_breakpoint (0, c, 1);
+  install_breakpoint (0, std::move (c), 1);
 }
 
 void
@@ -13559,7 +13550,6 @@ strace_marker_create_breakpoints_sal (struct gdbarch *gdbarch,
   for (i = 0; i < lsal->sals.nelts; ++i)
     {
       struct symtabs_and_lines expanded;
-      struct tracepoint *tp;
       event_location_up location;
 
       expanded.nelts = 1;
@@ -13567,8 +13557,8 @@ strace_marker_create_breakpoints_sal (struct gdbarch *gdbarch,
 
       location = copy_event_location (canonical->location.get ());
 
-      tp = new tracepoint ();
-      init_breakpoint_sal (tp, gdbarch, expanded,
+      std::unique_ptr<tracepoint> tp (new tracepoint ());
+      init_breakpoint_sal (tp.get (), gdbarch, expanded,
 			   std::move (location), NULL,
 			   std::move (cond_string),
 			   std::move (extra_string),
@@ -13584,7 +13574,7 @@ strace_marker_create_breakpoints_sal (struct gdbarch *gdbarch,
 	 corresponds to this one  */
       tp->static_trace_marker_id_idx = i;
 
-      install_breakpoint (internal, tp, 0);
+      install_breakpoint (internal, std::move (tp), 0);
     }
 }
 
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index d955184..dc2b098 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -1296,7 +1296,7 @@ extern void init_catchpoint (struct breakpoint *b,
    the internal breakpoint count.  If UPDATE_GLL is non-zero,
    update_global_location_list will be called.  */
 
-extern void install_breakpoint (int internal, struct breakpoint *b,
+extern void install_breakpoint (int internal, std::unique_ptr<breakpoint> &&b,
 				int update_gll);
 
 /* Flags that can be passed down to create_breakpoint, etc., to affect
-- 
2.9.4

Re: [RFA 2/2] Change install_breakpoint to take a std::unique_ptr

[Pedro Alves]

On 08/20/2017 02:54 PM, Tom Tromey wrote:
> @@ -8540,8 +8540,10 @@ init_catchpoint (struct breakpoint *b,
>  }
>  
>  void
> -install_breakpoint (int internal, struct breakpoint *b, int update_gll)
> +install_breakpoint (int internal, std::unique_ptr<breakpoint> &&arg, int update_gll)
>  {
> +  breakpoint *b = arg.release ();
> +
>    add_to_breakpoint_chain (b);

I think we should defer .release() until the ownership
transfer actually happened, for stronger exception safety.

I.e., either:

    add_to_breakpoint_chain (arg.get ());
    breakpoint *b = arg.release ();

or change add_to_breakpoint_chain to take an rvalue-ref unique_ptr
too, and return the raw breakpoint, and then write here:

    breakpoint *b = add_to_breakpoint_chain (std::move (arg));

Some spots were/are already only doing the release after
adding the breakpoint to the chain, but not all.  See e.g.,
set_raw_breakpoint.

The latter approach is my preferred for enforcing the pattern.

Otherwise both patches LGTM.

Thanks,
Pedro Alves

Re: [RFA 2/2] Change install_breakpoint to take a std::unique_ptr

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

[...]
Pedro> or change add_to_breakpoint_chain to take an rvalue-ref unique_ptr
Pedro> too, and return the raw breakpoint, and then write here:
Pedro>     breakpoint *b = add_to_breakpoint_chain (std::move (arg));

Sounds good.  I actually considered this but thought maybe it was a step
too far... hah.

How's this?

Tom

commit e13a6f18770ff6dc329202e68711b5b34fc1c897
Author: Tom Tromey <tom@tromey.com>
Date:   Sat Aug 19 22:26:20 2017 -0600

    Change install_breakpoint to take a std::unique_ptr
    
    This changes install_breakpoint to take a std::unique_ptr rvalue-ref
    argument.  This makes it clear that install_breakpoint takes ownership
    of the pointer, and prevents bugs like the one fixed by the previous
    patch.
    
    ChangeLog
    2017-08-21  Tom Tromey  <tom@tromey.com>
    
            * breakpoint.h (install_breakpoint): Update.
            * breakpoint.c (add_solib_catchpoint): Update.
            (install_breakpoint): Change argument to a std::unique_ptr.
            (create_fork_vfork_event_catchpoint): Use std::unique_ptr.
            (create_breakpoint_sal, create_breakpoint): Update.
            (watch_command_1, catch_exec_command_1)
            (strace_marker_create_breakpoints_sal): Use std::unique_ptr.
            (add_to_breakpoint_chain): Change argument to a std::unique_ptr.
            Return the breakpoint.
            (set_raw_breakpoint_without_location, set_raw_breakpoint)
            (new_single_step_breakpoint): Update.
            * break-catch-throw.c (handle_gnu_v3_exceptions): Use
            std::unique_ptr.
            * break-catch-syscall.c (create_syscall_event_catchpoint): Use
            std::unique_ptr.
            * break-catch-sig.c (create_signal_catchpoint): Use
            std::unique_ptr.
            * ada-lang.c (create_ada_exception_catchpoint): Use
            std::unique_ptr.

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index b9de079..a7f13d4 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,27 @@
 2017-08-21  Tom Tromey  <tom@tromey.com>
 
+	* breakpoint.h (install_breakpoint): Update.
+	* breakpoint.c (add_solib_catchpoint): Update.
+	(install_breakpoint): Change argument to a std::unique_ptr.
+	(create_fork_vfork_event_catchpoint): Use std::unique_ptr.
+	(create_breakpoint_sal, create_breakpoint): Update.
+	(watch_command_1, catch_exec_command_1)
+	(strace_marker_create_breakpoints_sal): Use std::unique_ptr.
+	(add_to_breakpoint_chain): Change argument to a std::unique_ptr.
+	Return the breakpoint.
+	(set_raw_breakpoint_without_location, set_raw_breakpoint)
+	(new_single_step_breakpoint): Update.
+	* break-catch-throw.c (handle_gnu_v3_exceptions): Use
+	std::unique_ptr.
+	* break-catch-syscall.c (create_syscall_event_catchpoint): Use
+	std::unique_ptr.
+	* break-catch-sig.c (create_signal_catchpoint): Use
+	std::unique_ptr.
+	* ada-lang.c (create_ada_exception_catchpoint): Use
+	std::unique_ptr.
+
+2017-08-21  Tom Tromey  <tom@tromey.com>
+
 	* breakpoint.c (add_solib_catchpoint): Use std::unique_ptr.
 
 2017-08-21  John Baldwin  <jhb@FreeBSD.org>
diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
index 280247b..476f700 100644
--- a/gdb/ada-lang.c
+++ b/gdb/ada-lang.c
@@ -13010,20 +13010,19 @@ create_ada_exception_catchpoint (struct gdbarch *gdbarch,
 				 int disabled,
 				 int from_tty)
 {
-  struct ada_catchpoint *c;
   char *addr_string = NULL;
   const struct breakpoint_ops *ops = NULL;
   struct symtab_and_line sal
     = ada_exception_sal (ex_kind, excep_string, &addr_string, &ops);
 
-  c = new ada_catchpoint ();
-  init_ada_exception_breakpoint (c, gdbarch, sal, addr_string,
+  std::unique_ptr<ada_catchpoint> c (new ada_catchpoint ());
+  init_ada_exception_breakpoint (c.get (), gdbarch, sal, addr_string,
 				 ops, tempflag, disabled, from_tty);
   c->excep_string = excep_string;
-  create_excep_cond_exprs (c);
+  create_excep_cond_exprs (c.get ());
   if (cond_string != NULL)
-    set_breakpoint_condition (c, cond_string, from_tty);
-  install_breakpoint (0, c, 1);
+    set_breakpoint_condition (c.get (), cond_string, from_tty);
+  install_breakpoint (0, std::move (c), 1);
 }
 
 /* Implement the "catch exception" command.  */
diff --git a/gdb/break-catch-sig.c b/gdb/break-catch-sig.c
index 98888c9..9b8cf64 100644
--- a/gdb/break-catch-sig.c
+++ b/gdb/break-catch-sig.c
@@ -317,15 +317,14 @@ static void
 create_signal_catchpoint (int tempflag, std::vector<gdb_signal> &&filter,
 			  bool catch_all)
 {
-  struct signal_catchpoint *c;
   struct gdbarch *gdbarch = get_current_arch ();
 
-  c = new signal_catchpoint ();
-  init_catchpoint (c, gdbarch, tempflag, NULL, &signal_catchpoint_ops);
+  std::unique_ptr<signal_catchpoint> c (new signal_catchpoint ());
+  init_catchpoint (c.get (), gdbarch, tempflag, NULL, &signal_catchpoint_ops);
   c->signals_to_be_caught = std::move (filter);
   c->catch_all = catch_all;
 
-  install_breakpoint (0, c, 1);
+  install_breakpoint (0, std::move (c), 1);
 }
 
 
diff --git a/gdb/break-catch-syscall.c b/gdb/break-catch-syscall.c
index 701645e..1be29be 100644
--- a/gdb/break-catch-syscall.c
+++ b/gdb/break-catch-syscall.c
@@ -370,14 +370,13 @@ static void
 create_syscall_event_catchpoint (int tempflag, std::vector<int> &&filter,
                                  const struct breakpoint_ops *ops)
 {
-  struct syscall_catchpoint *c;
   struct gdbarch *gdbarch = get_current_arch ();
 
-  c = new syscall_catchpoint ();
-  init_catchpoint (c, gdbarch, tempflag, NULL, ops);
+  std::unique_ptr<syscall_catchpoint> c (new syscall_catchpoint ());
+  init_catchpoint (c.get (), gdbarch, tempflag, NULL, ops);
   c->syscalls_to_be_caught = std::move (filter);
 
-  install_breakpoint (0, c, 1);
+  install_breakpoint (0, std::move (c), 1);
 }
 
 /* Splits the argument using space as delimiter.  */
diff --git a/gdb/break-catch-throw.c b/gdb/break-catch-throw.c
index 5318e5f..b5322fc 100644
--- a/gdb/break-catch-throw.c
+++ b/gdb/break-catch-throw.c
@@ -387,8 +387,7 @@ handle_gnu_v3_exceptions (int tempflag, std::string &&except_rx,
 
   re_set_exception_catchpoint (cp.get ());
 
-  install_breakpoint (0, cp.get (), 1);
-  cp.release ();
+  install_breakpoint (0, std::move (cp), 1);
 }
 
 /* Look for an "if" token in *STRING.  The "if" token must be preceded
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 135741a..334c76a 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -7402,23 +7402,26 @@ decref_bp_location (struct bp_location **blp)
 
 /* Add breakpoint B at the end of the global breakpoint chain.  */
 
-static void
-add_to_breakpoint_chain (struct breakpoint *b)
+static breakpoint *
+add_to_breakpoint_chain (std::unique_ptr<breakpoint> &&b)
 {
   struct breakpoint *b1;
+  struct breakpoint *result = b.get ();
 
   /* Add this breakpoint to the end of the chain so that a list of
      breakpoints will come out in order of increasing numbers.  */
 
   b1 = breakpoint_chain;
   if (b1 == 0)
-    breakpoint_chain = b;
+    breakpoint_chain = b.release ();
   else
     {
       while (b1->next)
 	b1 = b1->next;
-      b1->next = b;
+      b1->next = b.release ();
     }
+
+  return result;
 }
 
 /* Initializes breakpoint B with type BPTYPE and no locations yet.  */
@@ -7450,9 +7453,7 @@ set_raw_breakpoint_without_location (struct gdbarch *gdbarch,
   std::unique_ptr<breakpoint> b = new_breakpoint_from_type (bptype);
 
   init_raw_breakpoint_without_location (b.get (), gdbarch, bptype, ops);
-  add_to_breakpoint_chain (b.get ());
-
-  return b.release ();
+  return add_to_breakpoint_chain (std::move (b));
 }
 
 /* Initialize loc->function_name.  EXPLICIT_LOC says no indirect function
@@ -7567,9 +7568,7 @@ set_raw_breakpoint (struct gdbarch *gdbarch,
   std::unique_ptr<breakpoint> b = new_breakpoint_from_type (bptype);
 
   init_raw_breakpoint (b.get (), gdbarch, sal, bptype, ops);
-  add_to_breakpoint_chain (b.get ());
-
-  return b.release ();
+  return add_to_breakpoint_chain (std::move (b));
 }
 
 /* Call this routine when stepping and nexting to enable a breakpoint
@@ -8485,7 +8484,7 @@ add_solib_catchpoint (const char *arg, int is_load, int is_temp, int enabled)
 
   c->enable_state = enabled ? bp_enabled : bp_disabled;
 
-  install_breakpoint (0, c.release (), 1);
+  install_breakpoint (0, std::move (c), 1);
 }
 
 /* A helper function that does all the work for "catch load" and
@@ -8540,9 +8539,9 @@ init_catchpoint (struct breakpoint *b,
 }
 
 void
-install_breakpoint (int internal, struct breakpoint *b, int update_gll)
+install_breakpoint (int internal, std::unique_ptr<breakpoint> &&arg, int update_gll)
 {
-  add_to_breakpoint_chain (b);
+  breakpoint *b = add_to_breakpoint_chain (std::move (arg));
   set_breakpoint_number (internal, b);
   if (is_tracepoint (b))
     set_tracepoint_count (breakpoint_count);
@@ -8559,13 +8558,13 @@ create_fork_vfork_event_catchpoint (struct gdbarch *gdbarch,
 				    int tempflag, const char *cond_string,
                                     const struct breakpoint_ops *ops)
 {
-  struct fork_catchpoint *c = new fork_catchpoint ();
+  std::unique_ptr<fork_catchpoint> c (new fork_catchpoint ());
 
-  init_catchpoint (c, gdbarch, tempflag, cond_string, ops);
+  init_catchpoint (c.get (), gdbarch, tempflag, cond_string, ops);
 
   c->forked_inferior_pid = null_ptid;
 
-  install_breakpoint (0, c, 1);
+  install_breakpoint (0, std::move (c), 1);
 }
 
 /* Exec catchpoints.  */
@@ -8810,9 +8809,9 @@ enable_breakpoints_after_startup (void)
 static struct breakpoint *
 new_single_step_breakpoint (int thread, struct gdbarch *gdbarch)
 {
-  struct breakpoint *b = new breakpoint ();
+  std::unique_ptr<breakpoint> b (new breakpoint ());
 
-  init_raw_breakpoint_without_location (b, gdbarch, bp_single_step,
+  init_raw_breakpoint_without_location (b.get (), gdbarch, bp_single_step,
 					&momentary_breakpoint_ops);
 
   b->disposition = disp_donttouch;
@@ -8821,9 +8820,7 @@ new_single_step_breakpoint (int thread, struct gdbarch *gdbarch)
   b->thread = thread;
   gdb_assert (b->thread != 0);
 
-  add_to_breakpoint_chain (b);
-
-  return b;
+  return add_to_breakpoint_chain (std::move (b));
 }
 
 /* Set a momentary breakpoint of type TYPE at address specified by
@@ -9308,7 +9305,7 @@ create_breakpoint_sal (struct gdbarch *gdbarch,
 		       enabled, internal, flags,
 		       display_canonical);
 
-  install_breakpoint (internal, b.release (), 0);
+  install_breakpoint (internal, std::move (b), 0);
 }
 
 /* Add SALS.nelts breakpoints to the breakpoint table.  For each
@@ -9798,7 +9795,7 @@ create_breakpoint (struct gdbarch *gdbarch,
            && type_wanted != bp_hardware_breakpoint) || thread != -1)
 	b->pspace = current_program_space;
 
-      install_breakpoint (internal, b.release (), 0);
+      install_breakpoint (internal, std::move (b), 0);
     }
   
   if (VEC_length (linespec_sals, canonical.sals) > 1)
@@ -10961,7 +10958,6 @@ watch_command_1 (const char *arg, int accessflag, int from_tty,
      the hardware watchpoint.  */
   int use_mask = 0;
   CORE_ADDR mask = 0;
-  struct watchpoint *w;
   char *expression;
   struct cleanup *back_to;
 
@@ -11182,13 +11178,13 @@ watch_command_1 (const char *arg, int accessflag, int from_tty,
   else
     bp_type = bp_hardware_watchpoint;
 
-  w = new watchpoint ();
+  std::unique_ptr<watchpoint> w (new watchpoint ());
 
   if (use_mask)
-    init_raw_breakpoint_without_location (w, NULL, bp_type,
+    init_raw_breakpoint_without_location (w.get (), NULL, bp_type,
 					  &masked_watchpoint_breakpoint_ops);
   else
-    init_raw_breakpoint_without_location (w, NULL, bp_type,
+    init_raw_breakpoint_without_location (w.get (), NULL, bp_type,
 					  &watchpoint_breakpoint_ops);
   w->thread = thread;
   w->disposition = disp_donttouch;
@@ -11243,26 +11239,17 @@ watch_command_1 (const char *arg, int accessflag, int from_tty,
       /* The scope breakpoint is related to the watchpoint.  We will
 	 need to act on them together.  */
       w->related_breakpoint = scope_breakpoint;
-      scope_breakpoint->related_breakpoint = w;
+      scope_breakpoint->related_breakpoint = w.get ();
     }
 
   if (!just_location)
     value_free_to_mark (mark);
 
-  TRY
-    {
-      /* Finally update the new watchpoint.  This creates the locations
-	 that should be inserted.  */
-      update_watchpoint (w, 1);
-    }
-  CATCH (e, RETURN_MASK_ALL)
-    {
-      delete_breakpoint (w);
-      throw_exception (e);
-    }
-  END_CATCH
+  /* Finally update the new watchpoint.  This creates the locations
+     that should be inserted.  */
+  update_watchpoint (w.get (), 1);
 
-  install_breakpoint (internal, w, 1);
+  install_breakpoint (internal, std::move (w), 1);
   do_cleanups (back_to);
 }
 
@@ -11710,7 +11697,6 @@ catch_exec_command_1 (char *arg_entry, int from_tty,
 		      struct cmd_list_element *command)
 {
   const char *arg = arg_entry;
-  struct exec_catchpoint *c;
   struct gdbarch *gdbarch = get_current_arch ();
   int tempflag;
   const char *cond_string = NULL;
@@ -11731,12 +11717,12 @@ catch_exec_command_1 (char *arg_entry, int from_tty,
   if ((*arg != '\0') && !isspace (*arg))
     error (_("Junk at end of arguments."));
 
-  c = new exec_catchpoint ();
-  init_catchpoint (c, gdbarch, tempflag, cond_string,
+  std::unique_ptr<exec_catchpoint> c (new exec_catchpoint ());
+  init_catchpoint (c.get (), gdbarch, tempflag, cond_string,
 		   &catch_exec_breakpoint_ops);
   c->exec_pathname = NULL;
 
-  install_breakpoint (0, c, 1);
+  install_breakpoint (0, std::move (c), 1);
 }
 
 void
@@ -13559,7 +13545,6 @@ strace_marker_create_breakpoints_sal (struct gdbarch *gdbarch,
   for (i = 0; i < lsal->sals.nelts; ++i)
     {
       struct symtabs_and_lines expanded;
-      struct tracepoint *tp;
       event_location_up location;
 
       expanded.nelts = 1;
@@ -13567,8 +13552,8 @@ strace_marker_create_breakpoints_sal (struct gdbarch *gdbarch,
 
       location = copy_event_location (canonical->location.get ());
 
-      tp = new tracepoint ();
-      init_breakpoint_sal (tp, gdbarch, expanded,
+      std::unique_ptr<tracepoint> tp (new tracepoint ());
+      init_breakpoint_sal (tp.get (), gdbarch, expanded,
 			   std::move (location), NULL,
 			   std::move (cond_string),
 			   std::move (extra_string),
@@ -13584,7 +13569,7 @@ strace_marker_create_breakpoints_sal (struct gdbarch *gdbarch,
 	 corresponds to this one  */
       tp->static_trace_marker_id_idx = i;
 
-      install_breakpoint (internal, tp, 0);
+      install_breakpoint (internal, std::move (tp), 0);
     }
 }
 
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index d955184..dc2b098 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -1296,7 +1296,7 @@ extern void init_catchpoint (struct breakpoint *b,
    the internal breakpoint count.  If UPDATE_GLL is non-zero,
    update_global_location_list will be called.  */
 
-extern void install_breakpoint (int internal, struct breakpoint *b,
+extern void install_breakpoint (int internal, std::unique_ptr<breakpoint> &&b,
 				int update_gll);
 
 /* Flags that can be passed down to create_breakpoint, etc., to affect

Re: [RFA 2/2] Change install_breakpoint to take a std::unique_ptr

[Pedro Alves]

On 08/22/2017 04:55 AM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
> 
> [...]
> Pedro> or change add_to_breakpoint_chain to take an rvalue-ref unique_ptr
> Pedro> too, and return the raw breakpoint, and then write here:
> Pedro>     breakpoint *b = add_to_breakpoint_chain (std::move (arg));
> 
> Sounds good.  I actually considered this but thought maybe it was a step
> too far... hah.
> 
> How's this?

Perfect.

Thanks,
Pedro Alves