[android/process list] support showing process arguments

[wallace]

Summary

The qfProcessInfo and qsProcessInfo packets currently don't set the processes' arguments, however the platform process list -v command tries to print it.
In this diff I'm adding the arguments as part of the packet, and now the command shows the arguments just like on mac.

On Mac:

507    1      wallace    1876110778 wallace    1876110778 x86_64-apple-macosx      /usr/libexec/secd
503    1      wallace    1876110778 wallace    1876110778 x86_64-apple-macosx      /usr/libexec/secinitd
501    1      wallace    1876110778 wallace    1876110778 x86_64-apple-macosx      /usr/libexec/languageassetd --firstLogin
497    1      wallace    1876110778 wallace    1876110778 x86_64-apple-macosx      /usr/libexec/trustd --agent
496    1      wallace    1876110778 wallace    1876110778 x86_64-apple-macosx      /usr/libexec/lsd
494    1      wallace    1876110778 wallace    1876110778 x86_64-apple-macosx      /System/Library/Frameworks/CoreTelephony.framework/Support/CommCenter -L
491    1      wallace    1876110778 wallace    1876110778 x86_64-apple-macosx      /usr/sbin/distnoted agent
489    1      wallace    1876110778 wallace    1876110778 x86_64-apple-macosx      /usr/libexec/UserEventAgent (Aqua)
484    1      wallace    1876110778 wallace    1876110778 x86_64-apple-macosx      /usr/sbin/cfprefsd agent
483    1      wallace    1876110778 wallace    1876110778 x86_64-apple-macosx      /System/Library/Frameworks/LocalAuthentication.framework/Support/coreauthd

On android:

1561   1016   root       0                     0          aarch64-unknown-linux-android  /system/bin/ip6tables-restore--noflush -w -v
1805   982    1000       1000                  1000                                      android:drmService
1811   982    10189      10189                 10189                                     com.qualcomm.embms:remote
1999   1      1000       1000                  1000       aarch64-unknown-linux-android  /system/bin/tlc_serverCCM
2332   982    10038      10038                 10038                                     com.android.systemui
2378   983    1053       1053                  1053                                      webview_zygote
2448   982    5013       5013                  5013                                      com.sec.location.nsflp2
2465   982    10027      10027                 10027                                     com.google.android.gms.persistent

[labath]

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. :)

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

615–616

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.

1962–1966

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].

[wallace]

• Encoding differently the arguments. Now it shouldn't have any corner cases
• I'm also sending from the server all the args, including arg0
• In the client, when displaying the process name, I'm using arg0 as fallback if the exe path is empty
• Fixed formatting

I still need to finish testing

[wallace]

added a basic python test

It was a nice learning experience and fortunately there was no need to add any intrusive code

[wallace]

damn, i overwrote this patch lol

[wallace]

added a test

[labath]

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.

lldb/packages/Python/lldbsuite/test/functionalities/gdb_remote_client/TestPlatformClient.py

29

?

lldb/source/Host/linux/Host.cpp

159–160

This doesn't seem right... What about processes invoked with empty arguments? (my_prog "" "")

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

1932

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?

1934

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

1939

What happens if this fails?

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp

1193

s/.c_str()/.ref(). Generally, always try to use the non-c string apis when they are available...

lldb/source/Utility/ProcessInfo.cpp

42

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.

[aprantl]

This test fails on green dragon:

http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/2538/changes#detail0
http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/2538/testReport/junit/lldb-api/functionalities_gdb_remote_client/TestPlatformClient_py/

lldb-api.functionalities/gdb_remote_client.TestPlatformClient.py (from lldb-api)

