Split out the common base of Linux hardware breakpoint/watchpoint
support for AArch64 into a Utility class, and use it to implement
the matching support on FreeBSD.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
@labath, the Linux/common part is working here. FreeBSD is still pending further kernel changes, so I'm not going to commit it until all the kernel patches are committed, and then I'll add appropriate #ifs.
In the meantime, please review the utility part. This is roughly the Linux code, minus the two functions to read/write dbregs, and… I've just realized that getters for number of break/watchpoints can be made common now. So another update incoming soon. In any case, the only real change in the code is using llvm::Error in new API.
Move NumSupportedHardwareBreakpoints and NumSupportedHardwareWatchpoints into the shared code.
I'm torn about the ReadHardwareDebugInfo interface. On one hand, there's no use case now for distinguishing the failure to retrieve breakpoint info from successfully determining that we support zero breakpoints. So we could make the function return void, move the log statement into the function, and simplify a bunch of stuff. OTOH, if those functions are later changed to propagate errors, they would become simpler too, and we might be able to forward this error all the way to the user...
Apart from that, I think this is fine, but maybe @omjavaid could take a quick look.
lldb/source/Plugins/Process/FreeBSD/NativeRegisterContextFreeBSD_arm64.h | ||
---|---|---|
67–68 | maybe Optional<dbreg> |
lldb/source/Plugins/Process/FreeBSD/NativeRegisterContextFreeBSD_arm64.h | ||
---|---|---|
67–68 | Tried it but it feels really artificial here. I think the bool is both cleaner and results in less code. |
Ok, the changes have landed in FreeBSD src, so I think we're ready to go here.
Changes from previous version:
- enabled hardware breakpoints on FreeBSD
- changed the existing names to use hbp consistently (instead of mix of hbp and hbr)
- added FreeBSD version checks
The FreeBSD API usage looks correct to me, but I can't comment on too much more than that. The code could benefit from fewer unnamed constants.
I should also note that it is my intention to merge the FreeBSD watchpoint patches into the 13.0 branch, in about a week's time. So your __FreeBSD_version checks will need to be updated after that point.
lldb/source/Plugins/Process/Utility/NativeRegisterContextBreakWatchpoint_arm64.cpp | ||
---|---|---|
57 ↗ | (On Diff #324394) | These magic numbers would be better with names. I see quite a few instances of checking/setting the enable bit, but it is not obvious what is being done unless you already understand the control register layout. |
clang-tidy would not be happy;)
lldb/source/Plugins/Process/FreeBSD/NativeProcessFreeBSD.cpp | ||
---|---|---|
663–664 | Redundant else after return. | |
lldb/source/Plugins/Process/Utility/NativeRegisterContextBreakWatchpoint_arm64.cpp | ||
441 ↗ | (On Diff #324394) | Redundant else after return. |
456 ↗ | (On Diff #324394) | Redundant else after return. |
This looks fine to me builds ok on AArch64/Linux with no testsuite regressions.
I am wondering Whats the need for version checks? you are excluding hardware breakpoint mangement code from the build but then that lldb-server executable may well run on FreeBSD version which actually supports HW breakpoints. Isnt there a dynamic way of checking support for hardware break/watch points. May be query hardware debug registers and if it fails just disable hardware breakpoints for that thread.
Moreover, can we shrink class name NativeRegisterContextBreakWatchpoint_arm64 to may be NativeDebugRegisterContext_arm64.
The code relies on struct members that are only present with this version. I'd like to avoid copying implementation details like that into the code.
Moreover, can we shrink class name NativeRegisterContextBreakWatchpoint_arm64 to may be NativeDebugRegisterContext_arm64.
I generally leave naming decisions to @labath ;-).
Only concern here is that someone might be cross-compiling lldb-server on an older system but eventually using it on a target which supports HW break/watchpoints. What chances do you think we have for such a scenario? if this is rare then this should be ok.
Moreover, can we shrink class name NativeRegisterContextBreakWatchpoint_arm64 to may be NativeDebugRegisterContext_arm64.
I generally leave naming decisions to @labath ;-).
Lets see what @labath has to say... IMO Debug Register is a general term used in other architecture specs for referring to hardware debug capabilities like breakpoints watchpoints etc.
I don't think it's very likely. @emaste, any opinion on this?
Moreover, can we shrink class name NativeRegisterContextBreakWatchpoint_arm64 to may be NativeDebugRegisterContext_arm64.
I generally leave naming decisions to @labath ;-).
Lets see what @labath has to say... IMO Debug Register is a general term used in other architecture specs for referring to hardware debug capabilities like breakpoints watchpoints etc.
I agree. For consistency with the x86 class, maybe NativeRegisterContextDBReg_arm64? I wonder if we should rename the x86 class too.
Do you have any suggestion how we could check for kernel version from Python at runtime? (to determine whether we should expect watchpoint tests to work)
lldb/source/Plugins/Process/Utility/NativeRegisterContextBreakWatchpoint_arm64.cpp | ||
---|---|---|
248 ↗ | (On Diff #324394) | That's something to consider for a followup work but I'm thinking that instead of changing size here we could set bit-mask appropriately to watch only the exact bytes we need. If I read the manual correctly (https://developer.arm.com/documentation/ddi0595/2020-12/AArch64-Registers/DBGWCR-n--EL1--Debug-Watchpoint-Control-Registers?lang=en#fieldset_0-12_5), this should be doable. e.g. if addr = 0x2002 and size = 2 , Am I grasping this correctly? |
- fixed broken FreeBSD release check in lldb
- fixed UB in FreeBSD's bp/wp impl
- implemented changes requested in comments
- refactored bp/wp code to use a few helper constants and utility functions
Please let me know if you think I should change more stuff here.
SBPlatform::GetOS{Major,Minor,Update}Version ?
Sounds reasonable to me. Renaming the x86 class as well would be great.
That comes from uname as well, so no difference. However, I'm going to replace the current code with it for reuse.
Sounds reasonable to me. Renaming the x86 class as well would be great.
I'll do x86 as a separate patch.
I don't think it's very likely. @emaste, any opinion on this?
I think it is fine to make this conditional on version. We are working on bringing arm64 to Tier-1 status in FreeBSD 13 (which will have the referenced kernel changes) and we can assume that as a minimum version for host and target.
Updated version conditions since watchpoint support was backported to FreeBSD 13.0-BETA4
lldb/packages/Python/lldbsuite/test/dotest.py | ||
---|---|---|
807 | It seems like this should become 13.0. | |
lldb/source/Plugins/Process/FreeBSD/NativeRegisterContextFreeBSD_arm64.h | ||
28 | This is technically correct, but I don't think it's important to make provisions for versions 1400000 to 1400004. The reason is that these represent development versions in a < 2 month time period. Anyone affected by this would simply be asked to update their kernel to a more recent snapshot. __FreeBSD_version >= 1300139 should be enough. |
Updated to enable on FreeBSD 13+ unconditionally. I've left the #define to avoid repeating the version in a few places.
It seems like this should become 13.0.