[lldb-dev] RFC: Break/Watchpoint refactor

[Daniel Austin Noland via lldb-dev]

On 09/27/2016 03:30 PM, Jim Ingham wrote:
>> On Sep 27, 2016, at 1:09 PM, Daniel Austin Noland via lldb-dev <lldb-dev at lists.llvm.org> wrote:
>>
>> 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).
> The main problem here is that Watchpoints & Breakpoints should share the Options class, and most of the StopInfo DoOnRemoval.  I don’t think you’ll need to write a lot of new code to do this, it’s mostly ripping out the WatchpointOptions, using BreakpointOptions instead adapting as necessary.  Ditto for the StopInfo{Watchpoint,Breakpoint}.
So you would avoid inheritance and just make Watchpoint and Breakpoint 
take the same StoppointOptions class?
>
>> 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>.
> This should not be done piecemeal.  If we decide to change how we do error reporting in lldb, that should be done consistently throughout.
Fair enough.
>
>>   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.
> Folks on the outside of the SB API’s need to be able to pass callbacks in.  We don’t currently have any templates you need to instantiate or classes you need to override to go from outside the SB API to inside it.  So whatever happens on the lldb_private side, for the SB API’s we’ll probably have to stick with void * & function pointers.
That is fair.  I may be able to avoid the entire problem with 
llvm::function_ref<>.  Will look into it.
>> 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.
> Again, most of the overlap isn’t actually in the breakpoint and watchpoint classes, but in the Options classes.  After all, the setting of watchpoints and breakpoints are very different, but that they have conditions and callbacks are very similar.  Keeping the setting part separate and sharing the reaction to hitting the stop points seems to more closely model the objects better.
>
>> * Review and clean the source docs
>>
>> * Expand test coverage where that seems easy (perhaps there are tests hiding somewhere I didn't see?)
> test/testcases/functionalities/breakpoint
>
> There are lots of tests there, but feel free to add tests as you go.
>
>> 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>.
> Again, this is a larger policy change.  We shouldn’t make different sections of lldb handle errors differently.
>
>> * Get rid of all the const char* vars / args in favor of a better string type (which?)
> StringRef’s seem to be the vogue these days.
>
>> * 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.
> We’ve been pretty strict about not requiring subclassing or specialization of SB API’s classes to use them.  We’re serious about maintaining binary compatibility across lldb versions.
>
>> 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.
> I think you would call back into the watchpoint to get the old and new values.  I don’t think you would need more than the frame passed into the watchpoint callback, and the location.
The last time I looked I didn't see a good way to do that.  I may be 
wrong tho, so I will check again.
>
>> 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.
> Not sure what you mean by this.
Problems discussed here 
(http://lists.llvm.org/pipermail/lldb-dev/2016-September/011203.html). I 
have seen quite a few crashes when talking to vgdb but I have not been 
able to take them apart well enough to even file a bug report.
>
>> * Finishpoints.  This just seems obvious. (problem 6)
> This is an overlap of functionality with stepping, finish and step-until.  We’d have to sketch out some use cases to see which functionality seems more natural, and if both, how to keep them from conflicting.
That is a good point.
>> * GDB style tracepoints.  This may be very difficult but it seems very desirable.
> That is a pretty big problem, since we would have to implement both the server & client sides.
That is why I put it as the last step.  I think it would be great, but 
may be a more long term issue.
> Jim
>
>> Your thoughts are appreciated,
>> \author{Daniel Noland}
>>
>> _______________________________________________
>> lldb-dev mailing list
>> lldb-dev at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev