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