Failing for the past 1 build (Since Failed#2538 )
Took 0.66 sec.
add description
Stacktrace

lldb version 10.0.99 (http://labmaster3.local/git/llvm-project.git revision 1edb7e0b6f390b066f5218208a7c8ac974ee243c)
  clang revision 089a334c39d06e958607e1e8e0c9796f9387f512
  llvm revision 089a334c39d06e958607e1e8e0c9796f9387f512
LLDB library dir: /Users/buildslave/jenkins/workspace/lldb-cmake/lldb-build/bin
LLDB import library dir: /Users/buildslave/jenkins/workspace/lldb-cmake/lldb-build/bin
libstdcxx tests will not be run because: Don't know how to build with libstdcxx on macosx
Skipping following debug info categories: ['dwo']

Session logs for test failures/errors/unexpected successes will go into directory '/Users/buildslave/jenkins/workspace/lldb-cmake/test/logs'
Command invoked: /Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/lldb/test/API/dotest.py --arch=x86_64 -s /Users/buildslave/jenkins/workspace/lldb-cmake/lldb-build/lldb-test-traces -S nm -u CXXFLAGS -u CFLAGS --executable /Users/buildslave/jenkins/workspace/lldb-cmake/lldb-build/./bin/lldb --dsymutil /Users/buildslave/jenkins/workspace/lldb-cmake/lldb-build/./bin/dsymutil --filecheck /Users/buildslave/jenkins/workspace/lldb-cmake/lldb-build/./bin/FileCheck -C /Users/buildslave/jenkins/workspace/lldb-cmake/lldb-build/./bin/clang --codesign-identity lldb_codesign --server /Users/buildslave/jenkins/workspace/lldb-cmake/lldb-build/./bin/debugserver --arch x86_64 --build-dir /Users/buildslave/jenkins/workspace/lldb-cmake/lldb-build/lldb-test-build.noindex -s=/Users/buildslave/jenkins/workspace/lldb-cmake/test/logs -t --env TERM=vt100 --env LLVM_LIBS_DIR=/Users/buildslave/jenkins/workspace/lldb-cmake/lldb-build/./lib --build-dir /Users/buildslave/jenkins/workspace/lldb-cmake/lldb-build/tools/lldb/lldb-test-build.noindex --lldb-module-cache-dir /Users/buildslave/jenkins/workspace/lldb-cmake/lldb-build/tools/lldb/lldb-test-build.noindex/module-cache-lldb/lldb-api --clang-module-cache-dir /Users/buildslave/jenkins/workspace/lldb-cmake/lldb-build/tools/lldb/lldb-test-build.noindex/module-cache-clang/lldb-api /Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/lldb/packages/Python/lldbsuite/test/functionalities/gdb_remote_client -p TestPlatformClient.py
Change dir to: /Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/lldb/packages/Python/lldbsuite/test/functionalities/gdb_remote_client
runCmd: settings set symbols.enable-external-lookup false
output: 

runCmd: settings set plugin.process.gdb-remote.packet-timeout 60
output: 

runCmd: settings set symbols.clang-modules-cache-path "/Users/buildslave/jenkins/workspace/lldb-cmake/lldb-build/tools/lldb/lldb-test-build.noindex/module-cache-lldb/lldb-api"
output: 

runCmd: settings set use-color false
output: 

runCmd: platform select remote-linux
output:   Platform: remote-linux
 Connected: no


runCmd: platform connect connect://localhost:52264
output:   Platform: remote-linux
  Hostname: (null)
 Connected: yes
WorkingDir: /


runCmd: platform process list -x
runCmd failed!
error: no processes were found on the "remote-linux" platform


FAIL: LLDB (/Users/buildslave/jenkins/workspace/lldb-cmake/lldb-build/bin/clang-10-x86_64) :: test_process_list_with_all_users (TestPlatformClient.TestPlatformClient)
Restore dir to: /Users/buildslave/jenkins/workspace/lldb-cmake/lldb-build/tools/lldb/test
======================================================================
FAIL: test_process_list_with_all_users (TestPlatformClient.TestPlatformClient)
   Test connecting to a remote linux platform
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/lldb/packages/Python/lldbsuite/test/functionalities/gdb_remote_client/TestPlatformClient.py", line 30, in test_process_list_with_all_users
    substrs=["1 matching process was found", "test_process"])
  File "/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/lldb/packages/Python/lldbsuite/test/lldbtest.py", line 2309, in expect
    inHistory=inHistory)
  File "/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/lldb/packages/Python/lldbsuite/test/lldbtest.py", line 2068, in runCmd
    msg if (msg) else CMD_MSG(cmd))
