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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
