<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><br class=""><div><blockquote type="cite" class=""><div class="">On Sep 27, 2016, at 2:23 PM, Zachary Turner via lldb-dev <<a href="mailto:lldb-dev@lists.llvm.org" class="">lldb-dev@lists.llvm.org</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div dir="ltr" class=""><br class=""><div class=""><br class=""></div><br class=""><div class="gmail_quote"><div dir="ltr" class="">On Tue, Sep 27, 2016 at 1:09 PM Daniel Austin Noland via lldb-dev <<a href="mailto:lldb-dev@lists.llvm.org" class="">lldb-dev@lists.llvm.org</a>> wrote:<br class=""></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">4. All of these classes are "old school" (not necessarily bad, but<br class="gmail_msg">
potentially a problem). For example:<br class="gmail_msg">
a. lots of const char* running around.<br class="gmail_msg"></blockquote><div class="">We should use llvm::StringRef here.</div><div class=""> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
b. DISALLOW_COPY_AND_ASSIGN(BreakpointEventData) to make ctors and<br class="gmail_msg">
such private rather than using ctor() = delete (which provides better<br class="gmail_msg">
compiler errors)<br class="gmail_msg">
c. use of Error& args in function signatures as opposed to<br class="gmail_msg">
Expected<ReturnType>.<br class="gmail_msg"></blockquote><div class="">LLDB already has its own class called Error, and it makes it confusing if we're going to be using llvm::Error as well. In an earlier thread I proposed changing lldb::Error to LLDBError for exactly this reason. I would suggest doing this first.</div></div></div></div></blockquote><div><br class=""></div><div>I don’t think we want different parts of lldb handling errors differently. That would be even more confusing.</div><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class="gmail_quote"><div class=""> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
d. callback implementation uses function pointers (an ever confusing<br class="gmail_msg">
topic, especially for new programmers) where I think we could use<br class="gmail_msg">
templated methods (or just a parent Callback class) to significant effect.<br class="gmail_msg"></blockquote><div class="">We should consider using llvm::function_ref<> here.</div><div class=""><br class=""></div><div class="">More details on each of these below.</div><div class=""> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br class="gmail_msg">
Phase 2: Restructure / Modernize the Private API / Implementation<br class="gmail_msg">
-----------------------------------------------------------------<br class="gmail_msg">
<br class="gmail_msg">
* Change Error& out parameters to Expected<ReturnType>.<br class="gmail_msg"></blockquote><div class="">As mentioned, we should rename lldb::Error first so as to avoid naming conflicts with llvm. Granted, they are each in their own namespace, but still, it will lead to confusion, and prevents the use of "using namespace llvm;" as it currently stands.</div><div class=""> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
* Get rid of all the const char* vars / args in favor of a better string<br class="gmail_msg">
type (which?)<br class="gmail_msg"></blockquote><div class="">llvm::StringRef. Anyone returning a const char* is saying they don't own the backing memory. That's exactly what a StringRef is for, but it has many other benefits such as very efficient searching, substring, and parsing methods. </div><div class=""> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
* Prefer explicitly deleted copy ctor / assignments over multiline macro<br class="gmail_msg">
DISALLOW_COPY_AND_ASSIGN<br class="gmail_msg">
* Try to reduce the (perhaps excessive) use of friendship between the<br class="gmail_msg">
support classes. For instance, is it necessary to give<br class="gmail_msg">
BreakpointLocation access to private data members from<br class="gmail_msg">
BreakpointLocationList, Process, BreakpointSite, and<br class="gmail_msg">
StopInfoBreakpoint? Wouldn't it be better to expand the functionality<br class="gmail_msg">
of those other classes?<br class="gmail_msg">
<br class="gmail_msg">
A more significant change could be a rewrite of the callback functionality.<br class="gmail_msg">
<br class="gmail_msg">
There are some problems with the way callbacks are implemented in terms<br class="gmail_msg">
of maintainability and extensibility. I think that using templates and<br class="gmail_msg">
simple inheritance we could shift to using callback objects instead of<br class="gmail_msg">
function pointers. I have a crude code sketch of this plan in the works<br class="gmail_msg">
now, and I will submit that if there is interest in this idea.<br class="gmail_msg">
Essentially, the idea is to let the user define their own Breakpoint or<br class="gmail_msg">
Watchpoint (or whatever) child class which overrides a pure virtual run<br class="gmail_msg">
method from a parent StoppointCallback templated class. The template<br class="gmail_msg">
parameter of StoppointCallback would allow us to define a<br class="gmail_msg">
std::unique_ptr<UserData> member where the user can hang any data they<br class="gmail_msg">
want for the callback. That saves us from void pointers (which I find<br class="gmail_msg">
very awkward).<br class="gmail_msg"></blockquote><div class="">I'm not a huge fan of this. Inheriting from Breakpoint or Watchpoint sounds old school to me. Implementation inheritance in general should be avoided wherever possible IMO. Also, you've mentioned that StoppointCallback would only have a template argument, but it's possible that different types of callback take completely incompatible types and numbers of arguments. This is something that can't be described by a single function with a template parameter.</div><div class=""><br class=""></div><div class="">I think the canonical pattern for what you want here is "double dispatch". You've got something like this:</div><div class=""><br class=""></div><div class="">struct CallbackArgs {</div><div class=""> virtual void dispatch(StoppointBase &point) = 0;</div><div class="">};</div><div class="">struct WatchpointArgs : public CallbackArgs {</div><div class="">private:</div><div class=""> int Old;</div><div class=""> int New;</div><div class="">public:</div><div class=""> void dispatch(StoppointBase &point) override {</div><div class=""> static_cast<Watchpoint&>(point).Callback(Old, New);</div><div class=""> };</div><div class="">};</div><div class=""><br class=""></div><div class="">struct BreakpointArgs : public CallbackArgs {</div><div class="">private:</div><div class=""> int id;</div><div class="">public:</div><div class=""> void dispatch(StoppointBase &point) override {</div><div class=""> static_cast<Breakpoint&>(point).Callback(id);</div><div class=""> }<br class=""></div><div class="">};</div><div class=""><br class=""></div><div class="">It's a little bit awkward though, so you can perhaps simplify this by making use of llvm's casting infrastructure. Check out llvm/include/Casting.h. But basically you could move all this dispatch logic into a single function that looks like this:</div><div class=""><br class=""></div><div class="">if (auto wp = llvm::dyn_cast<Watchpoint*>(stop_point)) {</div><div class=""> wp->Callback( ... );</div><div class="">} else if (auto bp = llvm::dyn_cast<Breakpoint*>(stop_point)) {</div><div class=""> bp->Callback( ... );</div><div class="">}</div><div class=""><br class=""></div></div></div></div></blockquote><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class="gmail_quote"><div class="">And these callback functions could have completely different signatures.</div><div class=""><br class=""></div><div class=""> </div></div></div></div></blockquote><div><br class=""></div><div>I’m not sure this is solving a problem that we have. The callbacks get arguments which describe the environment in which the stop occurred (the execution context) and something identifying the stop point (ID & loc ID). The data specific to the breakpoint/watchpoint should come from the breakpoint/watchpoint. I don’t see any convincing reason these need to have this flexibility.</div><div><br class=""></div><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br class="gmail_msg">
template <class UserData><br class="gmail_msg">
class StoppointCallback {<br class="gmail_msg">
private:<br class="gmail_msg">
std::unique_ptr<UserData> m_data;<br class="gmail_msg">
// ...<br class="gmail_msg">
};<br class="gmail_msg"></blockquote><div class="">Yes, this kind of pattern is good. Then just pass StoppointUserData& into the callback and let the caller cast it to the appropriate derived type.</div><div class=""> </div></div></div></div></blockquote><div><br class=""></div><div>Again, on the lldb_private side is fine. But we still need to pass callbacks and data across the SB API’s in a non-fragile way.</div><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br class="gmail_msg">
* GDB style tracepoints. This may be very difficult but it seems very<br class="gmail_msg">
desirable.<br class="gmail_msg"></blockquote><div class=""><br class=""></div><div class=""> I don't see why this would be hard. but I also don't think it's sufficiently different from a breakpoint that it needs to be *that* special. A tracepoint is just a breakpoint that does some stuff and then automatically continues. We can already stop at breakpoints, run commands, and resume commands. Why can't we just do all of those in a single scripted operation? I can envision a command like:</div><div class=""><br class=""></div><div class="">break set -n foo --exec "print p" --exec "break set -n bar" --continue</div><div class=""><br class=""></div><div class="">which sets a breakpoint at foo, and when it gets hit prints the value of p and sets another breakpoint in bar, then resumes.</div></div></div></div></blockquote><div><br class=""></div><div>That’s not what the tracepoints Daniel is talking about do. A tracepoint is an instruction to the server to stop, run an experiment (expressed in a little byte-coded language) without returning to the debugger, and store the result away. Then when the target stops at a “real" breakpoint, the debugger downloads the results of all the tracepoint experiments and presents this past history to the user in one way or the other.</div><div><br class=""></div><div>It isn’t rocket science, but it does require server side support to be useful.</div><div><br class=""></div><div>Jim</div><br class=""><blockquote type="cite" class=""><div class="">
_______________________________________________<br class="">lldb-dev mailing list<br class=""><a href="mailto:lldb-dev@lists.llvm.org" class="">lldb-dev@lists.llvm.org</a><br class="">http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev<br class=""></div></blockquote></div><br class=""></body></html>