Author: Chad Smith Date: 2025-08-13T22:43:45Z New Revision: bcb48aa5b2cfc75967c734a97201e0c91273169d
URL: https://github.com/llvm/llvm-project/commit/bcb48aa5b2cfc75967c734a97201e0c91273169d DIFF: https://github.com/llvm/llvm-project/commit/bcb48aa5b2cfc75967c734a97201e0c91273169d.diff LOG: [lldb] refactor PlatformAndroid and make threadsafe (#145382) ## Problem When the new setting ``` set target.parallel-module-load true ``` was added, lldb began fetching modules from the devices from multiple threads simultaneously. This caused crashes of lldb when debugging on android devices. The top of the stack in the crash look something like this: ``` #0 0x0000555aaf2b27fe llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) (/opt/llvm/bin/lldb-dap+0xb87fe) #1 0x0000555aaf2b0a99 llvm::sys::RunSignalHandlers() (/opt/llvm/bin/lldb-dap+0xb6a99) #2 0x0000555aaf2b2fda SignalHandler(int, siginfo_t*, void*) (/opt/llvm/bin/lldb-dap+0xb8fda) #3 0x00007f9c02444560 __restore_rt /home/engshare/third-party2/glibc/2.34/src/glibc-2.34/signal/../sysdeps/unix/sysv/linux/libc_sigaction.c:13:0 #4 0x00007f9c04ea7707 lldb_private::ConnectionFileDescriptor::Disconnect(lldb_private::Status*) (usr/bin/../lib/liblldb.so.15+0x22a7707) #5 0x00007f9c04ea5b41 lldb_private::ConnectionFileDescriptor::~ConnectionFileDescriptor() (usr/bin/../lib/liblldb.so.15+0x22a5b41) #6 0x00007f9c04ea5c1e lldb_private::ConnectionFileDescriptor::~ConnectionFileDescriptor() (usr/bin/../lib/liblldb.so.15+0x22a5c1e) #7 0x00007f9c052916ff lldb_private::platform_android::AdbClient::SyncService::Stat(lldb_private::FileSpec const&, unsigned int&, unsigned int&, unsigned int&) (usr/bin/../lib/liblldb.so.15+0x26916ff) #8 0x00007f9c0528b9dc lldb_private::platform_android::PlatformAndroid::GetFile(lldb_private::FileSpec const&, lldb_private::FileSpec const&) (usr/bin/../lib/liblldb.so.15+0x268b9dc) ``` Our workaround was to set `set target.parallel-module-load ` to `false` to avoid the crash. ## Background PlatformAndroid creates two different classes with one stateful adb connection shared between the two -- one through AdbClient and another through AdbClient::SyncService. The connection management and state is complex, and seems to be responsible for the segfault we are seeing. The AdbClient code resets these connections at times, and re-establishes connections if they are not active. Similarly, PlatformAndroid caches its SyncService, which uses an AdbClient class, but the SyncService puts its connection into a different 'sync' state that is incompatible with a standard connection. ## Changes in this diff * This diff refactors the code to (hopefully) have clearer ownership of the connection, clearer separation of AdbClient and SyncService by making a new class for clearer separations of concerns, called AdbSyncService. * New unit tests are added * Additional logs were added (see https://github.com/llvm/llvm-project/pull/145382#issuecomment-3055535017 for details) Added: Modified: lldb/source/Plugins/Platform/Android/AdbClient.cpp lldb/source/Plugins/Platform/Android/AdbClient.h lldb/source/Plugins/Platform/Android/PlatformAndroid.cpp lldb/source/Plugins/Platform/Android/PlatformAndroid.h lldb/source/Plugins/Platform/Android/PlatformAndroidRemoteGDBServer.cpp lldb/unittests/Platform/Android/AdbClientTest.cpp lldb/unittests/Platform/Android/PlatformAndroidTest.cpp Removed: ################################################################################ diff --git a/lldb/source/Plugins/Platform/Android/AdbClient.cpp b/lldb/source/Plugins/Platform/Android/AdbClient.cpp index a179260ca15f6..0fbb48a2e16a0 100644 --- a/lldb/source/Plugins/Platform/Android/AdbClient.cpp +++ b/lldb/source/Plugins/Platform/Android/AdbClient.cpp @@ -8,61 +8,48 @@ #include "AdbClient.h" -#include "llvm/ADT/STLExtras.h" -#include "llvm/ADT/SmallVector.h" -#include "llvm/ADT/StringRef.h" -#include "llvm/Support/FileUtilities.h" - #include "lldb/Host/ConnectionFileDescriptor.h" #include "lldb/Host/FileSystem.h" -#include "lldb/Host/PosixApi.h" -#include "lldb/Utility/DataBuffer.h" -#include "lldb/Utility/DataBufferHeap.h" +#include "lldb/Utility/Connection.h" #include "lldb/Utility/DataEncoder.h" #include "lldb/Utility/DataExtractor.h" #include "lldb/Utility/FileSpec.h" +#include "lldb/Utility/LLDBLog.h" +#include "lldb/Utility/Log.h" +#include "lldb/Utility/Status.h" #include "lldb/Utility/StreamString.h" #include "lldb/Utility/Timeout.h" +#include "llvm/ADT/SmallVector.h" +#include "llvm/ADT/StringRef.h" +#include "llvm/Support/FileUtilities.h" +#include <chrono> #include <climits> - -#include <algorithm> #include <cstdlib> #include <fstream> #include <sstream> -// On Windows, transitive dependencies pull in <Windows.h>, which defines a -// macro that clashes with a method name. -#ifdef SendMessage -#undef SendMessage -#endif - using namespace lldb; using namespace lldb_private; using namespace lldb_private::platform_android; using namespace std::chrono; +using namespace llvm; -static const seconds kReadTimeout(20); +static const char *kSocketNamespaceAbstract = "localabstract"; +static const char *kSocketNamespaceFileSystem = "localfilesystem"; +const seconds kReadTimeout(20); static const char *kOKAY = "OKAY"; static const char *kFAIL = "FAIL"; static const char *kDATA = "DATA"; static const char *kDONE = "DONE"; - static const char *kSEND = "SEND"; static const char *kRECV = "RECV"; static const char *kSTAT = "STAT"; - static const size_t kSyncPacketLen = 8; -// Maximum size of a filesync DATA packet. static const size_t kMaxPushData = 2 * 1024; -// Default mode for pushed files. -static const uint32_t kDefaultMode = 0100770; // S_IFREG | S_IRWXU | S_IRWXG - -static const char *kSocketNamespaceAbstract = "localabstract"; -static const char *kSocketNamespaceFileSystem = "localfilesystem"; +static const uint32_t kDefaultMode = 0100770; static Status ReadAllBytes(Connection &conn, void *buffer, size_t size) { - Status error; ConnectionStatus status; char *read_buffer = static_cast<char *>(buffer); @@ -85,86 +72,215 @@ static Status ReadAllBytes(Connection &conn, void *buffer, size_t size) { error = Status::FromErrorStringWithFormat( "Unable to read requested number of bytes. Connection status: %d.", status); + return error; } -Status AdbClient::CreateByDeviceID(const std::string &device_id, - AdbClient &adb) { - Status error; - std::string android_serial; - if (!device_id.empty()) - android_serial = device_id; - else if (const char *env_serial = std::getenv("ANDROID_SERIAL")) - android_serial = env_serial; +static Status ReadAdbMessage(Connection &conn, std::vector<char> &message) { + message.clear(); - if (android_serial.empty()) { - DeviceIDList connected_devices; - error = adb.GetDevices(connected_devices); - if (error.Fail()) - return error; + char buffer[5]; + buffer[4] = 0; + + auto error = ReadAllBytes(conn, buffer, 4); + if (error.Fail()) + return error; + + unsigned int packet_len = 0; + sscanf(buffer, "%x", &packet_len); + + message.resize(packet_len, 0); + error = ReadAllBytes(conn, &message[0], packet_len); + if (error.Fail()) + message.clear(); - if (connected_devices.size() != 1) - return Status::FromErrorStringWithFormat( - "Expected a single connected device, got instead %zu - try " - "setting 'ANDROID_SERIAL'", - connected_devices.size()); - adb.SetDeviceID(connected_devices.front()); - } else { - adb.SetDeviceID(android_serial); - } return error; } -AdbClient::AdbClient() = default; - -AdbClient::AdbClient(const std::string &device_id) : m_device_id(device_id) {} +static Status GetResponseError(Connection &conn, const char *response_id) { + if (strcmp(response_id, kFAIL) != 0) + return Status::FromErrorStringWithFormat( + "Got unexpected response id from adb: \"%s\"", response_id); -AdbClient::~AdbClient() = default; + std::vector<char> error_message; + auto error = ReadAdbMessage(conn, error_message); + if (!error.Success()) + return error; -void AdbClient::SetDeviceID(const std::string &device_id) { - m_device_id = device_id; + std::string error_str(&error_message[0], error_message.size()); + Log *log = GetLog(LLDBLog::Platform); + LLDB_LOGF(log, "ADB error: %s", error_str.c_str()); + return Status(error_str); } -const std::string &AdbClient::GetDeviceID() const { return m_device_id; } +static Status ReadResponseStatus(Connection &conn) { + char response_id[5]; -Status AdbClient::Connect() { + const size_t packet_len = 4; + response_id[packet_len] = 0; + + auto error = ReadAllBytes(conn, response_id, packet_len); + if (error.Fail()) + return error; + + if (strncmp(response_id, kOKAY, packet_len) != 0) + return GetResponseError(conn, response_id); + + return error; +} + +static Status SendAdbMessage(Connection &conn, llvm::StringRef packet) { Status error; - m_conn = std::make_unique<ConnectionFileDescriptor>(); + + char length_buffer[5]; + snprintf(length_buffer, sizeof(length_buffer), "%04x", + static_cast<int>(packet.size())); + + ConnectionStatus status; + + conn.Write(length_buffer, 4, status, &error); + if (error.Fail()) + return error; + + conn.Write(packet.str().c_str(), packet.size(), status, &error); + return error; +} + +static Status ConnectToAdb(Connection &conn) { std::string port = "5037"; - if (const char *env_port = std::getenv("ANDROID_ADB_SERVER_PORT")) { + if (const char *env_port = std::getenv("ANDROID_ADB_SERVER_PORT")) port = env_port; - } std::string uri = "connect://127.0.0.1:" + port; - m_conn->Connect(uri.c_str(), &error); + Log *log = GetLog(LLDBLog::Platform); + LLDB_LOGF(log, "Connecting to ADB server at %s", uri.c_str()); + + Status error; + conn.Connect(uri.c_str(), &error); return error; } -Status AdbClient::GetDevices(DeviceIDList &device_list) { - device_list.clear(); - - auto error = SendMessage("host:devices"); +static Status EnterSyncMode(Connection &conn) { + auto error = SendAdbMessage(conn, "sync:"); if (error.Fail()) return error; - error = ReadResponseStatus(); + return ReadResponseStatus(conn); +} + +static Status SelectTargetDevice(Connection &conn, llvm::StringRef device_id) { + Log *log = GetLog(LLDBLog::Platform); + LLDB_LOG(log, "Selecting device: {0}", device_id); + + std::ostringstream msg; + msg << "host:transport:" << device_id.str(); + + auto error = SendAdbMessage(conn, msg.str()); if (error.Fail()) return error; - std::vector<char> in_buffer; - error = ReadMessage(in_buffer); + return ReadResponseStatus(conn); +} - llvm::StringRef response(&in_buffer[0], in_buffer.size()); - llvm::SmallVector<llvm::StringRef, 4> devices; - response.split(devices, "\n", -1, false); +Expected<std::string> AdbClient::ResolveDeviceID(StringRef device_id) { + StringRef preferred_serial; + if (!device_id.empty()) + preferred_serial = device_id; + else if (const char *env_serial = std::getenv("ANDROID_SERIAL")) + preferred_serial = env_serial; - for (const auto &device : devices) - device_list.push_back(std::string(device.split('\t').first)); + if (preferred_serial.empty()) { + DeviceIDList connected_devices; - // Force disconnect since ADB closes connection after host:devices response - // is sent. - m_conn.reset(); - return error; + auto GetDevices = [](DeviceIDList &device_list) -> Status { + device_list.clear(); + + // Create temporary ADB client for this operation only + auto temp_conn = std::make_unique<ConnectionFileDescriptor>(); + auto error = ConnectToAdb(*temp_conn); + if (error.Fail()) + return error; + + // NOTE: ADB closes the connection after host:devices response. + // The connection is no longer valid + error = SendAdbMessage(*temp_conn, "host:devices"); + if (error.Fail()) + return error; + + error = ReadResponseStatus(*temp_conn); + if (error.Fail()) + return error; + + std::vector<char> in_buffer; + error = ReadAdbMessage(*temp_conn, in_buffer); + + StringRef response(&in_buffer[0], in_buffer.size()); + SmallVector<StringRef, 4> devices; + response.split(devices, "\n", -1, false); + + for (const auto &device : devices) + device_list.push_back(std::string(device.split('\t').first)); + return error; + }; + + Status error = GetDevices(connected_devices); + if (error.Fail()) + return error.ToError(); + + if (connected_devices.size() != 1) + return createStringError( + inconvertibleErrorCode(), + "Expected a single connected device, got instead %zu - try " + "setting 'ANDROID_SERIAL'", + connected_devices.size()); + + std::string resolved_device_id = std::move(connected_devices.front()); + Log *log = GetLog(LLDBLog::Platform); + LLDB_LOGF(log, "AdbClient::ResolveDeviceID Resolved device ID: %s", + resolved_device_id.c_str()); + return resolved_device_id; + } + + std::string resolved_device_id = preferred_serial.str(); + Log *log = GetLog(LLDBLog::Platform); + LLDB_LOGF(log, "AdbClient::ResolveDeviceID Resolved device ID: %s", + resolved_device_id.c_str()); + return resolved_device_id; +} + +AdbClient::AdbClient(llvm::StringRef device_id) : m_device_id(device_id) { + Log *log = GetLog(LLDBLog::Platform); + LLDB_LOGF(log, + "AdbClient::AdbClient(device_id='%s') - Creating AdbClient with " + "device ID", + device_id.str().c_str()); + m_conn = std::make_unique<ConnectionFileDescriptor>(); + Connect(); +} + +AdbClient::AdbClient() { + Log *log = GetLog(LLDBLog::Platform); + LLDB_LOGF( + log, + "AdbClient::AdbClient() - Creating AdbClient with default constructor"); + m_conn = std::make_unique<ConnectionFileDescriptor>(); + Connect(); +} + +AdbClient::~AdbClient() { + Log *log = GetLog(LLDBLog::Platform); + LLDB_LOGF(log, + "AdbClient::~AdbClient() - Destroying AdbClient for device: %s", + m_device_id.c_str()); +} + +llvm::StringRef AdbClient::GetDeviceID() const { return m_device_id; } + +Status AdbClient::Connect() { + if (m_conn->IsConnected()) + return Status(); + + return ConnectToAdb(*m_conn); } Status AdbClient::SetPortForwarding(const uint16_t local_port, @@ -177,7 +293,7 @@ Status AdbClient::SetPortForwarding(const uint16_t local_port, if (error.Fail()) return error; - return ReadResponseStatus(); + return ReadResponseStatus(*m_conn); } Status @@ -196,7 +312,7 @@ AdbClient::SetPortForwarding(const uint16_t local_port, if (error.Fail()) return error; - return ReadResponseStatus(); + return ReadResponseStatus(*m_conn); } Status AdbClient::DeletePortForwarding(const uint16_t local_port) { @@ -207,56 +323,13 @@ Status AdbClient::DeletePortForwarding(const uint16_t local_port) { if (error.Fail()) return error; - return ReadResponseStatus(); -} - -Status AdbClient::SendMessage(const std::string &packet, const bool reconnect) { - Status error; - if (!m_conn || reconnect) { - error = Connect(); - if (error.Fail()) - return error; - } - - char length_buffer[5]; - snprintf(length_buffer, sizeof(length_buffer), "%04x", - static_cast<int>(packet.size())); - - ConnectionStatus status; - - m_conn->Write(length_buffer, 4, status, &error); - if (error.Fail()) - return error; - - m_conn->Write(packet.c_str(), packet.size(), status, &error); - return error; + return ReadResponseStatus(*m_conn); } -Status AdbClient::SendDeviceMessage(const std::string &packet) { +Status AdbClient::SendDeviceMessage(llvm::StringRef packet) { std::ostringstream msg; - msg << "host-serial:" << m_device_id << ":" << packet; - return SendMessage(msg.str()); -} - -Status AdbClient::ReadMessage(std::vector<char> &message) { - message.clear(); - - char buffer[5]; - buffer[4] = 0; - - auto error = ReadAllBytes(buffer, 4); - if (error.Fail()) - return error; - - unsigned int packet_len = 0; - sscanf(buffer, "%x", &packet_len); - - message.resize(packet_len, 0); - error = ReadAllBytes(&message[0], packet_len); - if (error.Fail()) - message.clear(); - - return error; + msg << "host-serial:" << m_device_id << ":" << packet.str(); + return SendAdbMessage(*m_conn, msg.str()); } Status AdbClient::ReadMessageStream(std::vector<char> &message, @@ -264,6 +337,9 @@ Status AdbClient::ReadMessageStream(std::vector<char> &message, auto start = steady_clock::now(); message.clear(); + if (!m_conn) + return Status::FromErrorString("No connection available"); + Status error; lldb::ConnectionStatus status = lldb::eConnectionStatusSuccess; char buffer[1024]; @@ -282,87 +358,22 @@ Status AdbClient::ReadMessageStream(std::vector<char> &message, return error; } -Status AdbClient::ReadResponseStatus() { - char response_id[5]; - - static const size_t packet_len = 4; - response_id[packet_len] = 0; - - auto error = ReadAllBytes(response_id, packet_len); - if (error.Fail()) - return error; - - if (strncmp(response_id, kOKAY, packet_len) != 0) - return GetResponseError(response_id); - - return error; -} - -Status AdbClient::GetResponseError(const char *response_id) { - if (strcmp(response_id, kFAIL) != 0) - return Status::FromErrorStringWithFormat( - "Got unexpected response id from adb: \"%s\"", response_id); - - std::vector<char> error_message; - auto error = ReadMessage(error_message); - if (!error.Success()) - return error; - return Status(std::string(&error_message[0], error_message.size())); -} - -Status AdbClient::SwitchDeviceTransport() { - std::ostringstream msg; - msg << "host:transport:" << m_device_id; - - auto error = SendMessage(msg.str()); - if (error.Fail()) - return error; - - return ReadResponseStatus(); -} - -Status AdbClient::StartSync() { - auto error = SwitchDeviceTransport(); - if (error.Fail()) - return Status::FromErrorStringWithFormat( - "Failed to switch to device transport: %s", error.AsCString()); - - error = Sync(); - if (error.Fail()) - return Status::FromErrorStringWithFormat("Sync failed: %s", - error.AsCString()); - - return error; -} - -Status AdbClient::Sync() { - auto error = SendMessage("sync:", false); - if (error.Fail()) - return error; - - return ReadResponseStatus(); -} - -Status AdbClient::ReadAllBytes(void *buffer, size_t size) { - return ::ReadAllBytes(*m_conn, buffer, size); -} - Status AdbClient::internalShell(const char *command, milliseconds timeout, std::vector<char> &output_buf) { output_buf.clear(); - auto error = SwitchDeviceTransport(); + auto error = SelectTargetDevice(*m_conn, m_device_id); if (error.Fail()) return Status::FromErrorStringWithFormat( - "Failed to switch to device transport: %s", error.AsCString()); + "Failed to select target device: %s", error.AsCString()); StreamString adb_command; adb_command.Printf("shell:%s", command); - error = SendMessage(std::string(adb_command.GetString()), false); + error = SendAdbMessage(*m_conn, std::string(adb_command.GetString())); if (error.Fail()) return error; - error = ReadResponseStatus(); + error = ReadResponseStatus(*m_conn); if (error.Fail()) return error; @@ -417,18 +428,8 @@ Status AdbClient::ShellToFile(const char *command, milliseconds timeout, return Status(); } -std::unique_ptr<AdbClient::SyncService> -AdbClient::GetSyncService(Status &error) { - std::unique_ptr<SyncService> sync_service; - error = StartSync(); - if (error.Success()) - sync_service.reset(new SyncService(std::move(m_conn))); - - return sync_service; -} - -Status AdbClient::SyncService::internalPullFile(const FileSpec &remote_file, - const FileSpec &local_file) { +Status AdbSyncService::PullFileImpl(const FileSpec &remote_file, + const FileSpec &local_file) { const auto local_file_path = local_file.GetPath(); llvm::FileRemover local_file_remover(local_file_path); @@ -462,8 +463,8 @@ Status AdbClient::SyncService::internalPullFile(const FileSpec &remote_file, return error; } -Status AdbClient::SyncService::internalPushFile(const FileSpec &local_file, - const FileSpec &remote_file) { +Status AdbSyncService::PushFileImpl(const FileSpec &local_file, + const FileSpec &remote_file) { const auto local_file_path(local_file.GetPath()); std::ifstream src(local_file_path.c_str(), std::ios::in | std::ios::binary); if (!src.is_open()) @@ -487,7 +488,9 @@ Status AdbClient::SyncService::internalPushFile(const FileSpec &local_file, error.AsCString()); } error = SendSyncRequest( - kDONE, llvm::sys::toTimeT(FileSystem::Instance().GetModificationTime(local_file)), + kDONE, + llvm::sys::toTimeT( + FileSystem::Instance().GetModificationTime(local_file)), nullptr); if (error.Fail()) return error; @@ -500,7 +503,7 @@ Status AdbClient::SyncService::internalPushFile(const FileSpec &local_file, error.AsCString()); if (response_id == kFAIL) { std::string error_message(data_len, 0); - error = ReadAllBytes(&error_message[0], data_len); + error = ReadAllBytes(*m_conn, &error_message[0], data_len); if (error.Fail()) return Status::FromErrorStringWithFormat( "Failed to read DONE error message: %s", error.AsCString()); @@ -518,9 +521,8 @@ Status AdbClient::SyncService::internalPushFile(const FileSpec &local_file, return error; } -Status AdbClient::SyncService::internalStat(const FileSpec &remote_file, - uint32_t &mode, uint32_t &size, - uint32_t &mtime) { +Status AdbSyncService::StatImpl(const FileSpec &remote_file, uint32_t &mode, + uint32_t &size, uint32_t &mtime) { const std::string remote_file_path(remote_file.GetPath(false)); auto error = SendSyncRequest(kSTAT, remote_file_path.length(), remote_file_path.c_str()); @@ -532,7 +534,7 @@ Status AdbClient::SyncService::internalStat(const FileSpec &remote_file, static const size_t response_len = stat_len + (sizeof(uint32_t) * 3); std::vector<char> buffer(response_len); - error = ReadAllBytes(&buffer[0], buffer.size()); + error = ReadAllBytes(*m_conn, &buffer[0], buffer.size()); if (error.Fail()) return Status::FromErrorStringWithFormat("Failed to read response: %s", error.AsCString()); @@ -555,51 +557,57 @@ Status AdbClient::SyncService::internalStat(const FileSpec &remote_file, return Status(); } -Status AdbClient::SyncService::PullFile(const FileSpec &remote_file, - const FileSpec &local_file) { - return executeCommand([this, &remote_file, &local_file]() { - return internalPullFile(remote_file, local_file); +Status AdbSyncService::PullFile(const FileSpec &remote_file, + const FileSpec &local_file) { + return ExecuteCommand([this, &remote_file, &local_file]() { + return PullFileImpl(remote_file, local_file); }); } -Status AdbClient::SyncService::PushFile(const FileSpec &local_file, - const FileSpec &remote_file) { - return executeCommand([this, &local_file, &remote_file]() { - return internalPushFile(local_file, remote_file); +Status AdbSyncService::PushFile(const FileSpec &local_file, + const FileSpec &remote_file) { + return ExecuteCommand([this, &local_file, &remote_file]() { + return PushFileImpl(local_file, remote_file); }); } -Status AdbClient::SyncService::Stat(const FileSpec &remote_file, uint32_t &mode, - uint32_t &size, uint32_t &mtime) { - return executeCommand([this, &remote_file, &mode, &size, &mtime]() { - return internalStat(remote_file, mode, size, mtime); +Status AdbSyncService::Stat(const FileSpec &remote_file, uint32_t &mode, + uint32_t &size, uint32_t &mtime) { + return ExecuteCommand([this, &remote_file, &mode, &size, &mtime]() { + return StatImpl(remote_file, mode, size, mtime); }); } -bool AdbClient::SyncService::IsConnected() const { +bool AdbSyncService::IsConnected() const { return m_conn && m_conn->IsConnected(); } -AdbClient::SyncService::SyncService(std::unique_ptr<Connection> &&conn) - : m_conn(std::move(conn)) {} - -Status -AdbClient::SyncService::executeCommand(const std::function<Status()> &cmd) { - if (!m_conn) - return Status::FromErrorString("SyncService is disconnected"); +AdbSyncService::AdbSyncService(const std::string device_id) + : m_device_id(device_id) { + m_conn = std::make_unique<ConnectionFileDescriptor>(); + Log *log = GetLog(LLDBLog::Platform); + LLDB_LOGF(log, + "AdbSyncService::AdbSyncService() - Creating AdbSyncService for " + "device: %s", + m_device_id.c_str()); +} +Status AdbSyncService::ExecuteCommand(const std::function<Status()> &cmd) { Status error = cmd(); - if (error.Fail()) - m_conn.reset(); - return error; } -AdbClient::SyncService::~SyncService() = default; +AdbSyncService::~AdbSyncService() { + Log *log = GetLog(LLDBLog::Platform); + LLDB_LOGF(log, + "AdbSyncService::~AdbSyncService() - Destroying AdbSyncService for " + "device: %s", + m_device_id.c_str()); +} -Status AdbClient::SyncService::SendSyncRequest(const char *request_id, - const uint32_t data_len, - const void *data) { +Status AdbSyncService::SendSyncRequest(const char *request_id, + const uint32_t data_len, + const void *data) { DataEncoder encoder(eByteOrderLittle, sizeof(void *)); encoder.AppendData(llvm::StringRef(request_id)); encoder.AppendU32(data_len); @@ -615,11 +623,11 @@ Status AdbClient::SyncService::SendSyncRequest(const char *request_id, return error; } -Status AdbClient::SyncService::ReadSyncHeader(std::string &response_id, - uint32_t &data_len) { +Status AdbSyncService::ReadSyncHeader(std::string &response_id, + uint32_t &data_len) { char buffer[kSyncPacketLen]; - auto error = ReadAllBytes(buffer, kSyncPacketLen); + auto error = ReadAllBytes(*m_conn, buffer, kSyncPacketLen); if (error.Success()) { response_id.assign(&buffer[0], 4); DataExtractor extractor(&buffer[4], 4, eByteOrderLittle, sizeof(void *)); @@ -630,8 +638,7 @@ Status AdbClient::SyncService::ReadSyncHeader(std::string &response_id, return error; } -Status AdbClient::SyncService::PullFileChunk(std::vector<char> &buffer, - bool &eof) { +Status AdbSyncService::PullFileChunk(std::vector<char> &buffer, bool &eof) { buffer.clear(); std::string response_id; @@ -642,14 +649,14 @@ Status AdbClient::SyncService::PullFileChunk(std::vector<char> &buffer, if (response_id == kDATA) { buffer.resize(data_len, 0); - error = ReadAllBytes(&buffer[0], data_len); + error = ReadAllBytes(*m_conn, &buffer[0], data_len); if (error.Fail()) buffer.clear(); } else if (response_id == kDONE) { eof = true; } else if (response_id == kFAIL) { std::string error_message(data_len, 0); - error = ReadAllBytes(&error_message[0], data_len); + error = ReadAllBytes(*m_conn, &error_message[0], data_len); if (error.Fail()) return Status::FromErrorStringWithFormat( "Failed to read pull error message: %s", error.AsCString()); @@ -662,6 +669,15 @@ Status AdbClient::SyncService::PullFileChunk(std::vector<char> &buffer, return Status(); } -Status AdbClient::SyncService::ReadAllBytes(void *buffer, size_t size) { - return ::ReadAllBytes(*m_conn, buffer, size); +Status AdbSyncService::SetupSyncConnection() { + Status error = ConnectToAdb(*m_conn); + if (error.Fail()) + return error; + + error = SelectTargetDevice(*m_conn, m_device_id); + if (error.Fail()) + return error; + + error = EnterSyncMode(*m_conn); + return error; } diff --git a/lldb/source/Plugins/Platform/Android/AdbClient.h b/lldb/source/Plugins/Platform/Android/AdbClient.h index 851c09957bd4a..341a9fa4b93ad 100644 --- a/lldb/source/Plugins/Platform/Android/AdbClient.h +++ b/lldb/source/Plugins/Platform/Android/AdbClient.h @@ -10,6 +10,7 @@ #define LLDB_SOURCE_PLUGINS_PLATFORM_ANDROID_ADBCLIENT_H #include "lldb/Utility/Status.h" +#include "llvm/Support/Error.h" #include <chrono> #include <functional> #include <list> @@ -32,59 +33,21 @@ class AdbClient { using DeviceIDList = std::list<std::string>; - class SyncService { - friend class AdbClient; - - public: - virtual ~SyncService(); - - virtual Status PullFile(const FileSpec &remote_file, - const FileSpec &local_file); - - Status PushFile(const FileSpec &local_file, const FileSpec &remote_file); - - virtual Status Stat(const FileSpec &remote_file, uint32_t &mode, - uint32_t &size, uint32_t &mtime); - - bool IsConnected() const; - - protected: - explicit SyncService(std::unique_ptr<Connection> &&conn); - - private: - Status SendSyncRequest(const char *request_id, const uint32_t data_len, - const void *data); - - Status ReadSyncHeader(std::string &response_id, uint32_t &data_len); - - Status PullFileChunk(std::vector<char> &buffer, bool &eof); - - Status ReadAllBytes(void *buffer, size_t size); - - Status internalPullFile(const FileSpec &remote_file, - const FileSpec &local_file); - - Status internalPushFile(const FileSpec &local_file, - const FileSpec &remote_file); - - Status internalStat(const FileSpec &remote_file, uint32_t &mode, - uint32_t &size, uint32_t &mtime); - - Status executeCommand(const std::function<Status()> &cmd); - - std::unique_ptr<Connection> m_conn; - }; - - static Status CreateByDeviceID(const std::string &device_id, AdbClient &adb); + /// Resolves a device identifier to its canonical form. + /// + /// \param device_id the device identifier to resolve (may be empty). + /// + /// \returns Expected<std::string> containing the resolved device ID on + /// success, or an Error if the device ID cannot be resolved or + /// is ambiguous. + static llvm::Expected<std::string> ResolveDeviceID(llvm::StringRef device_id); AdbClient(); - explicit AdbClient(const std::string &device_id); + explicit AdbClient(llvm::StringRef device_id); virtual ~AdbClient(); - const std::string &GetDeviceID() const; - - Status GetDevices(DeviceIDList &device_list); + llvm::StringRef GetDeviceID() const; Status SetPortForwarding(const uint16_t local_port, const uint16_t remote_port); @@ -102,39 +65,50 @@ class AdbClient { std::chrono::milliseconds timeout, const FileSpec &output_file_spec); - virtual std::unique_ptr<SyncService> GetSyncService(Status &error); - - Status SwitchDeviceTransport(); - -private: Status Connect(); - void SetDeviceID(const std::string &device_id); - - Status SendMessage(const std::string &packet, const bool reconnect = true); - - Status SendDeviceMessage(const std::string &packet); - - Status ReadMessage(std::vector<char> &message); +private: + Status SendDeviceMessage(llvm::StringRef packet); Status ReadMessageStream(std::vector<char> &message, std::chrono::milliseconds timeout); - Status GetResponseError(const char *response_id); + Status internalShell(const char *command, std::chrono::milliseconds timeout, + std::vector<char> &output_buf); - Status ReadResponseStatus(); + std::string m_device_id; + std::unique_ptr<Connection> m_conn; +}; - Status Sync(); +class AdbSyncService { +public: + explicit AdbSyncService(const std::string device_id); + virtual ~AdbSyncService(); + Status SetupSyncConnection(); - Status StartSync(); + virtual Status PullFile(const FileSpec &remote_file, + const FileSpec &local_file); + virtual Status PushFile(const FileSpec &local_file, + const FileSpec &remote_file); + virtual Status Stat(const FileSpec &remote_file, uint32_t &mode, + uint32_t &size, uint32_t &mtime); + virtual bool IsConnected() const; - Status internalShell(const char *command, std::chrono::milliseconds timeout, - std::vector<char> &output_buf); + llvm::StringRef GetDeviceId() const { return m_device_id; } - Status ReadAllBytes(void *buffer, size_t size); +private: + Status SendSyncRequest(const char *request_id, const uint32_t data_len, + const void *data); + Status ReadSyncHeader(std::string &response_id, uint32_t &data_len); + Status PullFileChunk(std::vector<char> &buffer, bool &eof); + Status PullFileImpl(const FileSpec &remote_file, const FileSpec &local_file); + Status PushFileImpl(const FileSpec &local_file, const FileSpec &remote_file); + Status StatImpl(const FileSpec &remote_file, uint32_t &mode, uint32_t &size, + uint32_t &mtime); + Status ExecuteCommand(const std::function<Status()> &cmd); - std::string m_device_id; std::unique_ptr<Connection> m_conn; + std::string m_device_id; }; } // namespace platform_android diff --git a/lldb/source/Plugins/Platform/Android/PlatformAndroid.cpp b/lldb/source/Plugins/Platform/Android/PlatformAndroid.cpp index 5bc9cc133fbd3..600cc0a04cd22 100644 --- a/lldb/source/Plugins/Platform/Android/PlatformAndroid.cpp +++ b/lldb/source/Plugins/Platform/Android/PlatformAndroid.cpp @@ -9,10 +9,8 @@ #include "lldb/Core/Module.h" #include "lldb/Core/PluginManager.h" #include "lldb/Core/Section.h" -#include "lldb/Host/HostInfo.h" #include "lldb/Utility/LLDBLog.h" #include "lldb/Utility/Log.h" -#include "lldb/Utility/Scalar.h" #include "lldb/Utility/UriParser.h" #include "lldb/ValueObject/ValueObject.h" @@ -194,12 +192,10 @@ Status PlatformAndroid::ConnectRemote(Args &args) { auto error = PlatformLinux::ConnectRemote(args); if (error.Success()) { - AdbClient adb; - error = AdbClient::CreateByDeviceID(m_device_id, adb); - if (error.Fail()) - return error; - - m_device_id = adb.GetDeviceID(); + auto resolved_device_id_or_error = AdbClient::ResolveDeviceID(m_device_id); + if (!resolved_device_id_or_error) + return Status::FromError(resolved_device_id_or_error.takeError()); + m_device_id = *resolved_device_id_or_error; } return error; } @@ -216,29 +212,33 @@ Status PlatformAndroid::GetFile(const FileSpec &source, Status error; auto sync_service = GetSyncService(error); - if (error.Fail()) - return error; - - uint32_t mode = 0, size = 0, mtime = 0; - error = sync_service->Stat(source_spec, mode, size, mtime); - if (error.Fail()) - return error; - if (mode != 0) - return sync_service->PullFile(source_spec, destination); + // If sync service is available, try to use it + if (error.Success() && sync_service) { + uint32_t mode = 0, size = 0, mtime = 0; + error = sync_service->Stat(source_spec, mode, size, mtime); + if (error.Success()) { + if (mode != 0) + return sync_service->PullFile(source_spec, destination); + + // mode == 0 can signify that adbd cannot access the file due security + // constraints - fall through to try "cat ..." as a fallback. + Log *log = GetLog(LLDBLog::Platform); + LLDB_LOGF(log, "Got mode == 0 on '%s': try to get file via 'shell cat'", + source_spec.GetPath(false).c_str()); + } + } + // Fallback to shell cat command if sync service failed or returned mode == 0 std::string source_file = source_spec.GetPath(false); Log *log = GetLog(LLDBLog::Platform); - LLDB_LOGF(log, "Got mode == 0 on '%s': try to get file via 'shell cat'", - source_file.c_str()); + LLDB_LOGF(log, "Using shell cat fallback for '%s'", source_file.c_str()); if (strchr(source_file.c_str(), '\'') != nullptr) return Status::FromErrorString( "Doesn't support single-quotes in filenames"); - // mode == 0 can signify that adbd cannot access the file due security - // constraints - try "cat ..." as a fallback. AdbClientUP adb(GetAdbClient(error)); if (error.Fail()) return error; @@ -275,12 +275,19 @@ Status PlatformAndroid::DownloadModuleSlice(const FileSpec &src_file_spec, const uint64_t src_offset, const uint64_t src_size, const FileSpec &dst_file_spec) { + std::string source_file = src_file_spec.GetPath(false); + if (source_file.empty()) + return Status::FromErrorString("Source file path cannot be empty"); + + std::string destination_file = dst_file_spec.GetPath(false); + if (destination_file.empty()) + return Status::FromErrorString("Destination file path cannot be empty"); + // In Android API level 23 and above, dynamic loader is able to load .so // file directly from APK. In that case, src_offset will be an non-zero. if (src_offset == 0) // Use GetFile for a normal file. return GetFile(src_file_spec, dst_file_spec); - std::string source_file = src_file_spec.GetPath(false); if (source_file.find('\'') != std::string::npos) return Status::FromErrorString( "Doesn't support single-quotes in filenames"); @@ -424,7 +431,7 @@ PlatformAndroid::GetLibdlFunctionDeclarations(lldb_private::Process *process) { std::vector<const char *> dl_open_names = {"__dl_dlopen", "dlopen"}; const char *dl_open_name = nullptr; Target &target = process->GetTarget(); - for (auto name : dl_open_names) { + for (auto *name : dl_open_names) { target.GetImages().FindFunctionSymbols( ConstString(name), eFunctionNameTypeFull, matching_symbols); if (matching_symbols.GetSize()) { @@ -445,11 +452,8 @@ PlatformAndroid::GetLibdlFunctionDeclarations(lldb_private::Process *process) { } PlatformAndroid::AdbClientUP PlatformAndroid::GetAdbClient(Status &error) { - AdbClientUP adb(std::make_unique<AdbClient>(m_device_id)); - if (adb) - error.Clear(); - else - error = Status::FromErrorString("Failed to create AdbClient"); + AdbClientUP adb = std::make_unique<AdbClient>(m_device_id); + error = adb->Connect(); return adb; } @@ -473,14 +477,10 @@ std::string PlatformAndroid::GetRunAs() { } return run_as.str(); } - -AdbClient::SyncService *PlatformAndroid::GetSyncService(Status &error) { - if (m_adb_sync_svc && m_adb_sync_svc->IsConnected()) - return m_adb_sync_svc.get(); - - AdbClientUP adb(GetAdbClient(error)); +std::unique_ptr<AdbSyncService> PlatformAndroid::GetSyncService(Status &error) { + auto sync_service = std::make_unique<AdbSyncService>(m_device_id); + error = sync_service->SetupSyncConnection(); if (error.Fail()) return nullptr; - m_adb_sync_svc = adb->GetSyncService(error); - return (error.Success()) ? m_adb_sync_svc.get() : nullptr; + return sync_service; } diff --git a/lldb/source/Plugins/Platform/Android/PlatformAndroid.h b/lldb/source/Plugins/Platform/Android/PlatformAndroid.h index 5602edf73c1d3..3384525362ecf 100644 --- a/lldb/source/Plugins/Platform/Android/PlatformAndroid.h +++ b/lldb/source/Plugins/Platform/Android/PlatformAndroid.h @@ -75,14 +75,15 @@ class PlatformAndroid : public platform_linux::PlatformLinux { typedef std::unique_ptr<AdbClient> AdbClientUP; virtual AdbClientUP GetAdbClient(Status &error); + std::string GetRunAs(); + +public: virtual llvm::StringRef GetPropertyPackageName(); - std::string GetRunAs(); +protected: + virtual std::unique_ptr<AdbSyncService> GetSyncService(Status &error); private: - AdbClient::SyncService *GetSyncService(Status &error); - - std::unique_ptr<AdbClient::SyncService> m_adb_sync_svc; std::string m_device_id; uint32_t m_sdk_version; }; diff --git a/lldb/source/Plugins/Platform/Android/PlatformAndroidRemoteGDBServer.cpp b/lldb/source/Plugins/Platform/Android/PlatformAndroidRemoteGDBServer.cpp index 0cf64807ec0d6..461ee8e3b1826 100644 --- a/lldb/source/Plugins/Platform/Android/PlatformAndroidRemoteGDBServer.cpp +++ b/lldb/source/Plugins/Platform/Android/PlatformAndroidRemoteGDBServer.cpp @@ -21,6 +21,7 @@ using namespace lldb; using namespace lldb_private; using namespace platform_android; +using namespace llvm; static const lldb::pid_t g_remote_platform_pid = 0; // Alias for the process id of lldb-platform @@ -32,12 +33,12 @@ static Status ForwardPortWithAdb( std::string &device_id) { Log *log = GetLog(LLDBLog::Platform); - AdbClient adb; - auto error = AdbClient::CreateByDeviceID(device_id, adb); - if (error.Fail()) - return error; + auto resolved_device_id_or_error = AdbClient::ResolveDeviceID(device_id); + if (!resolved_device_id_or_error) + return Status::FromError(resolved_device_id_or_error.takeError()); + device_id = *resolved_device_id_or_error; - device_id = adb.GetDeviceID(); + AdbClient adb(device_id); LLDB_LOGF(log, "Connected to Android device \"%s\"", device_id.c_str()); if (remote_port != 0) { diff --git a/lldb/unittests/Platform/Android/AdbClientTest.cpp b/lldb/unittests/Platform/Android/AdbClientTest.cpp index 0808b96f69fc8..719b7ca63801b 100644 --- a/lldb/unittests/Platform/Android/AdbClientTest.cpp +++ b/lldb/unittests/Platform/Android/AdbClientTest.cpp @@ -6,8 +6,11 @@ // //===----------------------------------------------------------------------===// -#include "gtest/gtest.h" #include "Plugins/Platform/Android/AdbClient.h" +#include "lldb/Host/Socket.h" +#include "lldb/Host/common/TCPSocket.h" +#include "gtest/gtest.h" +#include <chrono> #include <cstdlib> static void set_env(const char *var, const char *value) { @@ -20,32 +23,117 @@ static void set_env(const char *var, const char *value) { using namespace lldb; using namespace lldb_private; - -namespace lldb_private { -namespace platform_android { +using namespace lldb_private::platform_android; class AdbClientTest : public ::testing::Test { public: - void SetUp() override { set_env("ANDROID_SERIAL", ""); } + void SetUp() override { + set_env("ANDROID_SERIAL", ""); + set_env("ANDROID_ADB_SERVER_PORT", ""); + } - void TearDown() override { set_env("ANDROID_SERIAL", ""); } + void TearDown() override { + set_env("ANDROID_SERIAL", ""); + set_env("ANDROID_ADB_SERVER_PORT", ""); + } }; -TEST(AdbClientTest, CreateByDeviceId) { - AdbClient adb; - Status error = AdbClient::CreateByDeviceID("device1", adb); - EXPECT_TRUE(error.Success()); - EXPECT_EQ("device1", adb.GetDeviceID()); +TEST_F(AdbClientTest, ResolveDeviceId_ExplicitDeviceId) { + auto result = AdbClient::ResolveDeviceID("device1"); + EXPECT_TRUE(static_cast<bool>(result)); + EXPECT_EQ("device1", *result); } -TEST(AdbClientTest, CreateByDeviceId_ByEnvVar) { +TEST_F(AdbClientTest, ResolveDeviceId_ByEnvVar) { set_env("ANDROID_SERIAL", "device2"); - AdbClient adb; - Status error = AdbClient::CreateByDeviceID("", adb); - EXPECT_TRUE(error.Success()); - EXPECT_EQ("device2", adb.GetDeviceID()); + auto result = AdbClient::ResolveDeviceID(""); + EXPECT_TRUE(static_cast<bool>(result)); + EXPECT_EQ("device2", *result); +} + +TEST_F(AdbClientTest, ResolveDeviceId_PrefersExplicitOverEnvVar) { + set_env("ANDROID_SERIAL", "env_device"); + + // Explicit device ID should take precedence over environment variable + auto result = AdbClient::ResolveDeviceID("explicit_device"); + EXPECT_TRUE(static_cast<bool>(result)); + EXPECT_EQ("explicit_device", *result); +} + +TEST_F(AdbClientTest, AdbClient_Constructor_StoresDeviceId) { + AdbClient client("test_device_123"); + EXPECT_EQ(client.GetDeviceID(), "test_device_123"); +} + +TEST_F(AdbClientTest, AdbClient_DefaultConstructor) { + AdbClient client; + EXPECT_EQ(client.GetDeviceID(), ""); } -} // end namespace platform_android -} // end namespace lldb_private +TEST_F(AdbClientTest, AdbSyncService_Constructor_StoresDeviceId) { + AdbSyncService sync("device123"); + EXPECT_EQ(sync.GetDeviceId(), "device123"); +} + +TEST_F(AdbClientTest, AdbSyncService_OperationsFailWhenNotConnected) { + AdbSyncService sync_service("test_device"); + + // Verify service is not connected initially + EXPECT_FALSE(sync_service.IsConnected()); + + // File operations should fail when not connected + FileSpec remote_file("/data/test.txt"); + FileSpec local_file("/tmp/test.txt"); + uint32_t mode, size, mtime; + + Status stat_result = sync_service.Stat(remote_file, mode, size, mtime); + EXPECT_TRUE(stat_result.Fail()); + + Status pull_result = sync_service.PullFile(remote_file, local_file); + EXPECT_TRUE(pull_result.Fail()); + + Status push_result = sync_service.PushFile(local_file, remote_file); + EXPECT_TRUE(push_result.Fail()); +} + +static uint16_t FindUnusedPort() { + auto temp_socket = std::make_unique<TCPSocket>(true); + Status error = temp_socket->Listen("localhost:0", 1); + if (error.Fail()) { + return 0; // fallback + } + uint16_t port = temp_socket->GetLocalPortNumber(); + temp_socket.reset(); // Close the socket to free the port + return port; +} + +TEST_F(AdbClientTest, RealTcpConnection) { + uint16_t unused_port = FindUnusedPort(); + ASSERT_NE(unused_port, 0) << "Failed to find an unused port"; + + std::string port_str = std::to_string(unused_port); + setenv("ANDROID_ADB_SERVER_PORT", port_str.c_str(), 1); + + AdbClient client; + const auto status1 = client.Connect(); + EXPECT_FALSE(status1.Success()) + << "Connection should fail when no server is listening on port " + << unused_port; + + // now start a server on the port and try again + auto listen_socket = std::make_unique<TCPSocket>(true); + std::string listen_address = "localhost:" + port_str; + Status error = listen_socket->Listen(listen_address.c_str(), 5); + ASSERT_TRUE(error.Success()) << "Failed to create listening socket on port " + << unused_port << ": " << error.AsCString(); + + // Verify the socket is listening on the expected port + ASSERT_EQ(listen_socket->GetLocalPortNumber(), unused_port) + << "Socket is not listening on the expected port"; + + const auto status2 = client.Connect(); + EXPECT_TRUE(status2.Success()) + << "Connection should succeed when server is listening on port " + << unused_port; +} diff --git a/lldb/unittests/Platform/Android/PlatformAndroidTest.cpp b/lldb/unittests/Platform/Android/PlatformAndroidTest.cpp index d021562d94d28..514bce1c71576 100644 --- a/lldb/unittests/Platform/Android/PlatformAndroidTest.cpp +++ b/lldb/unittests/Platform/Android/PlatformAndroidTest.cpp @@ -8,8 +8,6 @@ #include "Plugins/Platform/Android/PlatformAndroid.h" #include "Plugins/Platform/Android/PlatformAndroidRemoteGDBServer.h" -#include "TestingSupport/SubsystemRAII.h" -#include "TestingSupport/TestUtilities.h" #include "lldb/Utility/Connection.h" #include "gmock/gmock.h" @@ -20,212 +18,281 @@ using namespace testing; namespace { -class MockSyncService : public AdbClient::SyncService { -public: - MockSyncService() : SyncService(std::unique_ptr<Connection>()) {} - - MOCK_METHOD2(PullFile, - Status(const FileSpec &remote_file, const FileSpec &local_file)); - MOCK_METHOD4(Stat, Status(const FileSpec &remote_file, uint32_t &mode, - uint32_t &size, uint32_t &mtime)); -}; - -typedef std::unique_ptr<AdbClient::SyncService> SyncServiceUP; - class MockAdbClient : public AdbClient { public: - explicit MockAdbClient() : AdbClient("mock") {} + explicit MockAdbClient() : AdbClient() {} MOCK_METHOD3(ShellToFile, Status(const char *command, std::chrono::milliseconds timeout, const FileSpec &output_file_spec)); - MOCK_METHOD1(GetSyncService, SyncServiceUP(Status &error)); }; class PlatformAndroidTest : public PlatformAndroid, public ::testing::Test { public: PlatformAndroidTest() : PlatformAndroid(false) { m_remote_platform_sp = PlatformSP(new PlatformAndroidRemoteGDBServer()); + + // Set up default mock behavior to avoid uninteresting call warnings + ON_CALL(*this, GetSyncService(_)) + .WillByDefault([](Status &error) -> std::unique_ptr<AdbSyncService> { + error = Status::FromErrorString("Sync service unavailable"); + return nullptr; + }); } MOCK_METHOD1(GetAdbClient, AdbClientUP(Status &error)); MOCK_METHOD0(GetPropertyPackageName, llvm::StringRef()); + MOCK_METHOD1(GetSyncService, std::unique_ptr<AdbSyncService>(Status &error)); + + // Make GetSyncService public for testing + using PlatformAndroid::GetSyncService; }; } // namespace -TEST_F(PlatformAndroidTest, DownloadModuleSliceWithAdbClientError) { +TEST_F(PlatformAndroidTest, + DownloadModuleSlice_AdbClientError_FailsGracefully) { EXPECT_CALL(*this, GetAdbClient(_)) - .Times(1) .WillOnce(DoAll(WithArg<0>([](auto &arg) { arg = Status::FromErrorString( "Failed to create AdbClient"); }), Return(ByMove(AdbClientUP())))); - EXPECT_TRUE( - DownloadModuleSlice( - FileSpec("/system/app/Test/Test.apk!/lib/arm64-v8a/libtest.so"), 4096, - 3600, FileSpec()) - .Fail()); -} - -TEST_F(PlatformAndroidTest, DownloadModuleSliceWithNormalFile) { - auto sync_service = new MockSyncService(); - EXPECT_CALL(*sync_service, Stat(FileSpec("/system/lib64/libc.so"), _, _, _)) - .Times(1) - .WillOnce(DoAll(SetArgReferee<1>(1), Return(Status()))); - EXPECT_CALL(*sync_service, PullFile(FileSpec("/system/lib64/libc.so"), _)) - .Times(1) - .WillOnce(Return(Status())); - - auto adb_client = new MockAdbClient(); - EXPECT_CALL(*adb_client, GetSyncService(_)) - .Times(1) - .WillOnce(Return(ByMove(SyncServiceUP(sync_service)))); - - EXPECT_CALL(*this, GetAdbClient(_)) - .Times(1) - .WillOnce(Return(ByMove(AdbClientUP(adb_client)))); + Status result = DownloadModuleSlice( + FileSpec("/system/app/Test/Test.apk!/lib/arm64-v8a/libtest.so"), 4096, + 3600, FileSpec("/tmp/libtest.so")); - EXPECT_TRUE( - DownloadModuleSlice(FileSpec("/system/lib64/libc.so"), 0, 0, FileSpec()) - .Success()); + EXPECT_TRUE(result.Fail()); + EXPECT_THAT(result.AsCString(), HasSubstr("Failed to create AdbClient")); } -TEST_F(PlatformAndroidTest, DownloadModuleSliceWithZipFile) { - auto adb_client = new MockAdbClient(); +TEST_F(PlatformAndroidTest, DownloadModuleSlice_ZipFile_UsesCorrectDdCommand) { + auto *adb_client = new MockAdbClient(); EXPECT_CALL(*adb_client, ShellToFile(StrEq("dd if='/system/app/Test/Test.apk' " "iflag=skip_bytes,count_bytes " "skip=4096 count=3600 status=none"), _, _)) - .Times(1) .WillOnce(Return(Status())); + EXPECT_CALL(*this, GetPropertyPackageName()) + .WillOnce(Return(llvm::StringRef(""))); + EXPECT_CALL(*this, GetAdbClient(_)) - .Times(1) .WillOnce(Return(ByMove(AdbClientUP(adb_client)))); - EXPECT_TRUE( - DownloadModuleSlice( - FileSpec("/system/app/Test/Test.apk!/lib/arm64-v8a/libtest.so"), 4096, - 3600, FileSpec()) - .Success()); + Status result = DownloadModuleSlice( + FileSpec("/system/app/Test/Test.apk!/lib/arm64-v8a/libtest.so"), 4096, + 3600, FileSpec("/tmp/libtest.so")); + + EXPECT_TRUE(result.Success()); } -TEST_F(PlatformAndroidTest, DownloadModuleSliceWithZipFileAndRunAs) { - auto adb_client = new MockAdbClient(); +TEST_F(PlatformAndroidTest, + DownloadModuleSlice_ZipFileWithRunAs_UsesRunAsCommand) { + auto *adb_client = new MockAdbClient(); EXPECT_CALL(*adb_client, ShellToFile(StrEq("run-as 'com.example.test' " "dd if='/system/app/Test/Test.apk' " "iflag=skip_bytes,count_bytes " "skip=4096 count=3600 status=none"), _, _)) - .Times(1) .WillOnce(Return(Status())); EXPECT_CALL(*this, GetPropertyPackageName()) - .Times(1) .WillOnce(Return(llvm::StringRef("com.example.test"))); EXPECT_CALL(*this, GetAdbClient(_)) - .Times(1) .WillOnce(Return(ByMove(AdbClientUP(adb_client)))); - EXPECT_TRUE( - DownloadModuleSlice( - FileSpec("/system/app/Test/Test.apk!/lib/arm64-v8a/libtest.so"), 4096, - 3600, FileSpec()) - .Success()); + Status result = DownloadModuleSlice( + FileSpec("/system/app/Test/Test.apk!/lib/arm64-v8a/libtest.so"), 4096, + 3600, FileSpec("/tmp/libtest.so")); + + EXPECT_TRUE(result.Success()); } -TEST_F(PlatformAndroidTest, GetFileWithNormalFile) { - auto sync_service = new MockSyncService(); - EXPECT_CALL(*sync_service, Stat(FileSpec("/data/local/tmp/test"), _, _, _)) - .Times(1) - .WillOnce(DoAll(SetArgReferee<1>(1), Return(Status()))); - EXPECT_CALL(*sync_service, PullFile(FileSpec("/data/local/tmp/test"), _)) - .Times(1) +TEST_F(PlatformAndroidTest, + DownloadModuleSlice_LargeFile_CalculatesParametersCorrectly) { + const uint64_t large_offset = 100 * 1024 * 1024; // 100MB offset + const uint64_t large_size = 50 * 1024 * 1024; // 50MB size + + auto *adb_client = new MockAdbClient(); + EXPECT_CALL(*adb_client, + ShellToFile(StrEq("dd if='/system/app/Large.apk' " + "iflag=skip_bytes,count_bytes " + "skip=104857600 count=52428800 status=none"), + _, _)) .WillOnce(Return(Status())); - auto adb_client = new MockAdbClient(); - EXPECT_CALL(*adb_client, GetSyncService(_)) - .Times(1) - .WillOnce(Return(ByMove(SyncServiceUP(sync_service)))); + EXPECT_CALL(*this, GetPropertyPackageName()) + .WillOnce(Return(llvm::StringRef(""))); EXPECT_CALL(*this, GetAdbClient(_)) - .Times(1) .WillOnce(Return(ByMove(AdbClientUP(adb_client)))); - EXPECT_TRUE(GetFile(FileSpec("/data/local/tmp/test"), FileSpec()).Success()); + Status result = DownloadModuleSlice( + FileSpec("/system/app/Large.apk!/lib/arm64-v8a/large.so"), large_offset, + large_size, FileSpec("/tmp/large.so")); + + EXPECT_TRUE(result.Success()); } -TEST_F(PlatformAndroidTest, GetFileWithCatFallback) { - auto sync_service = new MockSyncService(); - EXPECT_CALL( - *sync_service, - Stat(FileSpec("/data/data/com.example.app/lib-main/libtest.so"), _, _, _)) - .Times(1) - .WillOnce(DoAll(SetArgReferee<1>(0), Return(Status()))); +TEST_F(PlatformAndroidTest, + GetFile_SyncServiceUnavailable_FallsBackToShellCat) { + auto *adb_client = new MockAdbClient(); + EXPECT_CALL(*adb_client, + ShellToFile(StrEq("cat '/data/local/tmp/test'"), _, _)) + .WillOnce(Return(Status())); - auto adb_client0 = new MockAdbClient(); - EXPECT_CALL(*adb_client0, GetSyncService(_)) - .Times(1) - .WillOnce(Return(ByMove(SyncServiceUP(sync_service)))); + EXPECT_CALL(*this, GetPropertyPackageName()) + .WillOnce(Return(llvm::StringRef(""))); + + EXPECT_CALL(*this, GetAdbClient(_)) + .WillOnce(DoAll(WithArg<0>([](auto &arg) { arg.Clear(); }), + Return(ByMove(AdbClientUP(adb_client))))); + + EXPECT_CALL(*this, GetSyncService(_)) + .WillOnce([](Status &error) -> std::unique_ptr<AdbSyncService> { + error = Status::FromErrorString("Sync service unavailable"); + return nullptr; + }); + + Status result = + GetFile(FileSpec("/data/local/tmp/test"), FileSpec("/tmp/test")); + EXPECT_TRUE(result.Success()); +} - auto adb_client1 = new MockAdbClient(); +TEST_F(PlatformAndroidTest, GetFile_WithRunAs_UsesRunAsInShellCommand) { + auto *adb_client = new MockAdbClient(); EXPECT_CALL( - *adb_client1, - ShellToFile(StrEq("cat '/data/data/com.example.app/lib-main/libtest.so'"), + *adb_client, + ShellToFile(StrEq("run-as 'com.example.app' " + "cat '/data/data/com.example.app/lib-main/libtest.so'"), _, _)) - .Times(1) .WillOnce(Return(Status())); + EXPECT_CALL(*this, GetPropertyPackageName()) + .WillOnce(Return(llvm::StringRef("com.example.app"))); + EXPECT_CALL(*this, GetAdbClient(_)) - .Times(2) - .WillOnce(Return(ByMove(AdbClientUP(adb_client0)))) - .WillOnce(Return(ByMove(AdbClientUP(adb_client1)))); + .WillOnce(DoAll(WithArg<0>([](auto &arg) { arg.Clear(); }), + Return(ByMove(AdbClientUP(adb_client))))); + + EXPECT_CALL(*this, GetSyncService(_)) + .WillOnce([](Status &error) -> std::unique_ptr<AdbSyncService> { + error = Status::FromErrorString("Sync service unavailable"); + return nullptr; + }); - EXPECT_TRUE( + Status result = GetFile(FileSpec("/data/data/com.example.app/lib-main/libtest.so"), - FileSpec()) - .Success()); + FileSpec("/tmp/libtest.so")); + EXPECT_TRUE(result.Success()); } -TEST_F(PlatformAndroidTest, GetFileWithCatFallbackAndRunAs) { - auto sync_service = new MockSyncService(); - EXPECT_CALL( - *sync_service, - Stat(FileSpec("/data/data/com.example.app/lib-main/libtest.so"), _, _, _)) - .Times(1) - .WillOnce(DoAll(SetArgReferee<1>(0), Return(Status()))); +TEST_F(PlatformAndroidTest, GetFile_FilenameWithSingleQuotes_Rejected) { + EXPECT_CALL(*this, GetSyncService(_)) + .WillOnce([](Status &error) -> std::unique_ptr<AdbSyncService> { + error = Status::FromErrorString("Sync service unavailable"); + return nullptr; + }); - auto adb_client0 = new MockAdbClient(); - EXPECT_CALL(*adb_client0, GetSyncService(_)) - .Times(1) - .WillOnce(Return(ByMove(SyncServiceUP(sync_service)))); + Status result = + GetFile(FileSpec("/test/file'with'quotes"), FileSpec("/tmp/output")); - auto adb_client1 = new MockAdbClient(); - EXPECT_CALL( - *adb_client1, - ShellToFile(StrEq("run-as 'com.example.app' " - "cat '/data/data/com.example.app/lib-main/libtest.so'"), - _, _)) - .Times(1) + EXPECT_TRUE(result.Fail()); + EXPECT_THAT(result.AsCString(), HasSubstr("single-quotes")); +} + +TEST_F(PlatformAndroidTest, + DownloadModuleSlice_FilenameWithSingleQuotes_Rejected) { + Status result = DownloadModuleSlice(FileSpec("/test/file'with'quotes"), 100, + 200, FileSpec("/tmp/output")); + + EXPECT_TRUE(result.Fail()); + EXPECT_THAT(result.AsCString(), HasSubstr("single-quotes")); +} + +TEST_F(PlatformAndroidTest, GetFile_NetworkTimeout_PropagatesErrorCorrectly) { + auto *adb_client = new MockAdbClient(); + EXPECT_CALL(*adb_client, ShellToFile(_, _, _)) + .WillOnce(Return(Status::FromErrorString("Network timeout"))); + + EXPECT_CALL(*this, GetPropertyPackageName()) + .WillOnce(Return(llvm::StringRef(""))); + + EXPECT_CALL(*this, GetAdbClient(_)) + .WillOnce(DoAll(WithArg<0>([](auto &arg) { arg.Clear(); }), + Return(ByMove(AdbClientUP(adb_client))))); + + EXPECT_CALL(*this, GetSyncService(_)) + .WillOnce([](Status &error) -> std::unique_ptr<AdbSyncService> { + error = Status::FromErrorString("Sync service unavailable"); + return nullptr; + }); + + Status result = + GetFile(FileSpec("/data/large/file.so"), FileSpec("/tmp/large.so")); + EXPECT_TRUE(result.Fail()); + EXPECT_THAT(result.AsCString(), HasSubstr("Network timeout")); +} + +TEST_F(PlatformAndroidTest, SyncService_ConnectionFailsGracefully) { + // Constructor should succeed even with a failing connection + AdbSyncService sync_service("test-device"); + + // The service should report as not connected initially + EXPECT_FALSE(sync_service.IsConnected()); + EXPECT_EQ(sync_service.GetDeviceId(), "test-device"); + + // Operations should fail gracefully when connection setup fails + FileSpec remote_file("/data/test.txt"); + FileSpec local_file("/tmp/test.txt"); + uint32_t mode, size, mtime; + + Status result = sync_service.Stat(remote_file, mode, size, mtime); + EXPECT_TRUE(result.Fail()); +} + +TEST_F(PlatformAndroidTest, GetRunAs_FormatsPackageNameCorrectly) { + // Empty package name + EXPECT_CALL(*this, GetPropertyPackageName()) + .WillOnce(Return(llvm::StringRef(""))); + EXPECT_EQ(this->GetRunAs(), ""); + + // Valid package name + EXPECT_CALL(*this, GetPropertyPackageName()) + .WillOnce(Return(llvm::StringRef("com.example.test"))); + EXPECT_EQ(this->GetRunAs(), "run-as 'com.example.test' "); +} + +TEST_F(PlatformAndroidTest, + DownloadModuleSlice_ZeroOffset_CallsGetFileInsteadOfDd) { + // When offset=0, DownloadModuleSlice calls GetFile which uses 'cat', not 'dd' + // We need to ensure the sync service fails so GetFile falls back to shell cat + auto *adb_client = new MockAdbClient(); + EXPECT_CALL(*adb_client, + ShellToFile(StrEq("cat '/system/lib64/libc.so'"), _, _)) .WillOnce(Return(Status())); EXPECT_CALL(*this, GetPropertyPackageName()) - .Times(1) - .WillOnce(Return(llvm::StringRef("com.example.app"))); + .WillOnce(Return(llvm::StringRef(""))); EXPECT_CALL(*this, GetAdbClient(_)) - .Times(2) - .WillOnce(Return(ByMove(AdbClientUP(adb_client0)))) - .WillOnce(Return(ByMove(AdbClientUP(adb_client1)))); + .WillOnce(DoAll(WithArg<0>([](auto &arg) { arg.Clear(); }), + Return(ByMove(AdbClientUP(adb_client))))); - EXPECT_TRUE( - GetFile(FileSpec("/data/data/com.example.app/lib-main/libtest.so"), - FileSpec()) - .Success()); + // Mock GetSyncService to fail, forcing GetFile to use shell cat fallback + EXPECT_CALL(*this, GetSyncService(_)) + .WillOnce(DoAll(WithArg<0>([](auto &arg) { + arg = + Status::FromErrorString("Sync service unavailable"); + }), + Return(ByMove(std::unique_ptr<AdbSyncService>())))); + + Status result = DownloadModuleSlice(FileSpec("/system/lib64/libc.so"), 0, 0, + FileSpec("/tmp/libc.so")); + EXPECT_TRUE(result.Success()); } _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
