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

[Aleksandr Urakov via Phabricator via lldb-commits]

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


================
Comment at: lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp:397
+    if (slot != LLDB_INVALID_INDEX32) {
+      int id = m_watchpoint_slots[slot];
+      LLDB_LOG(log,
----------------
amccarth wrote:
> The names here are a bit confusing:  `GetTriggeredHardwareBreakpointSlot` doesn't return a slot; it returns the index of a slot, so  `slot` also isn't a slot, but the index of a slot.  `m_watchpoint_slots` is not a collection of slots but IDs, that's indexed by an index called a `slot`.
> 
> It may not be possible to completely rationalize this, but doing even a little could help future readers understand.  For example, `slot` could be `slot_index`, and `m_watchpoint_slots` could be `m_watchpoint_ids` (or even just `m_watchpoints`).
The most confusing thing here is that watchpoints have two different IDs: ID in LLDB as it is saw by an user, and the number of corresponding debug register in CPU (from 0 to 3). That's why I have selected completely different name for the last one and have called it `slot`. But you are right, `m_watchpoint_slots` is a wrong name, changed it to `m_watchpoint_ids` (because we already have `m_watchpoints` - it is map from LLDB IDs to watchpoint information).


================
Comment at: lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp:525
   for (const auto &thread_info : m_session_data->m_new_threads) {
-    ThreadSP thread(new TargetThreadWindows(*this, thread_info.second));
-    thread->SetID(thread_info.first);
-    new_thread_list.AddThread(thread);
+    new_thread_list.AddThread(thread_info.second);
     ++new_size;
----------------
amccarth wrote:
> This no longer sets the thread ID.  Was that intentional?
Before this commit `m_new_threads` kept `HostThread`s, not `ThreadSP`s. But a `HostThread` have no `RegisterContext`, which we need to set all watchpoints when a new thread is created. That's why we create `ThreadSP` on thread creation now, so we set the thread ID there (in `OnCreateThread` function).


================
Comment at: lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp:743
 
-void ProcessWindows::OnCreateThread(const HostThread &new_thread) {
+void ProcessWindows::OnCreateThread(HostThread &new_thread) {
   llvm::sys::ScopedLock lock(m_mutex);
----------------
amccarth wrote:
> Can you help me why we needed to get rid of the `const` on the `HostThread &`?
> 
> If `native_new_thread` were also a const ref, I don't see any conflicting constraint.
Oh, I'm sorry, it was left unintentionally after my previous implementation. Fixed it, thanks!


================
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,
----------------
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!


================
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;
----------------
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!


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D67168