Author: David Spickett Date: 2023-11-02T10:27:37Z New Revision: 805a36aaf571a8d53781c0c413d1af125137bfa2
URL: https://github.com/llvm/llvm-project/commit/805a36aaf571a8d53781c0c413d1af125137bfa2 DIFF: https://github.com/llvm/llvm-project/commit/805a36aaf571a8d53781c0c413d1af125137bfa2.diff LOG: [lldb][AArch64] Simplify handing of scalable registers using vg and svg (#70914) This removes explicit invalidation of vg and svg that was done in `GDBRemoteRegisterContext::AArch64Reconfigure`. This was in fact covering up a bug elsehwere. Register information says that a write to vg also invalidates svg (it does not unless you are in streaming mode, but we decided to keep it simple and say it always does). This invalidation was not being applied until *after* AArch64Reconfigure was called. This meant that without those manual invalidates this happened: * vg is written * svg is not invalidated * Reconfigure uses the written vg value * Reconfigure uses the *old* svg value I have moved the AArch64Reconfigure call to after we've processed the invalidations caused by the register write, so we no longer need the manual invalidates in AArch64Reconfigure. In addition I have changed the order in which expedited registers as parsed. These registers come with a stop notification and include, amongst others, vg and svg. So now we: * Parse them and update register values (including vg and svg) * AArch64Reconfigure, which uses those values, and invalidates every register, because offsets may have changed. * Parse the expedited registers again, knowing that none of the values will have changed due to the scaling. This means we use the expedited registers during the reconfigure, but the invalidate does not mean we throw all of them away. The cost is we parse them twice client side, but this is cheap compared to a network packet, and is limited to AArch64 targets only. On a system with SVE and SME, these are the packets sent for a step: ``` (lldb) b-remote.async> < 803> read packet: $T05thread:p1f80.1f80;name:main.o;threads:1f80;thread-pcs:000000000040056c<...>a1:0800000000000000;d9:0400000000000000;reason:trace;#fc intern-state < 21> send packet: $xfffffffff200,200#5e intern-state < 516> read packet: $e4f2ffffffff000000<...>#71 intern-state < 15> send packet: $Z0,400568,4#4d intern-state < 6> read packet: $OK#9a dbg.evt-handler < 16> send packet: $jThreadsInfo#c1 dbg.evt-handler < 224> read packet: $[{"name":"main.o","reason":"trace","registers":{"161":"0800000000000000",<...>}],"signal":5,"tid":8064}]]#73 ``` You can see there are no extra register reads which means we're using the expedited registers. For a write to vg: ``` (lldb) register write vg 4 lldb < 37> send packet: $Pa1=0400000000000000;thread:1f80;#4a lldb < 6> read packet: $OK#9a lldb < 20> send packet: $pa1;thread:1f80;#29 lldb < 20> read packet: $0400000000000000#04 lldb < 20> send packet: $pd9;thread:1f80;#34 lldb < 20> read packet: $0400000000000000#04 ``` There is the initial P write, and lldb correctly assumes that SVG is invalidated by this also so we read back the new vg and svg values afterwards. Added: Modified: lldb/source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.cpp lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h Removed: ################################################################################ diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.cpp b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.cpp index 72280927471f883..013b2bbc0e67f27 100644 --- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.cpp +++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.cpp @@ -434,11 +434,6 @@ bool GDBRemoteRegisterContext::WriteRegisterBytes(const RegisterInfo *reg_info, } else { // This is an actual register, write it success = SetPrimordialRegister(reg_info, gdb_comm); - - if (success && do_reconfigure_arm64_sve) { - AArch64Reconfigure(); - InvalidateAllRegisters(); - } } // Check if writing this register will invalidate any other register @@ -452,6 +447,11 @@ bool GDBRemoteRegisterContext::WriteRegisterBytes(const RegisterInfo *reg_info, false); } + if (success && do_reconfigure_arm64_sve) { + AArch64Reconfigure(); + InvalidateAllRegisters(); + } + return success; } } else { @@ -772,8 +772,6 @@ void GDBRemoteRegisterContext::AArch64Reconfigure() { std::optional<uint64_t> vg_reg_value; const RegisterInfo *vg_reg_info = m_reg_info_sp->GetRegisterInfo("vg"); if (vg_reg_info) { - // Make sure we get the latest value of vg from the remote. - SetRegisterIsValid(vg_reg_info, false); uint32_t vg_reg_num = vg_reg_info->kinds[eRegisterKindLLDB]; uint64_t reg_value = ReadRegisterAsUnsigned(vg_reg_num, fail_value); if (reg_value != fail_value && reg_value <= 32) @@ -783,11 +781,6 @@ void GDBRemoteRegisterContext::AArch64Reconfigure() { std::optional<uint64_t> svg_reg_value; const RegisterInfo *svg_reg_info = m_reg_info_sp->GetRegisterInfo("svg"); if (svg_reg_info) { - // When vg is written it is automatically made invalid. Writing vg will also - // change svg if we're in streaming mode but it will not be made invalid - // so do this manually so the following read gets the latest svg value. - SetRegisterIsValid(svg_reg_info, false); - uint32_t svg_reg_num = svg_reg_info->kinds[eRegisterKindLLDB]; uint64_t reg_value = ReadRegisterAsUnsigned(svg_reg_num, fail_value); if (reg_value != fail_value && reg_value <= 32) diff --git a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp index 16b3ba661d07162..c50ac5de77904f6 100644 --- a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp +++ b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp @@ -1612,6 +1612,22 @@ bool ProcessGDBRemote::CalculateThreadStopInfo(ThreadGDBRemote *thread) { return false; } +void ProcessGDBRemote::ParseExpeditedRegisters( + ExpeditedRegisterMap &expedited_register_map, ThreadSP thread_sp) { + ThreadGDBRemote *gdb_thread = static_cast<ThreadGDBRemote *>(thread_sp.get()); + RegisterContextSP gdb_reg_ctx_sp(gdb_thread->GetRegisterContext()); + + for (const auto &pair : expedited_register_map) { + StringExtractor reg_value_extractor(pair.second); + WritableDataBufferSP buffer_sp( + new DataBufferHeap(reg_value_extractor.GetStringRef().size() / 2, 0)); + reg_value_extractor.GetHexBytes(buffer_sp->GetData(), '\xcc'); + uint32_t lldb_regnum = gdb_reg_ctx_sp->ConvertRegisterKindToRegisterNumber( + eRegisterKindProcessPlugin, pair.first); + gdb_thread->PrivateSetRegisterValue(lldb_regnum, buffer_sp->GetData()); + } +} + ThreadSP ProcessGDBRemote::SetThreadStopInfo( lldb::tid_t tid, ExpeditedRegisterMap &expedited_register_map, uint8_t signo, const std::string &thread_name, const std::string &reason, @@ -1646,32 +1662,35 @@ ThreadSP ProcessGDBRemote::SetThreadStopInfo( reg_ctx_sp->InvalidateIfNeeded(true); + auto iter = std::find(m_thread_ids.begin(), m_thread_ids.end(), tid); + if (iter != m_thread_ids.end()) + SetThreadPc(thread_sp, iter - m_thread_ids.begin()); + + ParseExpeditedRegisters(expedited_register_map, thread_sp); + // AArch64 SVE/SME specific code below updates SVE and ZA register sizes and // offsets if value of VG or SVG registers has changed since last stop. const ArchSpec &arch = GetTarget().GetArchitecture(); if (arch.IsValid() && arch.GetTriple().isAArch64()) { - GDBRemoteRegisterContext *gdb_remote_reg_ctx = - static_cast<GDBRemoteRegisterContext *>(reg_ctx_sp.get()); + GDBRemoteRegisterContext *reg_ctx_sp = + static_cast<GDBRemoteRegisterContext *>( + gdb_thread->GetRegisterContext().get()); - if (gdb_remote_reg_ctx) { - gdb_remote_reg_ctx->AArch64Reconfigure(); - gdb_remote_reg_ctx->InvalidateAllRegisters(); + if (reg_ctx_sp) { + reg_ctx_sp->AArch64Reconfigure(); + // Now we have changed the offsets of all the registers, so the values + // will be corrupted. + reg_ctx_sp->InvalidateAllRegisters(); + + // Expedited registers values will never contain registers that would be + // resized by AArch64Reconfigure. So we are safe to continue using these + // values. These values include vg, svg and useful general purpose + // registers so this saves a few read packets each time we make use of + // them. + ParseExpeditedRegisters(expedited_register_map, thread_sp); } } - auto iter = std::find(m_thread_ids.begin(), m_thread_ids.end(), tid); - if (iter != m_thread_ids.end()) - SetThreadPc(thread_sp, iter - m_thread_ids.begin()); - - for (const auto &pair : expedited_register_map) { - StringExtractor reg_value_extractor(pair.second); - WritableDataBufferSP buffer_sp( - new DataBufferHeap(reg_value_extractor.GetStringRef().size() / 2, 0)); - reg_value_extractor.GetHexBytes(buffer_sp->GetData(), '\xcc'); - uint32_t lldb_regnum = reg_ctx_sp->ConvertRegisterKindToRegisterNumber( - eRegisterKindProcessPlugin, pair.first); - gdb_thread->PrivateSetRegisterValue(lldb_regnum, buffer_sp->GetData()); - } thread_sp->SetName(thread_name.empty() ? nullptr : thread_name.c_str()); gdb_thread->SetThreadDispatchQAddr(thread_dispatch_qaddr); diff --git a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h index f0ead4c38c237ab..f3787e7169047e2 100644 --- a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h +++ b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h @@ -470,6 +470,9 @@ class ProcessGDBRemote : public Process, void DidForkSwitchSoftwareBreakpoints(bool enable); void DidForkSwitchHardwareTraps(bool enable); + void ParseExpeditedRegisters(ExpeditedRegisterMap &expedited_register_map, + lldb::ThreadSP thread_sp); + // Lists of register fields generated from the remote's target XML. // Pointers to these RegisterFlags will be set in the register info passed // back to the upper levels of lldb. Doing so is safe because this class will _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