AssertionError: False is not True : Command 'platform process list -x
Error output:
error: no processes were found on the "remote-linux" platform
' returns successfully
Config=x86_64-/Users/buildslave/jenkins/workspace/lldb-cmake/lldb-build/bin/clang-10
----------------------------------------------------------------------
Ran 1 test in 0.040s

RESULT: FAILED (0 passes, 1 failures, 0 errors, 0 skipped, 0 expected failures, 0 unexpected successes)

Can you please take a look?

If you're not available I'll revert the patch later today to get the bots green again.

[wallace]

i'm reverting it, thanks!

[aprantl]

The re-committed variant still doesn't work :-)

[wallace]

address comments

Btw, @labath, could you point me to a example of a full end to end test like the attach one you mention?
I haven't been able to find it :(

[labath]

In D68293#1708863, @wallace wrote:

address comments

Btw, @labath, could you point me to a example of a full end to end test like the attach one you mention?
I haven't been able to find it :(

I think you can use TestProcessAttach.py as a starting point. If you look at the test_attach_to_process_by_id function, all it does is launch a process, and then immediately attaches to it.

So, what could your test do, is also call self.spawnSubprocess, but also pass it some arguments. Then, instead of attaching, you'd run "platform process list" and verify that the process (and its arguments) are there.

[wallace]

Add end to end test

[labath]

lldb/docs/lldb-gdb-remote.txt

1456 ↗

