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

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

It seems to me that the Host class is too low level to be encoding some of the android specifics into it. I think it would be better to handle these things higher up. Please see inline comments for details.



================
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();
----------------
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?


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


================
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
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68289