diff mbox

Make breakpoint subclasses inherit from breakpoint, add virtual destructor

Message ID 20170502191811.32333-1-simon.marchi@ericsson.com
State New, archived
Headers

Commit Message

Simon Marchi May 2, 2017, 7:18 p.m. UTC 
  From: Simon Marchi <simon.marchi@polymtl.ca>

Tom recently mentioned on IRC how breakpoint deallocation looked fishy.  A
syscall catchpoint, for example, is created with "new syscall_catchpoint", but
free'd using "delete bpt", where bpt is a breakpoint *.  I had this patch lying
around in a branch, so I decided to post it by itself.

---

I want to replace the vectors in the various breakpoint subclasses by
std::vector.  The problem right now is that while breakpoint
subclasses are constructed using new, they are not properly deleted.
The only place breakpoints are deleted is through a breakpoint pointer
in delete_breakpoint.  This means that even if I add a destructor in a
subclass (e.g. syscall_catchpoint), it's not going to be called, for two
reasons:

1. The destructor of breakpoint needs to be virtual if we want the
   destructors from the subclasses to be called.
2. The subclasses need to be actual subclasses, not just include the
   base class as a field.

It turns out at #2 generates a lot of small changes (removing "base."
everywhere), but it makes the code generally a bit nicer.

gdb/ChangeLog:

	* ada-lang.c (struct ada_catchpoint): Inherit from struct
	breakpoint.
	<base>: Remove.
	(create_excep_cond_exprs): Adjust.
	(create_ada_exception_catchpoint): Adjust.
	* break-catch-sig.c (struct signal_catchpoint): Inherit from
	struct breakpoint.
	<base>: Remove.
	(create_signal_catchpoint): Adjust.
	* break-catch-syscall.c (UNKNOWN): Adjust.
	(create_syscall_event_catchpoint): Adjust.
	* break-catch-throw.c (static): Adjust.
	(handle_gnu_v3_exceptions): Adjust.
	* breakpoint.c (is_watchpoint): Adjust.
	(watchpoint_in_thread_scope): Adjust.
	(update_watchpoint): Adjust.
	(watchpoint_check): Adjust.
	(bpstat_check_watchpoint): Adjust.
	(disable_breakpoints_in_freed_objfile): Adjust.
	(print_recreate_catch_vfork): Adjust.
	(breakpoint_hit_catch_solib): Adjust.
	(add_solib_catchpoint): Adjust.
	(create_fork_vfork_event_catchpoint): Adjust.
	(create_breakpoint_sal): Adjust.
	(create_breakpoint): Adjust.
	(static): Adjust.
	(watch_command_1): Adjust.
	(catch_exec_command_1): Adjust.
	(strace_marker_create_breakpoints_sal): Adjust.
	(create_tracepoint_from_upload): Adjust.
	(static): Adjust.
	* breakpoint.h (extern): Adjust.
	(struct breakpoint): Adjust.
	(extern): Adjust.
	* ctf.c (ctf_get_traceframe_address): Adjust.
	* mi/mi-cmd-break.c (mi_cmd_break_passcount): Adjust.
	* remote.c (remote_get_tracepoint_status): Adjust.
	* tracefile-tfile.c (tfile_get_traceframe_address): Adjust.
	* tracefile.c (tracefile_fetch_registers): Adjust.
	* tracepoint.c (actions_command): Adjust.
	(validate_actionline): Adjust.
	(tfind_1): Adjust.
	(get_traceframe_location): Adjust.
	(find_matching_tracepoint_location): Adjust.
	(merge_uploaded_tracepoints): Adjust.
	(parse_tracepoint_status): Adjust.
---
 gdb/ada-lang.c            |  21 ++----
 gdb/break-catch-sig.c     |  14 +---
 gdb/break-catch-syscall.c |  13 +---
 gdb/break-catch-throw.c   |  14 ++--
 gdb/breakpoint.c          | 187 ++++++++++++++++++++--------------------------
 gdb/breakpoint.h          |  22 +++---
 gdb/ctf.c                 |   4 +-
 gdb/mi/mi-cmd-break.c     |   2 +-
 gdb/remote.c              |   4 +-
 gdb/tracefile-tfile.c     |   4 +-
 gdb/tracefile.c           |  10 +--
 gdb/tracepoint.c          |  20 ++---
 12 files changed, 132 insertions(+), 183 deletions(-)

Comments

Pedro Alves May 3, 2017, 10:17 a.m. UTC | #1 
Hi Simon,

Many thanks for doing this.

On 05/02/2017 08:18 PM, Simon Marchi wrote:
> From: Simon Marchi <simon.marchi@polymtl.ca>
> 
> Tom recently mentioned on IRC how breakpoint deallocation looked fishy.  A
> syscall catchpoint, for example, is created with "new syscall_catchpoint", but
> free'd using "delete bpt", where bpt is a breakpoint *.

Note that currently the the "syscall_catchpoint"
part is freed by dtor_catch_syscall, called via the breakpoint_ops->dtor.

  bpt->ops->dtor (bpt); <<< here
  /* On the chance that someone will soon try again to delete this
     same bp, we mark it as deleted before freeing its storage.  */
  bpt->type = bp_none;
  delete bpt;

But of course, that only works as long as "syscall_catchpoint"'s fields
are trivially destructible.  Otherwise the breakpoint_ops->dtor method
would have to call desctructors manually.  Urgh.

> I had this patch lying
> around in a branch, so I decided to post it by itself.

> 
> I want to replace the vectors in the various breakpoint subclasses by
> std::vector.  The problem right now is that while breakpoint
> subclasses are constructed using new, they are not properly deleted.

I think "properly deleted" might not be 100% accurate.

> The only place breakpoints are deleted is through a breakpoint pointer
> in delete_breakpoint.  This means that even if I add a destructor in a
> subclass (e.g. syscall_catchpoint), it's not going to be called, for two
> reasons:
> 
> 1. The destructor of breakpoint needs to be virtual if we want the
>    destructors from the subclasses to be called.
> 2. The subclasses need to be actual subclasses, not just include the
>    base class as a field.
> 
> It turns out at #2 generates a lot of small changes (removing "base."
> everywhere), but it makes the code generally a bit nicer.

Most of the breakpoint_ops function pointers should really be virtual methods
of struct breakpoint.  Over the years, they've been adjusted to map better
to a vtable model [1], though there are a few that are really factory
methods that don't translate properly, because they would require a breakpoint
instance to be called on, when their purpose is to create said instances.
"breakpoint_ops::dtor" is really the most obvious one and best one
to kickstart such a conversion.

[1] e.g. https://sourceware.org/ml/gdb-patches/2011-06/msg00269.html,
https://sourceware.org/ml/gdb-patches/2011-06/msg00296.html.

But I'm then surprised that the patch doesn't eliminate breakpoint_ops::dtor
at the same time.  The patch would be simple to justify in those terms
(breakpoint_ops::dtor -> real breakpoint C++ dtor).  If breakpoint_ops::dtor
is still necessary, then this patch is probably not complete?  If we keep
it, then destruction still looks fishy to me, with the C++ dtor potentially
destroying objects that breakpoint_ops::dtor already freed.  Could you take
a look at that, see if it doesn't cause this patch to grow too much?  I think
not, I think mostly you'll just need to rename a few dtor_foo methods
to foo::~foo.

