Page MenuHomePhabricator

Adding Support for Error Strings in Remote Packets
ClosedPublic

Authored by ravitheja on Jul 3 2017, 7:17 AM.

Details

Summary

This patch adds support for sending strings along with
error codes in the reply packets. The implementation is
based on the feedback recieved in the lldb-dev mailing
list. The patch also adds an extra packet for the client
to query if the server has the capability to provide
strings along with error replys.

Event Timeline

ravitheja created this revision.Jul 3 2017, 7:17 AM
labath added inline comments.Jul 3 2017, 8:50 AM
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
3204

AsCString unnecessary

3329

I don't think every packet function should need to write these four lines of code. This should be abstracted away. Ideally I'd like to see this as a method on the response class (i.e., simply error = response.GetStatus()), but if it's hard to plumb that information into the class cleanly, I can imagine it being a method on the communication class as well (error = this->GetStatus(response))

update: It looks like the GetStatus function already supports that: Why not just write this as error = response.GetStatus() ???

source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.cpp
110

Condensed version without dodgy C functions:
return SendPacketNoLock(llvm::formatv("E{0:x-2};{1}", error.GetError(), error).str());

116

Aren't you supposed to send these only if the client enabled the error response?

source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.h
71

Why the reference?

source/Utility/StringExtractorGDBRemote.cpp
473

replace 47 with the actual error code

ravitheja updated this revision to Diff 105150.Jul 4 2017, 4:14 AM

changes from feedback.

ravitheja marked 4 inline comments as done.Jul 4 2017, 4:16 AM
ravitheja added inline comments.
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.cpp
116

From the discussions on the dev-list I thought the client needs to query if the server will send the error string or not and from the
server side its always enabled ?

source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.h
71

how about const reference ?

With this patch, I see the extra packet to query from server is probably unnecessary. I mean the error code is still there and it should be sufficient for the client to detect an error. As long as it does not mistake it for something else it should not be a problem ? .

labath edited edge metadata.Jul 4 2017, 4:40 AM

With this patch, I see the extra packet to query from server is probably unnecessary. I mean the error code is still there and it should be sufficient for the client to detect an error. As long as it does not mistake it for something else it should not be a problem ? .

