Page MenuHomePhabricator

Handle O reply packets during qRcmd
ClosedPublic

Authored by owenpshaw on Jan 4 2018, 4:32 PM.

Details

Summary

Gdb servers like openocd may send many $O reply packets for the client to output during a qRcmd command sequence. Currently, lldb interprets the first O packet as an unexpected response. Besides generating no output, this causes lldb to get out of sync with future commands because it continues reading O packets from the first command as response to subsequent commands.

This patch handles any O packets during an qRcmd, treating the first non-O packet as the true response.

Looking for guidance on how best to write test cases, haven't found anything currently testing qRcmd but may have just missed it.

Preliminary discussion at http://lists.llvm.org/pipermail/lldb-dev/2018-January/013078.html

Event Timeline

owenpshaw created this revision.Jan 4 2018, 4:32 PM
labath added a subscriber: labath.Jan 6 2018, 1:31 AM

The ProcessGdbRemote part is not really testable in it's current form, but for the rest, you should be able to write a gdbremoteclient unittest (see unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp).

source/Plugins/Process/gdb-remote/GDBRemoteClientBase.h
53 ↗(On Diff #128991)

this could be an llvm::function_ref (more lightweight than std::function).

source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
5158 ↗(On Diff #128991)

I think the API would be cleaner if the output callback was an argument to the standard SendPacket function, where the (default) value of null would mean "don't do anything special with the O packets", although that might just be a personal preference.

However, even if we choose to have separate functions as the API, I believe they should delegate to a single unified implementation internally.

owenpshaw updated this revision to Diff 128991.Jan 8 2018, 3:15 PM

Updated patch with new function name suggested by @clayborg. Added unit test and changed to llvm::function_ref as suggested by @labath.

Based on Greg's comments in the other thread, I've kept the new function separate, rather than use a flag in SendPacketAndWaitForResponse.

Is it worth creating a function to share the locking code that's common to SendPacketAndWaitForResponse and SendPacketAndReceiveResponseWithOutputSupport? Alternatively, should the lock-related error logging be unique to distinguish between the two functions?

clayborg accepted this revision.Jan 8 2018, 3:31 PM

Updated patch with new function name suggested by @clayborg. Added unit test and changed to llvm::function_ref as suggested by @labath.

Looks great.

Based on Greg's comments in the other thread, I've kept the new function separate, rather than use a flag in SendPacketAndWaitForResponse.

Yeah, it doesn't seem like any other commands will us it, so this is best just so people don't think it is normal.

Is it worth creating a function to share the locking code that's common to SendPacketAndWaitForResponse and SendPacketAndReceiveResponseWithOutputSupport?

I don't think so since it is so simple.

Alternatively, should the lock-related error logging be unique to distinguish between the two functions?

I am not sure it matters since this only affects the rCmd, and the packet reception stuff is still the same.

This revision is now accepted and ready to land.Jan 8 2018, 3:31 PM

Thanks! I don't have commit access, so someone needs to commit this for me, right?

This revision was automatically updated to reflect the committed changes.

Committed as r322190. I've changed one more std::function to function_ref and re-clang-formatted the patch.