> On Sep 27, 2016, at 1:09 PM, Daniel Austin Noland via lldb-dev
> <lldb-dev@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
_______________________________________________
lldb-dev mailing list
lldb-dev@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