Well, if we don't care about newer lldb-server being able to communicate with older clients than it's theoretically superfluous. I'm not sure whether we care about this scenario, but I was kinda expecting we do it this way, because that's how it's done elsewhere (e.g. QListThreadsInStopReply). Let's see if others have anything to say about this (might take a bit of time as it's holiday in the US now).

docs/lldb-gdb-remote.txt
137

The quotes here are misleading -- after reading this I would expect that the string is sent quoted. Best be explicit about the encoding of the error string.

source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.h
71

much better

Yeah that would be broken, as long as we make the error packet more than 3 bytes, then it won't work with the older lldb clients.

clayborg edited edge metadata.Jul 4 2017, 10:37 AM

I would think we would try to enable this using something like:

QEnableErrorStrings

And if the the server responds with "OK" then we know it is enabled. By default the server should not enable any fancy features without being asked. We would like lldb-server to stay compatible with other GDB remote clients.

The error string should be hex ASCII encoded so the string can contain any characters. Any string that can contain any content should be hex ASCII encoded.

docs/lldb-gdb-remote.txt
137

Maybe:

EXX;AAAAAA

Where XX is a hex byte error number, followed by a semicolon and where AAAAAA is a hex ascii encoded error message.
284

Just make similar to whatever we decide on in the docs for QErrorStringInPacketSupported

318

Just make similar to whatever we decide on in the docs for QErrorStringInPacketSupported

351

Just make similar to whatever we decide on in the docs for QErrorStringInPacketSupported

393

Just make similar to whatever we decide on in the docs for QErrorStringInPacketSupported

ravitheja updated this revision to Diff 105272.Jul 5 2017, 8:03 AM

Updating Doc and changing feature to be enabled by client.

ravitheja updated this revision to Diff 105273.Jul 5 2017, 8:06 AM

Forgot this in the doc.

ravitheja marked 7 inline comments as done.Jul 5 2017, 8:08 AM
labath added inline comments.Jul 5 2017, 8:19 AM
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
3176

I don't like how every packet function needs to enable this manually. Every function can benefit from this. Can we just set this once at startup, when we probe other server features?

source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.cpp
111

Doesn't look like this is hex-encoding.

source/Utility/StringExtractorGDBRemote.cpp
477

This doesn't look like hex-encoding.

clayborg requested changes to this revision.Jul 5 2017, 11:42 AM

See inlined comments.

source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
3176

I would move this into ProcessGDBRemote::ConnectToDebugserver() where many one time things happen. We should enable this once at the beginning and then deal with the error messages always possibly being present. The code in ProcessGDBRemote::ConnectToDebugserver() where you would insert:

m_gdb_comm.GetEchoSupported();
m_gdb_comm.GetThreadSuffixSupported();
m_gdb_comm.GetListThreadsInStopReplySupported();
m_gdb_comm.GetHostInfo();
m_gdb_comm.GetVContSupported('c');
m_gdb_comm.GetVAttachOrWaitSupported();
m_gdb_comm.EnableErrorStringInPacket(); /// <<< HERE
3220

remove

3253

remove

3262

remove

3275

remove

3345

remove

source/Utility/StringExtractorGDBRemote.cpp
21–22

We should probably be a bit more specific with the check to check for the ';' or NULL terminator at index 3:

if (isxdigit(m_packet[1]) && isxdigit(m_packet[2])) {
  if (m_packet.size() == 3)
    return eError; 
  if (m_packet[3] == ';') {
    if (verify all remaining bytes are valid HEX ASCII bytes)
      return eError; 
  }
}

We don't want some random string packet response like "Each time I do" to make this the response claim to be an error response.

477

hex encode

This revision now requires changes to proceed.Jul 5 2017, 11:42 AM
clayborg added inline comments.Jul 5 2017, 11:47 AM
source/Utility/StringExtractorGDBRemote.cpp
477

There is also a hex decode that will need to be added somewhere...

ravitheja updated this revision to Diff 105380.Jul 6 2017, 1:04 AM
ravitheja edited edge metadata.

Support for Hex encoded strings and more error checking.

ravitheja marked 11 inline comments as done.Jul 6 2017, 1:07 AM
labath added a comment.Jul 6 2017, 2:42 AM

I am generally happy with this, just a couple of things I noticed below:

source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
3329

The indenting look wrong. Please run the patch through clang-format before submission.

source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h
158 ↗(On Diff #105380)

It doesn't look like you're implementing this anywhere. Please remove.

source/Utility/StringExtractorGDBRemote.cpp
26

Please use StringRef here. No need to copy the string just to examine it's contents.

29

This will break out of the inner for loop, which is probably not what you intended.

ravitheja updated this revision to Diff 105395.Jul 6 2017, 4:24 AM

Correcting mistakes.

Looks good. Just one question on why we register a packet handler.

source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.cpp
32–35

Why is this done here and not where all of the other packets are registered?

ravitheja added inline comments.Jul 11 2017, 12:30 AM
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.cpp
32–35

I did it here coz this class has the member SendErrorResponse which I wanted to overload. Also I think the remote packets are a bit spreadout among inherited classes, so it will be available for all these classes and then these won't be spread out like packet handling code will be in one place.

clayborg accepted this revision.Jul 11 2017, 8:27 AM

Wait for an OK from Pavel as well.

This revision is now accepted and ready to land.Jul 11 2017, 8:27 AM
labath accepted this revision.Jul 11 2017, 8:30 AM
ravitheja closed this revision.Jul 12 2017, 4:15 AM

Note that if you put "Differential Revision: " followed by the URL for this:

Differential Revision: https://reviews.llvm.org/D34945

in your source control commit message it will automatically close this for you and add the SVN revision number into this as a message. Please note the SVN revision in here if possible since you did it manually.

Uploaded SVN Revision Number - 307773 to fix Android Builder.