(On Diff #225082)

Does that include argv[0]? I take it does, but argv[0] is funny, so it's better to be explicit about that.

lldb/packages/Python/lldbsuite/test/functionalities/gdb_remote_client/TestPlatformClient.py

41–44

Is this a negative check? We usually use expect(..., error=True) for that

[wallace]

lldb/docs/lldb-gdb-remote.txt

1456 ↗

(On Diff #225082)

Ohh, I've just read this in ProcessInfo.h

// argv[0] if supported. If empty, then use m_executable.

// Not all process plug-ins support specifying an argv[0] that differs from
// the resolved platform executable (which is in m_executable)
Args m_arguments; // All program arguments except argv[0

I think I have to make some adjustments

lldb/packages/Python/lldbsuite/test/functionalities/gdb_remote_client/TestPlatformClient.py

41–44

It's a negative test, but error=True checks for the success of the command itself, it doesn't check the substrs.
I took at look at the base classes for testing and it seems I could add something like not_contain_substrs, but the code seems very fragile

[labath]

lldb/packages/Python/lldbsuite/test/functionalities/gdb_remote_client/TestPlatformClient.py

41–44

Ok, how about matching=False then? Or maybe you could just put the entirety of the output to expect into the patterns argument, as it should not contain any values which vary from run to run.

[wallace]

lldb/packages/Python/lldbsuite/test/functionalities/gdb_remote_client/TestPlatformClient.py

41–44

damn, i was looking at the wrong implementation of def expect...

[wallace]

Addressed comments. It turns out from the definition of ProcessInfo that Arguments shuoldn't contain arg0, so I updated the code accordingly.
I ran the test suite locally and I didn't see any failure caused due to that, so it should be okay.

[labath]

Looks pretty good, just two quick comments.

lldb/packages/Python/lldbsuite/test/commands/platform/process/TestProcessList.py

15–17 ↗

(On Diff #225097)

You don't need this. This was present in the other test because it was (also) attaching by name, but when attaching by pid, you can do with the default "a.out".

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

1941

Why a single space? I can understand an empty string, but a " " seems very arbitrary...

[wallace]

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

1941

that's indeed a typo...

[wallace]

addressed comments

[stella.stamenova]

This is causing a failure on the Windows Bot:

http://lab.llvm.org:8011/builders/lldb-x64-windows-ninja/builds/9920

lldb/packages/Python/lldbsuite/test/functionalities/gdb_remote_client/TestPlatformClient.py

 import lldb
 import binascii
 import os
 from lldbsuite.test.lldbtest import *
 from lldbsuite.test.decorators import *
 from gdbclientutils import *
 
 
 class TestPlatformClient(GDBRemoteTestBase):
 
     def test_process_list(self):
         """Test connecting to a remote linux platform"""
 
         class MyResponder(MockGDBServerResponder):
             def qfProcessInfo(self, packet):
                 if "all_users:1" in packet:
-                    return "pid:10;ppid:1;uid:2;gid:3;euid:4;egid:5;name:" + binascii.hexlify("/a/process") + ";args:"
+                    name = binascii.hexlify("/a/process")
+                    args = "-".join(map(binascii.hexlify,
+                                        ["/system/bin/sh", "-c", "/data/local/tmp/lldb-server"]))
+                    return "pid:10;ppid:1;uid:2;gid:3;euid:4;egid:5;name:" + name + ";args:" + args + ";"
                 else:
                     return "E04"
 
         self.server.responder = MyResponder()
 
         self.runCmd("platform select remote-linux")
 
         try:
+            self.runCmd("log enable gdb-remote all")
             self.runCmd("platform connect connect://localhost:%d" %
                         self.server.port)
             self.assertTrue(self.dbg.GetSelectedPlatform().IsConnected())
             self.expect("platform process list -x",
                         startstr="1 matching process was found", endstr="process" + os.linesep)
             self.expect("platform process list -xv",
                         substrs=[
                             "PID    PARENT USER       GROUP      EFF USER   EFF GROUP",
-                            "10     1      2          3          4          5"])
+                            "10     1      2          3          4          5",
+                            "/system/bin/sh -c /data/local/tmp/lldb-server"])
             self.expect("platform process list",
                         error="error: no processes were found on the \"remote-linux\" platform")
         finally:
             self.runCmd("platform disconnect")

lldb/source/Host/linux/Host.cpp

static void GetProcessArgs(::pid_t pid, ProcessInstanceInfo &process_info) {
   std::unique_ptr<llvm::MemoryBuffer> Cmdline = std::move(*BufferOrError);
 
   llvm::StringRef Arg0, Rest;
   std::tie(Arg0, Rest) = Cmdline->getBuffer().split('\0');
   process_info.SetArg0(Arg0);
   while (!Rest.empty()) {
     llvm::StringRef Arg;
     std::tie(Arg, Rest) = Rest.split('\0');
+    if (Arg.empty())
+      continue;
     process_info.GetArguments().AppendArgument(Arg);
   }
 }
 
 static void GetExePathAndArch(::pid_t pid, ProcessInstanceInfo &process_info) {
   Log *log(GetLogIfAllCategoriesSet(LIBLLDB_LOG_PROCESS));
   std::string ExePath(PATH_MAX, '\0');
 
static void GetExePathAndArch(::pid_t pid, ProcessInstanceInfo &process_info) {
   }
 }
 
 static void GetProcessEnviron(::pid_t pid, ProcessInstanceInfo &process_info) {
   // Get the process environment.
   auto BufferOrError = getProcFile(pid, "environ");
   if (!BufferOrError)
     return;
- 
   std::unique_ptr<llvm::MemoryBuffer> Environ = std::move(*BufferOrError);
   llvm::StringRef Rest = Environ->getBuffer();
   while (!Rest.empty()) {
     llvm::StringRef Var;
     std::tie(Var, Rest) = Rest.split('\0');
     process_info.GetEnvironment().insert(Var);
   }
 }

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

 }
 
 bool GDBRemoteCommunicationClient::GetLoadedDynamicLibrariesInfosSupported() {
   if (m_supports_jLoadedDynamicLibrariesInfos == eLazyBoolCalculate) {
     StringExtractorGDBRemote response;
     m_supports_jLoadedDynamicLibrariesInfos = eLazyBoolNo;
     if (SendPacketAndWaitForResponse("jGetLoadedDynamicLibrariesInfos:",
                                      response,
                                      false) == PacketResult::Success) {
       if (response.IsOKResponse()) {
         m_supports_jLoadedDynamicLibrariesInfos = eLazyBoolYes;
       }
     }
   }
   return m_supports_jLoadedDynamicLibrariesInfos;
 }
 
 bool GDBRemoteCommunicationClient::GetSharedCacheInfoSupported() {
while (response.GetNameColonValue(name, value)) {
         process_info.GetArchitecture().SetTriple(triple.c_str());
       } else if (name.equals("name")) {
         StringExtractor extractor(value);
         // The process name from ASCII hex bytes since we can't control the
         // characters in a process name
         std::string name;
         extractor.GetHexByteString(name);
         process_info.GetExecutableFile().SetFile(name, FileSpec::Style::native);
+      } else if (name.equals("args")) {
+        StringExtractor extractor(value);
+        std::stringstream stream(value);
+
+        auto extractOneArg = [&](std::string &arg) {
+          std::string hex_arg;
+          if (!getline(stream, hex_arg, '-'))
+            return false;
+          extractor = StringExtractor(hex_arg);
+          extractor.GetHexByteString(arg);
+          return true;
+        };
+
+        std::string arg;
+        if (!extractOneArg(arg))
+          continue;
+        process_info.SetArg0(arg);
+        process_info.GetArguments().AppendArgument(arg);
+
+        while (extractOneArg(arg))
+          process_info.GetArguments().AppendArgument(arg);
       } else if (name.equals("cputype")) {
         value.getAsInteger(0, cpu);
       } else if (name.equals("cpusubtype")) {
         value.getAsInteger(0, sub);
       } else if (name.equals("vendor")) {
         vendor = value;
       } else if (name.equals("ostype")) {
         os_type = value;
       }
     }
 
     if (cpu != LLDB_INVALID_CPUTYPE && !vendor.empty() && !os_type.empty()) {
       if (vendor == "apple") {
         process_info.GetArchitecture().SetArchitecture(eArchTypeMachO, cpu,
                                                        sub);
         process_info.GetArchitecture().GetTriple().setVendorName(
             llvm::StringRef(vendor));
         process_info.GetArchitecture().GetTriple().setOSName(
             llvm::StringRef(os_type));
       }
     }
 
     if (process_info.GetProcessID() != LLDB_INVALID_PROCESS_ID)
       return true;

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp

