[lldb][AArch64] Fix arm64 hardware breakpoint/watchpoint to arm32 process. #147198
Conversation
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
|
@llvm/pr-subscribers-lldb Author: None (b10902118) ChangesThis bug skips the test because the wrong ptrace call to detect avaliable hardware/breakpoint number that resuls in 0. After tracing linux's compat_ptrace in arch/arm64/kernel/ptrace.c, found that arm64 lldb-server should just keep using the ptrace commands for 64bit tracees. See: torvalds/linux@5d220ff So the solution is copying the implementation in NativeRegisterContextLinux_arm64.cpp. Full diff: https://github.com/llvm/llvm-project/pull/147198.diff 2 Files Affected:
diff --git a/lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm.cpp b/lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm.cpp
index dc7fb103e87c0..9123a577008bd 100644
--- a/lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm.cpp
+++ b/lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm.cpp
@@ -23,13 +23,18 @@
#include <elf.h>
#include <sys/uio.h>
+#if defined(__arm64__) || defined(__aarch64__)
+#include "lldb/Host/linux/Ptrace.h"
+#include <asm/ptrace.h>
+#endif
+
#define REG_CONTEXT_SIZE (GetGPRSize() + sizeof(m_fpr))
#ifndef PTRACE_GETVFPREGS
#define PTRACE_GETVFPREGS 27
#define PTRACE_SETVFPREGS 28
#endif
-#ifndef PTRACE_GETHBPREGS
+#if defined(__arm__) && !defined(PTRACE_GETHBPREGS)
#define PTRACE_GETHBPREGS 29
#define PTRACE_SETHBPREGS 30
#endif
@@ -723,6 +728,7 @@ Status NativeRegisterContextLinux_arm::ReadHardwareDebugInfo() {
return Status();
}
+#ifdef __arm__
unsigned int cap_val;
error = NativeProcessLinux::PtraceWrapper(PTRACE_GETHBPREGS, m_thread.GetID(),
@@ -737,12 +743,43 @@ Status NativeRegisterContextLinux_arm::ReadHardwareDebugInfo() {
m_refresh_hwdebug_info = false;
return error;
+#else // __aarch64__
+ ::pid_t tid = m_thread.GetID();
+
+ int regset = NT_ARM_HW_WATCH;
+ struct iovec ioVec;
+ struct user_hwdebug_state dreg_state;
+
+ ioVec.iov_base = &dreg_state;
+ ioVec.iov_len = sizeof(dreg_state);
+
+ error = NativeProcessLinux::PtraceWrapper(PTRACE_GETREGSET, tid, ®set,
+ &ioVec, ioVec.iov_len);
+
+ if (error.Fail())
+ return error;
+
+ m_max_hwp_supported = dreg_state.dbg_info & 0xff;
+
+ regset = NT_ARM_HW_BREAK;
+ error = NativeProcessLinux::PtraceWrapper(PTRACE_GETREGSET, tid, ®set,
+ &ioVec, ioVec.iov_len);
+
+ if (error.Fail())
+ return error;
+
+ m_max_hbp_supported = dreg_state.dbg_info & 0xff;
+ m_refresh_hwdebug_info = false;
+
+ return error;
+#endif // __arm__
}
-Status NativeRegisterContextLinux_arm::WriteHardwareDebugRegs(int hwbType,
+Status NativeRegisterContextLinux_arm::WriteHardwareDebugRegs(DREGType hwbType,
int hwb_index) {
Status error;
+#ifdef __arm__
lldb::addr_t *addr_buf;
uint32_t *ctrl_buf;
@@ -781,6 +818,40 @@ Status NativeRegisterContextLinux_arm::WriteHardwareDebugRegs(int hwbType,
}
return error;
+#else // __aarch64__
+ struct iovec ioVec;
+ struct user_hwdebug_state dreg_state;
+ int regset;
+
+ memset(&dreg_state, 0, sizeof(dreg_state));
+ ioVec.iov_base = &dreg_state;
+
+ switch (hwbType) {
+ case eDREGTypeWATCH:
+ regset = NT_ARM_HW_WATCH;
+ ioVec.iov_len = sizeof(dreg_state.dbg_info) + sizeof(dreg_state.pad) +
+ (sizeof(dreg_state.dbg_regs[0]) * m_max_hwp_supported);
+
+ for (uint32_t i = 0; i < m_max_hwp_supported; i++) {
+ dreg_state.dbg_regs[i].addr = m_hwp_regs[i].address;
+ dreg_state.dbg_regs[i].ctrl = m_hwp_regs[i].control;
+ }
+ break;
+ case eDREGTypeBREAK:
+ regset = NT_ARM_HW_BREAK;
+ ioVec.iov_len = sizeof(dreg_state.dbg_info) + sizeof(dreg_state.pad) +
+ (sizeof(dreg_state.dbg_regs[0]) * m_max_hbp_supported);
+
+ for (uint32_t i = 0; i < m_max_hbp_supported; i++) {
+ dreg_state.dbg_regs[i].addr = m_hbr_regs[i].address;
+ dreg_state.dbg_regs[i].ctrl = m_hbr_regs[i].control;
+ }
+ break;
+ }
+
+ return NativeProcessLinux::PtraceWrapper(PTRACE_SETREGSET, m_thread.GetID(),
+ ®set, &ioVec, ioVec.iov_len);
+#endif // __arm__
}
uint32_t NativeRegisterContextLinux_arm::CalculateFprOffset(
diff --git a/lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm.h b/lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm.h
index 15b46609c286b..6d0ad38d7eb75 100644
--- a/lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm.h
+++ b/lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm.h
@@ -125,7 +125,7 @@ class NativeRegisterContextLinux_arm : public NativeRegisterContextLinux {
Status ReadHardwareDebugInfo();
- Status WriteHardwareDebugRegs(int hwbType, int hwb_index);
+ Status WriteHardwareDebugRegs(DREGType hwbType, int hwb_index);
uint32_t CalculateFprOffset(const RegisterInfo *reg_info) const;
|
|
Your analysis is correct -- the kernel uses the bitwidth of the tracer (not the tracee) when deciding whether to use the compatibility interface. However, copying the code sounds like a fairly fragile solution. It'd be better to figure out a way to share the code somehow. The x86 classes don't have this problem because NativeRegisterContextLinux_x86_64 knows how to debug both 32 and 64-bit processes. That may not be the right solution here, but it's one thing you can use for inspiration. |
|
Oh thanks you make it clear. This fix actually follows kernel's 64bit logic. I wrongly mentioned compat_ptrace, which is for 32bit tracer use old HBP commands. |
By this you mean it skips any test that requires hardware breakpoints? Or you mean that in the general course of using lldb it doesn't use hardware breakpoints because of this? (I assume you could make the test suite launch all debugees as AArch32 but seems unlikely you went to that effort) |
Yes, try putting the 64-bit code in a separate file in Process/Linux then using it in both places. Currently arm64 includes the arm header, for reasons I am not sure, so if there is a dependency, it's going the wrong way for us to use it for this. |
Oh sorry please ignore that. I mixed lldb's test up with other sources (probably android's), making a wrong comment. |
The include of arm header is just for the factory function |
|
That's correct, and my earlier question basically was what if we make |
If arm64 did not have umpteen kinds of registers maybe, but going by the header the only common bit would be the detection and writing of breakpoint registers (for some reason I thought we would also read them back but no of course we don't need to do that). I haven't tried actually debugging something though so maybe there's more here that doesn't work. ...if there is a way to hack the test suite config to produce AArch32 that's one way to find that out. CMake options LLDB_TEST_COMPILER and LLDB_TEST_ARCH maybe. See how terrible the results are. |
|
Sorry for the delay. I tried to first let |
|
I have run API tests on watchpoint ( The new commit message is for this PR. To merge, please use that. (I have no commit access) |
lldb/source/Plugins/Process/Linux/NativeRegisterContextLinuxArm64Shared.cpp
Outdated
Show resolved
Hide resolved
lldb/source/Plugins/Process/Linux/NativeRegisterContextLinuxArm64Shared.cpp
Outdated
Show resolved
Hide resolved
lldb/source/Plugins/Process/Linux/NativeRegisterContextLinuxArm64Shared.cpp
Outdated
Show resolved
Hide resolved
lldb/source/Plugins/Process/Linux/NativeRegisterContextLinuxArm64Shared.h
Outdated
Show resolved
Hide resolved
lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm.h
Outdated
Show resolved
Hide resolved
lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm.h
Outdated
Show resolved
Hide resolved
DavidSpickett
left a comment
DavidSpickett
left a comment
There was a problem hiding this comment.
Some small comments and you'll need to rebase when the cleanup PR lands. It's good to go in once that's done.
lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64dbreg.h
Outdated
Show resolved
Hide resolved
lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm.cpp
Outdated
Show resolved
Hide resolved
lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64dbreg.cpp
Outdated
Show resolved
Hide resolved
|
From the PR description:
Reword this so it's a bit clearer. I get what you mean now, but I had to read it several times. "lldb-server should keep using the ptrace commands intended for 64-bit bit tracees, even when debugging a 32-bit tracee" |
|
Linux CI failure is: |
Sure! Sorry for causing the confusion. I have updated the description with the new message in the second commit and your suggestion. |
|
I am fixing the ci failure. Just wonder how you guys prevent this? I can build locally and clangd also didn't complain in my editor. |
|
Yeah that is odd. Is it because Edit: because the CI is x86_64. |
Thanks for pointing that out. The failure is probably cause by the missing |
|
Oh no I massed it up with github's conflict resolve. I still have to fix my previous no-reply commit. |
b10902118
force-pushed
the
main
branch
2 times, most recently
from
d2bb930 to
4fdef06
Compare
…cess. This bug skips the test because the wrong ptrace call to detect avaliable hardware/breakpoint number that resuls in 0. After tracing linux's compat_ptrace in arch/arm64/kernel/ptrace.c, found that arm64 lldb-server should just keep using the ptrace commands for 64bit tracees. See: torvalds/linux@5d220ff So the solution is copying the implementation in NativeRegisterContextLinux_arm64.cpp.
…n arm64. When debugging arm32 process on arm64 machine, arm64 lldb-server will use `NativeRegisterContextLinux_arm`, but it should use 64-bit ptrace commands for hardware watchpoint/breakpoint. See: torvalds/linux@5d220ff There have been many conditional compilation handling arm32 debuggees on arm64, but this one is missed out. To reuse the 64-bit implementation, I separate the shared code from `NativeRegisterContextLinux_arm64.cpp` to `NativeRegisterContextLinuxArm64Shared.cpp`, with other adjustments to share data structures of debug registers.
move includes to .cpp and remove <sys/ptrace> that causes error when build.
|
Sorry again. I made a big mass. Just tried to recover the history but... |
|
If it helps, people (including me) have made much bigger messes :) We're all good, going to merge this now. |
|
@b10902118 Congratulations on having your first Pull Request (PR) merged into the LLVM Project! Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR. Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues. How to do this, and the rest of the post-merge process, is covered in detail here. If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again. If you don't get any reports, no action is required from you. Your changes are working as expected, well done! |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/59/builds/21747 Here is the relevant piece of the build log for the reference |
|
Ignore that failure, it "fixed" itself. |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/141/builds/10458 Here is the relevant piece of the build log for the reference |
I have rebased those changes to include this and found #151973 along the way. Once that's in I'll put up a PR to port Arm to NativeRegisterContextDBReg. |
|
Thanks! That will be helpful to my arm stuff. I am looking forward to try that. |
5 participants
When debugging arm32 process on arm64 machine, arm64 lldb-server will use
NativeRegisterContextLinux_arm, but the server should keep using 64-bit ptrace commands for hardware watchpoint/breakpoint, even when debugging a 32-bit tracee.See: torvalds/linux@5d220ff
There have been many conditional compilation handling arm32 tracee on arm64, but this one is missed out.
To reuse the 64-bit implementation, I separate the shared code from
NativeRegisterContextLinux_arm64.cpptoNativeRegisterContextLinux_arm64dbreg.cpp, with other adjustments to share data structures of debug registers.