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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
