[lldb] Add RegisterCheckpoint overload for register methods in RegisterContextThreadMemory #132079
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…erContextThreadMemory These allow for more efficient saving/restoring state after an expression is evaluated.
|
@llvm/pr-subscribers-lldb Author: Felipe de Azevedo Piovezan (felipepiovezan) ChangesThese allow for more efficient saving/restoring state after an expression is evaluated. Full diff: https://github.com/llvm/llvm-project/pull/132079.diff 2 Files Affected:
diff --git a/lldb/source/Plugins/Process/Utility/RegisterContextThreadMemory.cpp b/lldb/source/Plugins/Process/Utility/RegisterContextThreadMemory.cpp
index abddae5d9ced4..75438550ce914 100644
--- a/lldb/source/Plugins/Process/Utility/RegisterContextThreadMemory.cpp
+++ b/lldb/source/Plugins/Process/Utility/RegisterContextThreadMemory.cpp
@@ -114,6 +114,14 @@ bool RegisterContextThreadMemory::ReadAllRegisterValues(
return false;
}
+bool RegisterContextThreadMemory::ReadAllRegisterValues(
+ lldb_private::RegisterCheckpoint ®_checkpoint) {
+ UpdateRegisterContext();
+ if (m_reg_ctx_sp)
+ return m_reg_ctx_sp->ReadAllRegisterValues(reg_checkpoint);
+ return false;
+}
+
bool RegisterContextThreadMemory::WriteAllRegisterValues(
const lldb::DataBufferSP &data_sp) {
UpdateRegisterContext();
@@ -122,6 +130,14 @@ bool RegisterContextThreadMemory::WriteAllRegisterValues(
return false;
}
+bool RegisterContextThreadMemory::WriteAllRegisterValues(
+ const lldb_private::RegisterCheckpoint ®_checkpoint) {
+ UpdateRegisterContext();
+ if (m_reg_ctx_sp)
+ return m_reg_ctx_sp->WriteAllRegisterValues(reg_checkpoint);
+ return false;
+}
+
bool RegisterContextThreadMemory::CopyFromRegisterContext(
lldb::RegisterContextSP reg_ctx_sp) {
UpdateRegisterContext();
diff --git a/lldb/source/Plugins/Process/Utility/RegisterContextThreadMemory.h b/lldb/source/Plugins/Process/Utility/RegisterContextThreadMemory.h
index 0a7314528f0ae..23f675508cf38 100644
--- a/lldb/source/Plugins/Process/Utility/RegisterContextThreadMemory.h
+++ b/lldb/source/Plugins/Process/Utility/RegisterContextThreadMemory.h
@@ -52,8 +52,12 @@ class RegisterContextThreadMemory : public lldb_private::RegisterContext {
// so these API's should only be used when this behavior is needed.
bool ReadAllRegisterValues(lldb::WritableDataBufferSP &data_sp) override;
+ bool ReadAllRegisterValues(
+ lldb_private::RegisterCheckpoint ®_checkpoint) override;
bool WriteAllRegisterValues(const lldb::DataBufferSP &data_sp) override;
+ bool WriteAllRegisterValues(
+ const lldb_private::RegisterCheckpoint ®_checkpoint) override;
bool CopyFromRegisterContext(lldb::RegisterContextSP context);
|
|
Based on the 2013 commit that added those, I don't think there is a clean way of testing this. |
|
Here's a regression test: diff --git a/lldb/test/API/lang/swift/async/expr/TestSwiftAsyncExpressions.py b/lldb/test/API/lang/swift/async/expr/TestSwiftAsyncExpressions.py
index 1510faa5820d..8cc699b3f0bf 100644
--- a/lldb/test/API/lang/swift/async/expr/TestSwiftAsyncExpressions.py
+++ b/lldb/test/API/lang/swift/async/expr/TestSwiftAsyncExpressions.py
@@ -18,3 +18,6 @@ class TestSwiftAsyncExpressions(lldbtest.TestBase):
target, process, thread, main_bkpt = lldbutil.run_to_source_breakpoint(
self, 'break here', lldb.SBFileSpec("main.swift"))
self.expect("expr n", substrs=["42"])
+ process.Continue()
+ stop_desc = process.GetSelectedThread().GetStopDescription(1024)
+ self.assertNotIn("EXC_BAD_ACCESS", stop_desc) |
|
oh, that regression test will need to be added to the swiftlang cherrypick. |
kastiglione
approved these changes
Mar 19, 2025
Nice! I've added it to that PR. |
jasonmolenda
added a commit
that referenced
this pull request
Mar 20, 2025
) In 2013 we added the QSaveRegisterState and QRestoreRegisterState packets to checkpoint a thread's register state while executing an inferior function call, instead of using the g packet to read all registers into lldb, then the G packet to set them again after the func call. Since then, lldb has not sent g/G (except as a bug) - it either asks for registers individually (p/P) or or asks debugserver to save and restore the entire register set with these lldb extensions. Felipe recently had a codepath that fell back to using g/G and found that it does not work with the modern signed fp/sp/pc/lr registers that we can get -- it sidesteps around the clearing of the non-addressable bits that we do when reading/writing them, and results in a crash. ( #132079 ) Instead of fixing that issue, I am removing g/G from debugserver because it's not needed by lldb, and it will would be easy for future bugs to creep in to this packet that lldb should not use, but it can accidentally fall back to and result in subtle bugs. This does mean that a debugger using debugserver on darwin which doesn't use QSaveRegisterState/QRestoreRegisterState will need to fall back to reading & writing each register individually. I'm open to re-evaluating this decision if this proves to be needed, and supporting these lldb extensions is onerous.
llvm-sync bot
pushed a commit
to arm/arm-toolchain
that referenced
this pull request
Mar 20, 2025
…erver (#132127) In 2013 we added the QSaveRegisterState and QRestoreRegisterState packets to checkpoint a thread's register state while executing an inferior function call, instead of using the g packet to read all registers into lldb, then the G packet to set them again after the func call. Since then, lldb has not sent g/G (except as a bug) - it either asks for registers individually (p/P) or or asks debugserver to save and restore the entire register set with these lldb extensions. Felipe recently had a codepath that fell back to using g/G and found that it does not work with the modern signed fp/sp/pc/lr registers that we can get -- it sidesteps around the clearing of the non-addressable bits that we do when reading/writing them, and results in a crash. ( llvm/llvm-project#132079 ) Instead of fixing that issue, I am removing g/G from debugserver because it's not needed by lldb, and it will would be easy for future bugs to creep in to this packet that lldb should not use, but it can accidentally fall back to and result in subtle bugs. This does mean that a debugger using debugserver on darwin which doesn't use QSaveRegisterState/QRestoreRegisterState will need to fall back to reading & writing each register individually. I'm open to re-evaluating this decision if this proves to be needed, and supporting these lldb extensions is onerous.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
These allow for more efficient saving/restoring state after an expression is evaluated.