[Lldb-commits] [PATCH] D67168: [Windows] Add support of watchpoints to `ProcessWindows`

[Aleksandr Urakov via Phabricator via lldb-commits]

aleksandr.urakov marked 3 inline comments as done.
aleksandr.urakov added inline comments.


================
Comment at: lldb/source/Plugins/Process/Windows/Common/RegisterContextWindows.cpp:82
 
-bool RegisterContextWindows::ClearHardwareBreakpoint(uint32_t hw_idx) {
-  return false;
-}
+  if (!size || size > 8 || size & (size - 1))
+    return false;
----------------
labath wrote:
> aleksandr.urakov wrote:
> > zturner wrote:
> > > amccarth wrote:
> > > > Clever!  It took me a minute or two to figure out what the point of that was checking.  Perhaps a comment to explain?
> > > Isn't this equivalent to:
> > > 
> > > ```
> > > switch (size)
> > > {
> > >     case 1:
> > >     case 2:
> > >     case 4:
> > >     case 8:
> > >         break;
> > >     default:
> > >         return false;
> > > }
> > > ```
> > > 
> > > ?  That definitely seems much clearer.
> > > 
> > > I'm also pretty sure that on x86 you can't add a 64-bit watch, So you'd have to do something different depending on the target bitness if you want this to be correct for x86.
> > Yes, it is equivalent, I've chosen the previous form due to its less verbosity. But you are right, clearance is better (especially after adding the architecture check). Fixed it, thanks!
> .... or, you could just use `llvm::isPowerOf2_32` from `MathExtras.h`.
I didn't know about the such function, thanks! But I think that Zachary's approach is better in exactly this case (taking in account the bitness check).


================
Comment at: lldb/source/Plugins/Process/Windows/Common/RegisterContextWindows.cpp:87-89
+#if defined(_M_AMD64)
+  case 8:
+#endif
----------------
In the old plugin a required register context is created based on the target architecture check and the bitness of LLDB, but cross-targets (e.g. WoW64) are not supported, and even the type of `m_context` is determined statically. So we can safely determine the max watchpoint size statically too.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D67168