aadsm updated this revision to Diff 203235.
aadsm marked an inline comment as done.
aadsm added a comment.
Address final 2 comments
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D62499/new/
https://reviews.llvm.org/D62499
Files:
lldb/include/lldb/Utility/StringExtractorGDBRemote.h
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.cpp
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.h
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h
lldb/source/Utility/StringExtractorGDBRemote.cpp
lldb/unittests/Process/gdb-remote/CMakeLists.txt
lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationServerTest.cpp
lldb/unittests/Process/gdb-remote/GDBRemoteTestUtils.h
Index: lldb/unittests/Process/gdb-remote/GDBRemoteTestUtils.h
===================================================================
--- lldb/unittests/Process/gdb-remote/GDBRemoteTestUtils.h
+++ lldb/unittests/Process/gdb-remote/GDBRemoteTestUtils.h
@@ -8,9 +8,11 @@
#ifndef lldb_unittests_Process_gdb_remote_GDBRemoteTestUtils_h
#define lldb_unittests_Process_gdb_remote_GDBRemoteTestUtils_h
+#include "gmock/gmock.h"
#include "gtest/gtest.h"
#include "Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.h"
+#include "lldb/Utility/Connection.h"
namespace lldb_private {
namespace process_gdb_remote {
@@ -21,10 +23,38 @@
static void TearDownTestCase();
};
-struct MockServer : public GDBRemoteCommunicationServer {
+class MockConnection : public lldb_private::Connection {
+public:
+ MockConnection(std::vector<std::string> &packets) { m_packets = &packets; };
+
+ MOCK_METHOD2(Connect,
+ lldb::ConnectionStatus(llvm::StringRef url, Status *error_ptr));
+ MOCK_METHOD5(Read, size_t(void *dst, size_t dst_len,
+ const Timeout<std::micro> &timeout,
+ lldb::ConnectionStatus &status, Status *error_ptr));
+ MOCK_METHOD0(GetURI, std::string());
+ MOCK_METHOD0(InterruptRead, bool());
+
+ lldb::ConnectionStatus Disconnect(Status *error_ptr) {
+ return lldb::eConnectionStatusSuccess;
+ };
+
+ bool IsConnected() const { return true; };
+ size_t Write(const void *dst, size_t dst_len, lldb::ConnectionStatus &status,
+ Status *error_ptr) {
+ m_packets->emplace_back(static_cast<const char *>(dst), dst_len);
+ return dst_len;
+ };
+
+ std::vector<std::string> *m_packets;
+};
+
+class MockServer : public GDBRemoteCommunicationServer {
+public:
MockServer()
: GDBRemoteCommunicationServer("mock-server", "mock-server.listener") {
m_send_acks = false;
+ m_send_error_strings = true;
}
PacketResult SendPacket(llvm::StringRef payload) {
@@ -37,10 +67,22 @@
sync_on_timeout);
}
+ using GDBRemoteCommunicationServer::SendErrorResponse;
using GDBRemoteCommunicationServer::SendOKResponse;
using GDBRemoteCommunicationServer::SendUnimplementedResponse;
};
+class MockServerWithMockConnection : public MockServer {
+public:
+ MockServerWithMockConnection() : MockServer() {
+ SetConnection(new MockConnection(m_packets));
+ }
+
+ llvm::ArrayRef<std::string> GetPackets() { return m_packets; };
+
+ std::vector<std::string> m_packets;
+};
+
} // namespace process_gdb_remote
} // namespace lldb_private
Index: lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationServerTest.cpp
===================================================================
--- /dev/null
+++ lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationServerTest.cpp
@@ -0,0 +1,73 @@
+//===-- GDBRemoteCommunicationServerTest.cpp --------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+#include "GDBRemoteTestUtils.h"
+
+#include "Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.h"
+#include "lldb/Utility/Connection.h"
+
+namespace lldb_private {
+namespace process_gdb_remote {
+
+TEST(GDBRemoteCommunicationServerTest, SendErrorResponse_ErrorNumber) {
+ MockServerWithMockConnection server;
+ server.SendErrorResponse(0x42);
+
+ EXPECT_THAT(server.GetPackets(), testing::ElementsAre("$E42#ab"));
+}
+
+TEST(GDBRemoteCommunicationServerTest, SendErrorResponse_Status) {
+ MockServerWithMockConnection server;
+ Status status;
+
+ status.SetError(0x42, lldb::eErrorTypeGeneric);
+ status.SetErrorString("Test error message");
+ server.SendErrorResponse(status);
+
+ EXPECT_THAT(
+ server.GetPackets(),
+ testing::ElementsAre("$E42;54657374206572726f72206d657373616765#ad"));
+}
+
+TEST(GDBRemoteCommunicationServerTest, SendErrorResponse_UnimplementedError) {
+ MockServerWithMockConnection server;
+
+ auto error =
+ llvm::make_error<PacketUnimplementedError>("Test unimplemented error");
+ server.SendErrorResponse(std::move(error));
+
+ EXPECT_THAT(server.GetPackets(), testing::ElementsAre("$#00"));
+}
+
+TEST(GDBRemoteCommunicationServerTest, SendErrorResponse_StringError) {
+ MockServerWithMockConnection server;
+
+ auto error = llvm::createStringError(llvm::inconvertibleErrorCode(),
+ "String error test");
+ server.SendErrorResponse(std::move(error));
+
+ EXPECT_THAT(
+ server.GetPackets(),
+ testing::ElementsAre("$Eff;537472696e67206572726f722074657374#b0"));
+}
+
+TEST(GDBRemoteCommunicationServerTest, SendErrorResponse_ErrorList) {
+ MockServerWithMockConnection server;
+
+ auto error = llvm::joinErrors(llvm::make_error<PacketUnimplementedError>(),
+ llvm::make_error<PacketUnimplementedError>());
+
+ server.SendErrorResponse(std::move(error));
+ // Make sure only one packet is sent even when there are multiple errors.
+ EXPECT_EQ(server.GetPackets().size(), 1UL);
+}
+
+} // namespace process_gdb_remote
+} // namespace lldb_private
Index: lldb/unittests/Process/gdb-remote/CMakeLists.txt
===================================================================
--- lldb/unittests/Process/gdb-remote/CMakeLists.txt
+++ lldb/unittests/Process/gdb-remote/CMakeLists.txt
@@ -1,6 +1,7 @@
add_lldb_unittest(ProcessGdbRemoteTests
GDBRemoteClientBaseTest.cpp
GDBRemoteCommunicationClientTest.cpp
+ GDBRemoteCommunicationServerTest.cpp
GDBRemoteCommunicationTest.cpp
GDBRemoteTestUtils.cpp
Index: lldb/source/Utility/StringExtractorGDBRemote.cpp
===================================================================
--- lldb/source/Utility/StringExtractorGDBRemote.cpp
+++ lldb/source/Utility/StringExtractorGDBRemote.cpp
@@ -285,8 +285,8 @@
break;
case 'X':
- if (PACKET_STARTS_WITH("qXfer:auxv:read::"))
- return eServerPacketType_qXfer_auxv_read;
+ if (PACKET_STARTS_WITH("qXfer:"))
+ return eServerPacketType_qXfer;
break;
}
break;
Index: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h
===================================================================
--- lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h
+++ lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h
@@ -82,7 +82,7 @@
MainLoop::ReadHandleUP m_stdio_handle_up;
lldb::StateType m_inferior_prev_state = lldb::StateType::eStateInvalid;
- std::unique_ptr<llvm::MemoryBuffer> m_active_auxv_buffer_up;
+ llvm::StringMap<std::unique_ptr<llvm::MemoryBuffer>> m_xfer_buffer_map;
std::mutex m_saved_registers_mutex;
std::unordered_map<uint32_t, lldb::DataBufferSP> m_saved_registers_map;
uint32_t m_next_saved_registers_id = 1;
@@ -150,7 +150,7 @@
PacketResult Handle_s(StringExtractorGDBRemote &packet);
- PacketResult Handle_qXfer_auxv_read(StringExtractorGDBRemote &packet);
+ PacketResult Handle_qXfer(StringExtractorGDBRemote &packet);
PacketResult Handle_QSaveRegisterState(StringExtractorGDBRemote &packet);
@@ -193,6 +193,9 @@
FileSpec FindModuleFile(const std::string &module_path,
const ArchSpec &arch) override;
+ llvm::Expected<std::unique_ptr<llvm::MemoryBuffer>>
+ ReadXferObject(llvm::StringRef object, llvm::StringRef annex);
+
private:
void HandleInferiorState_Exited(NativeProcessProtocol *process);
Index: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
===================================================================
--- lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
+++ lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
@@ -144,8 +144,8 @@
StringExtractorGDBRemote::eServerPacketType_qWatchpointSupportInfo,
&GDBRemoteCommunicationServerLLGS::Handle_qWatchpointSupportInfo);
RegisterMemberFunctionHandler(
- StringExtractorGDBRemote::eServerPacketType_qXfer_auxv_read,
- &GDBRemoteCommunicationServerLLGS::Handle_qXfer_auxv_read);
+ StringExtractorGDBRemote::eServerPacketType_qXfer,
+ &GDBRemoteCommunicationServerLLGS::Handle_qXfer);
RegisterMemberFunctionHandler(StringExtractorGDBRemote::eServerPacketType_s,
&GDBRemoteCommunicationServerLLGS::Handle_s);
RegisterMemberFunctionHandler(
@@ -2747,94 +2747,99 @@
return PacketResult::Success;
}
-GDBRemoteCommunication::PacketResult
-GDBRemoteCommunicationServerLLGS::Handle_qXfer_auxv_read(
- StringExtractorGDBRemote &packet) {
-// *BSD impls should be able to do this too.
-#if defined(__linux__) || defined(__NetBSD__)
- Log *log(GetLogIfAnyCategoriesSet(LIBLLDB_LOG_PROCESS));
-
- // Parse out the offset.
- packet.SetFilePos(strlen("qXfer:auxv:read::"));
- if (packet.GetBytesLeft() < 1)
- return SendIllFormedResponse(packet,
- "qXfer:auxv:read:: packet missing offset");
-
- const uint64_t auxv_offset =
- packet.GetHexMaxU64(false, std::numeric_limits<uint64_t>::max());
- if (auxv_offset == std::numeric_limits<uint64_t>::max())
- return SendIllFormedResponse(packet,
- "qXfer:auxv:read:: packet missing offset");
-
- // Parse out comma.
- if (packet.GetBytesLeft() < 1 || packet.GetChar() != ',')
- return SendIllFormedResponse(
- packet, "qXfer:auxv:read:: packet missing comma after offset");
-
- // Parse out the length.
- const uint64_t auxv_length =
- packet.GetHexMaxU64(false, std::numeric_limits<uint64_t>::max());
- if (auxv_length == std::numeric_limits<uint64_t>::max())
- return SendIllFormedResponse(packet,
- "qXfer:auxv:read:: packet missing length");
-
- // Grab the auxv data if we need it.
- if (!m_active_auxv_buffer_up) {
+llvm::Expected<std::unique_ptr<llvm::MemoryBuffer>>
+GDBRemoteCommunicationServerLLGS::ReadXferObject(llvm::StringRef object,
+ llvm::StringRef annex) {
+ if (object == "auxv") {
// Make sure we have a valid process.
if (!m_debugged_process_up ||
(m_debugged_process_up->GetID() == LLDB_INVALID_PROCESS_ID)) {
- if (log)
- log->Printf(
- "GDBRemoteCommunicationServerLLGS::%s failed, no process available",
- __FUNCTION__);
- return SendErrorResponse(0x10);
+ return llvm::createStringError(llvm::inconvertibleErrorCode(),
+ "No process available");
}
// Grab the auxv data.
auto buffer_or_error = m_debugged_process_up->GetAuxvData();
- if (!buffer_or_error) {
- std::error_code ec = buffer_or_error.getError();
- LLDB_LOG(log, "no auxv data retrieved: {0}", ec.message());
- return SendErrorResponse(ec.value());
- }
- m_active_auxv_buffer_up = std::move(*buffer_or_error);
+ if (!buffer_or_error)
+ return llvm::errorCodeToError(buffer_or_error.getError());
+ return std::move(*buffer_or_error);
}
+ return llvm::make_error<PacketUnimplementedError>(
+ "Xfer object not supported");
+}
+
+GDBRemoteCommunication::PacketResult
+GDBRemoteCommunicationServerLLGS::Handle_qXfer(
+ StringExtractorGDBRemote &packet) {
+ SmallVector<StringRef, 5> fields;
+ // The packet format is "qXfer:<object>:<action>:<annex>:offset,length"
+ StringRef(packet.GetStringRef()).split(fields, ':', 4);
+ if (fields.size() != 5)
+ return SendIllFormedResponse(packet, "malformed qXfer packet");
+ StringRef &xfer_object = fields[1];
+ StringRef &xfer_action = fields[2];
+ StringRef &xfer_annex = fields[3];
+ StringExtractor offset_data(fields[4]);
+ if (xfer_action != "read")
+ return SendUnimplementedResponse("qXfer action not supported");
+ // Parse offset.
+ const uint64_t xfer_offset =
+ offset_data.GetHexMaxU64(false, std::numeric_limits<uint64_t>::max());
+ if (xfer_offset == std::numeric_limits<uint64_t>::max())
+ return SendIllFormedResponse(packet, "qXfer packet missing offset");
+ // Parse out comma.
+ if (offset_data.GetChar() != ',')
+ return SendIllFormedResponse(packet,
+ "qXfer packet missing comma after offset");
+ // Parse out the length.
+ const uint64_t xfer_length =
+ offset_data.GetHexMaxU64(false, std::numeric_limits<uint64_t>::max());
+ if (xfer_length == std::numeric_limits<uint64_t>::max())
+ return SendIllFormedResponse(packet, "qXfer packet missing length");
+
+ // Get a previously constructed buffer if it exists or create it now.
+ std::string buffer_key = (xfer_object + xfer_action + xfer_annex).str();
+ auto buffer_it = m_xfer_buffer_map.find(buffer_key);
+ if (buffer_it == m_xfer_buffer_map.end()) {
+ auto buffer_up = ReadXferObject(xfer_object, xfer_annex);
+ if (!buffer_up)
+ return SendErrorResponse(buffer_up.takeError());
+ buffer_it = m_xfer_buffer_map
+ .insert(std::make_pair(buffer_key, std::move(*buffer_up)))
+ .first;
+ }
+
+ // Send back the response
StreamGDBRemote response;
bool done_with_buffer = false;
-
- llvm::StringRef buffer = m_active_auxv_buffer_up->getBuffer();
- if (auxv_offset >= buffer.size()) {
+ llvm::StringRef buffer = buffer_it->second->getBuffer();
+ if (xfer_offset >= buffer.size()) {
// We have nothing left to send. Mark the buffer as complete.
response.PutChar('l');
done_with_buffer = true;
} else {
// Figure out how many bytes are available starting at the given offset.
- buffer = buffer.drop_front(auxv_offset);
-
+ buffer = buffer.drop_front(xfer_offset);
// Mark the response type according to whether we're reading the remainder
- // of the auxv data.
- if (auxv_length >= buffer.size()) {
+ // of the data.
+ if (xfer_length >= buffer.size()) {
// There will be nothing left to read after this
response.PutChar('l');
done_with_buffer = true;
} else {
// There will still be bytes to read after this request.
response.PutChar('m');
- buffer = buffer.take_front(auxv_length);
+ buffer = buffer.take_front(xfer_length);
}
-
// Now write the data in encoded binary form.
response.PutEscapedBytes(buffer.data(), buffer.size());
}
if (done_with_buffer)
- m_active_auxv_buffer_up.reset();
+ m_xfer_buffer_map.erase(buffer_it);
return SendPacketNoLock(response.GetString());
-#else
- return SendUnimplementedResponse("not implemented on this platform");
-#endif
}
GDBRemoteCommunication::PacketResult
@@ -3259,8 +3264,8 @@
void GDBRemoteCommunicationServerLLGS::ClearProcessSpecificData() {
Log *log(GetLogIfAnyCategoriesSet(LIBLLDB_LOG_PROCESS));
- LLDB_LOG(log, "clearing auxv buffer: {0}", m_active_auxv_buffer_up.get());
- m_active_auxv_buffer_up.reset();
+ LLDB_LOG(log, "clearing {0} xfer buffers", m_xfer_buffer_map.size());
+ m_xfer_buffer_map.clear();
}
FileSpec
Index: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.h
===================================================================
--- lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.h
+++ lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.h
@@ -15,6 +15,9 @@
#include "GDBRemoteCommunication.h"
#include "lldb/lldb-private-forward.h"
+#include "llvm/Support/Errc.h"
+#include "llvm/Support/Error.h"
+
class StringExtractorGDBRemote;
namespace lldb_private {
@@ -59,6 +62,8 @@
PacketResult SendErrorResponse(const Status &error);
+ PacketResult SendErrorResponse(llvm::Error error);
+
PacketResult SendUnimplementedResponse(const char *packet);
PacketResult SendErrorResponse(uint8_t error);
@@ -72,6 +77,18 @@
DISALLOW_COPY_AND_ASSIGN(GDBRemoteCommunicationServer);
};
+class PacketUnimplementedError
+ : public llvm::ErrorInfo<PacketUnimplementedError, llvm::StringError> {
+public:
+ static char ID;
+ using llvm::ErrorInfo<PacketUnimplementedError,
+ llvm::StringError>::ErrorInfo; // inherit constructors
+ PacketUnimplementedError(const llvm::Twine &S)
+ : ErrorInfo(S, llvm::errc::not_supported) {}
+
+ PacketUnimplementedError() : ErrorInfo(llvm::errc::not_supported) {}
+};
+
} // namespace process_gdb_remote
} // namespace lldb_private
Index: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.cpp
===================================================================
--- lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.cpp
+++ lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.cpp
@@ -113,6 +113,22 @@
}
GDBRemoteCommunication::PacketResult
+GDBRemoteCommunicationServer::SendErrorResponse(llvm::Error error) {
+ std::unique_ptr<llvm::ErrorInfoBase> EIB;
+ std::unique_ptr<PacketUnimplementedError> PUE;
+ llvm::handleAllErrors(
+ std::move(error),
+ [&](std::unique_ptr<PacketUnimplementedError> E) { PUE = std::move(E); },
+ [&](std::unique_ptr<llvm::ErrorInfoBase> E) { EIB = std::move(E); });
+
+ if (EIB)
+ return SendErrorResponse(Status(llvm::Error(std::move(EIB))));
+ if (PUE)
+ return SendUnimplementedResponse(PUE->message().c_str());
+ return SendErrorResponse(Status("Unknown Error"));
+}
+
+GDBRemoteCommunication::PacketResult
GDBRemoteCommunicationServer::Handle_QErrorStringEnable(
StringExtractorGDBRemote &packet) {
m_send_error_strings = true;
@@ -138,3 +154,5 @@
bool GDBRemoteCommunicationServer::HandshakeWithClient() {
return GetAck() == PacketResult::Success;
}
+
+char PacketUnimplementedError::ID;
Index: lldb/include/lldb/Utility/StringExtractorGDBRemote.h
===================================================================
--- lldb/include/lldb/Utility/StringExtractorGDBRemote.h
+++ lldb/include/lldb/Utility/StringExtractorGDBRemote.h
@@ -128,7 +128,7 @@
eServerPacketType_qVAttachOrWaitSupported,
eServerPacketType_qWatchpointSupportInfo,
eServerPacketType_qWatchpointSupportInfoSupported,
- eServerPacketType_qXfer_auxv_read,
+ eServerPacketType_qXfer,
eServerPacketType_jSignalsInfo,
eServerPacketType_jModulesInfo,
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
