I found another issue, Daniel: GDBRemoteCommunicationClient::GetWatchpointSupportInfo calls SendPacketAndWaitForResponse, and assumes that a return of PacketResult::Success means that qWatchpointSupportInfo is handled by the remote server. That's not true; a null response also returns PacketResult::Success. It also needs to check that response.Empty() isn't true.
-- Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project -----Original Message----- From: lldb-dev [mailto:lldb-dev-boun...@lists.llvm.org] On Behalf Of Jim Ingham via lldb-dev Sent: Monday, September 12, 2016 2:01 PM To: Daniel Noland <daniel.nol...@gmail.com> Cc: lldb-dev@lists.llvm.org Subject: Re: [lldb-dev] Problem with watchpoints > On Sep 12, 2016, at 11:53 AM, Daniel Noland <daniel.nol...@gmail.com> wrote: > > > > 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. Excellent, feel free to pester us with whatever questions you have as you proceed. This is a piece of work that really does need to get done so I'm happy to see somebody taking it on. > > 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. Great! I look forward to your ideas. One note is that we have promised not to break compatibility with the current SB API's till we decide to do a whole scale revision. So for now whatever we change needs to fit within the current schema. > >> 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. > That sounds right. Jim >> 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} _______________________________________________ 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
