This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH] Make breakpoint subclasses inherit from breakpoint, add virtual destructor
- From: Pedro Alves <palves at redhat dot com>
- To: Simon Marchi <simon dot marchi at ericsson dot com>, gdb-patches at sourceware dot org
- Cc: Simon Marchi <simon dot marchi at polymtl dot ca>
- Date: Wed, 3 May 2017 11:17:06 +0100
- Subject: Re: [PATCH] Make breakpoint subclasses inherit from breakpoint, add virtual destructor
- Authentication-results: sourceware.org; auth=none
- Authentication-results: ext-mx10.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com
- Authentication-results: ext-mx10.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=palves at redhat dot com
- Dkim-filter: OpenDKIM Filter v2.11.0 mx1.redhat.com 2351A61D09
- Dmarc-filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 2351A61D09
- References: <20170502191811.32333-1-simon.marchi@ericsson.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