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

[Pavel Labath via Phabricator via lldb-commits]

labath 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();
----------------
wallace wrote:
> 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?
Having "com.my.app" as an executable (more like, process)  name is probably fine. However, having that as an executable _path_ (of type FileSpec!) is streching it too far, I believe. If this function was returning some higher-level concept of a process, which had a "name" as a separate property, then putting argv[0] there would be fine. However, for something of type FileSpec, I think it's better to have nothing rather than something that is definitely _not_ a file.

> the Arg0 fallback is done inside the client when the exe path is empty
agreed, and this doesn't need to be android-specific

> 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.
I am not sure what you mean by that. Can you elaborate?

> the current function, in the case of android, returns a possible empty exe path. For anything not android, empty exe paths are not allowed
yes, and I think we should, like for the case of environment, just permit a ProcessInstanceInfo result with no executable path, regardless of the OS.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68289