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