<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<p>On 09/27/2016 03:23 PM, Zachary Turner wrote:<br>
</p>
<blockquote
cite="mid:CAAErz9gCOkv=_5ga3t81A_8Sd5Z4CXrpgDdHN0XKsEV4nogJgw@mail.gmail.com"
type="cite">
<meta http-equiv="Context-Type" content="text/html; charset=UTF-8">
<div dir="ltr"><br>
<div><br>
</div>
<br>
<div class="gmail_quote">
<div dir="ltr">On Tue, Sep 27, 2016 at 1:09 PM Daniel Austin
Noland via lldb-dev <<a moz-do-not-send="true"
href="mailto:lldb-dev@lists.llvm.org">lldb-dev@lists.llvm.org</a>>
wrote:<br>
</div>
<blockquote class="gmail_quote">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>We should use llvm::StringRef here.</div>
</div>
</div>
</blockquote>
Sounds good to me. Just wanted to make sure I'm on the same page as
everyone else.<br>
<blockquote
cite="mid:CAAErz9gCOkv=_5ga3t81A_8Sd5Z4CXrpgDdHN0XKsEV4nogJgw@mail.gmail.com"
type="cite">
<div dir="ltr">
<div class="gmail_quote">
<div> </div>
<blockquote class="gmail_quote">
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>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>
<blockquote class="gmail_quote">
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>We should consider using llvm::function_ref<> here.</div>
<div><br>
</div>
<div>More details on each of these below.</div>
<div> </div>
<blockquote class="gmail_quote"><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>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>
</div>
</blockquote>
<blockquote
cite="mid:CAAErz9gCOkv=_5ga3t81A_8Sd5Z4CXrpgDdHN0XKsEV4nogJgw@mail.gmail.com"
type="cite">
<div dir="ltr">
<div class="gmail_quote">
<div> </div>
<blockquote class="gmail_quote">
* 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>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> </div>
<blockquote class="gmail_quote">
* 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>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><br>
</div>
<div>I think the canonical pattern for what you want here is
"double dispatch". You've got something like this:</div>
<div><br>
</div>
<div>struct CallbackArgs {</div>
<div> virtual void dispatch(StoppointBase &point) = 0;</div>
<div>};</div>
<div>struct WatchpointArgs : public CallbackArgs {</div>
<div>private:</div>
<div> int Old;</div>
<div> int New;</div>
<div>public:</div>
<div> void dispatch(StoppointBase &point) override {</div>
<div>
static_cast<Watchpoint&>(point).Callback(Old,
New);</div>
<div> };</div>
<div>};</div>
<div><br>
</div>
<div>struct BreakpointArgs : public CallbackArgs {</div>
<div>private:</div>
<div> int id;</div>
<div>public:</div>
<div> void dispatch(StoppointBase &point) override {</div>
<div>
static_cast<Breakpoint&>(point).Callback(id);</div>
<div> }<br>
</div>
<div>};</div>
<div><br>
</div>
<div>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><br>
</div>
<div>if (auto wp =
llvm::dyn_cast<Watchpoint*>(stop_point)) {</div>
<div> wp->Callback( ... );</div>
<div>} else if (auto bp =
llvm::dyn_cast<Breakpoint*>(stop_point)) {</div>
<div> bp->Callback( ... );</div>
<div>}</div>
<div><br>
</div>
<div>And these callback functions could have completely
different signatures.</div>
<div><br>
</div>
<div> </div>
<blockquote class="gmail_quote">
<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>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>
</div>
</blockquote>
I will look into the llvm::function_ref<> as that may be the
real answer here. <br>
I also tend to agree that inheritance is mostly undesirable. My
argument for it in this case is that it would be nice to enforce a
consistent interface, but that may be better done manually.<br>
I always try to avoid casting, but that may be unrealistic here.<br>
<blockquote
cite="mid:CAAErz9gCOkv=_5ga3t81A_8Sd5Z4CXrpgDdHN0XKsEV4nogJgw@mail.gmail.com"
type="cite">
<div dir="ltr">
<div class="gmail_quote">
<div> </div>
<blockquote class="gmail_quote"><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><br>
</div>
<div> 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><br>
</div>
<div>break set -n foo --exec "print p" --exec "break set -n
bar" --continue</div>
<div><br>
</div>
<div>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>
</blockquote>
That is correct for an infinite speed computer. The issue is that
GDB Tracepoints run their instructions without talking to the
debugger (until the next time the program stops). This allows you
to watch variables in applications that are timing sensitive (e.g.,
you introduce a bug in a VOIP application just from the lag
associated with watching the data).<br>
</body>
</html>