> 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
