Page MenuHomePhabricator

[lldb] Remote disk file/directory completion for platform commands
ClosedPublic

Authored by MrHate on Aug 5 2020, 1:42 AM.

Details

Summary
  1. Extended the gdb-remote communication related classes with disk file/directory completion functions;
  2. Added two common completion functions RemoteDiskFiles and RemoteDiskDirectories based on the functions above;
  3. Added completion for these commands:
    1. platform get-file <remote-file> <local-file>;
    2. platform put-file <local-file> <remote-file>;
    3. platform get-size <remote-file>;
    4. platform settings -w <remote-dir>;
    5. platform open file <remote-file>.
  4. Added related tests for client and server;
  5. Updated docs/lldb-platform-packets.txt.

Diff Detail

Event Timeline

MrHate requested review of this revision.Aug 5 2020, 1:42 AM
MrHate created this revision.

The generic command part seems fine, but I'm not familiar enough with the GDB remote code to review that. Pavel and Jonas should know this better. Also + Jason and Greg.

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
2990

I think this breaks for paths that have a comma in them?

MrHate added inline comments.Aug 6 2020, 4:34 AM
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
2990

I think so. It might be better if I put the hex32 value before the comma.

MrHate updated this revision to Diff 284253.Aug 9 2020, 9:15 PM
MrHate edited the summary of this revision. (Show Details)
  1. Corrected summary;
  2. Added a test case for client which provides mock response for autocomplete request and test completion on command line;
  3. Modified the argument order of the autocomplete request packet replacing vFile:autocomplete:<partial_path>,<only_dir> with vFile:autocomplete:<only_dir>,<partial_path>, so commas won't break partial path resolving.
MrHate updated this revision to Diff 284320.Aug 10 2020, 5:26 AM
MrHate retitled this revision from [lldb][WIP] Remote disk file/directory completion for platform commands to [lldb] Remote disk file/directory completion for platform commands.
MrHate edited the summary of this revision. (Show Details)
  • Added a server test.

This is mostly fine. You need to escape the completed filenames you send back. You should have a test case where multiple matches are returned, like "a" matches "abc1" and "abc2".

I'm not too convinced that adding this packet as a vFile: is the right approach, although I understand why you went that way. All of the existing vFile: packets are direct overlays with POSIX low level APIs - open, close, unlink, chmod, read, write, etc. This is an entirely different thing, even though it works with files. I would personally write this as a Q packet -- a lot of lldb extensions to gdb Remote Serial Protocol end up as Q packets. QFilenameCompletion or something. You can keep the same packet design - that's fine. With more complex packets we're starting to use JSON more often these days, but this is simple and I think you've got the right idea.

