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

[Pavel Labath via Phabricator via lldb-commits]

labath added a subscriber: jasonmolenda.
labath added a comment.

The encoding scheme looks fine to me. There's just two more additional things that came to mind:

- r373789 by @jasonmolenda reminded me that we have a document for describing the gdb-remote protocol extensions. It would be good to add sentence or two about the encoding of the process arguments there.
- It's great that you've added gdb-client test for this. Among other things, it enables us to inject "faulty" packets and test the handling of those (such as the `GetHexByteString` failure I mention in the inline comment). However, this only tests the client-side of the feature. Given that the arguments of a process (unlike the UID it runs as for instance) is something that we can reasonably control from a test, I think we should have an end-to-end test too. I think it would be easiest to test this with python. You can probably use one of the "attach" tests as a starting point, only instead of attaching to the process, you'd run "platform process list" and verify the process (and it's arguments) show up in the output.



================
Comment at: lldb/packages/Python/lldbsuite/test/functionalities/gdb_remote_client/TestPlatformClient.py:29
         try:
+            self.runCmd("log enable gdb-remote all")
             self.runCmd("platform connect connect://localhost:%d" %
----------------
?


================
Comment at: lldb/source/Host/linux/Host.cpp:159-160
     std::tie(Arg, Rest) = Rest.split('\0');
+    if (Arg.empty())
+      continue;
     process_info.GetArguments().AppendArgument(Arg);
----------------
This doesn't seem right... What about processes invoked with empty arguments? (`my_prog "" ""`)


================
Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp:1932
+        StringExtractor extractor(value);
+        std::stringstream stream(value);
+
----------------
We usually don't use STL iostreams in llvm. And it definitely doesn't look like they are needed here. Why do something like the `while(!empty) split('-')` pattern in `GetProcessArgs` above?


================
Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp:1934
+
+        auto extractOneArg = [&](std::string &arg) {
+          std::string hex_arg;
----------------
I think it would be simpler if you just parsed this in a regular loop, did something like `if(!process_info.GetArguments().empty()) process_info.SetArg0(...)` after the loop


================
Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp:1939
+          extractor = StringExtractor(hex_arg);
+          extractor.GetHexByteString(arg);
+          return true;
----------------
What happens if this fails?


================
Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp:1193
+    response.PutChar('-');
+    response.PutStringAsRawHex8(arg.c_str());
+  }
----------------
`s/.c_str()/.ref()`. Generally, always try to use the non-c string apis when they are available...


================
Comment at: lldb/source/Utility/ProcessInfo.cpp:42
 const char *ProcessInfo::GetName() const {
+  if (m_executable.GetFilename().IsEmpty())
+    return GetArguments().GetArgumentAtIndex(0);
----------------
Can you put this bit into a separate patch? I'd like to get more opinions on whether this is the right place to do this, and I don't want to blur that discussion with the mechanics of passing command line arguments through the gdb-remote protocol.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68293