mgorny marked 4 inline comments as done.
mgorny added inline comments.
================
Comment at: lldb/source/Plugins/Process/Utility/WatchpointMixin_x86.cpp:9
+
+#if defined(__i386__) || defined(__x86_64__)
+
----------------
labath wrote:
> This class won't be using any os- or arch-specific features, so we can just
> drop these.
Does it make sense to compile code that isn't going to be used on other
platforms?
================
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));
----------------
labath wrote:
> 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.
I'm going to go through all the code and see what can be improved and/or
documented better.
================
Comment at: lldb/source/Plugins/Process/Utility/WatchpointMixin_x86.cpp:183
+
+Status WatchpointMixin_x86::ClearWatchpointHit(uint32_t wp_index) {
+ if (wp_index >= NumSupportedHardwareWatchpoints())
----------------
labath wrote:
> 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.
I think it might have been done as part of the implicit disable/reenable.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D89874/new/
https://reviews.llvm.org/D89874
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
