Page MenuHomePhabricator

[lldb] Split out NetBSD/x86 watchpoint impl for unification
ClosedPublic

Authored by mgorny on Oct 21 2020, 5:49 AM.

Details

Summary

Split the current NetBSD watchpoint implementation for x86 into Utility,
and revamp it to improve readability. This code is meant to be used
as a common class for all x86 watchpoint implementation, particularly
these on FreeBSD and Linux.

The code uses global watchpoint enable bits, as required by the NetBSD
kernel. If it ever becomes necessary for any platform to use local
enable bits instead, this can be trivially absracted out.

The code also postpones clearing DR6 until a new different watchpoint
is being set in place of the old one. This is necessary since LLDB
repeatedly reenables watchpoints on all threads, by clearing
and restoring them. When DR6 is cleared as a part of that, then pending
events on other threads can no longer be associated with watchpoints
correctly.

Diff Detail

Event Timeline

mgorny created this revision.Oct 21 2020, 5:49 AM
mgorny requested review of this revision.Oct 21 2020, 5:49 AM
mgorny added inline comments.Oct 21 2020, 5:56 AM
lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.cpp
296 ↗(On Diff #299649)

I'm wondering if we can eliminate the init inside NativeRegisterContextRegisterInfo

lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.h
110 ↗(On Diff #299649)

I have changed this to facilitate WriteRegister(). Alternatively, I suppose we could reverse the loop to match ReadRegister().

lldb/source/Plugins/Process/Utility/WatchpointMixin_x86.cpp
146–155 ↗(On Diff #299649)

This is the DR6 cleanup I was talking about.

183 ↗(On Diff #299649)

On NetBSD, we have to call this explicitly when processing watchpoint hit (in SIGTRAP handling). Not sure if Linux would need that too (in which case we'd have to add a virtual method to the base class, I guess), or if the kernel takes care of it.

I like this, and I think this is in a pretty good shape as it stands. The thing I'm not sure of is the naming of the class. I'd probably name it something like NativeRegisterContext_x86 (and hope that it can include non-watchpoint functionality in the future). As it stands now, this is not really a mix-in but a full register context class and in theory one can imagine that some target which only supports a single architecture will not bother with the NativeRegisterContextLinux stuff, but inherit straight from NativeRegisterContext_x86. (It's comment can allude to the fact that it is indented to be mixed with subclasses implementing os-specific functionality, which will also help explaining the virtual inheritance.)

lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux.h
18 ↗(On Diff #299649)

I believe the more common spelling is public virtual

lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.cpp
296 ↗(On Diff #299649)

Actually I'd delete the constructor of NativeRegisterContextLinux.

C++ standard says:

An abstract base class is never a most derived class, this its constructors never initialize virtual base classes, therefore the corresponding mem-initializers may be omitted.

Since the only thing that constructor does (used to do) is call NativeRegisterContextRegisterInfo's ctor, we can just delete it.

lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.h
110 ↗(On Diff #299649)

I think this is fine.

lldb/source/Plugins/Process/Utility/WatchpointMixin_x86.cpp
9 ↗(On Diff #299649)

This class won't be using any os- or arch-specific features, so we can just drop these.

146–155 ↗(On Diff #299649)

This seems fine. Maybe worth mentioning that this is here to avoid confusing the NetBSD kernel.

183 ↗(On Diff #299649)

No clue. If it's not done currently, then I guess it's not needed.

I like this, and I think this is in a pretty good shape as it stands. The thing I'm not sure of is the naming of the class. I'd probably name it something like NativeRegisterContext_x86 (and hope that it can include non-watchpoint functionality in the future). As it stands now, this is not really a mix-in but a full register context class and in theory one can imagine that some target which only supports a single architecture will not bother with the NativeRegisterContextLinux stuff, but inherit straight from NativeRegisterContext_x86. (It's comment can allude to the fact that it is indented to be mixed with subclasses implementing os-specific functionality, which will also help explaining the virtual inheritance.)

I don't have a strong opinion here. I went for the mixin approach since that was your suggest wrt register caching. I suppose in this case it doesn't matter much but it could make things easier (or harder) as we have more potentially disjoint functions and partially transitioned source tree.

mgorny marked 4 inline comments as done.Oct 21 2020, 8:38 AM
mgorny added inline comments.
lldb/source/Plugins/Process/Utility/WatchpointMixin_x86.cpp
9 ↗(On Diff #299649)

Does it make sense to compile code that isn't going to be used on other platforms?

146–155 ↗(On Diff #299649)

I'm going to go through all the code and see what can be improved and/or documented better.

183 ↗(On Diff #299649)

I think it might have been done as part of the implicit disable/reenable.

mgorny updated this revision to Diff 299704.Oct 21 2020, 8:39 AM
mgorny marked an inline comment as done.

Deleted unnecessary constructor and switched public/virtual order. More changes coming.

For the record, I'm planning to clang-format the code at the very end.

krytarowski added inline comments.Oct 21 2020, 8:56 AM
lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.cpp
580 ↗(On Diff #299704)

Can we have IsGPR(reg_index) || IsDR(reg_index)?

mgorny updated this revision to Diff 299731.Oct 21 2020, 9:59 AM

Revamped the whole file to hopefully make it a bit less magical.

  • Moved all the constants into inline functions and one constexpr var.
  • Added more 'visual' comments what bits are touched.
  • Added comments about NetBSD-specific hacks.
  • Inlined RegisterInfo magic into GetDR().
  • Reduced the number of local variables, and renamed reg_value variables to clearly indicate which reg it is.
  • Removed duplicate cleaning of DR6 when clearing all hw watchpoints — the cleanup is done on adding new watchpoints anyway.
lldb/include/lldb/lldb-defines.h
57–58 ↗(On Diff #299731)

I'm wondering if we could change these two values to avoid having separate LLDB_GDB.... I suppose it could break if API clients relied on them.

lldb/source/Plugins/Process/Utility/WatchpointMixin_x86.cpp
59–60 ↗(On Diff #299704)

Technically this could be replaced by break, and return error below but I suppose it's more readable as-is.

Adding more people in case they wanted to chime in, as this code will be reused on FreeBSD and NetBSD too.

mgorny updated this revision to Diff 299800.Oct 21 2020, 2:00 PM

Migrate NetBSD plugin as well. This implied adding ClearWatchpointHit() as a virtual method to the base class.

This looks very nice.

I like this, and I think this is in a pretty good shape as it stands. The thing I'm not sure of is the naming of the class. I'd probably name it something like NativeRegisterContext_x86 (and hope that it can include non-watchpoint functionality in the future). As it stands now, this is not really a mix-in but a full register context class and in theory one can imagine that some target which only supports a single architecture will not bother with the NativeRegisterContextLinux stuff, but inherit straight from NativeRegisterContext_x86. (It's comment can allude to the fact that it is indented to be mixed with subclasses implementing os-specific functionality, which will also help explaining the virtual inheritance.)

I don't have a strong opinion here. I went for the mixin approach since that was your suggest wrt register caching. I suppose in this case it doesn't matter much but it could make things easier (or harder) as we have more potentially disjoint functions and partially transitioned source tree.

Ok, I see what you mean. We can keep this class watchpoint-specific, and re-evaluate later. I'd still like to have "NativeRegisterContext" in the name, though...

Migrate NetBSD plugin as well.

It might be reasonable to land these as separate changes -- to reduce churn if anything breaks. Maybe start with NetBSD, since on linux you'll also have to update all NativeRegisterContextLinux_ARCH classes ?

lldb/include/lldb/lldb-defines.h
62–65 ↗(On Diff #299800)

I don't want to add any more defines than what we already have. The modern way to handle this would be via an enum. But that's best left for a separate patch. Let's just revert this here.

lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.cpp
580 ↗(On Diff #299704)

Yeah, that does sound better.

lldb/source/Plugins/Process/Utility/WatchpointMixin_x86.cpp
9 ↗(On Diff #299649)

It helps with maintenance because people can check if it at least compiles. For example, this change (making NativeRegisterContextLinux inherit virtually) will require changes in all of the NativeRegisterContextLinux_ARCH constructors, but those will have to be done blindly. We could even think of adding unit tests for this stuff, if we wanted to be super fancy...

At the same time, I don't think this hurts much because even the simplest form of dead code removal should be able to strip a file thats completely unreferenced.

mgorny updated this revision to Diff 299950.Oct 22 2020, 6:32 AM
mgorny retitled this revision from [lldb] Unify x86 watchpoint implementation on Linux and NetBSD (and future FreeBSD plugin) [WIP] to [lldb] Split out NetBSD/x86 watchpoint impl for unification.
mgorny edited the summary of this revision. (Show Details)

Updated per comments, made it NetBSD-only for now.

labath accepted this revision.Oct 23 2020, 2:16 AM

Looks good. Just don't forget to reformat the patch.

This revision is now accepted and ready to land.Oct 23 2020, 2:16 AM
mgorny updated this revision to Diff 300199.Oct 23 2020, 2:26 AM

Reformatted version, uploaded for completeness.

Herald added a project: Restricted Project. · View Herald TranscriptOct 23 2020, 3:20 AM