[lldb-dev] Problem with watchpoints

Fri Sep 9 19:33:48 PDT 2016

Yes, that was pretty much my assessment when I read through the code.

My existing patch (which I will post when I get home) takes a very
conservative approach and only modifies what is strictly necessary to make
the callback feature work.

That said, I found myself copy / paste / slight changing a fair bit of code
to make it work.  Usually a very bad sign.

If we can agree that a more aggressive refactor is desirable I would prefer
that route.

It may be worth looking at the GDB Python API (
https://sourceware.org/gdb/onlinedocs/gdb/Breakpoints-In-Python.html#Breakpoints-In-Python).
Watchpoints are just a species of breakpoint in that implementation.

I have done extensive work with that API, and while there are things I
would do very differently, I generally consider it to be fairly good.

In fact, I think that the LLDB SB API could profit quite a bit from several
of the things GDB has done on that front.  That is particularly true with
respect to symbols, variables, and frames.  That subject likely deserves a
different thread.

\author{Dan}

On Fri, Sep 9, 2016 at 3:52 PM, Jim Ingham <jingham at apple.com> wrote:

> The main problem with the watchpoint code is that it doesn't share nearly
> as much of the implementation of options and callback handling with the
> breakpoints as it should.  For instance, there's very little need for
> WatchpointOptions and BreakpointOptions to be separate classes, they do
> pretty much exactly the same thing.  If you want to hack on this, the best
> approach I think would be to remove the watchpoint options, and see how far
> you can get using the breakpoint option class.  I bet a lot of goodness
> will fall out of that.  The PerformActions for the StopInfoWatchpoint and
> StopInfoBreakpoint could probably share much of their implementations as
> well.  This is one of those cleanups that's been floating around on all our
> to-do lists for a while now, but keeps getting displaced by other tasks.
>
> The BreakpointOptions.h and WatchpointOptions.h files a pretty well
> documented.  That's a fairly good example of how we used to do this.  We
> don't tend to put top-level docs in .cpp files.
>
> Jim
>
>
> > On Sep 9, 2016, at 2:13 PM, Daniel Noland via lldb-dev <
> lldb-dev at lists.llvm.org> wrote:
> >
> > I have also noticed a few problems similar to Ted's and I plan to
> > address them assuming nobody else is already on it.  That said, I am new
> > around here so please bear with me :)
> >
> > In fact, I have been hacking on a few watchpoint methods for a while
> > now.  I have implemented some features I personally wanted (specifically
> > callback functions in the SBWatchpoint api).
> >
> > I have not yet created a PR (or whatever SVN equivalent) for several
> > reasons (obviously including the big reformat), but mostly I am a bit
> > lost with respect to proper procedure here.  I have gone through the
> > code guidelines for LLVM and LLDB, but I am confused about some things.
> >
> > * I have written unit test logic, but I don't really understand the LLDB
> > testing framework.  Also, I understand from other threads that the
> > framework is currently in flux in any case.
> >
> > * I have written documentation, but I don't know if what I have written
> > is even desirable.  For instance, the corresponding breakpoint
> > implementation is almost totally lacking in source doc.
> >
> > * I will need to rebase this patch on the reformatted code.  That is no
> > big deal, but I have little experience with SVN and I will need to do
> > some research to avoid turning an eventual merge into a big chore.
> >
> > * I am pretty unclear on the appropriate way to make my changes work
> > with the Python API.  Should that be on a different PR?  Are we
> > targeting Python 2.7 and 3.{4,5} on all platforms?
> >
> > * Do I need to check that the test suite passes on other platforms, or
> > will other devs take care of that?  I don't use OSX or Windows.
> >
> > Basically, I would like to help, but more than that I want my "help" to
> > be helpful.
> >
> > If somebody who knows what is going on would help me out I would greatly
> > appreciate it.
> >
> > \author{Daniel Noland}
> >
> > On 09/09/2016 11:28 AM, Greg Clayton via lldb-dev wrote:
> >>> On Sep 8, 2016, at 4:47 PM, Ted Woodward via lldb-dev <
> lldb-dev at lists.llvm.org> wrote:
> >>>
> >>> I recently discovered a problem with watchpoints talking to the
> Hexagon simulator:
> >>>
> >>> (lldb) w s e 0x1000
> >>> error: Watchpoint creation failed (addr=0x1000, size=4).
> >>> error: Target supports (0) hardware watchpoint slots.
> >>>
> >>>
> >>> It seems that lldb now sends a qWatchpointSupportInfo packet. But this
> packet isn’t defined in lldb-gdb-remote.txt.
> >>>
> >>> Looking at the code, it expects to get back a pair “num:#”. If it
> doesn’t it returns 0. The caller will report the above error if the number
> returned is 0. So if qWatchpointSupportInfo isn’t supported, lldb can’t set
> a watchpoint.
> >>>
> >>>
> >>> What is the definition of the response to qWatchpointSupportInfo? Is
> the the number of supported watchpoints, or the number of available
> watchpoints? If it’s supported, then CheckIfWatchpointsExhausted won’t
> really check if the watchpoints are exhausted. If it’s available, then a
> return of 0 doesn’t let us aggregate watchpoints – a 4 byte watchpoint at
> 0x1000 and one at 0x1004 could be one going from 0x1000-0x1007.
> >> The person that checked this in no longer is working on LLDB and it has
> been like this since May 2012. It should return the total number of
> supported watchpoints.
> >>>
> >>> Wouldn’t it be better to try to set the watchpoint, then report a
> failure if we get an error back from the remote server?
> >> It is kind of nice to know that you can't set watchpoints because they
> aren't supported since we can provide a nicer message than "E98  returned
> from GDB server". Errors in GDB remote protocol are a horrible mess and
> they don't mean anything. So any clearer we can be about this, the better,
> so we should keep the qWatchpointSupportInfo packet IMHO. I am fine with us
> modifying the GDBRemoteCommunicationClient to try and send this packet and
> if it comes back as unimplemented (response of $#00), you can set the num
> supported hardware watchpoints to UINT32_MAX. We should document that this
> means we don't know how many hardware watchpoints are supported, but it
> should then allow us to set hardware watchpoints if the GDB server doesn't
> support this packet.
> >>
> >> Watchpoints definitely need some work as they were done quickly by
> someone that is no longer around and they could use some TLC.
> >>
> >>>
> >>> --
> >>> Qualcomm Innovation Center, Inc.
> >>> The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project
> >>>
> >>> _______________________________________________
> >>> lldb-dev mailing list
> >>> lldb-dev at lists.llvm.org
> >>> http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
> >> _______________________________________________
> >> lldb-dev mailing list
> >> lldb-dev at lists.llvm.org
> >> http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
> >
> >
> > _______________________________________________
> > lldb-dev mailing list
> > lldb-dev at lists.llvm.org
> > http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-dev/attachments/20160909/d023e6de/attachment.html>