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

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth 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,
----------------
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`).


================
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;
----------------
This no longer sets the thread ID.  Was that intentional?


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


================
Comment at: lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp:832
+  Status error;
+
+  WatchpointInfo info;
----------------
Should we check if the watchpoint is already enabled?  I noticed that `DisableWatchpoint` has an analogous guard clause.


================
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,
----------------
Since you have to explicitly downcast, wouldn't `auto *reg_ctx` on the left be sufficient and just as clear?


================
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;
----------------
Clever!  It took me a minute or two to figure out what the point of that was checking.  Perhaps a comment to explain?


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D67168