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

[Alex Langford via Phabricator via lldb-commits]

bulbazord added inline comments.


================
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
----------------
splhack wrote:
> 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.)
I see. You want to be able to run the android host tests but that's not easy to do right now. I think this is a reasonable thing to want to do (especially since so much of android support in lldb is not well tested AFAIK).

Instead of making this functionality specific to android hosts, why not make it possible to do on all platforms? This would do a few things:
- It would make it easier to test on more than just Linux and Android machines.
- It would open up the possibility of being able to use an apk on the host machine instead of needing to fetch it from the remote device via `adb shell dd`. An optimization for sure, but for large shared objects this may be able to improve performance.

What do you think?


================
Comment at: lldb/source/Host/android/ZipFile.cpp:1
+//===-- ZipFile.cpp -------------------------------------------------------===//
+//
----------------
BTW, I haven't looked at this file too closely yet, but I think a better candidate for its location would be `lldbUtility`. It doesn't rely on anything else from lldb other than something else in `lldbUtility`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152759