void GDBRemoteCommunicationServerCommon::CreateProcessInfoResponse(
     const ProcessInstanceInfo &proc_info, StreamString &response) {
   response.Printf(
       "pid:%" PRIu64 ";ppid:%" PRIu64 ";uid:%i;gid:%i;euid:%i;egid:%i;",
       proc_info.GetProcessID(), proc_info.GetParentProcessID(),
       proc_info.GetUserID(), proc_info.GetGroupID(),
       proc_info.GetEffectiveUserID(), proc_info.GetEffectiveGroupID());
   response.PutCString("name:");
   response.PutStringAsRawHex8(proc_info.GetExecutableFile().GetCString());
+
+  response.PutChar(';');
+  response.PutCString("args:");
+  response.PutStringAsRawHex8(proc_info.GetArg0());
+  for (auto &arg : proc_info.GetArguments()) {
+    response.PutChar('-');
+    response.PutStringAsRawHex8(arg.c_str());
+  }
+
   response.PutChar(';');
   const ArchSpec &proc_arch = proc_info.GetArchitecture();
   if (proc_arch.IsValid()) {
     const llvm::Triple &proc_triple = proc_arch.GetTriple();
     response.PutCString("triple:");
     response.PutStringAsRawHex8(proc_triple.getTriple());
     response.PutChar(';');
   }

lldb/source/Utility/ProcessInfo.cpp

void ProcessInfo::Clear() {
   m_environment.clear();
   m_uid = UINT32_MAX;
   m_gid = UINT32_MAX;
   m_arch.Clear();
   m_pid = LLDB_INVALID_PROCESS_ID;
 }
 
 const char *ProcessInfo::GetName() const {
+  if (m_executable.GetFilename().IsEmpty())
+    return GetArguments().GetArgumentAtIndex(0);
   return m_executable.GetFilename().GetCString();
 }
 
 llvm::StringRef ProcessInfo::GetNameAsStringRef() const {
   return m_executable.GetFilename().GetStringRef();
 }
 
 void ProcessInfo::Dump(Stream &s, Platform *platform) const {