Page MenuHomePhabricator

[android/process list] support showing process arguments
ClosedPublic

Authored by wallace on Oct 1 2019, 1:28 PM.

Details

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

Diff Detail

Event Timeline

wallace created this revision.Oct 1 2019, 1:28 PM
wallace edited the summary of this revision. (Show Details)Oct 1 2019, 1:31 PM

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.

1961–1965

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 updated this revision to Diff 223061.Oct 3 2019, 11:29 AM
  • 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 updated this revision to Diff 223356.Oct 4 2019, 6:46 PM

added a basic python test

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

wallace marked 2 inline comments as done.Oct 4 2019, 6:47 PM

damn, i overwrote this patch lol

wallace updated this revision to Diff 223376.Oct 5 2019, 10:18 AM

added a test

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
51

?

lldb/source/Host/linux/Host.cpp
159–160 ↗(On Diff #223376)

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
1194

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.

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.

i'm reverting it, thanks!

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

wallace updated this revision to Diff 224949.Oct 14 2019, 7:11 PM

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 :(

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 updated this revision to Diff 225082.Oct 15 2019, 10:57 AM
wallace marked 2 inline comments as done.

Add end to end test

labath added inline comments.Oct 15 2019, 11:07 AM
lldb/docs/lldb-gdb-remote.txt
1456

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
63–66

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

wallace marked 2 inline comments as done.Oct 15 2019, 11:18 AM
wallace added inline comments.
lldb/docs/lldb-gdb-remote.txt
1456

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
63–66

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 added inline comments.Oct 15 2019, 11:38 AM
lldb/packages/Python/lldbsuite/test/functionalities/gdb_remote_client/TestPlatformClient.py
63–66

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 marked an inline comment as done.Oct 15 2019, 12:00 PM
wallace added inline comments.
lldb/packages/Python/lldbsuite/test/functionalities/gdb_remote_client/TestPlatformClient.py
63–66

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

wallace updated this revision to Diff 225097.Oct 15 2019, 12:13 PM

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.

Looks pretty good, just two quick comments.

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

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 marked an inline comment as done.Wed, Oct 16, 10:36 AM
wallace added inline comments.
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
1941

that's indeed a typo...

wallace updated this revision to Diff 225269.Wed, Oct 16, 11:03 AM

addressed comments

labath accepted this revision.Wed, Oct 16, 11:11 AM
This revision is now accepted and ready to land.Wed, Oct 16, 11:11 AM
This revision was automatically updated to reflect the committed changes.