On 09/12/2016 11:30 AM, Jim Ingham wrote: > >> On Sep 9, 2016, at 7:33 PM, Daniel Noland <daniel.nol...@gmail.com> wrote: >> >> 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. > > This work really needs to be done, and shouldn't even be all that hard. So > putting too much effort into the current implementation seems throw-away > work. It's okay when small changes make useful functionality available, but > not the right general direction. If you are interested in taking a more > comprehensive change on, that would be great! > I am actually very relieved to hear that!
I have a strong personal motivation to see very robust watchpoint functionality in LLDB (the last two years of my life will be largely wasted without it). If the community is open to a more aggressive change I will be very happy to put in work on this front. >> >> 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. >> > > From the API perspective, watchpoints and breakpoints are equivalent, except > where the brokenness of the implementation shows through. > >> 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. > > Within the constraints of maintaining support for the extant SB API's, we can > certainly look to improving the breakpoint/watchpoint interface. But the > most logical approach seems to me to be to move the watchpoints over to the > sharing the extant and pretty functional breakpoint implementation first, and > then see what we can more efficiently add stuff to both. > I agree. I plan to start exactly as you suggest. My main thought here (which should likely get a different thread) is that, having worked with the GDB and LLDB api for a while now, I feel pretty strongly that GDB has a better cleaner handling of variable names, scopes, frames, and values than LLDB seems to offer (I would love to be wrong about this btw), and that adopting / adapting some of those abstractions may significantly improve the break/watchpoint api. I should be able to say that with more authority after giving the implementation more careful study. > We also have to be a little careful about taking things too directly from gdb > due to licensing issues. > Good point about licensing. That said, if the concern is duplicating the implementation, I doubt that I could do it if I tried :) Duplication of the API should not be an issue either as I plan to strictly adhere to the existing SB api. > Jim > > >> >> \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 >> >> > -- \author{Daniel Noland}
signature.asc
Description: OpenPGP digital signature
_______________________________________________ lldb-dev mailing list lldb-dev@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
