-
Notifications
You must be signed in to change notification settings - Fork 14.3k
[LLDB][LoongArch] Extend the maximum number of watchpoints #126204
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@llvm/pr-subscribers-lldb Author: Tiezhu Yang (seehearfeel) ChangesThe maximum number of load/store watchpoints and fetch instruction A new struct user_watch_state_v2 was added into uapi in the related In order to avoid undefined or redefined error, just add a new struct As far as I can tell, the only users for this struct in the userspace The compatibility problem has been considered while developing and
For example, there are four kind of combinations, all of them work well. (1) "older kernel + older app", the actual length is 8+(8+8+4+4)*8=200; Signed-off-by: Tiezhu Yang <[email protected]> Full diff: https://github.com/llvm/llvm-project/pull/126204.diff 1 Files Affected:
diff --git a/lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_loongarch64.cpp b/lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_loongarch64.cpp
index 601dde250094892..c197da595b984f2 100644
--- a/lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_loongarch64.cpp
+++ b/lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_loongarch64.cpp
@@ -50,6 +50,19 @@
#define REG_CONTEXT_SIZE \
(GetGPRSize() + GetFPRSize() + sizeof(m_lsx) + sizeof(m_lasx))
+// In order to avoid undefined or redefined error, just add a new struct
+// loongarch_user_watch_state in LLDB which is same with the uapi struct
+// user_watch_state_v2.
+struct loongarch_user_watch_state {
+ uint64_t dbg_info;
+ struct {
+ uint64_t addr;
+ uint64_t mask;
+ uint32_t ctrl;
+ uint32_t pad;
+ } dbg_regs[14];
+};
+
using namespace lldb;
using namespace lldb_private;
using namespace lldb_private::process_linux;
@@ -539,7 +552,7 @@ llvm::Error NativeRegisterContextLinux_loongarch64::ReadHardwareDebugInfo() {
int regset = NT_LOONGARCH_HW_WATCH;
struct iovec ioVec;
- struct user_watch_state dreg_state;
+ struct loongarch_user_watch_state dreg_state;
Status error;
ioVec.iov_base = &dreg_state;
@@ -567,7 +580,7 @@ llvm::Error NativeRegisterContextLinux_loongarch64::ReadHardwareDebugInfo() {
llvm::Error NativeRegisterContextLinux_loongarch64::WriteHardwareDebugRegs(
DREGType hwbType) {
struct iovec ioVec;
- struct user_watch_state dreg_state;
+ struct loongarch_user_watch_state dreg_state;
int regset;
memset(&dreg_state, 0, sizeof(dreg_state));
|
Thanks for the clear explanation. Part of my confusion before was that I mistakenly thought that the v2 struct added a padding field but this is not correct. The only thing that has changed is the number of debug regs. We send one |
@@ -50,6 +50,19 @@ | |||
#define REG_CONTEXT_SIZE \ | |||
(GetGPRSize() + GetFPRSize() + sizeof(m_lsx) + sizeof(m_lasx)) | |||
|
|||
// In order to avoid undefined or redefined error, just add a new struct | |||
// loongarch_user_watch_state in LLDB which is same with the uapi struct | |||
// user_watch_state_v2. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should reference user_watch_state
version 1 and say that all that changed is the size of dbg_regs. Something like:
// ptrace has a type user_watch_state, which was replaced by user_watch_state_v2 when more watchpoints
// were added. So this file may be built on systems with one or both in the headers. The type below has the same
// layout as user_watch_v2 but will not clash with that name if it exists. We can use the v2 layout even on old
// kernels as we will only see 8 watchpoints and the kernel will truncate any extra data we send to it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, let me update it.
LGTM, but @SixWeining for the final approval as the arch expert. |
The maximum number of load/store watchpoints and fetch instruction watchpoints is 14 each according to LoongArch Reference Manual [1], so extend the maximum number of watchpoints from 8 to 14 for ptrace. A new struct user_watch_state_v2 was added into uapi in the related kernel commit 531936dee53e ("LoongArch: Extend the maximum number of watchpoints") [2], but there may be no struct user_watch_state_v2 in the system header in time. In order to avoid undefined or redefined error, just add a new struct loongarch_user_watch_state in LLDB which is same with the uapi struct user_watch_state_v2, then replace the current user_watch_state with loongarch_user_watch_state. As far as I can tell, the only users for this struct in the userspace are GDB and LLDB, there are no any problems of software compatibility between the application and kernel according to the analysis. The compatibility problem has been considered while developing and testing. When the applications in the userspace get watchpoint state, the length will be specified which is no bigger than the sizeof struct user_watch_state or user_watch_state_v2, the actual length is assigned as the minimal value of the application and kernel in the generic code of ptrace: ``` kernel/ptrace.c: ptrace_regset(): kiov->iov_len = min(kiov->iov_len, (__kernel_size_t) (regset->n * regset->size)); if (req == PTRACE_GETREGSET) return copy_regset_to_user(task, view, regset_no, 0, kiov->iov_len, kiov->iov_base); else return copy_regset_from_user(task, view, regset_no, 0, kiov->iov_len, kiov->iov_base); ``` For example, there are four kind of combinations, all of them work well. (1) "older kernel + older app", the actual length is 8+(8+8+4+4)*8=200; (2) "newer kernel + newer app", the actual length is 8+(8+8+4+4)*14=344; (3) "older kernel + newer app", the actual length is 8+(8+8+4+4)*8=200; (4) "newer kernel + older app", the actual length is 8+(8+8+4+4)*8=200. [1] https://loongson.github.io/LoongArch-Documentation/LoongArch-Vol1-EN.html#control-and-status-registers-related-to-watchpoints [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=531936dee53e Signed-off-by: Tiezhu Yang <[email protected]>
The maximum number of load/store watchpoints and fetch instruction watchpoints is 14 each according to LoongArch Reference Manual [1], so extend the maximum number of watchpoints from 8 to 14 for ptrace. A new struct user_watch_state_v2 was added into uapi in the related kernel commit 531936dee53e ("LoongArch: Extend the maximum number of watchpoints") [2], but there may be no struct user_watch_state_v2 in the system header in time. In order to avoid undefined or redefined error, just add a new struct loongarch_user_watch_state in LLDB which is same with the uapi struct user_watch_state_v2, then replace the current user_watch_state with loongarch_user_watch_state. As far as I can tell, the only users for this struct in the userspace are GDB and LLDB, there are no any problems of software compatibility between the application and kernel according to the analysis. The compatibility problem has been considered while developing and testing. When the applications in the userspace get watchpoint state, the length will be specified which is no bigger than the sizeof struct user_watch_state or user_watch_state_v2, the actual length is assigned as the minimal value of the application and kernel in the generic code of ptrace: ``` kernel/ptrace.c: ptrace_regset(): kiov->iov_len = min(kiov->iov_len, (__kernel_size_t) (regset->n * regset->size)); if (req == PTRACE_GETREGSET) return copy_regset_to_user(task, view, regset_no, 0, kiov->iov_len, kiov->iov_base); else return copy_regset_from_user(task, view, regset_no, 0, kiov->iov_len, kiov->iov_base); ``` For example, there are four kind of combinations, all of them work well. (1) "older kernel + older app", the actual length is 8+(8+8+4+4)*8=200; (2) "newer kernel + newer app", the actual length is 8+(8+8+4+4)*14=344; (3) "older kernel + newer app", the actual length is 8+(8+8+4+4)*8=200; (4) "newer kernel + older app", the actual length is 8+(8+8+4+4)*8=200. [1] https://loongson.github.io/LoongArch-Documentation/LoongArch-Vol1-EN.html#control-and-status-registers-related-to-watchpoints [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=531936dee53e Signed-off-by: Tiezhu Yang <[email protected]>
The maximum number of load/store watchpoints and fetch instruction
watchpoints is 14 each according to LoongArch Reference Manual [1],
so extend the maximum number of watchpoints from 8 to 14 for ptrace.
A new struct user_watch_state_v2 was added into uapi in the related
kernel commit 531936dee53e ("LoongArch: Extend the maximum number of
watchpoints") [2], but there may be no struct user_watch_state_v2 in
the system header in time.
In order to avoid undefined or redefined error, just add a new struct
loongarch_user_watch_state in LLDB which is same with the uapi struct
user_watch_state_v2, then replace the current user_watch_state with
loongarch_user_watch_state.
As far as I can tell, the only users for this struct in the userspace
are GDB and LLDB, there are no any problems of software compatibility
between the application and kernel according to the analysis.
The compatibility problem has been considered while developing and
testing. When the applications in the userspace get watchpoint state,
the length will be specified which is no bigger than the sizeof struct
user_watch_state or user_watch_state_v2, the actual length is assigned
as the minimal value of the application and kernel in the generic code
of ptrace:
For example, there are four kind of combinations, all of them work well.
(1) "older kernel + older app", the actual length is 8+(8+8+4+4)*8=200;
(2) "newer kernel + newer app", the actual length is 8+(8+8+4+4)*14=344;
(3) "older kernel + newer app", the actual length is 8+(8+8+4+4)*8=200;
(4) "newer kernel + older app", the actual length is 8+(8+8+4+4)*8=200.
[1] https://loongson.github.io/LoongArch-Documentation/LoongArch-Vol1-EN.html#control-and-status-registers-related-to-watchpoints
[2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=531936dee53e
Signed-off-by: Tiezhu Yang <[email protected]>