guiandrade updated this revision to Diff 224120.
guiandrade added a comment.
Dissociating the use of g/G packets and trying to address the AVX/MPX offset bug
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D62931/new/
https://reviews.llvm.org/D62931
Files:
lldb/packages/Python/lldbsuite/test/functionalities/gdb_remote_client/TestGDBRemoteClient.py
lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux.cpp
lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux.h
lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.cpp
lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.h
lldb/source/Plugins/Process/Utility/RegisterInfos_x86_64.h
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
lldb/source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.cpp
lldb/source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.h
lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemoteProperties.td
lldb/source/Plugins/Process/gdb-remote/ThreadGDBRemote.cpp
lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp
Index: lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp
===================================================================
--- lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp
+++ lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp
@@ -128,6 +128,11 @@
ASSERT_EQ(0,
memcmp(buffer_sp->GetBytes(), one_register, sizeof one_register));
+ async_result = std::async(std::launch::async,
+ [&] { return client.GetgPacketSupported(tid); });
+ HandlePacket(server, "g;thread:0047;", all_registers_hex);
+ ASSERT_TRUE(async_result.get());
+
read_result = std::async(std::launch::async,
[&] { return client.ReadAllRegisters(tid); });
HandlePacket(server, "g;thread:0047;", all_registers_hex);
Index: lldb/source/Plugins/Process/gdb-remote/ThreadGDBRemote.cpp
===================================================================
--- lldb/source/Plugins/Process/gdb-remote/ThreadGDBRemote.cpp
+++ lldb/source/Plugins/Process/gdb-remote/ThreadGDBRemote.cpp
@@ -301,13 +301,15 @@
if (process_sp) {
ProcessGDBRemote *gdb_process =
static_cast<ProcessGDBRemote *>(process_sp.get());
- // read_all_registers_at_once will be true if 'p' packet is not
- // supported.
bool read_all_registers_at_once =
- !gdb_process->GetGDBRemote().GetpPacketSupported(GetID());
+ gdb_process->GetGDBRemote().GetgPacketSupported(GetID()) &&
+ gdb_process->m_use_g_packet_for_reading;
+ bool write_all_registers_at_once =
+ gdb_process->GetGDBRemote().GetGPacketSupported(GetID()) &&
+ gdb_process->m_use_g_packet_for_writing;
reg_ctx_sp = std::make_shared<GDBRemoteRegisterContext>(
*this, concrete_frame_idx, gdb_process->m_register_info,
- read_all_registers_at_once);
+ read_all_registers_at_once, write_all_registers_at_once);
}
} else {
Unwind *unwinder = GetUnwinder();
Index: lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemoteProperties.td
===================================================================
--- lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemoteProperties.td
+++ lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemoteProperties.td
@@ -13,4 +13,12 @@
Global,
DefaultFalse,
Desc<"If true, the libraries-svr4 feature will be used to get a hold of the process's loaded modules.">;
+ def UseGPacketForReading: Property<"use-g-packet-for-reading", "Boolean">,
+ Global,
+ DefaultTrue,
+ Desc<"Specify if the server should use 'g' packets to read registers.">;
+ def UseGPacketForWriting: Property<"use-g-packet-for-writing", "Boolean">,
+ Global,
+ DefaultFalse,
+ Desc<"Specify if the server should use 'g' packets to write registers.">;
}
Index: lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
===================================================================
--- lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
+++ lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
@@ -284,6 +284,8 @@
lldb::CommandObjectSP m_command_sp;
int64_t m_breakpoint_pc_offset;
lldb::tid_t m_initial_tid; // The initial thread ID, given by stub on attach
+ bool m_use_g_packet_for_reading;
+ bool m_use_g_packet_for_writing;
bool m_replay_mode;
bool m_allow_flash_writes;
Index: lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
===================================================================
--- lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
+++ lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
@@ -154,6 +154,16 @@
nullptr, idx,
g_processgdbremote_properties[idx].default_uint_value != 0);
}
+
+ bool GetUseGPacketForReading() const {
+ const uint32_t idx = ePropertyUseGPacketForReading;
+ return m_collection_sp->GetPropertyAtIndexAsBoolean(nullptr, idx, true);
+ }
+
+ bool GetUseGPacketForWriting() const {
+ const uint32_t idx = ePropertyUseGPacketForWriting;
+ return m_collection_sp->GetPropertyAtIndexAsBoolean(nullptr, idx, true);
+ }
};
typedef std::shared_ptr<PluginProperties> ProcessKDPPropertiesSP;
@@ -309,6 +319,11 @@
GetGlobalPluginProperties()->GetPacketTimeout();
if (timeout_seconds > 0)
m_gdb_comm.SetPacketTimeout(std::chrono::seconds(timeout_seconds));
+
+ m_use_g_packet_for_reading =
+ GetGlobalPluginProperties()->GetUseGPacketForReading();
+ m_use_g_packet_for_writing =
+ GetGlobalPluginProperties()->GetUseGPacketForWriting();
}
// Destructor
Index: lldb/source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.h
===================================================================
--- lldb/source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.h
+++ lldb/source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.h
@@ -41,7 +41,7 @@
public:
GDBRemoteRegisterContext(ThreadGDBRemote &thread, uint32_t concrete_frame_idx,
GDBRemoteDynamicRegisterInfo ®_info,
- bool read_all_at_once);
+ bool read_all_at_once, bool write_all_at_once);
~GDBRemoteRegisterContext() override;
@@ -114,6 +114,7 @@
std::vector<bool> m_reg_valid;
DataExtractor m_reg_data;
bool m_read_all_at_once;
+ bool m_write_all_at_once;
private:
// Helper function for ReadRegisterBytes().
Index: lldb/source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.cpp
===================================================================
--- lldb/source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.cpp
+++ lldb/source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.cpp
@@ -31,9 +31,11 @@
// GDBRemoteRegisterContext constructor
GDBRemoteRegisterContext::GDBRemoteRegisterContext(
ThreadGDBRemote &thread, uint32_t concrete_frame_idx,
- GDBRemoteDynamicRegisterInfo ®_info, bool read_all_at_once)
+ GDBRemoteDynamicRegisterInfo ®_info, bool read_all_at_once,
+ bool write_all_at_once)
: RegisterContext(thread, concrete_frame_idx), m_reg_info(reg_info),
- m_reg_valid(), m_reg_data(), m_read_all_at_once(read_all_at_once) {
+ m_reg_valid(), m_reg_data(), m_read_all_at_once(read_all_at_once),
+ m_write_all_at_once(write_all_at_once) {
// Resize our vector of bools to contain one bool for every register. We will
// use these boolean values to know when a register value is valid in
// m_reg_data.
@@ -333,7 +335,7 @@
{
GDBRemoteClientBase::Lock lock(gdb_comm, false);
if (lock) {
- if (m_read_all_at_once) {
+ if (m_write_all_at_once) {
// Invalidate all register values
InvalidateIfNeeded(true);
Index: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
===================================================================
--- lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
+++ lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
@@ -234,6 +234,10 @@
bool GetpPacketSupported(lldb::tid_t tid);
+ bool GetgPacketSupported(lldb::tid_t tid);
+
+ bool GetGPacketSupported(lldb::tid_t tid);
+
bool GetxPacketSupported();
bool GetVAttachOrWaitSupported();
@@ -516,6 +520,8 @@
LazyBool m_prepare_for_reg_writing_reply;
LazyBool m_supports_p;
LazyBool m_supports_x;
+ LazyBool m_supports_g;
+ LazyBool m_supports_G;
LazyBool m_avoid_g_packets;
LazyBool m_supports_QSaveRegisterState;
LazyBool m_supports_qXfer_auxv_read;
@@ -595,6 +601,8 @@
Status GetQXferMemoryMapRegionInfo(lldb::addr_t addr,
MemoryRegionInfo ®ion);
+ LazyBool GetThreadPacketSupported(lldb::tid_t tid, llvm::StringRef packetStr);
+
private:
DISALLOW_COPY_AND_ASSIGN(GDBRemoteCommunicationClient);
};
Index: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
===================================================================
--- lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
+++ lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
@@ -69,6 +69,7 @@
m_attach_or_wait_reply(eLazyBoolCalculate),
m_prepare_for_reg_writing_reply(eLazyBoolCalculate),
m_supports_p(eLazyBoolCalculate), m_supports_x(eLazyBoolCalculate),
+ m_supports_g(eLazyBoolCalculate), m_supports_G(eLazyBoolCalculate),
m_avoid_g_packets(eLazyBoolCalculate),
m_supports_QSaveRegisterState(eLazyBoolCalculate),
m_supports_qXfer_auxv_read(eLazyBoolCalculate),
@@ -543,21 +544,36 @@
//
// Takes a valid thread ID because p needs to apply to a thread.
bool GDBRemoteCommunicationClient::GetpPacketSupported(lldb::tid_t tid) {
- if (m_supports_p == eLazyBoolCalculate) {
- m_supports_p = eLazyBoolNo;
- StreamString payload;
- payload.PutCString("p0");
- StringExtractorGDBRemote response;
- if (SendThreadSpecificPacketAndWaitForResponse(tid, std::move(payload),
- response, false) ==
- PacketResult::Success &&
- response.IsNormalResponse()) {
- m_supports_p = eLazyBoolYes;
- }
- }
+ if (m_supports_p == eLazyBoolCalculate)
+ m_supports_p = GetThreadPacketSupported(tid, "p0");
return m_supports_p;
}
+bool GDBRemoteCommunicationClient::GetgPacketSupported(lldb::tid_t tid) {
+ if (m_supports_g == eLazyBoolCalculate)
+ m_supports_g = GetThreadPacketSupported(tid, "g");
+ return m_supports_g;
+}
+
+bool GDBRemoteCommunicationClient::GetGPacketSupported(lldb::tid_t tid) {
+ if (m_supports_G == eLazyBoolCalculate)
+ m_supports_G = GetThreadPacketSupported(tid, "G");
+ return m_supports_G;
+}
+
+LazyBool GDBRemoteCommunicationClient::GetThreadPacketSupported(
+ lldb::tid_t tid, llvm::StringRef packetStr) {
+ StreamString payload;
+ payload.PutCString(packetStr);
+ StringExtractorGDBRemote response;
+ if (SendThreadSpecificPacketAndWaitForResponse(
+ tid, std::move(payload), response, false) == PacketResult::Success &&
+ response.IsNormalResponse()) {
+ return eLazyBoolYes;
+ }
+ return eLazyBoolNo;
+}
+
StructuredData::ObjectSP GDBRemoteCommunicationClient::GetThreadsInfo() {
// Get information on all threads at one using the "jThreadsInfo" packet
StructuredData::ObjectSP object_sp;
Index: lldb/source/Plugins/Process/Utility/RegisterInfos_x86_64.h
===================================================================
--- lldb/source/Plugins/Process/Utility/RegisterInfos_x86_64.h
+++ lldb/source/Plugins/Process/Utility/RegisterInfos_x86_64.h
@@ -25,15 +25,18 @@
LLVM_EXTENSION offsetof(FPR, xsave) + \
LLVM_EXTENSION offsetof(XSAVE, ymmh[0]) + (32 * reg_index))
+// Guarantees BNDR/BNDC offsets do not overlap with YMM offsets.
+#define GDB_REMOTE_OFFSET 128
+
#define BNDR_OFFSET(reg_index) \
(LLVM_EXTENSION offsetof(UserArea, fpr) + \
LLVM_EXTENSION offsetof(FPR, xsave) + \
- LLVM_EXTENSION offsetof(XSAVE, mpxr[reg_index]))
+ LLVM_EXTENSION offsetof(XSAVE, mpxr[reg_index]) + GDB_REMOTE_OFFSET)
#define BNDC_OFFSET(reg_index) \
(LLVM_EXTENSION offsetof(UserArea, fpr) + \
LLVM_EXTENSION offsetof(FPR, xsave) + \
- LLVM_EXTENSION offsetof(XSAVE, mpxc[reg_index]))
+ LLVM_EXTENSION offsetof(XSAVE, mpxc[reg_index]) + GDB_REMOTE_OFFSET)
#ifdef DECLARE_REGISTER_INFOS_X86_64_STRUCT
Index: lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.h
===================================================================
--- lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.h
+++ lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.h
@@ -75,6 +75,9 @@
Status WriteFPR() override;
+ uint32_t GetPtraceOffset(uint32_t gdb_remote_offset,
+ uint32_t reg_index) override;
+
private:
// Private member types.
enum class XStateType { Invalid, FXSAVE, XSAVE };
Index: lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.cpp
===================================================================
--- lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.cpp
+++ lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.cpp
@@ -1213,4 +1213,11 @@
return 4;
}
+uint32_t
+NativeRegisterContextLinux_x86_64::GetPtraceOffset(uint32_t gdb_remote_offset,
+ uint32_t reg_index) {
+ // If register is MPX, remove extra factor from gdb offset
+ return gdb_remote_offset - (IsMPX(reg_index) ? 128 : 0);
+}
+
#endif // defined(__i386__) || defined(__x86_64__)
Index: lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux.h
===================================================================
--- lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux.h
+++ lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux.h
@@ -60,6 +60,11 @@
virtual size_t GetFPRSize() = 0;
+ virtual uint32_t GetPtraceOffset(uint32_t gdb_remote_offset,
+ uint32_t reg_index) {
+ return gdb_remote_offset;
+ }
+
// The Do*** functions are executed on the privileged thread and can perform
// ptrace
// operations directly.
Index: lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux.cpp
===================================================================
--- lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux.cpp
+++ lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux.cpp
@@ -34,8 +34,8 @@
if (!reg_info)
return Status("register %" PRIu32 " not found", reg_index);
- return DoReadRegisterValue(reg_info->byte_offset, reg_info->name,
- reg_info->byte_size, reg_value);
+ return DoReadRegisterValue(GetPtraceOffset(reg_info->byte_offset, reg_index),
+ reg_info->name, reg_info->byte_size, reg_value);
}
Status
@@ -91,7 +91,8 @@
"for write register index %" PRIu32,
__FUNCTION__, reg_to_write);
- return DoWriteRegisterValue(reg_info->byte_offset, reg_info->name, reg_value);
+ return DoWriteRegisterValue(GetPtraceOffset(reg_info->byte_offset, reg_index),
+ reg_info->name, reg_value);
}
Status NativeRegisterContextLinux::ReadGPR() {
Index: lldb/packages/Python/lldbsuite/test/functionalities/gdb_remote_client/TestGDBRemoteClient.py
===================================================================
--- lldb/packages/Python/lldbsuite/test/functionalities/gdb_remote_client/TestGDBRemoteClient.py
+++ lldb/packages/Python/lldbsuite/test/functionalities/gdb_remote_client/TestGDBRemoteClient.py
@@ -7,6 +7,14 @@
class TestGDBRemoteClient(GDBRemoteTestBase):
+ def setUp(self):
+ super(TestGDBRemoteClient, self).setUp()
+ self._initial_platform = lldb.DBG.GetSelectedPlatform()
+
+ def tearDown(self):
+ lldb.DBG.SetSelectedPlatform(self._initial_platform)
+ super(TestGDBRemoteClient, self).tearDown()
+
def test_connect(self):
"""Test connecting to a remote gdb server"""
target = self.createTarget("a.yaml")
@@ -37,3 +45,17 @@
error = lldb.SBError()
target.AttachToProcessWithID(lldb.SBListener(), 47, error)
self.assertEquals(error_msg, error.GetCString())
+
+ def test_get_registers_uses_g_packet(self):
+ """Test if the client uses a 'g' packet to read registers by default"""
+ class MyResponder(MockGDBServerResponder):
+ def readRegisters(self):
+ return '0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000'
+
+ self.server.responder = MyResponder()
+
+ target = self.createTarget("a.yaml")
+ process = self.connect(target)
+ registers = process.GetThreadAtIndex(0).GetFrameAtIndex(0).GetRegisters()
+ self.assertGreater(registers.GetSize(), 0)
+ self.assertPacketLogContains(["g"])
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
