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

[Simon Marchi <simon dot marchi at polymtl dot ca>]

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