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@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@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
_______________________________________________
lldb-dev mailing list
lldb-dev@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
