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 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits