[Lldb-commits] [PATCH] D67168: [Windows] Add support of watchpoints to `ProcessWindows`
This revision was automatically updated to reflect the committed changes.
Closed by commit rL371166: [Windows] Add support of watchpoints to
`ProcessWindows` (authored by aleksandr.urakov, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D67168?vs=218897=219028#toc
Repository:
rL LLVM
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D67168/new/
https://reviews.llvm.org/D67168
Files:
lldb/trunk/packages/Python/lldbsuite/test/commands/watchpoints/hello_watchlocation/TestWatchLocation.py
lldb/trunk/packages/Python/lldbsuite/test/commands/watchpoints/hello_watchpoint/TestMyFirstWatchpoint.py
lldb/trunk/packages/Python/lldbsuite/test/commands/watchpoints/multiple_hits/TestMultipleHits.py
lldb/trunk/packages/Python/lldbsuite/test/commands/watchpoints/multiple_threads/TestWatchpointMultipleThreads.py
lldb/trunk/packages/Python/lldbsuite/test/commands/watchpoints/step_over_watchpoint/TestStepOverWatchpoint.py
lldb/trunk/packages/Python/lldbsuite/test/commands/watchpoints/watchpoint_commands/TestWatchpointCommands.py
lldb/trunk/packages/Python/lldbsuite/test/commands/watchpoints/watchpoint_commands/command/TestWatchpointCommandLLDB.py
lldb/trunk/packages/Python/lldbsuite/test/commands/watchpoints/watchpoint_commands/command/TestWatchpointCommandPython.py
lldb/trunk/packages/Python/lldbsuite/test/commands/watchpoints/watchpoint_commands/condition/TestWatchpointConditionCmd.py
lldb/trunk/packages/Python/lldbsuite/test/commands/watchpoints/watchpoint_disable/TestWatchpointDisable.py
lldb/trunk/packages/Python/lldbsuite/test/commands/watchpoints/watchpoint_events/TestWatchpointEvents.py
lldb/trunk/packages/Python/lldbsuite/test/commands/watchpoints/watchpoint_on_vectors/TestValueOfVectorVariable.py
lldb/trunk/packages/Python/lldbsuite/test/commands/watchpoints/watchpoint_set_command/TestWatchLocationWithWatchSet.py
lldb/trunk/packages/Python/lldbsuite/test/commands/watchpoints/watchpoint_size/TestWatchpointSizes.py
lldb/trunk/packages/Python/lldbsuite/test/python_api/watchpoint/TestSetWatchpoint.py
lldb/trunk/packages/Python/lldbsuite/test/python_api/watchpoint/TestWatchpointIgnoreCount.py
lldb/trunk/packages/Python/lldbsuite/test/python_api/watchpoint/TestWatchpointIter.py
lldb/trunk/packages/Python/lldbsuite/test/python_api/watchpoint/condition/TestWatchpointConditionAPI.py
lldb/trunk/packages/Python/lldbsuite/test/python_api/watchpoint/watchlocation/TestSetWatchlocation.py
lldb/trunk/packages/Python/lldbsuite/test/python_api/watchpoint/watchlocation/TestTargetWatchAddress.py
lldb/trunk/source/Plugins/Process/Windows/Common/ProcessDebugger.h
lldb/trunk/source/Plugins/Process/Windows/Common/ProcessWindows.cpp
lldb/trunk/source/Plugins/Process/Windows/Common/ProcessWindows.h
lldb/trunk/source/Plugins/Process/Windows/Common/RegisterContextWindows.cpp
lldb/trunk/source/Plugins/Process/Windows/Common/RegisterContextWindows.h
lldb/trunk/source/Plugins/Process/Windows/Common/x64/RegisterContextWindows_x64.cpp
lldb/trunk/source/Plugins/Process/Windows/Common/x86/RegisterContextWindows_x86.cpp
Index: lldb/trunk/packages/Python/lldbsuite/test/python_api/watchpoint/condition/TestWatchpointConditionAPI.py
===
--- lldb/trunk/packages/Python/lldbsuite/test/python_api/watchpoint/condition/TestWatchpointConditionAPI.py
+++ lldb/trunk/packages/Python/lldbsuite/test/python_api/watchpoint/condition/TestWatchpointConditionAPI.py
@@ -36,7 +36,6 @@
archs=["aarch64"],
triple=no_match(".*-android"),
bugnumber="llvm.org/pr27710")
-@skipIfWindows # Watchpoints not supported on Windows, and this test hangs
def test_watchpoint_cond_api(self):
"""Test watchpoint condition API."""
self.build(dictionary=self.d)
Index: lldb/trunk/packages/Python/lldbsuite/test/python_api/watchpoint/TestWatchpointIgnoreCount.py
===
--- lldb/trunk/packages/Python/lldbsuite/test/python_api/watchpoint/TestWatchpointIgnoreCount.py
+++ lldb/trunk/packages/Python/lldbsuite/test/python_api/watchpoint/TestWatchpointIgnoreCount.py
@@ -25,9 +25,6 @@
self.source, '// Set break point at this line.')
@add_test_categories(['pyapi'])
-@expectedFailureAll(
-oslist=["windows"],
-bugnumber="llvm.org/pr24446: WINDOWS XFAIL TRIAGE - Watchpoints not supported on Windows")
# Read-write watchpoints not supported on SystemZ
@expectedFailureAll(archs=['s390x'])
def test_set_watch_ignore_count(self):
Index: lldb/trunk/packages/Python/lldbsuite/test/python_api/watchpoint/TestSetWatchpoint.py
===
--- lldb/trunk/packages/Python/lldbsuite/test/python_api/watchpoint/TestSetWatchpoint.py
+++
[Lldb-commits] [PATCH] D67168: [Windows] Add support of watchpoints to `ProcessWindows`
aleksandr.urakov added a comment. Thanks all! 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
[Lldb-commits] [PATCH] D67168: [Windows] Add support of watchpoints to `ProcessWindows`
amccarth accepted this revision. amccarth added a comment. This revision is now accepted and ready to land. Thanks for the changes! I think this looks good now. 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
[Lldb-commits] [PATCH] D67168: [Windows] Add support of watchpoints to `ProcessWindows`
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
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D67168: [Windows] Add support of watchpoints to `ProcessWindows`
aleksandr.urakov updated this revision to Diff 218897. aleksandr.urakov added a comment. Determine whether 8-byte watchpoints are supported or not statically. Repository: rLLDB LLDB CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67168/new/ https://reviews.llvm.org/D67168 Files: lldb/packages/Python/lldbsuite/test/commands/watchpoints/hello_watchlocation/TestWatchLocation.py lldb/packages/Python/lldbsuite/test/commands/watchpoints/hello_watchpoint/TestMyFirstWatchpoint.py lldb/packages/Python/lldbsuite/test/commands/watchpoints/multiple_hits/TestMultipleHits.py lldb/packages/Python/lldbsuite/test/commands/watchpoints/multiple_threads/TestWatchpointMultipleThreads.py lldb/packages/Python/lldbsuite/test/commands/watchpoints/step_over_watchpoint/TestStepOverWatchpoint.py lldb/packages/Python/lldbsuite/test/commands/watchpoints/watchpoint_commands/TestWatchpointCommands.py lldb/packages/Python/lldbsuite/test/commands/watchpoints/watchpoint_commands/command/TestWatchpointCommandLLDB.py lldb/packages/Python/lldbsuite/test/commands/watchpoints/watchpoint_commands/command/TestWatchpointCommandPython.py lldb/packages/Python/lldbsuite/test/commands/watchpoints/watchpoint_commands/condition/TestWatchpointConditionCmd.py lldb/packages/Python/lldbsuite/test/commands/watchpoints/watchpoint_disable/TestWatchpointDisable.py lldb/packages/Python/lldbsuite/test/commands/watchpoints/watchpoint_events/TestWatchpointEvents.py lldb/packages/Python/lldbsuite/test/commands/watchpoints/watchpoint_on_vectors/TestValueOfVectorVariable.py lldb/packages/Python/lldbsuite/test/commands/watchpoints/watchpoint_set_command/TestWatchLocationWithWatchSet.py lldb/packages/Python/lldbsuite/test/commands/watchpoints/watchpoint_size/TestWatchpointSizes.py lldb/packages/Python/lldbsuite/test/python_api/watchpoint/TestSetWatchpoint.py lldb/packages/Python/lldbsuite/test/python_api/watchpoint/TestWatchpointIgnoreCount.py lldb/packages/Python/lldbsuite/test/python_api/watchpoint/TestWatchpointIter.py lldb/packages/Python/lldbsuite/test/python_api/watchpoint/condition/TestWatchpointConditionAPI.py lldb/packages/Python/lldbsuite/test/python_api/watchpoint/watchlocation/TestSetWatchlocation.py lldb/packages/Python/lldbsuite/test/python_api/watchpoint/watchlocation/TestTargetWatchAddress.py lldb/source/Plugins/Process/Windows/Common/ProcessDebugger.h lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp lldb/source/Plugins/Process/Windows/Common/ProcessWindows.h lldb/source/Plugins/Process/Windows/Common/RegisterContextWindows.cpp lldb/source/Plugins/Process/Windows/Common/RegisterContextWindows.h lldb/source/Plugins/Process/Windows/Common/x64/RegisterContextWindows_x64.cpp lldb/source/Plugins/Process/Windows/Common/x86/RegisterContextWindows_x86.cpp Index: lldb/source/Plugins/Process/Windows/Common/x86/RegisterContextWindows_x86.cpp === --- lldb/source/Plugins/Process/Windows/Common/x86/RegisterContextWindows_x86.cpp +++ lldb/source/Plugins/Process/Windows/Common/x86/RegisterContextWindows_x86.cpp @@ -267,9 +267,7 @@ } // Physically update the registers in the target process. - TargetThreadWindows = static_cast(m_thread); - return ::SetThreadContext( - wthread.GetHostThread().GetNativeThread().GetSystemHandle(), _context); + return ApplyAllRegisterValues(); } bool RegisterContextWindows_x86::ReadRegisterHelper( Index: lldb/source/Plugins/Process/Windows/Common/x64/RegisterContextWindows_x64.cpp === --- lldb/source/Plugins/Process/Windows/Common/x64/RegisterContextWindows_x64.cpp +++ lldb/source/Plugins/Process/Windows/Common/x64/RegisterContextWindows_x64.cpp @@ -532,9 +532,7 @@ } // Physically update the registers in the target process. - TargetThreadWindows = static_cast(m_thread); - return ::SetThreadContext( - wthread.GetHostThread().GetNativeThread().GetSystemHandle(), _context); + return ApplyAllRegisterValues(); } #endif // defined(__x86_64__) || defined(_M_X64) Index: lldb/source/Plugins/Process/Windows/Common/RegisterContextWindows.h === --- lldb/source/Plugins/Process/Windows/Common/RegisterContextWindows.h +++ lldb/source/Plugins/Process/Windows/Common/RegisterContextWindows.h @@ -33,28 +33,28 @@ uint32_t ConvertRegisterKindToRegisterNumber(lldb::RegisterKind kind, uint32_t num) override; - // Subclasses can override these functions if desired - uint32_t NumSupportedHardwareBreakpoints() override; - - uint32_t SetHardwareBreakpoint(lldb::addr_t addr, size_t size) override; - - bool ClearHardwareBreakpoint(uint32_t hw_idx) override; - - uint32_t NumSupportedHardwareWatchpoints() override; + bool HardwareSingleStep(bool enable) override; - uint32_t
[Lldb-commits] [PATCH] D67168: [Windows] Add support of watchpoints to `ProcessWindows`
labath added inline comments.
Comment at: lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp:852
+RegisterContextWindows *reg_ctx = static_cast(
+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
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D67168: [Windows] Add support of watchpoints to `ProcessWindows`
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 _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 _thread) {
+void ProcessWindows::OnCreateThread(HostThread _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(
+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
[Lldb-commits] [PATCH] D67168: [Windows] Add support of watchpoints to `ProcessWindows`
aleksandr.urakov updated this revision to Diff 218840. aleksandr.urakov marked an inline comment as done. aleksandr.urakov added a comment. Check bitness of the target when checking the watchpoint size. Repository: rLLDB LLDB CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67168/new/ https://reviews.llvm.org/D67168 Files: lldb/packages/Python/lldbsuite/test/commands/watchpoints/hello_watchlocation/TestWatchLocation.py lldb/packages/Python/lldbsuite/test/commands/watchpoints/hello_watchpoint/TestMyFirstWatchpoint.py lldb/packages/Python/lldbsuite/test/commands/watchpoints/multiple_hits/TestMultipleHits.py lldb/packages/Python/lldbsuite/test/commands/watchpoints/multiple_threads/TestWatchpointMultipleThreads.py lldb/packages/Python/lldbsuite/test/commands/watchpoints/step_over_watchpoint/TestStepOverWatchpoint.py lldb/packages/Python/lldbsuite/test/commands/watchpoints/watchpoint_commands/TestWatchpointCommands.py lldb/packages/Python/lldbsuite/test/commands/watchpoints/watchpoint_commands/command/TestWatchpointCommandLLDB.py lldb/packages/Python/lldbsuite/test/commands/watchpoints/watchpoint_commands/command/TestWatchpointCommandPython.py lldb/packages/Python/lldbsuite/test/commands/watchpoints/watchpoint_commands/condition/TestWatchpointConditionCmd.py lldb/packages/Python/lldbsuite/test/commands/watchpoints/watchpoint_disable/TestWatchpointDisable.py lldb/packages/Python/lldbsuite/test/commands/watchpoints/watchpoint_events/TestWatchpointEvents.py lldb/packages/Python/lldbsuite/test/commands/watchpoints/watchpoint_on_vectors/TestValueOfVectorVariable.py lldb/packages/Python/lldbsuite/test/commands/watchpoints/watchpoint_set_command/TestWatchLocationWithWatchSet.py lldb/packages/Python/lldbsuite/test/commands/watchpoints/watchpoint_size/TestWatchpointSizes.py lldb/packages/Python/lldbsuite/test/python_api/watchpoint/TestSetWatchpoint.py lldb/packages/Python/lldbsuite/test/python_api/watchpoint/TestWatchpointIgnoreCount.py lldb/packages/Python/lldbsuite/test/python_api/watchpoint/TestWatchpointIter.py lldb/packages/Python/lldbsuite/test/python_api/watchpoint/condition/TestWatchpointConditionAPI.py lldb/packages/Python/lldbsuite/test/python_api/watchpoint/watchlocation/TestSetWatchlocation.py lldb/packages/Python/lldbsuite/test/python_api/watchpoint/watchlocation/TestTargetWatchAddress.py lldb/source/Plugins/Process/Windows/Common/ProcessDebugger.h lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp lldb/source/Plugins/Process/Windows/Common/ProcessWindows.h lldb/source/Plugins/Process/Windows/Common/RegisterContextWindows.cpp lldb/source/Plugins/Process/Windows/Common/RegisterContextWindows.h lldb/source/Plugins/Process/Windows/Common/x64/RegisterContextWindows_x64.cpp lldb/source/Plugins/Process/Windows/Common/x86/RegisterContextWindows_x86.cpp Index: lldb/source/Plugins/Process/Windows/Common/x86/RegisterContextWindows_x86.cpp === --- lldb/source/Plugins/Process/Windows/Common/x86/RegisterContextWindows_x86.cpp +++ lldb/source/Plugins/Process/Windows/Common/x86/RegisterContextWindows_x86.cpp @@ -267,9 +267,7 @@ } // Physically update the registers in the target process. - TargetThreadWindows = static_cast(m_thread); - return ::SetThreadContext( - wthread.GetHostThread().GetNativeThread().GetSystemHandle(), _context); + return ApplyAllRegisterValues(); } bool RegisterContextWindows_x86::ReadRegisterHelper( Index: lldb/source/Plugins/Process/Windows/Common/x64/RegisterContextWindows_x64.cpp === --- lldb/source/Plugins/Process/Windows/Common/x64/RegisterContextWindows_x64.cpp +++ lldb/source/Plugins/Process/Windows/Common/x64/RegisterContextWindows_x64.cpp @@ -532,9 +532,7 @@ } // Physically update the registers in the target process. - TargetThreadWindows = static_cast(m_thread); - return ::SetThreadContext( - wthread.GetHostThread().GetNativeThread().GetSystemHandle(), _context); + return ApplyAllRegisterValues(); } #endif // defined(__x86_64__) || defined(_M_X64) Index: lldb/source/Plugins/Process/Windows/Common/RegisterContextWindows.h === --- lldb/source/Plugins/Process/Windows/Common/RegisterContextWindows.h +++ lldb/source/Plugins/Process/Windows/Common/RegisterContextWindows.h @@ -33,28 +33,28 @@ uint32_t ConvertRegisterKindToRegisterNumber(lldb::RegisterKind kind, uint32_t num) override; - // Subclasses can override these functions if desired - uint32_t NumSupportedHardwareBreakpoints() override; - - uint32_t SetHardwareBreakpoint(lldb::addr_t addr, size_t size) override; - - bool ClearHardwareBreakpoint(uint32_t hw_idx) override; - - uint32_t NumSupportedHardwareWatchpoints() override; + bool
[Lldb-commits] [PATCH] D67168: [Windows] Add support of watchpoints to `ProcessWindows`
aleksandr.urakov updated this revision to Diff 218838. aleksandr.urakov added a comment. Address the requested changes. Repository: rLLDB LLDB CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67168/new/ https://reviews.llvm.org/D67168 Files: lldb/packages/Python/lldbsuite/test/commands/watchpoints/hello_watchlocation/TestWatchLocation.py lldb/packages/Python/lldbsuite/test/commands/watchpoints/hello_watchpoint/TestMyFirstWatchpoint.py lldb/packages/Python/lldbsuite/test/commands/watchpoints/multiple_hits/TestMultipleHits.py lldb/packages/Python/lldbsuite/test/commands/watchpoints/multiple_threads/TestWatchpointMultipleThreads.py lldb/packages/Python/lldbsuite/test/commands/watchpoints/step_over_watchpoint/TestStepOverWatchpoint.py lldb/packages/Python/lldbsuite/test/commands/watchpoints/watchpoint_commands/TestWatchpointCommands.py lldb/packages/Python/lldbsuite/test/commands/watchpoints/watchpoint_commands/command/TestWatchpointCommandLLDB.py lldb/packages/Python/lldbsuite/test/commands/watchpoints/watchpoint_commands/command/TestWatchpointCommandPython.py lldb/packages/Python/lldbsuite/test/commands/watchpoints/watchpoint_commands/condition/TestWatchpointConditionCmd.py lldb/packages/Python/lldbsuite/test/commands/watchpoints/watchpoint_disable/TestWatchpointDisable.py lldb/packages/Python/lldbsuite/test/commands/watchpoints/watchpoint_events/TestWatchpointEvents.py lldb/packages/Python/lldbsuite/test/commands/watchpoints/watchpoint_on_vectors/TestValueOfVectorVariable.py lldb/packages/Python/lldbsuite/test/commands/watchpoints/watchpoint_set_command/TestWatchLocationWithWatchSet.py lldb/packages/Python/lldbsuite/test/commands/watchpoints/watchpoint_size/TestWatchpointSizes.py lldb/packages/Python/lldbsuite/test/python_api/watchpoint/TestSetWatchpoint.py lldb/packages/Python/lldbsuite/test/python_api/watchpoint/TestWatchpointIgnoreCount.py lldb/packages/Python/lldbsuite/test/python_api/watchpoint/TestWatchpointIter.py lldb/packages/Python/lldbsuite/test/python_api/watchpoint/condition/TestWatchpointConditionAPI.py lldb/packages/Python/lldbsuite/test/python_api/watchpoint/watchlocation/TestSetWatchlocation.py lldb/packages/Python/lldbsuite/test/python_api/watchpoint/watchlocation/TestTargetWatchAddress.py lldb/source/Plugins/Process/Windows/Common/ProcessDebugger.h lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp lldb/source/Plugins/Process/Windows/Common/ProcessWindows.h lldb/source/Plugins/Process/Windows/Common/RegisterContextWindows.cpp lldb/source/Plugins/Process/Windows/Common/RegisterContextWindows.h lldb/source/Plugins/Process/Windows/Common/x64/RegisterContextWindows_x64.cpp lldb/source/Plugins/Process/Windows/Common/x86/RegisterContextWindows_x86.cpp Index: lldb/source/Plugins/Process/Windows/Common/x86/RegisterContextWindows_x86.cpp === --- lldb/source/Plugins/Process/Windows/Common/x86/RegisterContextWindows_x86.cpp +++ lldb/source/Plugins/Process/Windows/Common/x86/RegisterContextWindows_x86.cpp @@ -267,9 +267,7 @@ } // Physically update the registers in the target process. - TargetThreadWindows = static_cast(m_thread); - return ::SetThreadContext( - wthread.GetHostThread().GetNativeThread().GetSystemHandle(), _context); + return ApplyAllRegisterValues(); } bool RegisterContextWindows_x86::ReadRegisterHelper( Index: lldb/source/Plugins/Process/Windows/Common/x64/RegisterContextWindows_x64.cpp === --- lldb/source/Plugins/Process/Windows/Common/x64/RegisterContextWindows_x64.cpp +++ lldb/source/Plugins/Process/Windows/Common/x64/RegisterContextWindows_x64.cpp @@ -532,9 +532,7 @@ } // Physically update the registers in the target process. - TargetThreadWindows = static_cast(m_thread); - return ::SetThreadContext( - wthread.GetHostThread().GetNativeThread().GetSystemHandle(), _context); + return ApplyAllRegisterValues(); } #endif // defined(__x86_64__) || defined(_M_X64) Index: lldb/source/Plugins/Process/Windows/Common/RegisterContextWindows.h === --- lldb/source/Plugins/Process/Windows/Common/RegisterContextWindows.h +++ lldb/source/Plugins/Process/Windows/Common/RegisterContextWindows.h @@ -33,28 +33,28 @@ uint32_t ConvertRegisterKindToRegisterNumber(lldb::RegisterKind kind, uint32_t num) override; - // Subclasses can override these functions if desired - uint32_t NumSupportedHardwareBreakpoints() override; - - uint32_t SetHardwareBreakpoint(lldb::addr_t addr, size_t size) override; - - bool ClearHardwareBreakpoint(uint32_t hw_idx) override; - - uint32_t NumSupportedHardwareWatchpoints() override; + bool HardwareSingleStep(bool enable) override; - uint32_t SetHardwareWatchpoint(lldb::addr_t addr, size_t
[Lldb-commits] [PATCH] D67168: [Windows] Add support of watchpoints to `ProcessWindows`
aleksandr.urakov updated this revision to Diff 218837. aleksandr.urakov added a comment. Address the requested changes. Repository: rLLDB LLDB CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67168/new/ https://reviews.llvm.org/D67168 Files: lldb/packages/Python/lldbsuite/test/commands/watchpoints/hello_watchlocation/TestWatchLocation.py lldb/packages/Python/lldbsuite/test/commands/watchpoints/hello_watchpoint/TestMyFirstWatchpoint.py lldb/packages/Python/lldbsuite/test/commands/watchpoints/multiple_hits/TestMultipleHits.py lldb/packages/Python/lldbsuite/test/commands/watchpoints/multiple_threads/TestWatchpointMultipleThreads.py lldb/packages/Python/lldbsuite/test/commands/watchpoints/step_over_watchpoint/TestStepOverWatchpoint.py lldb/packages/Python/lldbsuite/test/commands/watchpoints/watchpoint_commands/TestWatchpointCommands.py lldb/packages/Python/lldbsuite/test/commands/watchpoints/watchpoint_commands/command/TestWatchpointCommandLLDB.py lldb/packages/Python/lldbsuite/test/commands/watchpoints/watchpoint_commands/command/TestWatchpointCommandPython.py lldb/packages/Python/lldbsuite/test/commands/watchpoints/watchpoint_commands/condition/TestWatchpointConditionCmd.py lldb/packages/Python/lldbsuite/test/commands/watchpoints/watchpoint_disable/TestWatchpointDisable.py lldb/packages/Python/lldbsuite/test/commands/watchpoints/watchpoint_events/TestWatchpointEvents.py lldb/packages/Python/lldbsuite/test/commands/watchpoints/watchpoint_on_vectors/TestValueOfVectorVariable.py lldb/packages/Python/lldbsuite/test/commands/watchpoints/watchpoint_set_command/TestWatchLocationWithWatchSet.py lldb/packages/Python/lldbsuite/test/commands/watchpoints/watchpoint_size/TestWatchpointSizes.py lldb/packages/Python/lldbsuite/test/python_api/watchpoint/TestSetWatchpoint.py lldb/packages/Python/lldbsuite/test/python_api/watchpoint/TestWatchpointIgnoreCount.py lldb/packages/Python/lldbsuite/test/python_api/watchpoint/TestWatchpointIter.py lldb/packages/Python/lldbsuite/test/python_api/watchpoint/condition/TestWatchpointConditionAPI.py lldb/packages/Python/lldbsuite/test/python_api/watchpoint/watchlocation/TestSetWatchlocation.py lldb/packages/Python/lldbsuite/test/python_api/watchpoint/watchlocation/TestTargetWatchAddress.py lldb/source/Plugins/Process/Windows/Common/ProcessDebugger.h lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp lldb/source/Plugins/Process/Windows/Common/ProcessWindows.h lldb/source/Plugins/Process/Windows/Common/RegisterContextWindows.cpp lldb/source/Plugins/Process/Windows/Common/RegisterContextWindows.h lldb/source/Plugins/Process/Windows/Common/x64/RegisterContextWindows_x64.cpp lldb/source/Plugins/Process/Windows/Common/x86/RegisterContextWindows_x86.cpp Index: lldb/source/Plugins/Process/Windows/Common/x86/RegisterContextWindows_x86.cpp === --- lldb/source/Plugins/Process/Windows/Common/x86/RegisterContextWindows_x86.cpp +++ lldb/source/Plugins/Process/Windows/Common/x86/RegisterContextWindows_x86.cpp @@ -267,9 +267,7 @@ } // Physically update the registers in the target process. - TargetThreadWindows = static_cast(m_thread); - return ::SetThreadContext( - wthread.GetHostThread().GetNativeThread().GetSystemHandle(), _context); + return ApplyAllRegisterValues(); } bool RegisterContextWindows_x86::ReadRegisterHelper( Index: lldb/source/Plugins/Process/Windows/Common/x64/RegisterContextWindows_x64.cpp === --- lldb/source/Plugins/Process/Windows/Common/x64/RegisterContextWindows_x64.cpp +++ lldb/source/Plugins/Process/Windows/Common/x64/RegisterContextWindows_x64.cpp @@ -532,9 +532,7 @@ } // Physically update the registers in the target process. - TargetThreadWindows = static_cast(m_thread); - return ::SetThreadContext( - wthread.GetHostThread().GetNativeThread().GetSystemHandle(), _context); + return ApplyAllRegisterValues(); } #endif // defined(__x86_64__) || defined(_M_X64) Index: lldb/source/Plugins/Process/Windows/Common/RegisterContextWindows.h === --- lldb/source/Plugins/Process/Windows/Common/RegisterContextWindows.h +++ lldb/source/Plugins/Process/Windows/Common/RegisterContextWindows.h @@ -33,28 +33,28 @@ uint32_t ConvertRegisterKindToRegisterNumber(lldb::RegisterKind kind, uint32_t num) override; - // Subclasses can override these functions if desired - uint32_t NumSupportedHardwareBreakpoints() override; - - uint32_t SetHardwareBreakpoint(lldb::addr_t addr, size_t size) override; - - bool ClearHardwareBreakpoint(uint32_t hw_idx) override; - - uint32_t NumSupportedHardwareWatchpoints() override; + bool HardwareSingleStep(bool enable) override; - uint32_t SetHardwareWatchpoint(lldb::addr_t addr, size_t
[Lldb-commits] [PATCH] D67168: [Windows] Add support of watchpoints to `ProcessWindows`
zturner 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;
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.
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
[Lldb-commits] [PATCH] D67168: [Windows] Add support of watchpoints to `ProcessWindows`
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 _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 _thread) {
+void ProcessWindows::OnCreateThread(HostThread _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(
+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
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D67168: [Windows] Add support of watchpoints to `ProcessWindows`
aleksandr.urakov created this revision. aleksandr.urakov added reviewers: asmith, stella.stamenova, amccarth. aleksandr.urakov added a project: LLDB. Herald added subscribers: lldb-commits, JDevlieghere, abidh. aleksandr.urakov added a subscriber: leonid.mashinskiy. This patch adds support of watchpoints to the old `ProcessWindows` plugin. The `ProcessWindows` plugin uses the `RegisterContext` to set and reset watchpoints. The `RegisterContext` has some interface to access watchpoints, but it is very limited (e.g. it is impossible to retrieve the last triggered watchpoint with it), that's why I have implemented a slightly different interface in the `RegisterContextWindows`. Moreover, I have made the `ProcessWindows` plugin responsible for search of a vacant watchpoint slot, because watchpoints exist per-process (not per-thread), then we can place the same watchpoint in the same slot in different threads. With this scheme threads don't need to have their own watchpoint lists, and it simplifies identifying of the last triggered watchpoint. It looks like it is enough to just implement corresponding methods in the `NativeRegisterContextWindows`-based classes to support the feature in the new `NativeProcessWindows` plugin. I'll do it in the next review. Repository: rLLDB LLDB https://reviews.llvm.org/D67168 Files: lldb/packages/Python/lldbsuite/test/commands/watchpoints/hello_watchlocation/TestWatchLocation.py lldb/packages/Python/lldbsuite/test/commands/watchpoints/hello_watchpoint/TestMyFirstWatchpoint.py lldb/packages/Python/lldbsuite/test/commands/watchpoints/multiple_hits/TestMultipleHits.py lldb/packages/Python/lldbsuite/test/commands/watchpoints/multiple_threads/TestWatchpointMultipleThreads.py lldb/packages/Python/lldbsuite/test/commands/watchpoints/step_over_watchpoint/TestStepOverWatchpoint.py lldb/packages/Python/lldbsuite/test/commands/watchpoints/watchpoint_commands/TestWatchpointCommands.py lldb/packages/Python/lldbsuite/test/commands/watchpoints/watchpoint_commands/command/TestWatchpointCommandLLDB.py lldb/packages/Python/lldbsuite/test/commands/watchpoints/watchpoint_commands/command/TestWatchpointCommandPython.py lldb/packages/Python/lldbsuite/test/commands/watchpoints/watchpoint_commands/condition/TestWatchpointConditionCmd.py lldb/packages/Python/lldbsuite/test/commands/watchpoints/watchpoint_disable/TestWatchpointDisable.py lldb/packages/Python/lldbsuite/test/commands/watchpoints/watchpoint_events/TestWatchpointEvents.py lldb/packages/Python/lldbsuite/test/commands/watchpoints/watchpoint_on_vectors/TestValueOfVectorVariable.py lldb/packages/Python/lldbsuite/test/commands/watchpoints/watchpoint_set_command/TestWatchLocationWithWatchSet.py lldb/packages/Python/lldbsuite/test/commands/watchpoints/watchpoint_size/TestWatchpointSizes.py lldb/packages/Python/lldbsuite/test/python_api/watchpoint/TestSetWatchpoint.py lldb/packages/Python/lldbsuite/test/python_api/watchpoint/TestWatchpointIgnoreCount.py lldb/packages/Python/lldbsuite/test/python_api/watchpoint/TestWatchpointIter.py lldb/packages/Python/lldbsuite/test/python_api/watchpoint/condition/TestWatchpointConditionAPI.py lldb/packages/Python/lldbsuite/test/python_api/watchpoint/watchlocation/TestSetWatchlocation.py lldb/packages/Python/lldbsuite/test/python_api/watchpoint/watchlocation/TestTargetWatchAddress.py lldb/source/Plugins/Process/Windows/Common/IDebugDelegate.h lldb/source/Plugins/Process/Windows/Common/LocalDebugDelegate.cpp lldb/source/Plugins/Process/Windows/Common/LocalDebugDelegate.h lldb/source/Plugins/Process/Windows/Common/NativeProcessWindows.cpp lldb/source/Plugins/Process/Windows/Common/NativeProcessWindows.h lldb/source/Plugins/Process/Windows/Common/ProcessDebugger.cpp lldb/source/Plugins/Process/Windows/Common/ProcessDebugger.h lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp lldb/source/Plugins/Process/Windows/Common/ProcessWindows.h lldb/source/Plugins/Process/Windows/Common/RegisterContextWindows.cpp lldb/source/Plugins/Process/Windows/Common/RegisterContextWindows.h lldb/source/Plugins/Process/Windows/Common/x64/RegisterContextWindows_x64.cpp lldb/source/Plugins/Process/Windows/Common/x86/RegisterContextWindows_x86.cpp Index: lldb/source/Plugins/Process/Windows/Common/x86/RegisterContextWindows_x86.cpp === --- lldb/source/Plugins/Process/Windows/Common/x86/RegisterContextWindows_x86.cpp +++ lldb/source/Plugins/Process/Windows/Common/x86/RegisterContextWindows_x86.cpp @@ -267,9 +267,7 @@ } // Physically update the registers in the target process. - TargetThreadWindows = static_cast(m_thread); - return ::SetThreadContext( - wthread.GetHostThread().GetNativeThread().GetSystemHandle(), _context); + return ApplyAllRegisterValues(); } bool RegisterContextWindows_x86::ReadRegisterHelper( Index:
