[Lldb-commits] [PATCH] D68293: [android/process list] support showing process arguments

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Including the args sounds like a good idea, but I don't think the chosen encoding scheme is very good. The encoding done in `GetCommandString` is very naive and not reversible. Since you're hex-encoding the result anyway, and the nul character cannot be present inside an argument you could use it to delimit the individual arguments (i.e. `666f6f00626172` -> `{"foo", "bar"}`). Or, you could look at how the `$A` packet transmits arguments, and implement something similar.

Also, you should write a test for this. I think it should be possible to run a process with known arguments, and then verify that they show up in "platform process list" output. Alternatively, the client part of the protocol can also be tested via c++ unit tests (see unittests/Process/gdb-remote). The advantage of that approach is that you're able to inject invalid packets and test the handling of those. In an ideal world, we'd have both kinds of these tests, but we're pretty far from that, so I'd settle for at least one of them. :)



================
Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp:613-615
+    if (SendPacketAndWaitForResponse(
+            "jGetLoadedDynamicLibrariesInfos:", response, false) ==
+        PacketResult::Success) {
----------------
When you're running clang format, please make sure it only formats the lines that you have modified. I've you're using the standard git-clang-format integration, this should be as simple as `git clang-format HEAD^`. Please revert these unrelated changes.


================
Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp:1944-1948
+    if (!process_info.GetExecutableFile().GetPath().empty()) {
+      process_info.SetArguments(
+          Args(process_info.GetExecutableFile().GetPath() + args_command),
+          true);
+    }
----------------
I don't fully understand the what this does. Is it trying to set the executable name as argv[0]? Wouldn't it be better to send the argv[0] separately (together with the rest of the args array), just in case the process has a special argv[0].


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68293/new/

https://reviews.llvm.org/D68293