[lldb-dev] RFC: Break/Watchpoint refactor

[Greg Clayton via lldb-dev]

> 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).

It would be great to add any needed features internally (lldb_private::*) and in the public API (lldb::*).

> 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.

Internally we can use virtual functions and have the list be a list of some shared base class, that is fine. I don't believe we export the lists across the public API so that shouldn't be a problem.

> 4. All of these classes are "old school" (not necessarily bad, but potentially a problem).  For example:
>  a. lots of const char* running around.

Feel free to use llvm::StringRef internally (lldb_private namespace), but not in the public API. Anything in the "lldb" namespace can't change as we maintain backward compatibility. The main gist in the public API:
- no inheritance
- fixed ivars that never change that are either one of: std::shared_ptr, std::unique_ptr, or raw pointer.
- no virtual functions
- can't remove functions, you can add, but not remove or change. If you need to change, just add an overloaded version with different args.


>  b. DISALLOW_COPY_AND_ASSIGN(BreakpointEventData) to make ctors and such private rather than using ctor() = delete (which provides better compiler errors)

Change the DISALLOW_COPY_AND_ASSIGN macro. Done.

>  c. use of Error& args in function signatures as opposed to Expected<ReturnType>.

Not sure I would tackle this. It would need to happen everywhere and would need to not lose any lldb_private::Error functionality.

>  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.

Internally fine, but the public API needs a simple function pointer. I don't see the need to require templates.
> 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).

Feel free to fix.

> 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.

fine
> 
> * Review and clean the source docs

fine

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

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

No worries, improve the internals as much as you want as long as the SB APIs do not change.

> 
> 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>.

I am fine trying this out on a few classes like Watchpoint internally to see how we like it. 

> * Get rid of all the const char* vars / args in favor of a better string type (which?)

StringRef is fine as long as the ownership is never assumed to transfer, so these are perfect for arguments, but not for storing a reference to a string that is passed in as an argument inside an instance variable of a class. So "const char *" in the public API, and llvm::StringRef internally is fine.

> * Prefer explicitly deleted copy ctor / assignments over multiline macro DISALLOW_COPY_AND_ASSIGN

Just change the macro. Do that quickly as a separate change list if you want as we will OK that right away.

> * 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?

Feel free to give us patches, we can review these on a case by case basis.
> 
> 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 would switch to use std::function internally so we can use lambdas. I don't see the need to templatize anything since you can do almost anything with function objects and lambdas, both which work in place of a std::function. No need to invent anything. We didn't have C++11 and C++14 back when this was first coded, so we didn't use it back then. Public APIs will need to use standard function callbacks. So "std::*" or "llvm::*" in the public API.

> 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.

No templates in the public API please. 
> 
> 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.

Not sure why they are better. Again, use std::function internally will buy us labmdas, function pointers, and function object support, and in the public API we must keep it simple and use function pointers.

> 
> 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.

You would run out of watchpoints so quickly these would probably not be very useful. Most things we use have 2 - 4 watchpoints at most. If you watched every instance it would quickly run out. What should happen when it runs out of resources? Just stop?

> * 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.

If something does support infinite watchpoints, we should be able to take advantage of that and your case above could just work.

> * Finishpoints.  This just seems obvious. (problem 6)

no problems with adding this once we have other support.

> * GDB style tracepoints.  This may be very difficult but it seems very desirable.

I don't know these, but I know Jim does.

Watchpoints are in need of some serious work, so I am happy to see someone willing to dive in. I think you have a pretty good idea of where we stand now, let me know what you think of my comments.

Greg Clayton