Problem with watchpoints

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.

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?

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.

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 :slight_smile:

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}

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

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.

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!

It may be worth looking at the GDB Python API (Breakpoints In Python (Debugging with GDB)). 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.

We also have to be a little careful about taking things too directly from gdb due to licensing issues.

Jim

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 (Breakpoints In Python (Debugging with GDB)). 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 :slight_smile:

Duplication of the API should not be an issue either as I plan to
strictly adhere to the existing SB api.

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 (Breakpoints In Python (Debugging with GDB)). 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 :slight_smile:

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

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.