Page MenuHomePhabricator

[lldb] [Process/FreeBSDRemote] Introduce aarch64 hw break/watchpoint support
ClosedPublic

Authored by mgorny on Feb 11 2021, 2:01 PM.

Details

Summary

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.

Diff Detail

Event Timeline

mgorny created this revision.Feb 11 2021, 2:01 PM
mgorny requested review of this revision.Feb 11 2021, 2:01 PM

@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.

mgorny updated this revision to Diff 323150.Feb 11 2021, 2:13 PM

Move NumSupportedHardwareBreakpoints and NumSupportedHardwareWatchpoints into the shared code.

mgorny updated this revision to Diff 323779.Feb 15 2021, 8:50 AM

Switch to LLDB_LOG_ERROR.

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
62–63

maybe Optional<dbreg>

mgorny added inline comments.Feb 16 2021, 2:56 PM
lldb/source/Plugins/Process/FreeBSD/NativeRegisterContextFreeBSD_arm64.h
62–63

Tried it but it feels really artificial here. I think the bool is both cleaner and results in less code.

mgorny updated this revision to Diff 324393.Feb 17 2021, 12:41 PM
mgorny retitled this revision from [lldb] [Process/FreeBSDRemote] Introduce aarch64 hw break/watchpoint support [WIP] to [lldb] [Process/FreeBSDRemote] Introduce aarch64 hw break/watchpoint support.
mgorny added a reviewer: mhorne.

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

Oh, and added the method to copy dbregs to new threads.

mgorny updated this revision to Diff 324394.Feb 17 2021, 12:42 PM

clang-format.

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
58

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
662

Redundant else after return.

lldb/source/Plugins/Process/Utility/NativeRegisterContextBreakWatchpoint_arm64.cpp
442

Redundant else after return.

457

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.

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.

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 ;-).

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.

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.

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 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.

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.

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.

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.

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.

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)

mgorny marked 5 inline comments as done.Feb 19 2021, 9:52 AM
mgorny added inline comments.
lldb/source/Plugins/Process/Utility/NativeRegisterContextBreakWatchpoint_arm64.cpp
249

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 ,
we currently set addr = 0x2000 and size = 4 and therefore watch 0x2000..0x2003
but we could instead set BAS to 0b00001100 to watch 0x2002..0x2003.

Am I grasping this correctly?

mgorny updated this revision to Diff 325021.Feb 19 2021, 9:53 AM
  • 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.

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.

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)

SBPlatform::GetOS{Major,Minor,Update}Version ?

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.

Sounds reasonable to me. Renaming the x86 class as well would be great.

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.

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)

SBPlatform::GetOS{Major,Minor,Update}Version ?

That comes from uname as well, so no difference. However, I'm going to replace the current code with it for reuse.

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.

Sounds reasonable to me. Renaming the x86 class as well would be great.

I'll do x86 as a separate patch.

mgorny updated this revision to Diff 325500.Feb 22 2021, 11:11 AM

Renamed the utility class/file and switched the test to use SBPlatform.

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.

Gentle ping.

omjavaid accepted this revision.Mar 4 2021, 1:56 AM
This revision is now accepted and ready to land.Mar 4 2021, 1:56 AM
mgorny updated this revision to Diff 328189.Mar 4 2021, 8:38 AM

Updated version conditions since watchpoint support was backported to FreeBSD 13.0-BETA4

mhorne added inline comments.Mar 4 2021, 5:29 PM
lldb/packages/Python/lldbsuite/test/dotest.py
813

It seems like this should become 13.0.

lldb/source/Plugins/Process/FreeBSD/NativeRegisterContextFreeBSD_arm64.h
27

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.

mgorny updated this revision to Diff 328436.Mar 5 2021, 1:24 AM

Updated to enable on FreeBSD 13+ unconditionally. I've left the #define to avoid repeating the version in a few places.

mgorny marked 2 inline comments as done.Mar 5 2021, 1:24 AM
This revision was landed with ongoing or failed builds.Mar 10 2021, 9:36 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMar 10 2021, 9:36 AM