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

_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Reply via email to