[Lldb-commits] [PATCH] D152759: [lldb][Android] Support zip .so file

[Kazuki Sakamoto via Phabricator via lldb-commits]

splhack added a comment.

@bulbazord thanks for reviewing! will address the types, formats.



================
Comment at: lldb/source/Host/CMakeLists.txt:109-121
+
+    set(ANDROID_SOURCES
+      android/ZipFile.cpp
+      android/HostInfoAndroid.cpp
+      )
     if (CMAKE_SYSTEM_NAME MATCHES "Android")
+      list(APPEND ANDROID_SOURCES
----------------
bulbazord wrote:
> I don't think this is correct to do. lldbHost is different than other libraries in that it's meant to provide functionality that lldb and lldb-server needs to work on the host system. Unconditionally adding a host subdirectory for android even when we're on Linux doesn't make sense to do.
I agree with that, however, I think this is pretty much only way to unblock writing and running unit tests for the Android host system, which has been no tests at all. The AndroidPlatformTest D152855 requires this to run the tests on Linux, and Android is basically Linux, so, hope it still makes sense for the unit testing capability. (only android/LibcGlue.cpp is not buildable for Linux target.)


================
Comment at: lldb/source/Plugins/Platform/Android/PlatformAndroid.cpp:261-264
+  if (const char *run_as = std::getenv("ANDROID_PLATFORM_RUN_AS"))
+    snprintf(run_as_cmd, sizeof(run_as_cmd), "run-as '%s' ", run_as);
+  else
+    run_as_cmd[0] = '\0';
----------------
bulbazord wrote:
> Maybe it would be a good idea to centralize the `run-as` logic somewhere, right now it's pretty ad-hoc.
Will look into these run-as things with D152494 centralize and if plugin.platform.android.platform-run-as possible.


================
Comment at: lldb/unittests/Host/CMakeLists.txt:42
+
+if (CMAKE_SYSTEM_NAME MATCHES "Linux|Android")
+  add_subdirectory(android)
----------------
bulbazord wrote:
> Why not just match on `"Android"`?
The reason is, as commented above, to run AndroidPlatformTest D152855 on Linux.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152759