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: Simon Marchi <simon dot marchi at polymtl dot ca>
- To: Pedro Alves <palves at redhat dot com>
- Cc: Simon Marchi <simon dot marchi at ericsson dot com>, gdb-patches at sourceware dot org
- Date: Wed, 03 May 2017 10:36:04 -0400
- Subject: Re: [PATCH] Make breakpoint subclasses inherit from breakpoint, add virtual destructor
- Authentication-results: sourceware.org; auth=none
- References: <20170502191811.32333-1-simon.marchi@ericsson.com> <0f7aaf0f-6498-2b2e-7cc9-e7656fbc6079@redhat.com>
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