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

[Pedro Alves <palves@redhat.com>]

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