gdbsupport: remove undefined behaviour from (forward_)scope_exit
Our implementation of scope_exit and forward_scope_exit make use of
the Curiously Recurring Template Pattern (CRTP) to provide the
"release" functionality, this is done in the scope_exit_base class in
scope-exit.h.
The interesting (for this commit) parts of scope_exit_base look like
this:
template <typename CRTP>
class scope_exit_base
{
public:
scope_exit_base () = default;
~scope_exit_base ()
{
if (!m_released)
{
auto *self = static_cast<CRTP *> (this);
self->on_exit ();
}
}
... snip ...
};
By the time ~scope_exit_base is called the destructor for the derived
class (called CRTP here) has already been run, which means any data
members in the derived class will have also have had their destructors
run. As a result of this I believe that casting 'this' to the derived
type, and then calling the on_exit method is undefined behaviour, as
'this' can no longer be considered a valid instance of CRTP (the CRTP
part has been destructed).
In reality, the memory for the derived type is not reclaimed until
after ~scope_exit_base has finished, so as long as the data members of
the derived type are Plain Old Data (POD), then the current code
should be just fine; any data members of the derived class will remain
in place, and untouched, until ~scope_exit_base has completed.
But still, I don't think we should rely on undefined behaviour.
I actually ran into this when, in another series, I tried to reuse
scope_exit_base with a class where a data member was not POD, and in
this case GDB would crash because my new on_exit function was making
use of the non-POD data member after it had been destructed, and
resources released.
I propose that we move away from the CRTP, and instead flip the class
hierarchy. Instead of derived classes like scope_exit inheriting from
scope_exit_base, scope_exit_base should inherit from scope_exit.
What this means, is that when ~scope_exit_base is called, ~scope_exit
will not yet have been run, and the data members of scope_exit will
still be valid.
To allow the existing names to be used, the plan is that the existing
scope_exit and forward_scope_exit classes are renamed to
scope_exit_policy and forward_scope_exit_policy. These policy classes
will then be injected via a template argument as the base class for
scope_exit_base. Finally, we can:
template<typename EF>
using scope_exit = scope_exit_base<scope_exit_policy<EF>>;
which defines the scope_exit type. This type is a drop in replacement
with all of GDB's existing code, but avoids the undefined behaviour.
We can do something similar with forward_scope_exit.
There should be no user visible changes after this commit.
2 files changed
