[Lldb-commits] [PATCH] D89874: [lldb] Unify x86 watchpoint implementation on Linux and NetBSD (and future FreeBSD plugin) [WIP]

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

This looks very nice.

In D89874#2344759 <https://reviews.llvm.org/D89874#2344759>, @mgorny wrote:

> In D89874#2344561 <https://reviews.llvm.org/D89874#2344561>, @labath wrote:
>
>> 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...

In D89874#2345649 <https://reviews.llvm.org/D89874#2345649>, @mgorny wrote:

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



================
Comment at: lldb/include/lldb/lldb-defines.h:62-65
+// Watchpoint types in gdb-remote protocol
+#define LLDB_GDB_WATCH_TYPE_WRITE 1
+#define LLDB_GDB_WATCH_TYPE_READ 2
+#define LLDB_GDB_WATCH_TYPE_READ_WRITE 3
----------------
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.


================
Comment at: lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.cpp:580
 
-  if (IsGPR(reg_index))
+  if (IsGPROrDR(reg_index))
     return WriteRegisterRaw(reg_index, reg_value);
----------------
krytarowski wrote:
> Can we have `IsGPR(reg_index) || IsDR(reg_index)`?
Yeah, that does sound better.


================
Comment at: lldb/source/Plugins/Process/Utility/WatchpointMixin_x86.cpp:9
+
+#if defined(__i386__) || defined(__x86_64__)
+
----------------
mgorny wrote:
> 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?
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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89874/new/

https://reviews.llvm.org/D89874