labath added a comment.
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.)
================
Comment at: lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux.h:18
-class NativeRegisterContextLinux : public NativeRegisterContextRegisterInfo {
+class NativeRegisterContextLinux : virtual public
NativeRegisterContextRegisterInfo {
public:
----------------
I believe the more common spelling is `public virtual`
================
Comment at:
lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.cpp:296
const ArchSpec &target_arch, NativeThreadProtocol &native_thread)
- : NativeRegisterContextLinux(native_thread,
+ : NativeRegisterContextRegisterInfo(native_thread,
CreateRegisterInfoInterface(target_arch)),
+ NativeRegisterContextLinux(native_thread,
----------------
mgorny wrote:
> I'm wondering if we can eliminate the init inside
> `NativeRegisterContextRegisterInfo`
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.
================
Comment at:
lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.h:110
- bool IsGPR(uint32_t reg_index) const;
+ bool IsGPROrDR(uint32_t reg_index) const;
----------------
mgorny wrote:
> I have changed this to facilitate `WriteRegister()`. Alternatively, I suppose
> we could reverse the loop to match `ReadRegister()`.
I think this is fine.
================
Comment at: lldb/source/Plugins/Process/Utility/WatchpointMixin_x86.cpp:9
+
+#if defined(__i386__) || defined(__x86_64__)
+
----------------
This class won't be using any os- or arch-specific features, so we can just
drop these.
================
Comment at: lldb/source/Plugins/Process/Utility/WatchpointMixin_x86.cpp:146-155
+ // clear dr6 if address or bits changed (i.e. we're not reenabling the same
+ // watchpoint)
+ if (drN_value.GetAsUInt64() != addr ||
+ (dr7_value.GetAsUInt64() & bit_mask) != (rw_bits | size_bits)) {
+ ClearWatchpointHit(wp_index);
+
+ error = WriteRegister(reg_info_drN, RegisterValue(addr));
----------------
mgorny wrote:
> This is the DR6 cleanup I was talking about.
This seems fine. Maybe worth mentioning that this is here to avoid confusing
the NetBSD kernel.
================
Comment at: lldb/source/Plugins/Process/Utility/WatchpointMixin_x86.cpp:183
+
+Status WatchpointMixin_x86::ClearWatchpointHit(uint32_t wp_index) {
+ if (wp_index >= NumSupportedHardwareWatchpoints())
----------------
mgorny wrote:
> 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.
No clue. If it's not done currently, then I guess it's not needed.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D89874/new/
https://reviews.llvm.org/D89874
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