> 
> gdb/ChangeLog:
> 
> 	* ada-lang.c (struct ada_catchpoint): Inherit from struct
> 	breakpoint.
> 	<base>: Remove.
> 	(create_excep_cond_exprs): Adjust.
> 	(create_ada_exception_catchpoint): Adjust.
> 	* break-catch-sig.c (struct signal_catchpoint): Inherit from
> 	struct breakpoint.
> 	<base>: Remove.
> 	(create_signal_catchpoint): Adjust.
> 	* break-catch-syscall.c (UNKNOWN): Adjust.
> 	(create_syscall_event_catchpoint): Adjust.
> 	* break-catch-throw.c (static): Adjust.
> 	(handle_gnu_v3_exceptions): Adjust.
> 	* breakpoint.c (is_watchpoint): Adjust.
> 	(watchpoint_in_thread_scope): Adjust.
> 	(update_watchpoint): Adjust.
> 	(watchpoint_check): Adjust.
> 	(bpstat_check_watchpoint): Adjust.
> 	(disable_breakpoints_in_freed_objfile): Adjust.
> 	(print_recreate_catch_vfork): Adjust.
> 	(breakpoint_hit_catch_solib): Adjust.
> 	(add_solib_catchpoint): Adjust.
> 	(create_fork_vfork_event_catchpoint): Adjust.
> 	(create_breakpoint_sal): Adjust.
> 	(create_breakpoint): Adjust.

> 	(static): Adjust.

This entry doesn't look right.


> 	(watch_command_1): Adjust.
> 	(catch_exec_command_1): Adjust.
> 	(strace_marker_create_breakpoints_sal): Adjust.
> 	(create_tracepoint_from_upload): Adjust.
> 	(static): Adjust.

Ditto.

> 	* breakpoint.h (extern): Adjust.
> 	(struct breakpoint): Adjust.
> 	(extern): Adjust.

Ditto.

> 	* ctf.c (ctf_get_traceframe_address): Adjust.
> 	* mi/mi-cmd-break.c (mi_cmd_break_passcount): Adjust.
> 	* remote.c (remote_get_tracepoint_status): Adjust.
> 	* tracefile-tfile.c (tfile_get_traceframe_address): Adjust.
> 	* tracefile.c (tracefile_fetch_registers): Adjust.
> 	* tracepoint.c (actions_command): Adjust.
> 	(validate_actionline): Adjust.
> 	(tfind_1): Adjust.
> 	(get_traceframe_location): Adjust.
> 	(find_matching_tracepoint_location): Adjust.
> 	(merge_uploaded_tracepoints): Adjust.
> 	(parse_tracepoint_status): Adjust.

Thanks,
Pedro Alves
Simon Marchi May 3, 2017, 2:36 p.m. UTC | #2 
On 2017-05-03 06:17, Pedro Alves wrote:
> Hi Simon,
> 
> Many thanks for doing this.
> 
> On 05/02/2017 08:18 PM, Simon Marchi wrote:
>> From: Simon Marchi <simon.marchi@polymtl.ca>
>> 
>> Tom recently mentioned on IRC how breakpoint deallocation looked 
>> fishy.  A
>> syscall catchpoint, for example, is created with "new 
>> syscall_catchpoint", but
>> free'd using "delete bpt", where bpt is a breakpoint *.
> 
> Note that currently the the "syscall_catchpoint"
> part is freed by dtor_catch_syscall, called via the 
> breakpoint_ops->dtor.

Right.

>   bpt->ops->dtor (bpt); <<< here
>   /* On the chance that someone will soon try again to delete this
>      same bp, we mark it as deleted before freeing its storage.  */
>   bpt->type = bp_none;
>   delete bpt;
> 
> But of course, that only works as long as "syscall_catchpoint"'s fields
> are trivially destructible.  Otherwise the breakpoint_ops->dtor method
> would have to call desctructors manually.  Urgh.

Right.

>> I had this patch lying
>> around in a branch, so I decided to post it by itself.
> 
>> 
>> I want to replace the vectors in the various breakpoint subclasses by
>> std::vector.  The problem right now is that while breakpoint
>> subclasses are constructed using new, they are not properly deleted.
> 
> I think "properly deleted" might not be 100% accurate.

Hmm what do you suggest?  I could say:

   ... their C++ destructor is not being called.

>> The only place breakpoints are deleted is through a breakpoint pointer
>> in delete_breakpoint.  This means that even if I add a destructor in a
>> subclass (e.g. syscall_catchpoint), it's not going to be called, for 
>> two
>> reasons:
>> 
>> 1. The destructor of breakpoint needs to be virtual if we want the
>>    destructors from the subclasses to be called.
>> 2. The subclasses need to be actual subclasses, not just include the
>>    base class as a field.
>> 
>> It turns out at #2 generates a lot of small changes (removing "base."
>> everywhere), but it makes the code generally a bit nicer.
> 
> Most of the breakpoint_ops function pointers should really be virtual 
> methods
> of struct breakpoint.  Over the years, they've been adjusted to map 
> better
> to a vtable model [1], though there are a few that are really factory
> methods that don't translate properly, because they would require a 
> breakpoint
> instance to be called on, when their purpose is to create said 
> instances.
> "breakpoint_ops::dtor" is really the most obvious one and best one
> to kickstart such a conversion.

Indeed.

> [1] e.g. https://sourceware.org/ml/gdb-patches/2011-06/msg00269.html,
> https://sourceware.org/ml/gdb-patches/2011-06/msg00296.html.
> 
> But I'm then surprised that the patch doesn't eliminate 
> breakpoint_ops::dtor
> at the same time.  The patch would be simple to justify in those terms
> (breakpoint_ops::dtor -> real breakpoint C++ dtor).  If 
> breakpoint_ops::dtor
> is still necessary, then this patch is probably not complete?  If we 
> keep
> it, then destruction still looks fishy to me, with the C++ dtor 
> potentially
> destroying objects that breakpoint_ops::dtor already freed.  Could you 
> take
> a look at that, see if it doesn't cause this patch to grow too much?  I 
> think
> not, I think mostly you'll just need to rename a few dtor_foo methods
> to foo::~foo.

You're right, it would be confusing and ugly to leave it with a 
half-baked-dual-hybrid system with C++ destructors and dtor ops.  I'll 
remove the dtor op, it shouldn't be much work, as you said.

>> 
>> gdb/ChangeLog:
>> 
>> 	* ada-lang.c (struct ada_catchpoint): Inherit from struct
>> 	breakpoint.
>> 	<base>: Remove.
>> 	(create_excep_cond_exprs): Adjust.
>> 	(create_ada_exception_catchpoint): Adjust.
>> 	* break-catch-sig.c (struct signal_catchpoint): Inherit from
>> 	struct breakpoint.
>> 	<base>: Remove.
>> 	(create_signal_catchpoint): Adjust.
>> 	* break-catch-syscall.c (UNKNOWN): Adjust.
>> 	(create_syscall_event_catchpoint): Adjust.
>> 	* break-catch-throw.c (static): Adjust.
>> 	(handle_gnu_v3_exceptions): Adjust.
>> 	* breakpoint.c (is_watchpoint): Adjust.
>> 	(watchpoint_in_thread_scope): Adjust.
>> 	(update_watchpoint): Adjust.
>> 	(watchpoint_check): Adjust.
>> 	(bpstat_check_watchpoint): Adjust.
>> 	(disable_breakpoints_in_freed_objfile): Adjust.
>> 	(print_recreate_catch_vfork): Adjust.
>> 	(breakpoint_hit_catch_solib): Adjust.
>> 	(add_solib_catchpoint): Adjust.
>> 	(create_fork_vfork_event_catchpoint): Adjust.
>> 	(create_breakpoint_sal): Adjust.
>> 	(create_breakpoint): Adjust.
> 
>> 	(static): Adjust.
> 
> This entry doesn't look right.

