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 <jing...@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@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@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@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 > > > > > > _______________________________________________ > > 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
