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

[Alex Langford via Phabricator via lldb-commits]

bulbazord added a comment.
Herald added a subscriber: JDevlieghere.

This is definitely useful to support, I had a few comments mostly about matching lldb's coding style.

Also, are you running clang-format on your patches? There are a few places where I'm not sure if the formatting is right.



================
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
----------------
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.


================
Comment at: lldb/source/Host/android/HostInfoAndroid.cpp:107
+  // #opening-shared-libraries-directly-from-an-apk
+  const std::string kZipSeparator = "!/";
+  auto path = file_spec.GetPath();
----------------
You can avoid using `std::string` here (and invoking the constructor) by using an llvm::StringLiteral. Like so:
```
static constexpr llvm::StringLiteral k_zip_separator("!/");
```


================
Comment at: lldb/source/Host/android/HostInfoAndroid.cpp:108
+  const std::string kZipSeparator = "!/";
+  auto path = file_spec.GetPath();
+  auto pos = path.find(kZipSeparator);
----------------
It's not obvious what the type of `path` is, please write out the type instead of using `auto`.


================
Comment at: lldb/source/Host/android/HostInfoAndroid.cpp:119-126
+  auto zip_path = path.substr(0, pos);
+  auto so_path = path.substr(pos + kZipSeparator.size());
+
+  auto zip_file_spec = FileSpec(zip_path);
+  auto zip_file_size = FileSystem::Instance().GetByteSize(zip_file_spec);
+  auto zip_data = FileSystem::Instance().CreateDataBuffer(zip_file_spec,
+                                                          zip_file_size,
----------------
It's not obvious what the types of these are (except for `zip_file_spec`), please write out the types explicitly.


================
Comment at: lldb/source/Host/android/HostInfoAndroid.cpp:126
+                                                          zip_file_size,
+                                                          0);
+  if (ZipFile::Find(zip_data, so_path, file_offset, file_size)) {
----------------
It's not obvious what 0 is as the parameter here, please make it look something like this:
`/* offset = */ 0);`


================
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';
----------------
Maybe it would be a good idea to centralize the `run-as` logic somewhere, right now it's pretty ad-hoc.


================
Comment at: lldb/unittests/Host/CMakeLists.txt:42
+
+if (CMAKE_SYSTEM_NAME MATCHES "Linux|Android")
+  add_subdirectory(android)
----------------
Why not just match on `"Android"`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152759