Oops, so this ChangeLog looked done when I cherry-picked the patch from 
the branch, but clearly it was still raw (those are artifacts from the 
generate-changelog.py script).  I'll put it back in the oven.

Thanks,

Simon
Pedro Alves May 3, 2017, 3:08 p.m. UTC | #3 
On 05/03/2017 03:36 PM, Simon Marchi wrote:
>>> I want to replace the vectors in the various breakpoint subclasses by
>>> std::vector.  The problem right now is that while breakpoint
>>> subclasses are constructed using new, they are not properly deleted.
>>

>> I think "properly deleted" might not be 100% accurate.
> 
> Hmm what do you suggest?  I could say:
> 
>   ... their C++ destructor is not being called.

Yeah.  It's not very important.  I was more referring to the fact that
there's actual destruction of the "subclasses" than talking about
properly-deleted-as-in-the-corresponding-dtor-is-called.

To be crystal clear, I'd put "subclasses" in quotes, and add an example:

~~~
 I want to replace the vectors in the various breakpoint subclasses by
 std::vector.  The problem right now is that while breakpoint
 "subclasses" are constructed using new, they are not properly deleted:

  struct syscall_catchpoint
  {
    /* The base class, old C style.  */
    struct breakpoint base;

    // trivial fields here
  };

  // first member is pointer-interconvertible.
  breakpoint *bp = (breakpoint *) new syscall_catchpoint ();

  // this calls ~breakpoint(), not ~syscall_catchpoint()...
  delete bp;  // in delete_breakpoint

 So if we add any non-trivially destructible field to
 syscall_catchpoint, it won't be properly destructed...
~~~

> You're right, it would be confusing and ugly to leave it with a
> half-baked-dual-hybrid system with C++ destructors and dtor ops.  I'll
> remove the dtor op, it shouldn't be much work, as you said.

Thanks much!
Simon Marchi May 3, 2017, 3:23 p.m. UTC | #4 
On 2017-05-03 11:08, Pedro Alves wrote:
> On 05/03/2017 03:36 PM, Simon Marchi wrote:
>>>> I want to replace the vectors in the various breakpoint subclasses 
>>>> by
>>>> std::vector.  The problem right now is that while breakpoint
>>>> subclasses are constructed using new, they are not properly deleted.
>>> 
> 
>>> I think "properly deleted" might not be 100% accurate.
>> 
>> Hmm what do you suggest?  I could say:
>> 
>>   ... their C++ destructor is not being called.
> 
> Yeah.  It's not very important.  I was more referring to the fact that
> there's actual destruction of the "subclasses" than talking about
> properly-deleted-as-in-the-corresponding-dtor-is-called.
> 
> To be crystal clear, I'd put "subclasses" in quotes, and add an 
> example:
> 
> ~~~
>  I want to replace the vectors in the various breakpoint subclasses by
>  std::vector.  The problem right now is that while breakpoint
>  "subclasses" are constructed using new, they are not properly deleted:
> 
>   struct syscall_catchpoint
>   {
>     /* The base class, old C style.  */
>     struct breakpoint base;
> 
>     // trivial fields here
>   };
> 
>   // first member is pointer-interconvertible.
>   breakpoint *bp = (breakpoint *) new syscall_catchpoint ();
> 
>   // this calls ~breakpoint(), not ~syscall_catchpoint()...
>   delete bp;  // in delete_breakpoint
> 
>  So if we add any non-trivially destructible field to
>  syscall_catchpoint, it won't be properly destructed...

Ok, this should make it clear.

> ~~~
> 
>> You're right, it would be confusing and ugly to leave it with a
>> half-baked-dual-hybrid system with C++ destructors and dtor ops.  I'll
>> remove the dtor op, it shouldn't be much work, as you said.
> 
> Thanks much!

FYI, I just looked at it, and it looks like the 
momentary_breakpoint/longjmp_breakpoint hierarchy will cause a bit of 
trouble.  longjmp_breakpoint has a dtor, but no struct/class of its own, 
so nowhere to put the destructor.  I think that to do it correctly, I'll 
have to introduce structs/classes for them and have:

   breakpoint
       ^
       |
   momentary_breakpoint
       ^
       |
   longjmp_breakpoint

To keep it clean, it might be better if I introduced the structs/classes 
for momentary_breakpoint and longjmp_breakpoint first with the old-style 
inheritance, and then converted them to "real" inheritance along with 
the other types.

I'll try that tonight, but if you have ideas in the mean time, I'm all 
ears.

Simon
Pedro Alves May 3, 2017, 3:27 p.m. UTC | #5 
On 05/03/2017 04:23 PM, Simon Marchi wrote:

> FYI, I just looked at it, and it looks like the
> momentary_breakpoint/longjmp_breakpoint hierarchy will cause a bit of
> trouble.  longjmp_breakpoint has a dtor, but no struct/class of its own,
> so nowhere to put the destructor.  I think that to do it correctly, I'll
> have to introduce structs/classes for them and have:
> 
>   breakpoint
>       ^
>       |
>   momentary_breakpoint
>       ^
>       |
>   longjmp_breakpoint
> 
> To keep it clean, it might be better if I introduced the structs/classes
> for momentary_breakpoint and longjmp_breakpoint first with the old-style
> inheritance, and then converted them to "real" inheritance along with
> the other types.

That sounds fine to me.

> 
> I'll try that tonight, but if you have ideas in the mean time, I'm all
> ears.

Thanks,
Pedro Alves
diff mbox

Patch

diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
index 58c8a2efc9..f4eb429dfe 100644
--- a/gdb/ada-lang.c
+++ b/gdb/ada-lang.c
@@ -12257,15 +12257,10 @@  static const struct bp_location_ops ada_catchpoint_location_ops =
   ada_catchpoint_location_dtor
 };
 
-/* An instance of this type is used to represent an Ada catchpoint.
-   It includes a "struct breakpoint" as a kind of base class; users
-   downcast to "struct breakpoint *" when needed.  */
+/* An instance of this type is used to represent an Ada catchpoint.  */
 
-struct ada_catchpoint
+struct ada_catchpoint : public breakpoint
 {
-  /* The base class.  */
-  struct breakpoint base;
-
   /* The name of the specific exception the user specified.  */
   char *excep_string;
 };
@@ -12285,7 +12280,7 @@  create_excep_cond_exprs (struct ada_catchpoint *c)
     return;
 
   /* Same if there are no locations... */
-  if (c->base.loc == NULL)
+  if (c->loc == NULL)
     return;
 
   /* Compute the condition expression in text form, from the specific
@@ -12295,7 +12290,7 @@  create_excep_cond_exprs (struct ada_catchpoint *c)
 
   /* Iterate over all the catchpoint's locations, and parse an
      expression for each.  */
-  for (bl = c->base.loc; bl != NULL; bl = bl->next)
+  for (bl = c->loc; bl != NULL; bl = bl->next)
     {
       struct ada_catchpoint_location *ada_loc
 	= (struct ada_catchpoint_location *) bl;
@@ -12316,7 +12311,7 @@  create_excep_cond_exprs (struct ada_catchpoint *c)
 	    {
 	      warning (_("failed to reevaluate internal exception condition "
 			 "for catchpoint %d: %s"),
-		       c->base.number, e.message);
+		       c->number, e.message);
 	    }
 	  END_CATCH
 	}
