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

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.


================
Comment at: lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp:852
+    RegisterContextWindows *reg_ctx = static_cast<RegisterContextWindows *>(
+        thread->GetRegisterContext().get());
+    if (!reg_ctx->AddHardwareBreakpoint(info.slot, info.address, info.size,
----------------
aleksandr.urakov wrote:
> amccarth wrote:
> > Since you have to explicitly downcast, wouldn't `auto *reg_ctx` on the left be sufficient and just as clear?
> Actually, I'm a big fan of auto, and some time ago @zturner have told me that normally it is not used so much in LLVM code, so I have reduced its usage in my patches :) But if it is OK to use auto here, I'll fix it (and in similar places too), thanks!
Yeah, llvm does not use auto too match, but in this case the type is already explicitly present, so there's no ambiguity, and this is fine. (I would still use `auto *` instead of plain `auto` though..)


================
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;
----------------
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`.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D67168