[LLDB] On AArch64, reconfigure register context first #70742
Conversation
On an SVE/SME the register context configuration may change after the inferior process has executed. This was handled via https://reviews.llvm.org/D159504 but it is reconfiguring and clearing the register context after we've parsed any expedited reigster values from the stop reply packet. That results in lldb having to read each register value one at a time while at that stop location, which will be a performance problem on non-local debug setups. The configuration & clearing needs to happen first. Also, update the names of the local variables for a little clarity.
|
@llvm/pr-subscribers-lldb Author: Jason Molenda (jasonmolenda) ChangesOn an SVE/SME the register context configuration may change after the inferior process has executed. This was handled via https://reviews.llvm.org/D159504 but it is reconfiguring and clearing the register context after we've parsed any expedited reigster values from the stop reply packet. That results in lldb having to read each register value one at a time while at that stop location, which will be a performance problem on non-local debug setups. The configuration & clearing needs to happen first. Also, update the names of the local variables for a little clarity. Full diff: https://github.com/llvm/llvm-project/pull/70742.diff 1 Files Affected:
diff --git a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
index 56fc5490657ea71..15bfda3e02e99e4 100644
--- a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
+++ b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
@@ -1642,9 +1642,22 @@ ThreadSP ProcessGDBRemote::SetThreadStopInfo(
}
ThreadGDBRemote *gdb_thread = static_cast<ThreadGDBRemote *>(thread_sp.get());
- RegisterContextSP gdb_reg_ctx_sp(gdb_thread->GetRegisterContext());
+ RegisterContextSP reg_ctx_sp(gdb_thread->GetRegisterContext());
- gdb_reg_ctx_sp->InvalidateIfNeeded(true);
+ reg_ctx_sp->InvalidateIfNeeded(true);
+
+ // 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());
+
+ if (gdb_remote_reg_ctx) {
+ gdb_remote_reg_ctx->AArch64Reconfigure();
+ gdb_remote_reg_ctx->InvalidateAllRegisters();
+ }
+ }
auto iter = std::find(m_thread_ids.begin(), m_thread_ids.end(), tid);
if (iter != m_thread_ids.end())
@@ -1655,25 +1668,10 @@ ThreadSP ProcessGDBRemote::SetThreadStopInfo(
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(
+ uint32_t lldb_regnum = reg_ctx_sp->ConvertRegisterKindToRegisterNumber(
eRegisterKindProcessPlugin, pair.first);
gdb_thread->PrivateSetRegisterValue(lldb_regnum, buffer_sp->GetData());
}
-
- // 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 *reg_ctx_sp =
- static_cast<GDBRemoteRegisterContext *>(
- gdb_thread->GetRegisterContext().get());
-
- if (reg_ctx_sp) {
- reg_ctx_sp->AArch64Reconfigure();
- reg_ctx_sp->InvalidateAllRegisters();
- }
- }
-
thread_sp->SetName(thread_name.empty() ? nullptr : thread_name.c_str());
gdb_thread->SetThreadDispatchQAddr(thread_dispatch_qaddr);
|
There was a problem hiding this comment.
Nice catch! Definitely happy to see us moving towards doing less unnecessary work. 😄
I know this is just code relocation but I'm a bit wary of the original code. Using static_cast to downcast like this may be UB if reg_ctx_sp.get() isn't actually backed by a GDBRemoteRegisterContext.... We should consider rewriting this to avoid potential UB in the near future.
Just an immediate reaction, the values we need to do that configuration come from the expedited registers. So at a glance, of course we would have to parse those out to be able to reconfigure. Unless you mean that we are invalidating all the other expedited registers in the process, which makes us re-read them. But also, this code isn't very clear anyway, so let me put this through the SVE/SME tests and see if there's any practical issue with it. |
And given that SVE/SME size can change every time we stop, "this stop" means "all stops". So yeah I can see the issue there. |
|
This passed all the tests, and I see a reduction in the packets sent for a single step. The value of vg is 8 and svg 4 is 4 in this case. Before: After: So this is an improvement but now I wonder why even expedite the vg and svg registers if we're going to reconfigure before parsing them, which means we send a register read anyway. Potential workarounds:
Not sure that's your responsibility to fix, so this change could go in as is and I'll look into the rest if you like. |
This one doesn't work because in changing the size of any register in the context, you invalidate the offsets of the rest. We also cannot be sure that the scalable registers are in a certain place in that context. They will be beyond general purpose registers, yes, but beyond that you could have a mix of scalable and unscalable registers depending on the target's extensions. |
This works but due to more pessimism in: We actually invalidate vg and svg before reading them again. I will check if this is still needed. So, as silly as it sounds this might be the way to go:
|
I'm fine with that, it doesn't require packets over the wire so the perf hit is tiny. Or fishing the vg and svg register values of of the expedited register list if they're there. I'm fine with abandoning this PR if you'd like to write that patch, it's not pressing that this be fixed immediately. Thanks! |
ignore that suggestion, we can't hardcode a register number here, the stub is allowed to use whatever numbers it wants for the registers and the expedited register numbers are all in the stub's numbering ordering at this point. I think your parse-reconfigure-parse plan is good. |
|
I'm gonna merge this so I can do a patch on top of this where we both understand the starting point. I think I've got to the bottom of the issues my workarounds were addressing. |
On an SVE/SME the register context configuration may change after the inferior process has executed. This was handled via https://reviews.llvm.org/D159504 but it is reconfiguring and clearing the register context after we've parsed any expedited reigster values from the stop reply packet. That results in lldb having to read each register value one at a time while at that stop location, which will be a performance problem on non-local debug setups. The configuration & clearing needs to happen first. Also, update the names of the local variables for a little clarity. (cherry picked from commit d402645)
…after-receiving-expedited-regs [LLDB] On AArch64, reconfigure register context first (llvm#70742)
On an SVE/SME the register context configuration may change after the inferior process has executed. This was handled via https://reviews.llvm.org/D159504 but it is reconfiguring and clearing the register context after we've parsed any expedited reigster values from the stop reply packet. That results in lldb having to read each register value one at a time while at that stop location, which will be a performance problem on non-local debug setups.
The configuration & clearing needs to happen first. Also, update the names of the local variables for a little clarity.