@@ -13060,13 +13055,13 @@  create_ada_exception_catchpoint (struct gdbarch *gdbarch,
     = ada_exception_sal (ex_kind, excep_string, &addr_string, &ops);
 
   c = new ada_catchpoint ();
-  init_ada_exception_breakpoint (&c->base, gdbarch, sal, addr_string,
+  init_ada_exception_breakpoint (c, gdbarch, sal, addr_string,
 				 ops, tempflag, disabled, from_tty);
   c->excep_string = excep_string;
   create_excep_cond_exprs (c);
   if (cond_string != NULL)
-    set_breakpoint_condition (&c->base, cond_string, from_tty);
-  install_breakpoint (0, &c->base, 1);
+    set_breakpoint_condition (c, cond_string, from_tty);
+  install_breakpoint (0, c, 1);
 }
 
 /* Implement the "catch exception" command.  */
diff --git a/gdb/break-catch-sig.c b/gdb/break-catch-sig.c
index 5f02fe6230..03bf319ae6 100644
--- a/gdb/break-catch-sig.c
+++ b/gdb/break-catch-sig.c
@@ -38,17 +38,11 @@  typedef enum gdb_signal gdb_signal_type;
 DEF_VEC_I (gdb_signal_type);
 
 /* An instance of this type is used to represent a signal catchpoint.
-   It includes a "struct breakpoint" as a kind of base class; users
-   downcast to "struct breakpoint *" when needed.  A breakpoint is
-   really of this type iff its ops pointer points to
+   A breakpoint is really of this type iff its ops pointer points to
    SIGNAL_CATCHPOINT_OPS.  */
 
-struct signal_catchpoint
+struct signal_catchpoint : public breakpoint
 {
-  /* The base class.  */
-
-  struct breakpoint base;
-
   /* Signal numbers used for the 'catch signal' feature.  If no signal
      has been specified for filtering, its value is NULL.  Otherwise,
      it holds a list of all signals to be caught.  */
@@ -372,11 +366,11 @@  create_signal_catchpoint (int tempflag, VEC (gdb_signal_type) *filter,
   struct gdbarch *gdbarch = get_current_arch ();
 
   c = new signal_catchpoint ();
-  init_catchpoint (&c->base, gdbarch, tempflag, NULL, &signal_catchpoint_ops);
+  init_catchpoint (c, gdbarch, tempflag, NULL, &signal_catchpoint_ops);
   c->signals_to_be_caught = filter;
   c->catch_all = catch_all;
 
-  install_breakpoint (0, &c->base, 1);
+  install_breakpoint (0, c, 1);
 }
 
 
diff --git a/gdb/break-catch-syscall.c b/gdb/break-catch-syscall.c
index 322f680ad8..3d0f77bb8e 100644
--- a/gdb/break-catch-syscall.c
+++ b/gdb/break-catch-syscall.c
@@ -31,16 +31,11 @@ 
 #include "xml-syscall.h"
 
 /* An instance of this type is used to represent a syscall catchpoint.
-   It includes a "struct breakpoint" as a kind of base class; users
-   downcast to "struct breakpoint *" when needed.  A breakpoint is
-   really of this type iff its ops pointer points to
+   A breakpoint is really of this type iff its ops pointer points to
    CATCH_SYSCALL_BREAKPOINT_OPS.  */
 
-struct syscall_catchpoint
+struct syscall_catchpoint : public breakpoint
 {
-  /* The base class.  */
-  struct breakpoint base;
-
   /* Syscall numbers used for the 'catch syscall' feature.  If no
      syscall has been specified for filtering, its value is NULL.
      Otherwise, it holds a list of all syscalls to be caught.  The
@@ -434,10 +429,10 @@  create_syscall_event_catchpoint (int tempflag, VEC(int) *filter,
   struct gdbarch *gdbarch = get_current_arch ();
 
   c = new syscall_catchpoint ();
-  init_catchpoint (&c->base, gdbarch, tempflag, NULL, ops);
+  init_catchpoint (c, gdbarch, tempflag, NULL, ops);
   c->syscalls_to_be_caught = filter;
 
-  install_breakpoint (0, &c->base, 1);
+  install_breakpoint (0, c, 1);
 }
 
 /* Splits the argument using space as delimiter.  Returns an xmalloc'd
diff --git a/gdb/break-catch-throw.c b/gdb/break-catch-throw.c
index 2714bbf26b..441e2d0c13 100644
--- a/gdb/break-catch-throw.c
+++ b/gdb/break-catch-throw.c
@@ -74,12 +74,8 @@  static struct breakpoint_ops gnu_v3_exception_catchpoint_ops;
 
 /* The type of an exception catchpoint.  */
 
-struct exception_catchpoint
+struct exception_catchpoint : public breakpoint
 {
-  /* The base class.  */
-
-  struct breakpoint base;
-
   /* The kind of exception catchpoint.  */
 
   enum exception_event_kind kind;
@@ -396,18 +392,18 @@  handle_gnu_v3_exceptions (int tempflag, char *except_rx,
 
   std::unique_ptr<exception_catchpoint> cp (new exception_catchpoint ());
 
-  init_catchpoint (&cp->base, get_current_arch (), tempflag, cond_string,
+  init_catchpoint (cp.get (), get_current_arch (), tempflag, cond_string,
 		   &gnu_v3_exception_catchpoint_ops);
   /* We need to reset 'type' in order for code in breakpoint.c to do
      the right thing.  */
-  cp->base.type = bp_breakpoint;
+  cp->type = bp_breakpoint;
   cp->kind = ex_event;
   cp->exception_rx = except_rx;
   cp->pattern = pattern;
 
-  re_set_exception_catchpoint (&cp->base);
+  re_set_exception_catchpoint (cp.get ());
 
-  install_breakpoint (0, &cp->base, 1);
+  install_breakpoint (0, cp.get (), 1);
   cp.release ();
 }
 
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index e8d8d09df5..1459be64a7 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -1706,7 +1706,7 @@  is_watchpoint (const struct breakpoint *bpt)
 static int
 watchpoint_in_thread_scope (struct watchpoint *b)
 {
-  return (b->base.pspace == current_program_space
+  return (b->pspace == current_program_space
 	  && (ptid_equal (b->watchpoint_thread, null_ptid)
 	      || (ptid_equal (inferior_ptid, b->watchpoint_thread)
 		  && !is_executing (inferior_ptid))));
@@ -1718,17 +1718,15 @@  watchpoint_in_thread_scope (struct watchpoint *b)
 static void
 watchpoint_del_at_next_stop (struct watchpoint *w)
 {
-  struct breakpoint *b = &w->base;
-
-  if (b->related_breakpoint != b)
+  if (w->related_breakpoint != w)
     {
-      gdb_assert (b->related_breakpoint->type == bp_watchpoint_scope);
-      gdb_assert (b->related_breakpoint->related_breakpoint == b);
-      b->related_breakpoint->disposition = disp_del_at_next_stop;
-      b->related_breakpoint->related_breakpoint = b->related_breakpoint;
-      b->related_breakpoint = b;
+      gdb_assert (w->related_breakpoint->type == bp_watchpoint_scope);
+      gdb_assert (w->related_breakpoint->related_breakpoint == w);
+      w->related_breakpoint->disposition = disp_del_at_next_stop;
+      w->related_breakpoint->related_breakpoint = w->related_breakpoint;
+      w->related_breakpoint = w;
     }
-  b->disposition = disp_del_at_next_stop;
+  w->disposition = disp_del_at_next_stop;
 }
 
 /* Extract a bitfield value from value VAL using the bit parameters contained in
@@ -1849,7 +1847,7 @@  update_watchpoint (struct watchpoint *b, int reparse)
   if (!watchpoint_in_thread_scope (b))
     return;
 
-  if (b->base.disposition == disp_del_at_next_stop)
+  if (b->disposition == disp_del_at_next_stop)
     return;
  
   frame_saved = 0;
@@ -1887,7 +1885,7 @@  update_watchpoint (struct watchpoint *b, int reparse)
   /* We don't free locations.  They are stored in the bp_location array
      and update_global_location_list will eventually delete them and
      remove breakpoints if needed.  */
-  b->base.loc = NULL;
+  b->loc = NULL;
 
   if (within_current_scope && reparse)
     {
@@ -1907,11 +1905,11 @@  update_watchpoint (struct watchpoint *b, int reparse)
       /* Note that unlike with breakpoints, the watchpoint's condition
 	 expression is stored in the breakpoint object, not in the
 	 locations (re)created below.  */
-      if (b->base.cond_string != NULL)
+      if (b->cond_string != NULL)
 	{
 	  b->cond_exp.reset ();
 
-	  s = b->base.cond_string;
+	  s = b->cond_string;
 	  b->cond_exp = parse_exp_1 (&s, 0, b->cond_exp_valid_block, 0);
 	}
     }
@@ -1928,8 +1926,8 @@  update_watchpoint (struct watchpoint *b, int reparse)
 	 the target gains execution, through breakpoint_re_set.  */
       if (!can_use_hw_watchpoints)
 	{
-	  if (b->base.ops->works_in_software_mode (&b->base))
-	    b->base.type = bp_watchpoint;
+	  if (b->ops->works_in_software_mode (b))
+	    b->type = bp_watchpoint;
 	  else
 	    error (_("Can't set read/access watchpoint when "
 		     "hardware watchpoints are disabled."));
@@ -1949,7 +1947,7 @@  update_watchpoint (struct watchpoint *b, int reparse)
 	 happens, the code that reports it updates b->val directly.
 	 We don't keep track of the memory value for masked
 	 watchpoints.  */
-      if (!b->val_valid && !is_masked_watchpoint (&b->base))
+      if (!b->val_valid && !is_masked_watchpoint (b))
 	{
 	  if (b->val_bitsize != 0)
 	    {
@@ -2012,13 +2010,13 @@  update_watchpoint (struct watchpoint *b, int reparse)
 		    }
 
 		  type = hw_write;
-		  if (b->base.type == bp_read_watchpoint)
+		  if (b->type == bp_read_watchpoint)
 		    type = hw_read;
-		  else if (b->base.type == bp_access_watchpoint)
+		  else if (b->type == bp_access_watchpoint)
 		    type = hw_access;
 
-		  loc = allocate_bp_location (&b->base);
-		  for (tmp = &(b->base.loc); *tmp != NULL; tmp = &((*tmp)->next))
+		  loc = allocate_bp_location (b);
+		  for (tmp = &(b->loc); *tmp != NULL; tmp = &((*tmp)->next))
 		    ;
 		  *tmp = loc;
 		  loc->gdbarch = get_type_arch (value_type (v));
@@ -2068,7 +2066,7 @@  update_watchpoint (struct watchpoint *b, int reparse)
 	      /* If this is a software watchpoint, we try to turn it
 		 to a hardware one -- count resources as if B was of
 		 hardware watchpoint type.  */
-	      type = b->base.type;
+	      type = b->type;
 	      if (type == bp_watchpoint)
 		type = bp_hardware_watchpoint;
 
@@ -2079,16 +2077,16 @@  update_watchpoint (struct watchpoint *b, int reparse)
 		 manually.  */
 
 	      /* Count resources used by all watchpoints except B.  */
-	      i = hw_watchpoint_used_count_others (&b->base, type, &other_type_used);
+	      i = hw_watchpoint_used_count_others (b, type, &other_type_used);
 
 	      /* Add in the resources needed for B.  */
-	      i += hw_watchpoint_use_count (&b->base);
+	      i += hw_watchpoint_use_count (b);
 
 	      target_resources_ok
 		= target_can_use_hardware_watchpoint (type, i, other_type_used);
 	      if (target_resources_ok <= 0)
 		{
-		  int sw_mode = b->base.ops->works_in_software_mode (&b->base);
+		  int sw_mode = b->ops->works_in_software_mode (b);
 
 		  if (target_resources_ok == 0 && !sw_mode)
 		    error (_("Target does not support this type of "
@@ -2098,7 +2096,7 @@  update_watchpoint (struct watchpoint *b, int reparse)
 			     "resources for this watchpoint."));
 
 		  /* Downgrade to software watchpoint.  */
-		  b->base.type = bp_watchpoint;
+		  b->type = bp_watchpoint;
 		}
 	      else
 		{
@@ -2106,10 +2104,10 @@  update_watchpoint (struct watchpoint *b, int reparse)
 		     found we have enough resources to turn it to a
 		     hardware watchpoint.  Otherwise, this is a
 		     nop.  */
-		  b->base.type = type;
+		  b->type = type;
 		}
 	    }
-	  else if (!b->base.ops->works_in_software_mode (&b->base))
+	  else if (!b->ops->works_in_software_mode (b))
 	    {
 	      if (!can_use_hw_watchpoints)
 		error (_("Can't set read/access watchpoint when "
@@ -2119,11 +2117,11 @@  update_watchpoint (struct watchpoint *b, int reparse)
 			 "read/access watchpoint."));
 	    }
 	  else
-	    b->base.type = bp_watchpoint;
+	    b->type = bp_watchpoint;
 
-	  loc_type = (b->base.type == bp_watchpoint? bp_loc_other
+	  loc_type = (b->type == bp_watchpoint? bp_loc_other
 		      : bp_loc_hardware_watchpoint);
-	  for (bl = b->base.loc; bl; bl = bl->next)
+	  for (bl = b->loc; bl; bl = bl->next)
 	    bl->loc_type = loc_type;
 	}
 
@@ -2138,15 +2136,15 @@  update_watchpoint (struct watchpoint *b, int reparse)
 	 above left it without any location set up.  But,
 	 bpstat_stop_status requires a location to be able to report
 	 stops, so make sure there's at least a dummy one.  */
-      if (b->base.type == bp_watchpoint && b->base.loc == NULL)
-	software_watchpoint_add_no_memory_location (&b->base, frame_pspace);
+      if (b->type == bp_watchpoint && b->loc == NULL)
+	software_watchpoint_add_no_memory_location (b, frame_pspace);
     }
   else if (!within_current_scope)
     {
       printf_filtered (_("\
 Watchpoint %d deleted because the program has left the block\n\
 in which its expression is valid.\n"),
-		       b->base.number);
+		       b->number);
       watchpoint_del_at_next_stop (b);
     }
 
@@ -5182,7 +5180,7 @@  watchpoint_check (void *p)
       struct value *mark;
       struct value *new_val;
 
-      if (is_masked_watchpoint (&b->base))
+      if (is_masked_watchpoint (b))
 	/* Since we don't know the exact trigger address (from
 	   stopped_data_address), just tell the user we've triggered
 	   a mask watchpoint.  */
@@ -5242,13 +5240,13 @@  watchpoint_check (void *p)
 	    uiout->field_string
 	      ("reason", async_reason_lookup (EXEC_ASYNC_WATCHPOINT_SCOPE));
 	  uiout->text ("\nWatchpoint ");
-	  uiout->field_int ("wpnum", b->base.number);
+	  uiout->field_int ("wpnum", b->number);
 	  uiout->text (" deleted because the program has left the block in\n"
 		       "which its expression is valid.\n");
 	}
 
       /* Make sure the watchpoint's commands aren't executed.  */
-      decref_counted_command_line (&b->base.commands);
+      decref_counted_command_line (&b->commands);
       watchpoint_del_at_next_stop (b);
 
       return WP_DELETED;
@@ -5290,7 +5288,7 @@  bpstat_check_watchpoint (bpstat bs)
     {
       int must_check_value = 0;
       
-      if (b->base.type == bp_watchpoint)
+      if (b->type == bp_watchpoint)
 	/* For a software watchpoint, we must always check the
 	   watched value.  */
 	must_check_value = 1;
@@ -5300,7 +5298,7 @@  bpstat_check_watchpoint (bpstat bs)
 	   this watchpoint.  */
 	must_check_value = 1;
       else if (b->watchpoint_triggered == watch_triggered_unknown
-	       && b->base.type == bp_hardware_watchpoint)
+	       && b->type == bp_hardware_watchpoint)
 	/* We were stopped by a hardware watchpoint, but the target could
 	   not report the data address.  We must check the watchpoint's
 	   value.  Access and read watchpoints are out of luck; without
@@ -5311,7 +5309,7 @@  bpstat_check_watchpoint (bpstat bs)
 	{
 	  char *message
 	    = xstrprintf ("Error evaluating expression for watchpoint %d\n",
-			  b->base.number);
+			  b->number);
 	  struct cleanup *cleanups = make_cleanup (xfree, message);
 	  int e = catch_errors (watchpoint_check, bs, message,
 				RETURN_MASK_ALL);
@@ -5328,7 +5326,7 @@  bpstat_check_watchpoint (bpstat bs)
 	      bs->stop = 0;
 	      break;
 	    case WP_VALUE_CHANGED:
-	      if (b->base.type == bp_read_watchpoint)
+	      if (b->type == bp_read_watchpoint)
 		{
 		  /* There are two cases to consider here:
 
@@ -5399,8 +5397,8 @@  bpstat_check_watchpoint (bpstat bs)
 		}
 	      break;
 	    case WP_VALUE_NOT_CHANGED:
-	      if (b->base.type == bp_hardware_watchpoint
-		  || b->base.type == bp_watchpoint)
+	      if (b->type == bp_hardware_watchpoint
+		  || b->type == bp_watchpoint)
 		{
 		  /* Don't stop: write watchpoints shouldn't fire if
 		     the value hasn't changed.  */
@@ -5417,7 +5415,7 @@  bpstat_check_watchpoint (bpstat bs)
 		SWITCH_THRU_ALL_UIS ()
 	          {
 		    printf_filtered (_("Watchpoint %d deleted.\n"),
-				     b->base.number);
+				     b->number);
 		  }
 		watchpoint_del_at_next_stop (b);
 		/* We've already printed what needs to be printed.  */
@@ -8016,16 +8014,11 @@  disable_breakpoints_in_freed_objfile (struct objfile *objfile)
 /* FORK & VFORK catchpoints.  */
 
 /* An instance of this type is used to represent a fork or vfork
-   catchpoint.  It includes a "struct breakpoint" as a kind of base
-   class; users downcast to "struct breakpoint *" when needed.  A
-   breakpoint is really of this type iff its ops pointer points to
-   CATCH_FORK_BREAKPOINT_OPS.  */
+   catchpoint.  A breakpoint is really of this type iff its ops pointer points
+   to CATCH_FORK_BREAKPOINT_OPS.  */
 
-struct fork_catchpoint
+struct fork_catchpoint : public breakpoint
 {
-  /* The base class.  */
-  struct breakpoint base;
-
   /* Process id of a child process whose forking triggered this
      catchpoint.  This field is only valid immediately after this
      catchpoint has triggered.  */
@@ -8264,16 +8257,11 @@  print_recreate_catch_vfork (struct breakpoint *b, struct ui_file *fp)
 static struct breakpoint_ops catch_vfork_breakpoint_ops;
 
 /* An instance of this type is used to represent an solib catchpoint.
-   It includes a "struct breakpoint" as a kind of base class; users
-   downcast to "struct breakpoint *" when needed.  A breakpoint is
-   really of this type iff its ops pointer points to
+   A breakpoint is really of this type iff its ops pointer points to
    CATCH_SOLIB_BREAKPOINT_OPS.  */
 
-struct solib_catchpoint
+struct solib_catchpoint : public breakpoint
 {
-  /* The base class.  */
-  struct breakpoint base;
-
   /* True for "catch load", false for "catch unload".  */
   unsigned char is_load;
 
@@ -8329,7 +8317,7 @@  breakpoint_hit_catch_solib (const struct bp_location *bl,
     if (other->type != bp_shlib_event)
       continue;
 
-    if (self->base.pspace != NULL && other->pspace != self->base.pspace)
+    if (self->pspace != NULL && other->pspace != self->pspace)
       continue;
 
     for (other_bl = other->loc; other_bl != NULL; other_bl = other_bl->next)
@@ -8503,13 +8491,13 @@  add_solib_catchpoint (const char *arg, int is_load, int is_temp, int enabled)
     }
 
   c->is_load = is_load;
-  init_catchpoint (&c->base, gdbarch, is_temp, NULL,
+  init_catchpoint (c, gdbarch, is_temp, NULL,
 		   &catch_solib_breakpoint_ops);
 
-  c->base.enable_state = enabled ? bp_enabled : bp_disabled;
+  c->enable_state = enabled ? bp_enabled : bp_disabled;
 
   discard_cleanups (cleanup);
-  install_breakpoint (0, &c->base, 1);
+  install_breakpoint (0, c, 1);
 }
 
 /* A helper function that does all the work for "catch load" and
@@ -8585,26 +8573,21 @@  create_fork_vfork_event_catchpoint (struct gdbarch *gdbarch,
 {
   struct fork_catchpoint *c = new fork_catchpoint ();
 
-  init_catchpoint (&c->base, gdbarch, tempflag, cond_string, ops);
+  init_catchpoint (c, gdbarch, tempflag, cond_string, ops);
 
   c->forked_inferior_pid = null_ptid;
 
-  install_breakpoint (0, &c->base, 1);
+  install_breakpoint (0, c, 1);
 }
 
 /* Exec catchpoints.  */
 
 /* An instance of this type is used to represent an exec catchpoint.
-   It includes a "struct breakpoint" as a kind of base class; users
-   downcast to "struct breakpoint *" when needed.  A breakpoint is
-   really of this type iff its ops pointer points to
+   A breakpoint is really of this type iff its ops pointer points to
    CATCH_EXEC_BREAKPOINT_OPS.  */
 
-struct exec_catchpoint
+struct exec_catchpoint : public breakpoint
 {
-  /* The base class.  */
-  struct breakpoint base;
-
   /* Filename of a program whose exec triggered this catchpoint.
      This field is only valid immediately after this catchpoint has
      triggered.  */
@@ -9338,12 +9321,7 @@  create_breakpoint_sal (struct gdbarch *gdbarch,
   struct cleanup *old_chain;
 
   if (is_tracepoint_type (type))
-    {
-      struct tracepoint *t;
-
-      t = new tracepoint ();
-      b = &t->base;
-    }
+    b = new tracepoint ();
   else
     b = new breakpoint ();
 
@@ -9831,12 +9809,7 @@  create_breakpoint (struct gdbarch *gdbarch,
       struct breakpoint *b;
 
       if (is_tracepoint_type (type_wanted))
-	{
-	  struct tracepoint *t;
-
-	  t = new tracepoint ();
-	  b = &t->base;
-	}
+	b = new tracepoint ();
       else
 	b = new breakpoint ();
 
@@ -11027,7 +11000,7 @@  static void
 watch_command_1 (const char *arg, int accessflag, int from_tty,
 		 int just_location, int internal)
 {
-  struct breakpoint *b, *scope_breakpoint = NULL;
+  struct breakpoint *scope_breakpoint = NULL;
   const struct block *exp_valid_block = NULL, *cond_exp_valid_block = NULL;
   struct value *val, *mark, *result;
   int saved_bitpos = 0, saved_bitsize = 0;
@@ -11266,16 +11239,16 @@  watch_command_1 (const char *arg, int accessflag, int from_tty,
     bp_type = bp_hardware_watchpoint;
 
   w = new watchpoint ();
-  b = &w->base;
+
   if (use_mask)
-    init_raw_breakpoint_without_location (b, NULL, bp_type,
+    init_raw_breakpoint_without_location (w, NULL, bp_type,
 					  &masked_watchpoint_breakpoint_ops);
   else
-    init_raw_breakpoint_without_location (b, NULL, bp_type,
+    init_raw_breakpoint_without_location (w, NULL, bp_type,
 					  &watchpoint_breakpoint_ops);
-  b->thread = thread;
-  b->disposition = disp_donttouch;
-  b->pspace = current_program_space;
+  w->thread = thread;
+  w->disposition = disp_donttouch;
+  w->pspace = current_program_space;
   w->exp = std::move (exp);
   w->exp_valid_block = exp_valid_block;
   w->cond_exp_valid_block = cond_exp_valid_block;
@@ -11295,7 +11268,7 @@  watch_command_1 (const char *arg, int accessflag, int from_tty,
 				  (int) (exp_end - exp_start), exp_start);
 
       /* The above expression is in C.  */
-      b->language = language_c;
+      w->language = language_c;
     }
   else
     w->exp_string = savestring (exp_start, exp_end - exp_start);
@@ -11313,9 +11286,9 @@  watch_command_1 (const char *arg, int accessflag, int from_tty,
     }
 
   if (cond_start)
-    b->cond_string = savestring (cond_start, cond_end - cond_start);
+    w->cond_string = savestring (cond_start, cond_end - cond_start);
   else
-    b->cond_string = 0;
+    w->cond_string = 0;
 
   if (frame_id_p (watchpoint_frame))
     {
@@ -11332,8 +11305,8 @@  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.  */
-      b->related_breakpoint = scope_breakpoint;
-      scope_breakpoint->related_breakpoint = b;
+      w->related_breakpoint = scope_breakpoint;
+      scope_breakpoint->related_breakpoint = w;
     }
 
   if (!just_location)
@@ -11347,12 +11320,12 @@  watch_command_1 (const char *arg, int accessflag, int from_tty,
     }
   CATCH (e, RETURN_MASK_ALL)
     {
-      delete_breakpoint (b);
+      delete_breakpoint (w);
       throw_exception (e);
     }
   END_CATCH
 
-  install_breakpoint (internal, b, 1);
+  install_breakpoint (internal, w, 1);
   do_cleanups (back_to);
 }
 
@@ -11822,11 +11795,11 @@  catch_exec_command_1 (char *arg_entry, int from_tty,
     error (_("Junk at end of arguments."));
 
   c = new exec_catchpoint ();
-  init_catchpoint (&c->base, gdbarch, tempflag, cond_string,
+  init_catchpoint (c, gdbarch, tempflag, cond_string,
 		   &catch_exec_breakpoint_ops);
   c->exec_pathname = NULL;
 
-  install_breakpoint (0, &c->base, 1);
+  install_breakpoint (0, c, 1);
 }
 
 void
@@ -13665,7 +13638,7 @@  strace_marker_create_breakpoints_sal (struct gdbarch *gdbarch,
       location = copy_event_location (canonical->location.get ());
 
       tp = new tracepoint ();
-      init_breakpoint_sal (&tp->base, gdbarch, expanded,
+      init_breakpoint_sal (tp, gdbarch, expanded,
 			   std::move (location), NULL,
 			   cond_string, extra_string,
 			   type_wanted, disposition,
@@ -13680,7 +13653,7 @@  strace_marker_create_breakpoints_sal (struct gdbarch *gdbarch,
 	 corresponds to this one  */
       tp->static_trace_marker_id_idx = i;
 
-      install_breakpoint (internal, &tp->base, 0);
+      install_breakpoint (internal, tp, 0);
     }
 }
 
@@ -15296,7 +15269,7 @@  create_tracepoint_from_upload (struct uploaded_tp *utp)
   if (utp->pass > 0)
     {
       xsnprintf (small_buf, sizeof (small_buf), "%d %d", utp->pass,
-		 tp->base.number);
+		 tp->number);
 
       trace_pass_command (small_buf, 0);
     }
@@ -15314,7 +15287,7 @@  create_tracepoint_from_upload (struct uploaded_tp *utp)
 
       cmd_list = read_command_lines_1 (read_uploaded_action, 1, NULL, NULL);
 
-      breakpoint_set_commands (&tp->base, std::move (cmd_list));
+      breakpoint_set_commands (tp, std::move (cmd_list));
     }
   else if (!VEC_empty (char_ptr, utp->actions)
 	   || !VEC_empty (char_ptr, utp->step_actions))
@@ -15323,7 +15296,7 @@  create_tracepoint_from_upload (struct uploaded_tp *utp)
 	     utp->number);
 
   /* Copy any status information that might be available.  */
-  tp->base.hit_count = utp->hit_count;
+  tp->hit_count = utp->hit_count;
   tp->traceframe_usage = utp->traceframe_usage;
 
   return tp;
@@ -15409,10 +15382,10 @@  static void
 trace_pass_set_count (struct tracepoint *tp, int count, int from_tty)
 {
   tp->pass_count = count;
-  observer_notify_breakpoint_modified (&tp->base);
+  observer_notify_breakpoint_modified (tp);
   if (from_tty)
     printf_filtered (_("Setting tracepoint %d's passcount to %d\n"),
-		     tp->base.number, count);
+		     tp->number, count);
 }
 
 /* Set passcount for tracepoint.
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index 26b0aa53a8..bc8cfc416a 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -680,6 +680,11 @@  extern int target_exact_watchpoints;
 
 struct breakpoint
 {
+  /* We need a virtual destructor, because we delete all kinds of breakpoints
+     through a pointer to breakpoint.  */
+
+  virtual ~breakpoint () = default;
+
   /* Methods associated with this breakpoint.  */
   const breakpoint_ops *ops = NULL;
 
@@ -783,15 +788,10 @@  struct breakpoint
   gdbscm_breakpoint_object *scm_bp_object = NULL;
 };
 
-/* An instance of this type is used to represent a watchpoint.  It
-   includes a "struct breakpoint" as a kind of base class; users
-   downcast to "struct breakpoint *" when needed.  */
+/* An instance of this type is used to represent a watchpoint.  */
 
-struct watchpoint
+struct watchpoint : public breakpoint
 {
-  /* The base class.  */
-  struct breakpoint base;
-
   /* String form of exp to use for displaying to the user (malloc'd),
      or NULL if none.  */
   char *exp_string;
@@ -867,14 +867,10 @@  extern int is_breakpoint (const struct breakpoint *bpt);
 extern int is_watchpoint (const struct breakpoint *bpt);
 
 /* An instance of this type is used to represent all kinds of
-   tracepoints.  It includes a "struct breakpoint" as a kind of base
-   class; users downcast to "struct breakpoint *" when needed.  */
+   tracepoints.  */
 
-struct tracepoint
+struct tracepoint : public breakpoint
 {
-  /* The base class.  */
-  struct breakpoint base;
-
   /* Number of times this tracepoint should single-step and collect
      additional data.  */
   long step_count;
diff --git a/gdb/ctf.c b/gdb/ctf.c
index 8716cadf97..82e7013f97 100644
--- a/gdb/ctf.c
+++ b/gdb/ctf.c
@@ -1522,8 +1522,8 @@  ctf_get_traceframe_address (void)
       struct tracepoint *tp
 	= get_tracepoint_by_number_on_target (tpnum);
 
-      if (tp && tp->base.loc)
-	addr = tp->base.loc->address;
+      if (tp && tp->loc)
+	addr = tp->loc->address;
     }
 
   /* Restore the position.  */
diff --git a/gdb/mi/mi-cmd-break.c b/gdb/mi/mi-cmd-break.c
index cfe2d34f23..9412af8e14 100644
--- a/gdb/mi/mi-cmd-break.c
+++ b/gdb/mi/mi-cmd-break.c
@@ -398,7 +398,7 @@  mi_cmd_break_passcount (const char *command, char **argv, int argc)
   if (t)
     {
       t->pass_count = p;
-      observer_notify_breakpoint_modified (&t->base);
+      observer_notify_breakpoint_modified (t);
     }
   else
     {
diff --git a/gdb/remote.c b/gdb/remote.c
index 2cd98502f7..de3951631b 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -12650,9 +12650,9 @@  remote_get_tracepoint_status (struct target_ops *self, struct breakpoint *bp,
 
   if (tp)
     {
-      tp->base.hit_count = 0;
+      tp->hit_count = 0;
       tp->traceframe_usage = 0;
-      for (loc = tp->base.loc; loc; loc = loc->next)
+      for (loc = tp->loc; loc; loc = loc->next)
 	{
 	  /* If the tracepoint was never downloaded, don't go asking for
 	     any status.  */
diff --git a/gdb/tracefile-tfile.c b/gdb/tracefile-tfile.c
index 255bbc9ef9..d479cefffa 100644
--- a/gdb/tracefile-tfile.c
+++ b/gdb/tracefile-tfile.c
@@ -650,8 +650,8 @@  tfile_get_traceframe_address (off_t tframe_offset)
 
   tp = get_tracepoint_by_number_on_target (tpnum);
   /* FIXME this is a poor heuristic if multiple locations.  */
-  if (tp && tp->base.loc)
-    addr = tp->base.loc->address;
+  if (tp && tp->loc)
+    addr = tp->loc->address;
 
   /* Restore our seek position.  */
   cur_offset = saved_offset;
diff --git a/gdb/tracefile.c b/gdb/tracefile.c
index cc90945bb2..e208fc6d2b 100644
--- a/gdb/tracefile.c
+++ b/gdb/tracefile.c
@@ -398,15 +398,15 @@  tracefile_fetch_registers (struct regcache *regcache, int regno)
 
   /* We can often usefully guess that the PC is going to be the same
      as the address of the tracepoint.  */
-  if (tp == NULL || tp->base.loc == NULL)
+  if (tp == NULL || tp->loc == NULL)
     return;
 
   /* But don't try to guess if tracepoint is multi-location...  */
-  if (tp->base.loc->next)
+  if (tp->loc->next)
     {
       warning (_("Tracepoint %d has multiple "
 		 "locations, cannot infer $pc"),
-	       tp->base.number);
+	       tp->number);
       return;
     }
   /* ... or does while-stepping.  */
@@ -414,13 +414,13 @@  tracefile_fetch_registers (struct regcache *regcache, int regno)
     {
       warning (_("Tracepoint %d does while-stepping, "
 		 "cannot infer $pc"),
-	       tp->base.number);
+	       tp->number);
       return;
     }
 
   /* Guess what we can from the tracepoint location.  */
   gdbarch_guess_tracepoint_registers (gdbarch, regcache,
-				      tp->base.loc->address);
+				      tp->loc->address);
 }
 
 /* This is the implementation of target_ops method to_has_all_memory.  */
diff --git a/gdb/tracepoint.c b/gdb/tracepoint.c
index 808afdeb68..bd70fb6afc 100644
--- a/gdb/tracepoint.c
+++ b/gdb/tracepoint.c
@@ -648,11 +648,11 @@  actions_command (char *args, int from_tty)
     {
       std::string tmpbuf =
 	string_printf ("Enter actions for tracepoint %d, one per line.",
-		       t->base.number);
+		       t->number);
 
       command_line_up l = read_command_lines (&tmpbuf[0], from_tty, 1,
 					      check_tracepoint_command, t);
-      breakpoint_set_commands (&t->base, std::move (l));
+      breakpoint_set_commands (t, std::move (l));
     }
   /* else just return */
 }
@@ -738,7 +738,7 @@  validate_actionline (const char *line, struct breakpoint *b)
 	      /* else fall thru, treat p as an expression and parse it!  */
 	    }
 	  tmp_p = p;
-	  for (loc = t->base.loc; loc; loc = loc->next)
+	  for (loc = t->loc; loc; loc = loc->next)
 	    {
 	      p = tmp_p;
 	      expression_up exp = parse_exp_1 (&p, loc->address,
@@ -788,7 +788,7 @@  validate_actionline (const char *line, struct breakpoint *b)
 	  p = skip_spaces_const (p);
 
 	  tmp_p = p;
-	  for (loc = t->base.loc; loc; loc = loc->next)
+	  for (loc = t->loc; loc; loc = loc->next)
 	    {
 	      p = tmp_p;
 
@@ -2246,7 +2246,7 @@  tfind_1 (enum trace_find_type type, int num,
   reinit_frame_cache ();
   target_dcache_invalidate ();
 
-  set_tracepoint_num (tp ? tp->base.number : target_tracept);
+  set_tracepoint_num (tp ? tp->number : target_tracept);
 
   if (target_frameno != get_traceframe_number ())
     observer_notify_traceframe_changed (target_frameno, tracepoint_number);
@@ -2870,7 +2870,7 @@  get_traceframe_location (int *stepping_frame_p)
      locations, assume it is a direct hit rather than a while-stepping
      frame.  (FIXME this is not reliable, should record each frame's
      type.)  */
-  for (tloc = t->base.loc; tloc; tloc = tloc->next)
+  for (tloc = t->loc; tloc; tloc = tloc->next)
     if (tloc->address == regcache_read_pc (regcache))
       {
 	*stepping_frame_p = 0;
@@ -2880,7 +2880,7 @@  get_traceframe_location (int *stepping_frame_p)
   /* If this is a stepping frame, we don't know which location
      triggered.  The first is as good (or bad) a guess as any...  */
   *stepping_frame_p = 1;
-  return t->base.loc;
+  return t->loc;
 }
 
 /* Return all the actions, including default collect, of a tracepoint
@@ -3233,7 +3233,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->base.cond_string, utp->cond_string)
+	  && cond_string_is_same (t->cond_string, utp->cond_string)
 	  /* FIXME also test actions.  */
 	  )
 	{
@@ -3302,7 +3302,7 @@  merge_uploaded_tracepoints (struct uploaded_tp **uploaded_tps)
 	  if (t)
 	    printf_filtered (_("Created tracepoint %d for "
 			       "target's tracepoint %d at %s.\n"),
-			     t->base.number, utp->number,
+			     t->number, utp->number,
 			     paddress (get_current_arch (), utp->addr));
 	  else
 	    printf_filtered (_("Failed to create tracepoint for target's "
@@ -3605,7 +3605,7 @@  parse_tracepoint_status (char *p, struct breakpoint *bp,
 
   p = unpack_varlen_hex (p, &uval);
   if (tp)
-    tp->base.hit_count += uval;
+    tp->hit_count += uval;
   else
     utp->hit_count += uval;
   p = unpack_varlen_hex (p + 1, &uval);