You should definitely update docs/lldb-platform-packets.txt as part of your patch. I wrote a standalone platform implementation a while back, and tried to document all of the packets involved in this.

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp
737 ↗(On Diff #284320)

We only return 8-byte error code and we try to pick a different number for each error return so you could track down WHICH error condition was being hit for each Enn received (by looking at the source). Of course, given that each packet has a unique set of error returns possible, it was unnecessary to have a completely unique number across all packets -- you only need to disambiguate between different error responses to this one packet.

There's also a newer extended error mechanism where error strings can be returned, but I don't see that being helpful here.

752 ↗(On Diff #284320)

This will include a , character before the first matched filename if there are more than one.

753 ↗(On Diff #284320)

You'll need to use asciihex-encoded strings in the response - that's not being done, right? If a filename has a #, }, or $ in the string anywhere, it will be the end of the debug session.

MrHate updated this revision to Diff 284561.Aug 10 2020, 7:36 PM
MrHate edited the summary of this revision. (Show Details)
  1. Renamed all "vFile:autocomplete" related into "qDiskAutocomplete";
  2. Refactored match response from "[cstr],[cstr]..." into "[length]:[str bytes][length]:[str bytes]...";
  3. Updated docs/lldb-platform-packets.txt;
  4. Use 85 as the server's response error code.

Thanks for the suggestions!

MrHate updated this revision to Diff 284568.Aug 10 2020, 8:08 PM
  • Extended the server test into 3 separated cases:
    1. disk file with 1 match and 2 matches;
    2. disk dir on non-windows platform (for '/');
    3. disk dir on windows (for '\').

Nice set of changes, and you're right it should be a lower-case "q" packet, not an upper-case "Q" packet like I suggested, it's just querying information from the remote side.

In the response, I would not specify the length of the filenames, it's unnecessary. This can be something simply like,

bool first = true;
for (const auto &match : matches) {
  if (first) {
    first = false;
  } else {
    response.PutChar(',');
  }
  response.PutStringAsRawHex8(match.c_str());
}

(or however you want to add the commas after the first one, that's not the important part)

This is a pretty common design for gdb remote serial protocol packets IMO.

Yeah, this looks very good. In particular, I like how you've separated the tests for the client and server-side implementations. Some comments inline.

Nice set of changes, and you're right it should be a lower-case "q" packet, not an upper-case "Q" packet like I suggested, it's just querying information from the remote side.

In the response, I would not specify the length of the filenames, it's unnecessary. This can be something simply like,

bool first = true;
for (const auto &match : matches) {
  if (first) {
    first = false;
  } else {
    response.PutChar(',');
  }
  response.PutStringAsRawHex8(match.c_str());
}

(or however you want to add the commas after the first one, that's not the important part)

This is a pretty common design for gdb remote serial protocol packets IMO.

+1 for length-less comma-separated hex-encoded file names.

lldb/docs/lldb-platform-packets.txt
241

I find myself not liking the word "Disk" in the packet name. I get that that's the name of the underlying api, but I'm not terribly fond of that either (and unlike the internal api, changing the packet name is not that easy). I'd rather go with something like qFilenameComplete like Jason suggested, as that does not imply any particular storage medium.

254

This should be cleared up more. It's not clear whether "true" means completing files or directories. And IIUC, completing directories are also included when completing files, but that is not obvious from the description.

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp
94 ↗(On Diff #284568)

This will make the command also available to the LLGS (gdb-server) personality of lldb-server. Do we want that? If not, we should put this in GDBRemoteCommunicationServerPlatform.

lldb/test/API/functionalities/gdb_remote_client/gdbclientutils.py
287–289

This is not likely to be useful in other tests. It'd be better to return an error here and provide an actual implementation in the test file. That will make it clear why does the test expect a particular string to be completed.

lldb/test/API/tools/lldb-server/TestGdbRemoteCompletion.py
64

what exactly is the expectation regarding the trailing directory separator? Should it be added only for directory completions or also for file completions if the completed thing happens to be a directory? This should be documented in the packet description.

Incidentally, I don't think it's useful to create a whole new test just to differentiate between the directory separator. You could just check the host platform in the test and adjust the expectation based on that.

MrHate added inline comments.Aug 11 2020, 7:18 PM
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp
94 ↗(On Diff #284568)

Yes, this is a platform dedicated feature.

And may I ask about how to create a lldb-server platform dedicated test? It seems the lldb-server test suite is mainly aimed at llgs and debugserver. I dont really understand what the debugserver is, and now it's not suitable for a llgs test.

labath added inline comments.Aug 12 2020, 7:36 AM
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp
94 ↗(On Diff #284568)

We don't currently have a good mechanism to write platform dedicated tests. The platform features are tested implicitly when running the test suite remotely, and we have one or two tests (TestPlatformProcessConnect) which also spawn platform instances manually (but only when running remotely).

That said, I don't think it should be too difficult to add a method for testing the platform features directly. I think all it would take is to write a new function similar to connect_to_debug_monitor which would run the platform instance and connect to it. After that, all of the usual test methods should work, because they are essentially speaking the same protocol. I think the launch sequence could roughly be:

port_file = self.getBuildArtifact("port")
server = self.spawnSubprocess(get_lldb_server_exe(), ["platform", "--listen", "*:0", "--socket-file", port_file)
port = lldbutil.wait_for_file_on_target(port_file)
# connect to port and start communicating
MrHate updated this revision to Diff 285433.Aug 13 2020, 11:26 AM
  1. Renamed packet related into "qPathComplete";
  2. Refactored response form into comma-separated;
  3. Moved the functions from GDBRemoteCommunicationServerCommon to GDBRemoteCommunicationServerPlatform;
  4. Refactored the server-side test into lldb-platform dedicated, combined test cases into just one;
  5. Now MockGDBServerResponder.qPathComplete responses with just an empty string.

My sincerest appreciation for all these suggestions/comments.

Now the server test works well except that the lldb-server subprocess cannot be terminated gracefully due to a TypeError thrown by sock.sendall(GdbRemoteTestCaseBase._GDBREMOTE_KILL_PACKET) in shutdown_socket, the backtrace is

Traceback (most recent call last):
  File "/home/ubuntu/llvm-project/lldb/packages/Python/lldbsuite/test/tools/lldb-server/gdbremote_testcase.py", line 349, in shutdown_socket
    sock.sendall(GdbRemoteTestCaseBase._GDBREMOTE_KILL_PACKET)
TypeError: a bytes-like object is required, not 'str'
MrHate updated this revision to Diff 285441.Aug 13 2020, 11:33 AM
  • Updated the packet doc.

Now the server test works well except that the lldb-server subprocess cannot be terminated gracefully due to a TypeError thrown by sock.sendall(GdbRemoteTestCaseBase._GDBREMOTE_KILL_PACKET) in shutdown_socket, the backtrace is

Traceback (most recent call last):
  File "/home/ubuntu/llvm-project/lldb/packages/Python/lldbsuite/test/tools/lldb-server/gdbremote_testcase.py", line 349, in shutdown_socket
    sock.sendall(GdbRemoteTestCaseBase._GDBREMOTE_KILL_PACKET)
TypeError: a bytes-like object is required, not 'str'

Interesting. This isn't really related to your patch, is it (like, I would expect all gdb-remote tests to suffer from this python3 incompatibility)? I'll try to take a look at this later today, but if the test comes out as successful I don't think you have to wait for that. The patch looks good to me, aside from some small clarifications in inline comments.

lldb/docs/lldb-platform-packets.txt
254

I still don't find this very clear. I was expecting some text like:

If the first argument is zero, the result should contain all files (including directories) starting with the given path. If the argument is one, the result should contain only directories.
The result should be a comma-separated list of hex-encoded paths. Paths denoting a directory should end with a directory separator ('/' or '\').

However, I'm not sure if it accurately describes the interface. If it does, feel free to reuse the text. What I'd like to achieve is include enough information so that it is possible to reimplement this packet without consulting the lldb source code.

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.cpp
360

I just realized that in case the completion returns no answers, this will return an empty packet. In gdb-remote parlance, an empty packet normally means "unimplemented". Now this is not normally going to cause an issue, since the only reasonable response to an unimplemented packet is to do no completion.

However, I think it would be more idiomatic to send an error response in this case. @jasonmolenda, would you agree?

361–364

You don't have to use this, but I figured I'd note that a usually cleaner way of skipping the first separator is:

StringRef separator;
for(...) {
  response << separator;
  separator = ",";
  ...
}
369–370

delete

lldb/test/API/tools/lldb-server/TestGdbRemoteCompletion.py
35–37

I'd consider doing something like:

encode = gdbremote_hex_encode_string
self.test_sequence.add_log_lines(
            ["read packet: $qPathComplete:0,%s#00"%encode("main"),
             ...

Now the server test works well except that the lldb-server subprocess cannot be terminated gracefully due to a TypeError thrown by sock.sendall(GdbRemoteTestCaseBase._GDBREMOTE_KILL_PACKET) in shutdown_socket, the backtrace is

Traceback (most recent call last):
  File "/home/ubuntu/llvm-project/lldb/packages/Python/lldbsuite/test/tools/lldb-server/gdbremote_testcase.py", line 349, in shutdown_socket
    sock.sendall(GdbRemoteTestCaseBase._GDBREMOTE_KILL_PACKET)
TypeError: a bytes-like object is required, not 'str'

Interesting. This isn't really related to your patch, is it (like, I would expect all gdb-remote tests to suffer from this python3 incompatibility)? I'll try to take a look at this later today, but if the test comes out as successful I don't think you have to wait for that. The patch looks good to me, aside from some small clarifications in inline comments.

That should be fixed by e6b1b61054.

Now the server test works well except that the lldb-server subprocess cannot be terminated gracefully due to a TypeError thrown by sock.sendall(GdbRemoteTestCaseBase._GDBREMOTE_KILL_PACKET) in shutdown_socket, the backtrace is

Traceback (most recent call last):
  File "/home/ubuntu/llvm-project/lldb/packages/Python/lldbsuite/test/tools/lldb-server/gdbremote_testcase.py", line 349, in shutdown_socket
    sock.sendall(GdbRemoteTestCaseBase._GDBREMOTE_KILL_PACKET)
TypeError: a bytes-like object is required, not 'str'

Interesting. This isn't really related to your patch, is it (like, I would expect all gdb-remote tests to suffer from this python3 incompatibility)? I'll try to take a look at this later today, but if the test comes out as successful I don't think you have to wait for that. The patch looks good to me, aside from some small clarifications in inline comments.

That should be fixed by e6b1b61054.

Thanks very much.

The TypeError problem is solved, but the lldb-platform still complains about SIGHUP. lldb-platform aborts for SIGHUP, so I still get error messages from the lldb-platform subprocess when test ends. It seems the kill packet works well for lldb-gdbserver, but I don't know if it works for lldb-platform as well.

Now it looks like this:

Session logs for test failures/errors/unexpected successes will go into directory '/home/ubuntu/llvm-project/build/lldb-test-traces'
SIGHUP received, exiting lldb-server...
PLEASE submit a bug report to https://bugs.llvm.org/ and include the crash backtrace.
Stack dump:
0.      Program arguments: /home/ubuntu/llvm-project/build/bin/lldb-server platform --listen *:0 --socket-file /tmp/tmpmyxqrfw8 
1.      Program arguments: /home/ubuntu/llvm-project/build/bin/lldb-server platform --listen *:0 --socket-file /tmp/tmpmyxqrfw8 
/home/ubuntu/llvm-project/build/bin/lldb-server[0x4349e4]
/home/ubuntu/llvm-project/build/bin/lldb-server[0x4329ee]
/home/ubuntu/llvm-project/build/bin/lldb-server[0x434f85]
/lib/x86_64-linux-gnu/libpthread.so.0(+0x128a0)[0x7f8de017c8a0]
/lib/x86_64-linux-gnu/libc.so.6(gsignal+0xc7)[0x7f8dde5d5f47]
/lib/x86_64-linux-gnu/libc.so.6(abort+0x141)[0x7f8dde5d78b1]
/home/ubuntu/llvm-project/build/bin/lldb-server[0x41a19c]
/lib/x86_64-linux-gnu/libc.so.6(+0x3efd0)[0x7f8dde5d5fd0]
/lib/x86_64-linux-gnu/libpthread.so.0(send+0x1a)[0x7f8de017b97a]
/home/ubuntu/llvm-project/build/bin/lldb-server[0x4925a0]
/home/ubuntu/llvm-project/build/bin/lldb-server[0x49936b]
/home/ubuntu/llvm-project/build/bin/lldb-server[0x4f69b2]
/home/ubuntu/llvm-project/build/bin/lldb-server[0x4ea43a]
/home/ubuntu/llvm-project/build/bin/lldb-server[0x4ea386]
/home/ubuntu/llvm-project/build/bin/lldb-server[0x4cd905]
/home/ubuntu/llvm-project/build/bin/lldb-server[0x419e65]
/home/ubuntu/llvm-project/build/bin/lldb-server[0x41b363]
/lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xe7)[0x7f8dde5b8b97]
/home/ubuntu/llvm-project/build/bin/lldb-server[0x415aca]
/home/ubuntu/llvm-project/build/bin/lldb-server[0x4349e4]
/home/ubuntu/llvm-project/build/bin/lldb-server[0x432a25]
/home/ubuntu/llvm-project/build/bin/lldb-server[0x434f85]
/lib/x86_64-linux-gnu/libpthread.so.0(+0x128a0)[0x7f8de017c8a0]
/lib/x86_64-linux-gnu/libc.so.6(gsignal+0xc7)[0x7f8dde5d5f47]
/lib/x86_64-linux-gnu/libc.so.6(abort+0x141)[0x7f8dde5d78b1]
/home/ubuntu/llvm-project/build/bin/lldb-server[0x41a19c]
/lib/x86_64-linux-gnu/libc.so.6(+0x3efd0)[0x7f8dde5d5fd0]
/lib/x86_64-linux-gnu/libpthread.so.0(send+0x1a)[0x7f8de017b97a]
/home/ubuntu/llvm-project/build/bin/lldb-server[0x4925a0]
/home/ubuntu/llvm-project/build/bin/lldb-server[0x49936b]
/home/ubuntu/llvm-project/build/bin/lldb-server[0x4f69b2]
/home/ubuntu/llvm-project/build/bin/lldb-server[0x4ea43a]
/home/ubuntu/llvm-project/build/bin/lldb-server[0x4ea386]
/home/ubuntu/llvm-project/build/bin/lldb-server[0x4cd905]
/home/ubuntu/llvm-project/build/bin/lldb-server[0x419e65]
/home/ubuntu/llvm-project/build/bin/lldb-server[0x41b363]
/lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xe7)[0x7f8dde5b8b97]
/home/ubuntu/llvm-project/build/bin/lldb-server[0x415aca]
PASS: LLDB (/home/ubuntu/llvm-project/build/bin/clang-x86_64) :: test_autocomplete_path (TestGdbRemoteCompletion.GdbRemoteCompletionTestCase)
----------------------------------------------------------------------
Ran 1 test in 0.283s
MrHate updated this revision to Diff 285810.Aug 14 2020, 8:50 PM
  1. Removed the unnecessary print line in GDBRemoteCommunicationServerPlatform.cpp;
  2. Updated the packet doc;
  3. Refactored the for-loop at line 359-364 in GDBRemoteCommunicationServerPlatform.cpp;
  4. Refactored the server-side test with str.encode().hex().

If we need to response for qPathComplete with empty match result. I think we could provide a char like R or what as the response. The match result is encoded into hex-bytes, so it might be easy to resolve the error symbol from the result IMHO.

  1. Removed the unnecessary print line in GDBRemoteCommunicationServerPlatform.cpp;
  2. Updated the packet doc;
  3. Refactored the for-loop at line 359-364 in GDBRemoteCommunicationServerPlatform.cpp;
  4. Refactored the server-side test with str.encode().hex().

If we need to response for qPathComplete with empty match result. I think we could provide a char like R or what as the response. The match result is encoded into hex-bytes, so it might be easy to resolve the error symbol from the result IMHO.

What if the response is "matches:<ASCIIHEX>[,<ASCIIHEX>]" and if there are no matches, just "matches:"? We don't want to send an empty string because that means "packet not known" in the remote serial protocol.

  1. Removed the unnecessary print line in GDBRemoteCommunicationServerPlatform.cpp;
  2. Updated the packet doc;
  3. Refactored the for-loop at line 359-364 in GDBRemoteCommunicationServerPlatform.cpp;
  4. Refactored the server-side test with str.encode().hex().

If we need to response for qPathComplete with empty match result. I think we could provide a char like R or what as the response. The match result is encoded into hex-bytes, so it might be easy to resolve the error symbol from the result IMHO.

What if the response is "matches:<ASCIIHEX>[,<ASCIIHEX>]" and if there are no matches, just "matches:"? We don't want to send an empty string because that means "packet not known" in the remote serial protocol.

"matches:" takes up 8 chars, maybe we can just use a 'M' as vFile:size uses 'F', so it may look like: "M<ASCIIHEX>[,<ASCIIHEX>]". What do you think?

  1. Removed the unnecessary print line in GDBRemoteCommunicationServerPlatform.cpp;
  2. Updated the packet doc;
  3. Refactored the for-loop at line 359-364 in GDBRemoteCommunicationServerPlatform.cpp;
  4. Refactored the server-side test with str.encode().hex().

If we need to response for qPathComplete with empty match result. I think we could provide a char like R or what as the response. The match result is encoded into hex-bytes, so it might be easy to resolve the error symbol from the result IMHO.

What if the response is "matches:<ASCIIHEX>[,<ASCIIHEX>]" and if there are no matches, just "matches:"? We don't want to send an empty string because that means "packet not known" in the remote serial protocol.

"matches:" takes up 8 chars, maybe we can just use a 'M' as vFile:size uses 'F', so it may look like: "M<ASCIIHEX>[,<ASCIIHEX>]". What do you think?

I don't have a real preference, but fwiw packet size basically doesn't effect performance over modern media (Bluetooth, WiFi, USB, TCP) - sending and receiving packets are expensive but extra bytes don't generally matter unless you're doing a large data transfer.

MrHate updated this revision to Diff 285877.Aug 15 2020, 8:21 PM
  • Added an 'M' at the beginning of the response packet, updated related doc and tests.

I looked at the gdb-remote docs, and I could find precedents for both returning error (or OK) for "empty" responses, and prefixing the responses with a random character. So I think adding the M character is fine too.

I tried out the lldb-server test, and I ran into some problems -- see inline comments. I wouldn't be too worried about the SIGHUP thing. lldb-server process terminates correctly when the test closes the connection, but it test suite is too quick to follow that up with a SIGHUP, which throws it for a spin. The fix for that would be fairly simple if we were on python 3 (popen as a wait() method with a timeout), but given that we're not on py3 just yet, we can leave this cleanup to a slightly later date...

lldb/test/API/tools/lldb-server/TestGdbRemoteCompletion.py
38

This check fails for me because I have a main.cpp~ in addition to main.cpp (I have edited that file some time in the last two years). Similar can happen with other checks too.

I think it would be much safer to create an appropriate directory structure in the build folder and test completion on that. The build folder is erased before each test, so you can be sure it contains only things you created (in addition to make outputs).

43–50

This fails for me because the files end up in the opposite order. We could either sort the files in the test or in the actual implementation of the completion function.

MrHate updated this revision to Diff 286611.Aug 19 2020, 10:45 AM
  1. Now the server uses std::sort to sort the match result before writing into the response packet;
  2. Refactored the server-side test to run within the build directory.
labath accepted this revision.Aug 24 2020, 2:51 AM

Sorry about the delay. This looks good to me now.

This revision is now accepted and ready to land.Aug 24 2020, 2:51 AM