[lldb-dev] RFC: Break/Watchpoint refactor

[Daniel Austin Noland via lldb-dev]

I have been giving study to the private and public classes for 
break/watchpoints and I have some ideas for how to improve them.  I am 
looking for comments from the community on this now to avoid wasted work 
and invite suggestion for improvements and any objections you may have.

Some problems with things as they stand:

1. The Watchpoint and SBWatchpoint classes share very little 
implementation with the Breakpoint and SBBreakpoint classes.  Both 
Breakpoint and Watchpoint inherit from Stoppoint, but that class 
provides very little functionality and not much of an interface. This is 
both a waste and a potential hazard (in the sense that we run the risk 
of allowing breakpoints and watchpoints to evolve in parallel thus 
complicating the API and making things more difficult to merge later).
2. The SBWatchpoint class lacks feature parity with SBBreakpoint (e.g. 
no callback implementation in SBWatchpoint).
3. The Breakpoint class has grown an ecosystem of support classes which 
are unnecessarily specific to it.  For example, I don't see a huge 
reason to maintain a totally distinct implementation for BreakpointList 
and WatchpointList.
4. All of these classes are "old school" (not necessarily bad, but 
potentially a problem).  For example:
   a. lots of const char* running around.
   b. DISALLOW_COPY_AND_ASSIGN(BreakpointEventData) to make ctors and 
such private rather than using ctor() = delete (which provides better 
compiler errors)
   c. use of Error& args in function signatures as opposed to 
Expected<ReturnType>.
   d. callback implementation uses function pointers (an ever confusing 
topic, especially for new programmers) where I think we could use 
templated methods (or just a parent Callback class) to significant effect.
5. Confusing or simply wrong documentation (e.g. 
Breakpoint::ResolveBreakpointInModules has arguments marked as 
@param[in] when the description of those arguments clearly indicates 
that they will be the output of the function).
6. We simply don't have a Finishpoint class (triggers callback at the 
conclusion of the function or scope)

My plan:

I would like to refactor this in a few phases.

Phase 1: Low hanging fruit
--------------------------

* Kick as much functionality as is reasonable from Breakpoint up to 
Stoppoint.  At a minimum I would expand the Stoppoint API to include 
features which should logically be available to Breakpoint, Watchpoint, 
a hypothetical Finishpoint, and any future *point class.  I would also 
try to refactor the Breakpoint support classes (e.g. BreakpointID) to 
derive from a new class (i.e. StoppointID) and kick functionality up the 
chain there as well.  This would include methods like GetDescription, 
SetOptions, GetOptions, AddName, and so on.

* Review and clean the source docs

* Expand test coverage where that seems easy (perhaps there are tests 
hiding somewhere I didn't see?)

I would try to break private APIs minimally if at all.  SB api will not 
be broken.

This should go a long ways toward resolving problems 1, 2, 3, and 5.

Phase 2: Restructure / Modernize the Private API / Implementation
-----------------------------------------------------------------

* Change Error& out parameters to Expected<ReturnType>.
* Get rid of all the const char* vars / args in favor of a better string 
type (which?)
* Prefer explicitly deleted copy ctor / assignments over multiline macro 
DISALLOW_COPY_AND_ASSIGN
* Try to reduce the (perhaps excessive) use of friendship between the 
support classes.  For instance, is it necessary to give 
BreakpointLocation access to private data members from 
BreakpointLocationList, Process, BreakpointSite, and 
StopInfoBreakpoint?  Wouldn't it be better to expand the functionality 
of those other classes?

A more significant change could be a rewrite of the callback functionality.

There are some problems with the way callbacks are implemented in terms 
of maintainability and extensibility.  I think that using templates and 
simple inheritance we could shift to using callback objects instead of 
function pointers.  I have a crude code sketch of this plan in the works 
now, and I will submit that if there is interest in this idea.  
Essentially, the idea is to let the user define their own Breakpoint or 
Watchpoint (or whatever) child class which overrides a pure virtual run 
method from a parent StoppointCallback templated class.  The template 
parameter of StoppointCallback would allow us to define a 
std::unique_ptr<UserData> member where the user can hang any data they 
want for the callback.  That saves us from void pointers (which I find 
very awkward).

template <class UserData>
class StoppointCallback {
private:
    std::unique_ptr<UserData> m_data;
// ...
};

I also think that such a decision requires significant thought before we 
pull that trigger.  It does constitute a significant addition to the SB 
api, tho I don't think it would break the SB api per se since we can 
always use overloading and template specialization to keep the old 
functionality.  On the other hand, ABI compatibility may be a problem.  
I don't know that much about SWIG or the needs of LLDB from a ABI 
stability perspective, so please speak up if I'm suggesting something 
crazy.

That said, it is somewhat difficult to keep the API consistent between 
Breakpoint and Watchpoint using function pointers; the signature 
associated with each will differ (e.g. Watchpoint should provide old and 
new values of the subject variable, while that does not apply to 
Breakpoint at all).  I welcome any suggestions which allow us to avoid 
logic duplication.  My goto just happens to be templates here.

Doing even a subset of phase 2 would go a long ways toward fixing 
problem 4.

Phase 3: Expanded Feature Set
-----------------------------

I would love to see these features:

* Watch variables from a function in every scope of that function OR in 
only select invocations.  Perhaps this already exists, but I can only 
get it to watch from a particular scope.
* Better (read functional) remote GDB server break / watch.  As has been 
disused in other threads, this is not really easy but is very 
desirable.  For instance, I would very much like to see lldb play nice 
with valgrind's vgdb server as it offers some amazing break / watch 
functionality.  As of today I need to do a great many hacks to make that 
work properly.
* Finishpoints.  This just seems obvious. (problem 6)
* GDB style tracepoints.  This may be very difficult but it seems very 
desirable.

Your thoughts are appreciated,
\author{Daniel Noland}