[Lldb-commits] [PATCH] D68289: [lldb-server/android] Show more processes and package name when necessary

[walter erquinigo via Phabricator via lldb-commits]

wallace marked 3 inline comments as done.
wallace added inline comments.


================
Comment at: lldb/source/Host/linux/Host.cpp:178-185
     LLDB_LOG(log, "failed to read link exe link for {0}: {1}", pid,
              Status(errno, eErrorTypePOSIX));
-    return false;
+    ExePath.resize(0);
+#if defined(__ANDROID__)
+    // On android we fallback to Arg0, which commonly is an apk package name.
+    if (!process_info.GetArg0().empty()) {
+      ExePath = process_info.GetArg0();
----------------
labath wrote:
> Why does this fail on android exactly? Is it some seccomp thingy? I'm surprised that we're allowed to read `/proc/cmdline`, but not the `/proc/exe` link..
> 
> Anyway, I don't think that pretending that "com.my.app" is an executable file name is a good idea. Maybe it would be better to implement something on the other end. I.e., we print out argv[0]  if the file name is not present?
>From what I've been seeing, /proc/cmdline is always available, even for root processes. On the other hand, /proc/exe is only available for the user that is seeing it. In fact, if you use run-as, you can see the contents of /proc/exe, but that would involve making too many slow calls (run-as is slow).

I do think that having "com.my.app" as executable name is okay, because android doesn't let you interact with an apk by any other means, unless you root your device. When using android commands, "com.my.app" is just like any other process.

I don't have any strong opinions though, so I'm okay with following your suggestion if you prefer so.

Handling the android case from the host by doing exePath =  argv[0] would look like this:

- the Arg0 fallback is done inside the client when the exe path is empty 
- a special additional handling would be required for cases when lldb is running on Android. With the approach of this diff, that case is covered.
- the current function, in the case of android, returns a possible empty exe path. For anything not android, empty exe paths are not allowed


what do you think?


================
Comment at: lldb/source/Host/linux/Host.cpp:213-218
+#if defined(__ANDROID__)
+  // It's okay if we can't get the environment on android
+  return true;
+#else
+  return false;
+#endif
----------------
labath wrote:
> I think we can just make this non-android specific and say that we're ok with returning the results if we were unable to fetch the environment. For instance, you could make the caller not fail hard (but just log or something) if this returns false.
agree!


================
Comment at: lldb/source/Host/linux/Host.cpp:254-260
+#if defined(__ANDROID__)
+        // On android each apk has its own user, so it's better to display all
+        // users instead of the current one.
+        true;
+#else
+        match_info.GetMatchAllUsers();
+#endif
----------------
labath wrote:
> Just from the perspective of the FindProcesses API, I don't think it's reasonable for it to unilaterally ignore the explicit request to limit the set of processes to return. Ideally, this decision would be made higher up (somewhere near the code that actually enables us to attach to processes of other users), and communicated here via the `GetMatchAllUsers` flag. However, that code doesn't exist yet. Maybe we could drop this bit for now? Or implement a flag to the "platform process list" which would allow one to see all processes? Disregarding android, it may still be interesting to see all processes of some remote system, even if one cannot attach to them. And later, once we're actually able to attach to processes of other "users" we can set it up so that this flag becomes the default for android targets?
this is a good plan. I'll implement that flag for now


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68289