This is the mail archive of the mailing list for the GDB project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] Make breakpoint subclasses inherit from breakpoint, add virtual destructor

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 <>

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.

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

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.,

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.


	* ada-lang.c (struct ada_catchpoint): Inherit from struct
	<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 script). I'll put it back in the oven.



Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]