[lldb] [mostly NFC] Large WP foundation: WatchpointResources #68845
Conversation
|
@llvm/pr-subscribers-lldb Author: Jason Molenda (jasonmolenda) ChangesThis patch is rearranging code a bit to add WatchpointResources to Process. A WatchpointResource is meant to represent a hardware watchpoint register in the inferior process. It has an address, a size, a type, and a list of Watchpoints that are using this WatchpointResource. This current patch doesn't add any of the features of WatchpointResources that make them interesting -- a user asking to watch a 24 byte object could watch this with three 8 byte WatchpointResources. Or a Watchpoint on 1 byte at 0x1002 and a second watchpoint on 1 byte at 0x1003, these must both be served by a single WatchpointResource on that doubleword at 0x1000 on a 64-bit target, if two hardware watchpoint registers were used to track these separately, one of them may not be hit. Or if you have one Watchpoint on a variable with a condition set, and another Watchpoint on that same variable with a command defined or different condition, or ignorecount, both of those Watchpoints need to evaluate their criteria/commands when their WatchpointResource has been hit. There's a bit of code movement to rearrange things in the direction I'll need for implementing this feature, so I want to start with reviewing & landing this mostly NFC patch and we can focus on the algorithmic choices about how WatchpointResources are shared and handled as they're triggeed, separately. This patch also stops printing "Watchpoint <n> hit: old value: <x>, new vlaue: <y>" for Read watchpoints. I could make an argument for print "Watchpoint <n> hit: current value <x>" but the current output doesn't make any sense, and the user can print the value if they are particularly interested. Read watchpoints are used primarily to understand what code is reading a variable. This patch adds more fallbacks for how to print the objects being watched if we have types, instead of assuming they are all integral values, so a struct will print its elements. As large watchpoints are added, we'll be doing a lot more of those. To track the WatchpointSP in the WatchpointResources, I changed the internal API which took a WatchpointSP and devolved it to a Watchpoint*, which meant touching several different Process files. I removed the watchpoint code in ProcessKDP which only reported that watchpoints aren't supported, the base class does that already. I haven't yet changed how we receive a watchpoint to identify the WatchpointResource responsible for the trigger, and identify all Watchpoints that are using this Resource to evaluate their conditions etc. This is the same work that a BreakpointSite needs to do when it has been tiggered, where multiple Breakpoints may be at the same address. There is not yet any printing of the Resources that a Watchpoint is implemented in terms of ("watchpoint list", or "watchpoint set var" and "watchpoint set expression" take a size argument which was previously 1, 2, 4, or 8 (an enum). I've changed this to an unsigned int. Most hardware implementations can only watch 1, 2, 4, 8 byte ranges, but with Resources we'll allow a user to ask for different sized watchpoints and set them in hardware-expressble terms soon. I've annotated areas where I know there is work still needed with LWP_TODO that I'll be working on once this is landed. I've tested this on aarch64 macOS, aarch64 Linux, and Intel macOS. https://discourse.llvm.org/t/rfc-large-watchpoint-support-in-lldb/72116 Patch is 54.63 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/68845.diff 28 Files Affected:
diff --git a/lldb/include/lldb/Breakpoint/Watchpoint.h b/lldb/include/lldb/Breakpoint/Watchpoint.h
index c86d66663c7f8c0..851162af24c74e0 100644
--- a/lldb/include/lldb/Breakpoint/Watchpoint.h
+++ b/lldb/include/lldb/Breakpoint/Watchpoint.h
@@ -126,7 +126,7 @@ class Watchpoint : public std::enable_shared_from_this<Watchpoint>,
void GetDescription(Stream *s, lldb::DescriptionLevel level);
void Dump(Stream *s) const override;
- void DumpSnapshots(Stream *s, const char *prefix = nullptr) const;
+ bool DumpSnapshots(Stream *s, const char *prefix = nullptr) const;
void DumpWithLevel(Stream *s, lldb::DescriptionLevel description_level) const;
Target &GetTarget() { return m_target; }
const Status &GetError() { return m_error; }
diff --git a/lldb/include/lldb/Interpreter/OptionGroupWatchpoint.h b/lldb/include/lldb/Interpreter/OptionGroupWatchpoint.h
index f009fa145f30344..527a2612b189bd3 100644
--- a/lldb/include/lldb/Interpreter/OptionGroupWatchpoint.h
+++ b/lldb/include/lldb/Interpreter/OptionGroupWatchpoint.h
@@ -9,6 +9,7 @@
#ifndef LLDB_INTERPRETER_OPTIONGROUPWATCHPOINT_H
#define LLDB_INTERPRETER_OPTIONGROUPWATCHPOINT_H
+#include "lldb/Interpreter/OptionValueUInt64.h"
#include "lldb/Interpreter/Options.h"
namespace lldb_private {
@@ -21,8 +22,6 @@ class OptionGroupWatchpoint : public OptionGroup {
~OptionGroupWatchpoint() override = default;
- static bool IsWatchSizeSupported(uint32_t watch_size);
-
llvm::ArrayRef<OptionDefinition> GetDefinitions() override;
Status SetOptionValue(uint32_t option_idx, llvm::StringRef option_value,
@@ -43,7 +42,7 @@ class OptionGroupWatchpoint : public OptionGroup {
};
WatchType watch_type;
- uint32_t watch_size;
+ OptionValueUInt64 watch_size;
bool watch_type_specified;
lldb::LanguageType language_type;
diff --git a/lldb/include/lldb/Target/Process.h b/lldb/include/lldb/Target/Process.h
index a6d3e6c2d16926e..7bc0b5f47a9a1d5 100644
--- a/lldb/include/lldb/Target/Process.h
+++ b/lldb/include/lldb/Target/Process.h
@@ -41,6 +41,8 @@
#include "lldb/Target/ThreadList.h"
#include "lldb/Target/ThreadPlanStack.h"
#include "lldb/Target/Trace.h"
+#include "lldb/Target/WatchpointResource.h"
+#include "lldb/Target/WatchpointResourceList.h"
#include "lldb/Utility/ArchSpec.h"
#include "lldb/Utility/Broadcaster.h"
#include "lldb/Utility/Event.h"
@@ -2133,9 +2135,10 @@ class Process : public std::enable_shared_from_this<Process>,
lldb::BreakpointSiteSP &bp_site_sp);
// Process Watchpoints (optional)
- virtual Status EnableWatchpoint(Watchpoint *wp, bool notify = true);
+ virtual Status EnableWatchpoint(lldb::WatchpointSP wp_sp, bool notify = true);
- virtual Status DisableWatchpoint(Watchpoint *wp, bool notify = true);
+ virtual Status DisableWatchpoint(lldb::WatchpointSP wp_sp,
+ bool notify = true);
// Thread Queries
@@ -2989,6 +2992,8 @@ void PruneThreadPlans();
m_queue_list; ///< The list of libdispatch queues at a given stop point
uint32_t m_queue_list_stop_id; ///< The natural stop id when queue list was
///last fetched
+ WatchpointResourceList
+ m_watchpoint_resource_list; ///< Watchpoint resources currently in use.
std::vector<Notifications> m_notifications; ///< The list of notifications
///that this process can deliver.
std::vector<lldb::addr_t> m_image_tokens;
@@ -3069,6 +3074,9 @@ void PruneThreadPlans();
std::unique_ptr<UtilityFunction> m_dlopen_utility_func_up;
llvm::once_flag m_dlopen_utility_func_flag_once;
+ // Watchpoint hardware registers currently in use.
+ std::vector<lldb::WatchpointResourceSP> m_watchpoint_resources;
+
/// Per process source file cache.
SourceManager::SourceFileCache m_source_file_cache;
diff --git a/lldb/include/lldb/Target/WatchpointResource.h b/lldb/include/lldb/Target/WatchpointResource.h
new file mode 100644
index 000000000000000..73b539e79f9bfbb
--- /dev/null
+++ b/lldb/include/lldb/Target/WatchpointResource.h
@@ -0,0 +1,57 @@
+//===-- WatchpointResource.h ------------------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLDB_TARGET_WATCHPOINTRESOURCE_H
+#define LLDB_TARGET_WATCHPOINTRESOURCE_H
+
+#include "lldb/lldb-public.h"
+
+#include <set>
+
+namespace lldb_private {
+
+class WatchpointResource
+ : public std::enable_shared_from_this<WatchpointResource> {
+
+public:
+ // Constructors and Destructors
+ WatchpointResource(lldb::addr_t addr, size_t size, bool read, bool write);
+
+ ~WatchpointResource();
+
+ void GetResourceMemoryRange(lldb::addr_t &addr, size_t &size) const;
+
+ void GetResourceType(bool &read, bool &write) const;
+
+ void RegisterWatchpoint(lldb::WatchpointSP &wp_sp);
+
+ void DeregisterWatchpoint(lldb::WatchpointSP &wp_sp);
+
+ size_t GetNumDependantWatchpoints();
+
+ bool DependantWatchpointsContains(lldb::WatchpointSP wp_sp_to_match);
+
+private:
+ WatchpointResource(const WatchpointResource &) = delete;
+ const WatchpointResource &operator=(const WatchpointResource &) = delete;
+
+ // start address & size aligned & expanded to be a valid watchpoint
+ // memory granule on this target.
+ lldb::addr_t m_addr;
+ size_t m_size;
+
+ bool m_watch_read; // true if we stop when the watched data is read from
+ bool m_watch_write; // true if we stop when the watched data is written to
+
+ // The watchpoints using this WatchpointResource.
+ std::set<lldb::WatchpointSP> m_watchpoints;
+};
+
+} // namespace lldb_private
+
+#endif // LLDB_TARGET_WATCHPOINTRESOURCE_H
diff --git a/lldb/include/lldb/Target/WatchpointResourceList.h b/lldb/include/lldb/Target/WatchpointResourceList.h
new file mode 100644
index 000000000000000..9570e337f1e6ccb
--- /dev/null
+++ b/lldb/include/lldb/Target/WatchpointResourceList.h
@@ -0,0 +1,85 @@
+//===-- WatchpointResourceList.h --------------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLDB_TARGET_WATCHPOINTRESOURCELIST_H
+#define LLDB_TARGET_WATCHPOINTRESOURCELIST_H
+
+#include "lldb/Utility/Iterable.h"
+#include "lldb/lldb-private.h"
+#include "lldb/lldb-public.h"
+
+#include <mutex>
+#include <vector>
+
+namespace lldb_private {
+
+class WatchpointResourceList {
+
+public:
+ // Constructors and Destructors
+ WatchpointResourceList();
+
+ ~WatchpointResourceList();
+
+ /// Get the number of WatchpointResources that are available.
+ ///
+ /// \return
+ /// The number of WatchpointResources that are stored in the
+ /// WatchpointResourceList.
+ uint32_t GetSize();
+
+ /// Get the WatchpointResource at a given index.
+ ///
+ /// \param [in] idx
+ /// The index of the resource.
+ /// \return
+ /// The WatchpointResource at that index number.
+ lldb::WatchpointResourceSP GetResourceAtIndex(uint32_t idx);
+
+ /// Remove a WatchpointResource from the list.
+ ///
+ /// The WatchpointResource must have already been disabled in the
+ /// Process; this method only removes it from the list.
+ ///
+ /// \param [in] wp_resource_sp
+ /// The WatchpointResource to remove.
+ void RemoveWatchpointResource(lldb::WatchpointResourceSP wp_resource_sp);
+
+ typedef std::vector<lldb::WatchpointResourceSP> collection;
+ typedef LockingAdaptedIterable<collection, lldb::WatchpointResourceSP,
+ vector_adapter, std::mutex>
+ WatchpointResourceIterable;
+
+ /// Iterate over the list of WatchpointResources.
+ ///
+ /// \return
+ /// An Iterable object which can be used to loop over the resources
+ /// that exist.
+ WatchpointResourceIterable Resources() {
+ return WatchpointResourceIterable(m_resources, m_mutex);
+ }
+
+ /// Clear out the list of resources from the WatchpointResourceList
+ void Clear();
+
+ /// Add a WatchpointResource to the WatchpointResourceList.
+ ///
+ /// \param [in] resource
+ /// A WatchpointResource to be added.
+ void AddResource(lldb::WatchpointResourceSP resource_sp);
+
+ std::mutex &GetMutex();
+
+private:
+ collection m_resources;
+ std::mutex m_mutex;
+};
+
+} // namespace lldb_private
+
+#endif // LLDB_TARGET_WATCHPOINTRESOURCELIST_H
diff --git a/lldb/include/lldb/lldb-forward.h b/lldb/include/lldb/lldb-forward.h
index 3cd71c8a4ba3c0a..6b2f0dfd68c0b85 100644
--- a/lldb/include/lldb/lldb-forward.h
+++ b/lldb/include/lldb/lldb-forward.h
@@ -284,6 +284,7 @@ class VariableList;
class Watchpoint;
class WatchpointList;
class WatchpointOptions;
+class WatchpointResource;
class WatchpointSetOptions;
struct CompilerContext;
struct LineEntry;
@@ -461,6 +462,7 @@ typedef std::shared_ptr<lldb_private::Variable> VariableSP;
typedef std::shared_ptr<lldb_private::VariableList> VariableListSP;
typedef std::shared_ptr<lldb_private::ValueObjectList> ValueObjectListSP;
typedef std::shared_ptr<lldb_private::Watchpoint> WatchpointSP;
+typedef std::shared_ptr<lldb_private::WatchpointResource> WatchpointResourceSP;
} // namespace lldb
diff --git a/lldb/source/API/SBWatchpoint.cpp b/lldb/source/API/SBWatchpoint.cpp
index 8b4e0ad3178b182..f8bbc844532c2e1 100644
--- a/lldb/source/API/SBWatchpoint.cpp
+++ b/lldb/source/API/SBWatchpoint.cpp
@@ -147,9 +147,9 @@ void SBWatchpoint::SetEnabled(bool enabled) {
const bool notify = true;
if (process_sp) {
if (enabled)
- process_sp->EnableWatchpoint(watchpoint_sp.get(), notify);
+ process_sp->EnableWatchpoint(watchpoint_sp, notify);
else
- process_sp->DisableWatchpoint(watchpoint_sp.get(), notify);
+ process_sp->DisableWatchpoint(watchpoint_sp, notify);
} else {
watchpoint_sp->SetEnabled(enabled, notify);
}
diff --git a/lldb/source/Breakpoint/Watchpoint.cpp b/lldb/source/Breakpoint/Watchpoint.cpp
index 14144214faea747..5417ec728577596 100644
--- a/lldb/source/Breakpoint/Watchpoint.cpp
+++ b/lldb/source/Breakpoint/Watchpoint.cpp
@@ -12,6 +12,7 @@
#include "lldb/Core/Value.h"
#include "lldb/Core/ValueObject.h"
#include "lldb/Core/ValueObjectMemory.h"
+#include "lldb/DataFormatters/DumpValueObjectOptions.h"
#include "lldb/Expression/UserExpression.h"
#include "lldb/Symbol/TypeSystem.h"
#include "lldb/Target/Process.h"
@@ -161,7 +162,7 @@ bool Watchpoint::VariableWatchpointDisabler(void *baton,
"callback for watchpoint %" PRId32
" matched internal breakpoint execution context",
watch_sp->GetID());
- process_sp->DisableWatchpoint(watch_sp.get());
+ process_sp->DisableWatchpoint(watch_sp);
return false;
}
LLDB_LOGF(log,
@@ -266,33 +267,73 @@ void Watchpoint::Dump(Stream *s) const {
// If prefix is nullptr, we display the watch id and ignore the prefix
// altogether.
-void Watchpoint::DumpSnapshots(Stream *s, const char *prefix) const {
- if (!prefix) {
- s->Printf("\nWatchpoint %u hit:", GetID());
- prefix = "";
- }
+bool Watchpoint::DumpSnapshots(Stream *s, const char *prefix) const {
+ bool printed_anything = false;
+
+ // For read watchpoints, don't display any before/after value changes.
+ if (m_watch_read && !m_watch_modify && !m_watch_write)
+ return printed_anything;
+
+ s->Printf("\n");
+ s->Printf("Watchpoint %u hit:\n", GetID());
+
+ StreamString values_ss;
+ if (prefix)
+ values_ss.Indent(prefix);
if (m_old_value_sp) {
const char *old_value_cstr = m_old_value_sp->GetValueAsCString();
- if (old_value_cstr && old_value_cstr[0])
- s->Printf("\n%sold value: %s", prefix, old_value_cstr);
- else {
+ if (old_value_cstr) {
+ values_ss.Printf("old value: %s", old_value_cstr);
+ } else {
const char *old_summary_cstr = m_old_value_sp->GetSummaryAsCString();
- if (old_summary_cstr && old_summary_cstr[0])
- s->Printf("\n%sold value: %s", prefix, old_summary_cstr);
+ if (old_summary_cstr)
+ values_ss.Printf("old value: %s", old_summary_cstr);
+ else {
+ StreamString strm;
+ DumpValueObjectOptions options;
+ options.SetUseDynamicType(eNoDynamicValues)
+ .SetHideRootType(true)
+ .SetHideRootName(true)
+ .SetHideName(true);
+ m_old_value_sp->Dump(strm, options);
+ if (strm.GetData())
+ values_ss.Printf("old value: %s", strm.GetData());
+ }
}
}
if (m_new_value_sp) {
+ if (values_ss.GetSize())
+ values_ss.Printf("\n");
+
const char *new_value_cstr = m_new_value_sp->GetValueAsCString();
- if (new_value_cstr && new_value_cstr[0])
- s->Printf("\n%snew value: %s", prefix, new_value_cstr);
+ if (new_value_cstr)
+ values_ss.Printf("new value: %s", new_value_cstr);
else {
const char *new_summary_cstr = m_new_value_sp->GetSummaryAsCString();
- if (new_summary_cstr && new_summary_cstr[0])
- s->Printf("\n%snew value: %s", prefix, new_summary_cstr);
+ if (new_summary_cstr)
+ values_ss.Printf("new value: %s", new_summary_cstr);
+ else {
+ StreamString strm;
+ DumpValueObjectOptions options;
+ options.SetUseDynamicType(eNoDynamicValues)
+ .SetHideRootType(true)
+ .SetHideRootName(true)
+ .SetHideName(true);
+ m_new_value_sp->Dump(strm, options);
+ if (strm.GetData())
+ values_ss.Printf("new value: %s", strm.GetData());
+ }
}
}
+
+ if (values_ss.GetSize()) {
+ s->Printf("%s", values_ss.GetData());
+ printed_anything = true;
+ }
+
+ return printed_anything;
}
void Watchpoint::DumpWithLevel(Stream *s,
diff --git a/lldb/source/Commands/CommandObjectWatchpoint.cpp b/lldb/source/Commands/CommandObjectWatchpoint.cpp
index dc5be0da43f5e62..92d3999827db238 100644
--- a/lldb/source/Commands/CommandObjectWatchpoint.cpp
+++ b/lldb/source/Commands/CommandObjectWatchpoint.cpp
@@ -930,9 +930,9 @@ corresponding to the byte size of the data type.");
if (addr_type == eAddressTypeLoad) {
// We're in business.
// Find out the size of this variable.
- size = m_option_watchpoint.watch_size == 0
+ size = m_option_watchpoint.watch_size.GetCurrentValue() == 0
? valobj_sp->GetByteSize().value_or(0)
- : m_option_watchpoint.watch_size;
+ : m_option_watchpoint.watch_size.GetCurrentValue();
}
compiler_type = valobj_sp->GetCompilerType();
} else {
@@ -1127,8 +1127,8 @@ class CommandObjectWatchpointSetExpression : public CommandObjectRaw {
return false;
}
- if (m_option_watchpoint.watch_size != 0)
- size = m_option_watchpoint.watch_size;
+ if (m_option_watchpoint.watch_size.GetCurrentValue() != 0)
+ size = m_option_watchpoint.watch_size.GetCurrentValue();
else
size = target->GetArchitecture().GetAddressByteSize();
diff --git a/lldb/source/Interpreter/OptionGroupWatchpoint.cpp b/lldb/source/Interpreter/OptionGroupWatchpoint.cpp
index c3708e7a1e80fd2..d1ae916cd74b1c0 100644
--- a/lldb/source/Interpreter/OptionGroupWatchpoint.cpp
+++ b/lldb/source/Interpreter/OptionGroupWatchpoint.cpp
@@ -39,35 +39,12 @@ static constexpr OptionEnumValueElement g_watch_type[] = {
},
};
-static constexpr OptionEnumValueElement g_watch_size[] = {
- {
- 1,
- "1",
- "Watch for byte size of 1",
- },
- {
- 2,
- "2",
- "Watch for byte size of 2",
- },
- {
- 4,
- "4",
- "Watch for byte size of 4",
- },
- {
- 8,
- "8",
- "Watch for byte size of 8",
- },
-};
-
static constexpr OptionDefinition g_option_table[] = {
{LLDB_OPT_SET_1, false, "watch", 'w', OptionParser::eRequiredArgument,
nullptr, OptionEnumValues(g_watch_type), 0, eArgTypeWatchType,
"Specify the type of watching to perform."},
{LLDB_OPT_SET_1, false, "size", 's', OptionParser::eRequiredArgument,
- nullptr, OptionEnumValues(g_watch_size), 0, eArgTypeByteSize,
+ nullptr, {}, 0, eArgTypeByteSize,
"Number of bytes to use to watch a region."},
{LLDB_OPT_SET_2,
false,
@@ -80,16 +57,6 @@ static constexpr OptionDefinition g_option_table[] = {
eArgTypeLanguage,
"Language of expression to run"}};
-bool OptionGroupWatchpoint::IsWatchSizeSupported(uint32_t watch_size) {
- for (const auto& size : g_watch_size) {
- if (0 == size.value)
- break;
- if (watch_size == size.value)
- return true;
- }
- return false;
-}
-
Status
OptionGroupWatchpoint::SetOptionValue(uint32_t option_idx,
llvm::StringRef option_arg,
@@ -120,8 +87,10 @@ OptionGroupWatchpoint::SetOptionValue(uint32_t option_idx,
break;
}
case 's':
- watch_size = (uint32_t)OptionArgParser::ToOptionEnum(
- option_arg, g_option_table[option_idx].enum_values, 0, error);
+ error = watch_size.SetValueFromString(option_arg);
+ if (watch_size.GetCurrentValue() == 0)
+ error.SetErrorStringWithFormat("invalid --size option value '%s'",
+ option_arg.str().c_str());
break;
default:
@@ -135,7 +104,7 @@ void OptionGroupWatchpoint::OptionParsingStarting(
ExecutionContext *execution_context) {
watch_type_specified = false;
watch_type = eWatchInvalid;
- watch_size = 0;
+ watch_size.Clear();
language_type = eLanguageTypeUnknown;
}
diff --git a/lldb/source/Plugins/Process/MacOSX-Kernel/ProcessKDP.cpp b/lldb/source/Plugins/Process/MacOSX-Kernel/ProcessKDP.cpp
index 79f8b15a7f229cc..2797b94ad67cc43 100644
--- a/lldb/source/Plugins/Process/MacOSX-Kernel/ProcessKDP.cpp
+++ b/lldb/source/Plugins/Process/MacOSX-Kernel/ProcessKDP.cpp
@@ -672,20 +672,6 @@ Status ProcessKDP::DisableBreakpointSite(BreakpointSite *bp_site) {
return DisableSoftwareBreakpoint(bp_site);
}
-Status ProcessKDP::EnableWatchpoint(Watchpoint *wp, bool notify) {
- Status error;
- error.SetErrorString(
- "watchpoints are not supported in kdp remote debugging");
- return error;
-}
-
-Status ProcessKDP::DisableWatchpoint(Watchpoint *wp, bool notify) {
- Status error;
- error.SetErrorString(
- "watchpoints are not supported in kdp remote debugging");
- return error;
-}
-
void ProcessKDP::Clear() { m_thread_list.Clear(); }
Status ProcessKDP::DoSignal(int signo) {
diff --git a/lldb/source/Plugins/Process/MacOSX-Kernel/ProcessKDP.h b/lldb/source/Plugins/Process/MacOSX-Kernel/ProcessKDP.h
index 3c12fd4074499a9..e5ec5914f9600d0 100644
--- a/lldb/source/Plugins/Process/MacOSX-Kernel/ProcessKDP.h
+++ b/lldb/source/Plugins/Process/MacOSX-Kernel/ProcessKDP.h
@@ -124,13 +124,6 @@ class ProcessKDP : public lldb_private::Process {
lldb_private::Status
DisableBreakpointSite(lldb_private::BreakpointSite *bp_site) override;
- // Process Watchpoints
- lldb_private::Status EnableWatchpoint(lldb_private::Watchpoint *wp,
- bool notify = true) override;
-
- lldb_private::Status DisableWatchpoint(lldb_private::Watchpoint *wp,
- bool notify = true) override;
-
CommunicationKDP &GetCommunication() { return m_comm; }
protected:
diff --git a/lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp b/lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp
index d60e6250c7c0aca..7ef0bebe54ef425 100644
--- a/lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp
+++ b/lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp
@@ -491,6 +491,9 @@ static StopInfoSP GetStopInfoForHardwareBP(Thread &thread, Target *target,
uint64_t exc_sub_sub_code) {
// Try hardware watchpoint.
if (target) {
+ // LWP_TODO: We need to find the WatchpointResource that matches
+ // the address, and evaluate its Watchpoints.
+
// The exc_sub_code indicates the data break address.
lldb::Wat...
[truncated]
|
You can test this locally with the following command:git-clang-format --diff 212a60ec37322f853e91e171b305479b1abff2f2 0c5a738985833571dba8afcf2ac6cf8b3189fac0 -- lldb/include/lldb/Breakpoint/StopPointSiteList.h lldb/include/lldb/Breakpoint/WatchpointResource.h lldb/include/lldb/Breakpoint/WatchpointResourceList.h lldb/source/Breakpoint/StopPointSiteList.cpp lldb/source/Breakpoint/WatchpointResource.cpp lldb/source/Breakpoint/WatchpointResourceList.cpp lldb/include/lldb/Breakpoint/BreakpointSite.h lldb/include/lldb/Breakpoint/Watchpoint.h lldb/include/lldb/Interpreter/OptionGroupWatchpoint.h lldb/include/lldb/Target/Process.h lldb/include/lldb/lldb-defines.h lldb/include/lldb/lldb-forward.h lldb/include/lldb/lldb-types.h lldb/source/API/SBThread.cpp lldb/source/API/SBWatchpoint.cpp lldb/source/Breakpoint/BreakpointLocation.cpp lldb/source/Breakpoint/BreakpointSite.cpp lldb/source/Breakpoint/Watchpoint.cpp lldb/source/Commands/CommandObjectProcess.cpp lldb/source/Commands/CommandObjectWatchpoint.cpp lldb/source/Interpreter/OptionGroupWatchpoint.cpp lldb/source/Plugins/LanguageRuntime/CPlusPlus/ItaniumABI/ItaniumABILanguageRuntime.cpp lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntime.cpp lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleThreadPlanStepThroughObjCTrampoline.cpp lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp lldb/source/Plugins/Process/MacOSX-Kernel/ProcessKDP.cpp lldb/source/Plugins/Process/MacOSX-Kernel/ProcessKDP.h lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp lldb/source/Plugins/Process/Windows/Common/ProcessWindows.h lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h lldb/source/Target/Platform.cpp lldb/source/Target/Process.cpp lldb/source/Target/StackFrameList.cpp lldb/source/Target/StopInfo.cpp lldb/source/Target/Target.cpp lldb/source/Target/ThreadPlanCallFunction.cpp lldb/source/Target/ThreadPlanStepOut.cpp lldb/source/Target/ThreadPlanStepRange.cpp lldb/source/Target/ThreadPlanStepUntil.cpp lldb/test/Shell/Watchpoint/Inputs/val.cView the diff from clang-format here.diff --git a/lldb/source/Breakpoint/WatchpointResource.cpp b/lldb/source/Breakpoint/WatchpointResource.cpp
index 4b8fbe42c8..7b167edcc6 100644
--- a/lldb/source/Breakpoint/WatchpointResource.cpp
+++ b/lldb/source/Breakpoint/WatchpointResource.cpp
@@ -15,8 +15,8 @@ using namespace lldb_private;
WatchpointResource::WatchpointResource(lldb::addr_t addr, size_t size,
bool read, bool write)
- : m_id(GetNextID()), m_addr(addr), m_size(size),
- m_watch_read(read), m_watch_write(write) {}
+ : m_id(GetNextID()), m_addr(addr), m_size(size), m_watch_read(read),
+ m_watch_write(write) {}
WatchpointResource::~WatchpointResource() {
std::lock_guard<std::mutex> guard(m_constituents_mutex);
diff --git a/lldb/source/Interpreter/OptionGroupWatchpoint.cpp b/lldb/source/Interpreter/OptionGroupWatchpoint.cpp
index d1ae916cd7..48cbc9ec0e 100644
--- a/lldb/source/Interpreter/OptionGroupWatchpoint.cpp
+++ b/lldb/source/Interpreter/OptionGroupWatchpoint.cpp
@@ -43,8 +43,15 @@ static constexpr OptionDefinition g_option_table[] = {
{LLDB_OPT_SET_1, false, "watch", 'w', OptionParser::eRequiredArgument,
nullptr, OptionEnumValues(g_watch_type), 0, eArgTypeWatchType,
"Specify the type of watching to perform."},
- {LLDB_OPT_SET_1, false, "size", 's', OptionParser::eRequiredArgument,
- nullptr, {}, 0, eArgTypeByteSize,
+ {LLDB_OPT_SET_1,
+ false,
+ "size",
+ 's',
+ OptionParser::eRequiredArgument,
+ nullptr,
+ {},
+ 0,
+ eArgTypeByteSize,
"Number of bytes to use to watch a region."},
{LLDB_OPT_SET_2,
false,
|
|
I started on the next step of updating ProcessGDBRemote to identify the WatchpointResource that was hit by a watchpoint stop, and evaluate all of its Watchpoints, when I looked at BreakpointSite and BreakpointSiteList and realize I should have started by copying their design/functionality. I'm rewriting WatchpointResource / WatchpointResourceList right now. |
| # but the lines being output by lldb are identical, | ||
| # by visual inspection. | ||
| # FileCheck is seeing some difference between them, | ||
| # which I need to get to the bottom of. |
There was a problem hiding this comment.
A trick I used to do for python doctests was str.replace(" ", "?"). Then you could see the trailing spaces. Might work here if you have that or extra newlines.
There was a problem hiding this comment.
Good idea, it must surely be something like that. The test wasn't adding anything super useful and I spent so much time trying to rearrange things to make it pass I finally punted on it for now. I'm going to need to add a bunch of new tests once I get the large watchpoint work finished.
| @@ -135,5 +135,5 @@ def test_watch_address_with_invalid_watch_size(self): | |||
| self.expect( | |||
| error.GetCString(), | |||
| exe=False, | |||
| substrs=["watch size of %d is not supported" % 365], | |||
| substrs=["Setting one of the watchpoint resources failed"], | |||
There was a problem hiding this comment.
Is the original error still relevant? Should it be one of the watchpoint resources failed because...
There was a problem hiding this comment.
This test is trying to watch a 365 byte region, it was intended to detect the fact that you can only watch 1/2/4/8 byte blocks of memory (the --size was really an enum and it had to be one of those). With this patch currently, lldb will accept the 365 byte watch request and pass it to the remote stub, which will probably return an error. (debugserver will actually break it into aligned watchable regions and do the right thing). When I finish the large watchpoint work, lldb will break this 365 byte request into 46 8-byte regions (for linux with BAS only watchpoints) and when we try to set the 5th one (or however many the core actually supports), that watch request will fail. Maybe the error message should say something about how your watchpoint was broken into watchpoint requests but only of them could be set.
There was a problem hiding this comment.
Yeah I don't think it would need to be super specific, that is what logs are for. Drawing the distinction between failed because the stub didn't have resources, and failed because the stub exploded, that's more what I'd like to see.
And here the original error message is actually still pretty vague. It tells me that size N isn't support, but not what size is supported (which is a whole other thing).
| uint8_t x1 = 0; | ||
| uint16_t x2 = 0; | ||
| long x1 = 0; | ||
| long x2 = 0; |
There was a problem hiding this comment.
We need the type change here because?
There was a problem hiding this comment.
You're right. I thought this test was unintentionally testing the case where we have two hardware watchpoint register watching the same doubleword of memory. But that was actually the point of the test (v. https://reviews.llvm.org/D77173 ). I'm not sure how thrilled I am about this test, it seems to be encoding the behavior of the cpu with multiple watchpoint registers on the same doubleword and I don't know how universal that is - if a CPU might only evaluate the first watchpoint on a doubleword and ignore the second.
This is very much in the wheelhouse of the work I need to do on large watchpoints where a single WatchpointResource should be used for both watchpoints, expanded to include the byte range of both, and then reduced back down again when one is disabled or deleted. I'll be adding tests like this one as part of that work. I'll revert my change here.
| if (idx < m_resources.size()) { | ||
| return m_resources[idx]; | ||
| } else { | ||
| return {}; |
There was a problem hiding this comment.
Also return in an else (as in, no need for the else)
|
@bulbazord @JDevlieghere OK this is the second set of refinement where it's closer to the structure of BreakpointSites. It's still not actually doing anything -- each one WatchpointResource corresponds to one Watchpoint, a single Watchpoint's request is not broken into multiple WatchpointResources, and when a watchpoint access is hit, I'm not iterating all of the Watchpoints (lol, all 1 of them) to bump their hit count, run conditions, etc. These remaining pieces of work will be smaller scale, more complicated I think, and it will be easier to consider those parts if the patches are small. |
| // ordering. If a Process can correctly identify the hardware watchpoint | ||
| // register index when it has created the Resource, it may initialize it | ||
| // before it is inserted in the WatchpointResourceList. | ||
| void SetID(lldb::wp_resource_id_t); |
There was a problem hiding this comment.
IIUC, this ID shouldn't change during the Resource's lifetime so why not set it once in the constructor, and get rid of this setter ?
There was a problem hiding this comment.
When the Process method is given a watchpoint request from the user, which must be broken into one or more WatchpointResources (or an existing WatchpointResource reused), we can only get the hardware register number by looking at the IDs of currently allocated WatchpointResources in the WatchpointResourceList, which start at 0. I assume that the remote stub will allocate its watchpoints in the hardware registers starting at index 0, and when removing/disabling them, will leave that slot unused until another request comes along, at which point the stub will start from 0 and look for the first unused hardware register.
In the specific case of gdb remote serial protocol (ProcessGDBRemote), it has none of this. The Z2/Z3/Z4 packets don't tell you what hardware register was used.
Today, lldb sets the hardware watchpoint index based on the watchpoint access stop. SOME ways of passing the watchpoint trap back to lldb can communicate the hardware watchpoint index. The standard gdb remote serial protocol does not with its watch/rwatch/awatch:address key-value in the stop packet. lldb-server has a different way of reporting watchpoints, reason:watchpoint and description:addr[ index] where the address is sent in the description: and it may be followed by a space and then the index number of the hardware watchpoint that was triggered. For macOS systems, the mach exception data is sent at the stop, and the hardware index is encoded as a piece of data in that.
In reality, in both cases I believe, the stubs are actually getting an address of the access, and trying to figure out which one of the hardware watchpoints they have set was the actual one triggered. Aarch64 will some day have a way reporting the actual hardware index triggered instead of the exception address (it may not even give an exception address at all), but today it's all a fiction.
So basically, today, you don't really know the hardware watchpoint index until it's been hit. And even then, it's being deduced by the remote stub, which lldb could do just as well. The idea to expose this piece of data we don't actually have was not a good one, IMO, but it's in SB API so I'm not tackling that for now.
If I wanted to have the Process plugin look at the IDs currently used in its WatchpointResourceList, use the available numbers for the WatchpointResources it is creating, and if they are set successfully in the inferior, add them to the WatchpointResourceList, I would need to lock the WatchpointResourceList for the duration of those operations, and I wasn't sure that was a good idea.
(sorry for the long reply, I spent so much time trying to decide what to do with these fictitious hardware index numbers we present in lldb)
There was a problem hiding this comment.
To be honest, I'm VERY close to just removing the hardware index numbers altogether and having SBWatchpoint::GetHardwareIndex return UINT32_MAX. It might be the better idea than these convoluted things I'm doing to try to heuristically guess the hw index.
There was a problem hiding this comment.
Thanks for the explanation, that makes more sense to me now.
|
|
||
| ~WatchpointResource(); | ||
|
|
||
| void GetMemoryRange(lldb::addr_t &addr, size_t &size) const; |
There was a problem hiding this comment.
Instead of out parameters, you could do std::pair<addr_t, size_t>.
But why have this and GetAddress() + GetByteSize()?
There was a problem hiding this comment.
I originally wrote the WatchpointResource::GetMemoryRange that returned both via out parameters, then at a later point I added a GetAddress & GetByteSize methods meaning to remove GetMemoryRange which I wasn't happy with... but forgot.
|
|
||
| size_t GetByteSize() const; | ||
|
|
||
| void GetType(bool &read, bool &write) const; |
There was a problem hiding this comment.
Suggestion: Return std::pair<bool, bool> instead of passing out parameters.
There was a problem hiding this comment.
idk about this one, GetType(bool &read, bool &write) makes the order easy to see, and matches the SetType (bool read, bool write) whereas the pair would be less explicitly named. I should follow the Watchpoint class method and have bool WatchpointResourceRead(), bool WatchpointResourceWrite().
| /// | ||
| /// \return | ||
| /// The number of owners. | ||
| size_t GetNumberOfOwners(); |
There was a problem hiding this comment.
This can be const-qualified right?
There was a problem hiding this comment.
I lock the object's mutex in all of these methods.
There was a problem hiding this comment.
Ah I could have sworn the mutex was marked as mutable. How are things like GetType and GetByteSize marked const then? Do they not lock?
There was a problem hiding this comment.
Oh wait, the mutex is to protect the Owners. Nevermind!
There was a problem hiding this comment.
tbh there could be an argument for locking the whole object because a WatchpointResource may be expanded to service multiple watchpoints (one watchpoint watches bytes 0-1, a second watchpoint watches bytes 2-3) and then reduced again when one of those watchpoints is deleted/disabled.
There was a problem hiding this comment.
none of that is done yet though.
| /// This method returns the Watchpoint at index \a index using this | ||
| /// Resource. The owners are listed ordinally from 0 to | ||
| /// GetNumberOfOwners() - 1 so you can use this method to iterate over the | ||
| /// owners. | ||
| /// | ||
| /// \param[in] idx | ||
| /// The index in the list of owners for which you wish the owner location. | ||
| /// | ||
| /// \return | ||
| /// The Watchpoint at that index. | ||
| lldb::WatchpointSP GetOwnerAtIndex(size_t idx); |
There was a problem hiding this comment.
Instead of exposing this primitive, maybe it would make more sense to return an iterator or an iterator range?
Something like:
IteratorRange<lldb::WatchpointSP> Owners();
IMO this would make it easier to write bug-free code since we're not going to have to manually set up index iteration logic.
There was a problem hiding this comment.
Just getting back to the review comments. I remember writing something about this last week but we have some API to select specific BreakpointLocations that own the BreakpointSite and the code to do that is most naturally exposed as "GetAtIndex" style access. I'm trying to create the WatchpointResource class similar to BreakpointSite so I can follow the same coding style for these Watchpoints that own the WatchpointResources. I may end up being able to drop the GetAtIndex method later if it's natural go access it via an Owners method, but I'm starting with the assumption that this will be more natural.
| /// This method copies the watchpoint resource's owners into a new collection. | ||
| /// It does this while the owners mutex is locked. | ||
| /// | ||
| /// \param[out] out_collection | ||
| /// The BreakpointLocationCollection into which to put the owners | ||
| /// of this breakpoint site. | ||
| /// | ||
| /// \return | ||
| /// The number of elements copied into out_collection. | ||
| size_t CopyOwnersList(WatchpointCollection &out_collection); |
There was a problem hiding this comment.
Why return the size instead of returning just a new WatchpointCollection object? That way callers don't have to create a new one and pass it in, they can just grab it from this method directly.
There was a problem hiding this comment.
Hm, I do not have a lot of confidence about avoiding an extra copy of the vector if I return by value.
There was a problem hiding this comment.
It should take advantage of copy elision. Specifically we should be able to take advantage of NRVO.
There was a problem hiding this comment.
Starting with C++17, copy elision is guaranteed.
| for (size_t i = 0; i < size; ++i) { | ||
| out_collection.Add(m_owners.GetByIndex(i)); | ||
| } |
There was a problem hiding this comment.
- no need for braces per llvm style guidelines.
- Instead of iterating by index, you could do something like:
for (auto watchpoint_sp : m_owners)
out_collection.Add(watchpoint_sp);
or something to this effect (use of auto is personal preference)
| @@ -491,14 +491,13 @@ static StopInfoSP GetStopInfoForHardwareBP(Thread &thread, Target *target, | |||
| uint64_t exc_sub_sub_code) { | |||
| // Try hardware watchpoint. | |||
| if (target) { | |||
| // LWP_TODO: We need to find the WatchpointResource that matches | |||
There was a problem hiding this comment.
LWP_TODO doesn't mean much to a lot of contributors, maybe you can change this to something like TODO(Large Watchpoint Support)? Mostly to avoid acronyms that aren't written down anywhere else.
There was a problem hiding this comment.
Yeah good point my idea was that these are temporary placeholders as I roll this out, so they wouldn't be in the source for very long. (this feature is not really useful with all the parts landed so it's unlikely I'll be distracted by some other issue ....)
| Status error; | ||
|
|
||
| if (wp->IsEnabled()) { | ||
| wp->SetEnabled(true, notify); | ||
| if (wp_sp->IsEnabled()) { |
There was a problem hiding this comment.
Should we check if wp_sp is even valid first?
There was a problem hiding this comment.
The original code didn't check it, but more importantly on this topic, I have real concern about touching any of ProcessWindows, NativeProcessWindows because I can't build or test it directly myself. I have a feeling I'll keep most/all of the existing interfaces for creating a StopInfo etc so the Windows code can continue to work unmodified.
| bool read, write; | ||
| wp_res_sp->GetType(read, write); | ||
|
|
||
| assert((read || write) && "read and write cannot both be false."); |
There was a problem hiding this comment.
The assertion message here could be fleshed out a bit more. Something like "WatchpointResource type is neither read nor write".
| if (wp) { | ||
| user_id_t watchID = wp->GetID(); | ||
| addr_t addr = wp->GetLoadAddress(); | ||
| if (wp_sp) { |
There was a problem hiding this comment.
Since you're already refactoring this part, maybe you could flip the condition and do the error handling up front. It's small, but it saves a level of indentation in an already large code block.
| if (it != m_owners.end()) | ||
| return true; | ||
| return false; |
| for (WatchpointCollection::const_iterator it = m_owners.begin(); | ||
| it != m_owners.end(); ++it) | ||
| if ((*it).get() == wp) | ||
| return true; | ||
| return false; |
There was a problem hiding this comment.
WatchpointCollection got removed right? I think you'll need to rewrite it... I'd suggest using find_if
| lldbassert(idx < m_owners.size()); | ||
| if (idx >= m_owners.size()) | ||
| return {}; | ||
|
|
||
| return m_owners.GetByIndex(idx); | ||
| return m_owners[idx]; |
There was a problem hiding this comment.
Looking over this again, is there a use case where you would want to reference a specific owner by index instead of using Owners()?
There was a problem hiding this comment.
Right now nothing is using the WatchpointResource owners - I need to add that in a follow on patch when a watchpoint has been triggered. BreakpointSite uses this method in many places, but I agree that the Owners() method could be used. We lose the range check/possible-assert in GetOwnerAtIndex() but hopefully that shouldn't actually happen...
There was a problem hiding this comment.
I'm not completely clear what having WatchpointResource::Owners() returning a WatchpointIterable is going to do beyond allowing someone to write for (WatchpointSP &wp_sp : wp_resource_sp->Owners()) - it's not exposing the underlying std::vector so I can't call wp_resource_sp->Owners().size() or wp_resource_sp->Owners()[2] is it? Right now the WatchpointResource has methods like AddOwner, RemoveOwner, GetNumberOfOwners, GetOwnerAtIndex, and some methods to check if the list of owners contains a watchpoint; should callers be able to lock the Resource's mutex and get a reference to the std::vector and query/manipulate it directly?
There was a problem hiding this comment.
Well, I'm not entirely sure how we're going to want to access the Owners list, so I'd like to understand more how you plan on using it. If you want to do something like wp_resource_sp->Owners()[2] then this method makes sense to keep around, but I'd also be curious to know why you would want to access the 3rd owner directly. What I would like to avoid is writing a loop like this:
for (int i = 0; i < wp_resource_sp.GetNumberOfOwners(); i++) {
WatchpointSP wp_sp = wp_resource_sp->GetOwnerAtIndex(i);
// Use wp_sp
}
The two main reasons I am not a fan of this pattern:
- As you pointed out, one would need to grab the
wp_resource_spowner mutex to do this safely since another thread may callRemoveOwnerin the middle of this loop. I can't say I'm a fan of exposing the vector primarily because it would mean we'd have to expose the mutex too. - Off-by-one errors are fairly easy to put in by mistake. Doubly so if we ever iterate in reverse (i >0 vs i >=0 for the termination condition).
If we don't need to be able to access any of the owners at an arbitrary index, I would suggest removing it so nobody can write that kind of loop. WDYT?
There was a problem hiding this comment.
This is a good point. I am mostly trying to copy the API of BreakpointSite which has methods like this, and that's how its Owners are accessed today in different parts of lldb. I haven't gotten to the point where I use the Owners anywhere yet, so I'm not locked in to the same API that BreakpointSite exposes. We have things like SBThread::GetStopReasonDataAtIndex() which for Breakpoints exposes all of the Breakpoints that were owning the BreakpointSite that was hit. We have other places like StackFrameList::ResetCurrentInlinedDepth which is iterating over a BreakpointSite's owners to see if any of them are internal; the IterableWatchpoint returned by WatchpointResources::Owners would be a cleaner approach in this case. We have some places in the code that need to know the address of the breakpoint that was hit and are grabbing the first BreakpointLocation associated with the BreakpointSite, e.g. PlatformDarwin::GetSoftwareBreakpointTrapOpcode (for some arm/thumb reason). In ThreadPlanCallFunction::DoPlanExplainsStop we're iterating over the Owners to log about them and detect if any are Internal breakpoint locations, which could be done with an Iterable.
| : public std::enable_shared_from_this<WatchpointResource> { | ||
|
|
||
| public: | ||
| // Constructors and Destructors |
There was a problem hiding this comment.
Nit: these comments are useless. We've been gradually removing them. Please remove this.
| /// | ||
| /// \result | ||
| /// true if this resource's owners includes the watchpoint. | ||
| bool OwnersContains(const lldb_private::Watchpoint *wp); |
There was a problem hiding this comment.
Can this be a const reference?
| /// | ||
| /// \result | ||
| /// true if this resource's owners includes the watchpoint. | ||
| bool OwnersContains(lldb::WatchpointSP &wp_sp); |
There was a problem hiding this comment.
Does this method care about the shared pointer or the underlying watchpoint? If it's the former, the docs should make that explicit.
| /// This method copies the watchpoint resource's owners into a new collection. | ||
| /// It does this while the owners mutex is locked. | ||
| /// | ||
| /// \param[out] out_collection | ||
| /// The BreakpointLocationCollection into which to put the owners | ||
| /// of this breakpoint site. | ||
| /// | ||
| /// \return | ||
| /// The number of elements copied into out_collection. | ||
| size_t CopyOwnersList(WatchpointCollection &out_collection); |
There was a problem hiding this comment.
Starting with C++17, copy elision is guaranteed.
| private: | ||
| lldb::wp_resource_id_t m_id; | ||
|
|
||
| // start address & size aligned & expanded to be a valid watchpoint |
| prefix = ""; | ||
| } | ||
| bool Watchpoint::DumpSnapshots(Stream *s, const char *prefix) const { | ||
| bool printed_anything = false; |
There was a problem hiding this comment.
Doesn't look like you actually need this. You change the value exactly once below. I would just return false on line 276 and 337 and true on line 334. I think the return value of a Dump function is more self explanatory than having to scroll up and see if printed_anything happened to be modified in the 50 lines between 275 and 325.
There was a problem hiding this comment.
The convention for these methods seems to be to return a stream without a final end-of-line character. In StopInfo.cpp we call this method and check if it printed anything, and if so, print an EOL. In Watchpoint::DumpWithLevel we ignore the return type from DumpSnapshots, but that's only because every line it adds is assumed to end without an EOL. I don't really need a variable in this method, I could return false in the two places it is false and true in the one place it is true.
| else | ||
| return UINT32_MAX; |
|
|
||
| WatchpointSP WatchpointResource::GetOwnerAtIndex(size_t idx) { | ||
| std::lock_guard<std::recursive_mutex> guard(m_owners_mutex); | ||
| lldbassert(idx < m_owners.size()); |
There was a problem hiding this comment.
I'd just make this a normal assert.
| for (auto wp_sp : m_owners) { | ||
| out_collection.push_back(wp_sp); | ||
| } |
There was a problem hiding this comment.
LGTM with a few nits. I like "constituents".
This is a pretty big patch which makes reviewing it challenging. I know it's a big change that touches a lot of things but I'm sure that this could've been broken up into smaller patches if you keep that goal in mind from the beginning. Something to look out for in the future.
| #if 0 | ||
| m_site_list.insert(iter, collection::value_type(site_load_addr, site)); | ||
| #endif |
| StopPointSiteSP site_sp(FindByID(site_id)); | ||
| if (site_sp) { |
There was a problem hiding this comment.
if(StopPointSiteSP site_sp = FindByID(site_id))
| template <typename StopPointSite> | ||
| void StopPointSiteList<StopPointSite>::Dump(Stream *s) const { | ||
| s->Printf("%p: ", static_cast<const void *>(this)); | ||
| // s->Indent(); |
| for (pos = lower; pos != upper; pos++) { | ||
| site_list.Add((*pos).second); | ||
| } |
| namespace lldb_private { | ||
| template class StopPointSiteList<BreakpointSite>; | ||
| template class StopPointSiteList<WatchpointResource>; | ||
| } // namespace lldb_private |
There was a problem hiding this comment.
Although this is perfectly fine way to allow you to define your templated methods in the .cpp file, as far as I can tell we don't do this anywhere else in LLDB. I found some prior art in LLVM and the fact that you have exactly two instantiations helps but given that this (1) doesn't pull in all that many dependencies and (2) probably ins't included soo much that we have to worry about compile time, I would've just implemented this in the header.
| Log *log(GetLog(GDBRLog::Watchpoints)); | ||
| LLDB_LOGF(log, "failed to find watchpoint"); | ||
| abort(); // LWP_TODO FIXME don't continue executing this block if |
There was a problem hiding this comment.
My refactored code added a new nullptr deref, when I saw what I'd done I threw in an abort and a FIXME comment to fix that. But I see how to fix it, let me do that.
Thanks for the feedback. Yeah originally this patch was a bit smaller but it has Grown as I've addressed (correct, good) feedback from everyone and now it's a little bit of a monster. I'm surely going to have to rebase it before I can merge. |
This patch is rearranging code a bit to add WatchpointResources to
Process. A WatchpointResource is meant to represent a hardware
watchpoint register in the inferior process. It has an address, a
size, a type, and a list of Watchpoints that are using this
WatchpointResource.
This current patch doesn't add any of the features of WatchpointResources
that make them interesting -- a user asking to watch a 24 byte
object could watch this with three 8 byte WatchpointResources. Or
a Watchpoint on 1 byte at 0x1002 and a second watchpoint on 1 byte
at 0x1003, these must both be served by a single WatchpointResource
on that doubleword at 0x1000 on a 64-bit target, if two hardware
watchpoint registers were used to track these separately, one of
them may not be hit. Or if you have one Watchpoint on a variable
with a condition set, and another Watchpoint on that same variable
with a command defined or different condition, or ignorecount, both
of those Watchpoints need to evaluate their criteria/commands when
their WatchpointResource has been hit.
There's a bit of code movement to rearrange things in the direction
I'll need for implementing this feature, so I want to start with
reviewing & landing this mostly NFC patch and we can focus on the
algorithmic choices about how WatchpointResources are shared and
handled as they're triggeed, separately.
This patch also stops printing "Watchpoint <n> hit: old value: <x>,
new vlaue: <y>" for Read watchpoints. I could make an argument for
print "Watchpoint <n> hit: current value <x>" but the current output
doesn't make any sense, and the user can print the value if they
are particularly interested. Read watchpoints are used primarily
to understand what code is reading a variable.
This patch adds more fallbacks for how to print the objects being
watched if we have types, instead of assuming they are all integral
values, so a struct will print its elements. As large watchpoints
are added, we'll be doing a lot more of those.
To track the WatchpointSP in the WatchpointResources, I changed
the internal API which took a WatchpointSP and devolved it to a
Watchpoint*, which meant touching several different Process files.
I removed the watchpoint code in ProcessKDP which only reported
that watchpoints aren't supported, the base class does that already.
I haven't yet changed how we receive a watchpoint to identify
the WatchpointResource responsible for the trigger, and identify
all Watchpoints that are using this Resource to evaluate their
conditions etc. This is the same work that a BreakpointSite needs
to do when it has been tiggered, where multiple Breakpoints may be
at the same address.
There is not yet any printing of the Resources that a Watchpoint
is implemented in terms of ("watchpoint list", or
SBWatchpoint::GetDescription).
"watchpoint set var" and "watchpoint set expression" take a size
argument which was previously 1, 2, 4, or 8 (an enum). I've
changed this to an unsigned int. Most hardware implementations
can only watch 1, 2, 4, 8 byte ranges, but with Resources we'll
allow a user to ask for different sized watchpoints and set
them in hardware-expressble terms soon.
I've annotated areas where I know there is work still needed with
LWP_TODO that I'll be working on once this is landed.
I've tested this on aarch64 macOS, aarch64 Linux, and Intel macOS.
https://discourse.llvm.org/t/rfc-large-watchpoint-support-in-lldb/72116
Restructure the way I added WatchpointResources to be much more in line with how BreakpointLocations relate to BreakpointSites. Similar to BreakpointSites, when we have a watchpoint access, we will identify the WatchpointResource that tiggered it, and evaluate all Watchpoints who own that WatchpointResource. There's been a "hardware index" in watchpoints which we initialize if the remote gdb stub happens to give us the hardware index of the watchpoint register, but there way lldb receives this index depended on non-standard additions to the watchpoint packet. Instead, I have WatchpointResources contain an ID which is the hardware target register used for that watchpoint, which I initialize heuristically assuming that the stub will set new watchpoints in the first available slot starting at 0, and when a watchpoint is deleted/disabled, that spot will now be unused. It's only a heuristic, but it's no worse than our current sometimes-we-can-get-an-index system. With this update to the PR, this patch is still not breaking a user's watchpoint up into multiple WatchpointResources, it is not sharing a single WatchpointResource for multiple Watchpoints, and it is still not updating all Watchpoints for a Resource when an access is received. I am starting to be a little worried about the work needing to be done in the Process plugin to finish this feature - I will have no ability to build or test ProcessWindows or NativeProcessWindows. I will do the rest of the work trying to keep it in base classes to reduce the amount of change needed in those, but I doubt it will be enough that I can change those blindly.
Remove WatchpointCollection, this was little more than a std::vector, so now it's a typedef to that. In the Breakpoint case where a BreakpointSite has a BreakpointLocationCollection, the lldb container class was needed. Clean up WatchpointResource class API. A few smaller fixups like using early returns to avoid unnecessary indentation.
Suggestions from Alex.
Jonas' suggestion of trying to unify BreakpointSiteList/WatchpointResourceList is something I still need to look at.
Missed this review comment by Jonas.
My original patch had a BreakpointSiteList and a WatchpointResourceList which are pretty simple collections with some unique lookup methods for finding things by the watchpoint resource/breakpoint site IDs. Jonas asked that to be a template specialized for both classes; this patch adds a StopPointSiteList template class. The BreakpointSite / WatchpointResources have "owners". A BreakpointSite is owned by the BreakpointLocations that it is used for. A WatchpointResource is owned by the Watchpoints at that memory range. For some reason the owner name doesn't seem clear to me, and I thought of a politican who has constituents that they serve. I tried out a rename from a Site having "owners" to having "constituents". I don't know if it's really better in any way. And the "owners" terminology is used in other places that Jim wrote; BreakpointLocations have owners (Breakpoints). ThreadPlans have owners (Threads). I don't feel strongly about this and "constituent" is easy to global search and replace back if anyone thinks this isn't a good choice.
jasonmolenda
force-pushed
the
watchpointresources-initial-noop-implementation
branch
from
475c7ba
to
243af47
Compare
I have one method which is only defined for BreakpointSites which I specialize in the .cpp file still, everything else is in the .h header file. I still have one test failure in TestWatchpointCount.py that should be easy to fix, then I might be done with this PR.
I still had my original attempts to intuit the most likely hardware index number when adding a WatchpointResource to the process resource list. I've dropped that whole scheme in a separate commit; this commit correctly goes back to allocating the IDs sequentially for the Resources.
| bool StopPointSiteList<BreakpointSite>::StopPointSiteContainsBreakpoint( | ||
| typename BreakpointSite::SiteID site_id, lldb::break_id_t bp_id) { | ||
| std::lock_guard<std::recursive_mutex> guard(m_mutex); | ||
| typename collection::const_iterator pos = GetIDConstIterator(site_id); |
There was a problem hiding this comment.
This would be a good candidate for auto.
jasonmolenda
deleted the
watchpointresources-initial-noop-implementation
branch
|
Seems that it caused #73580 no? |
I believe so. @jasonmolenda I see you attempted to make a fix in a0a1ff3, however I still have two buildbots that are failing to build. Can you either fix it or revert if you need time to investigate? https://lab.llvm.org/buildbot/#/builders/217/builds/31915 |
…68845)" ...and follow ups. As it has caused test failures on Linux Arm and AArch64: https://lab.llvm.org/buildbot/#/builders/96/builds/49126 https://lab.llvm.org/buildbot/#/builders/17/builds/45824 ``` lldb-shell :: Subprocess/clone-follow-child-wp.test lldb-shell :: Subprocess/fork-follow-child-wp.test lldb-shell :: Subprocess/vfork-follow-child-wp.test ``` This reverts commit a6c62bf, commit a0a1ff3 and commit fc6b725.
|
I've reverted this due to failures on Linaro's Linux bots. |
This patch is rearranging code a bit to add WatchpointResources to
Process. A WatchpointResource is meant to represent a hardware
watchpoint register in the inferior process. It has an address, a size,
a type, and a list of Watchpoints that are using this
WatchpointResource.
This current patch doesn't add any of the features of
WatchpointResources that make them interesting -- a user asking to watch
a 24 byte object could watch this with three 8 byte WatchpointResources.
Or a Watchpoint on 1 byte at 0x1002 and a second watchpoint on 1 byte at
0x1003, these must both be served by a single WatchpointResource on that
doubleword at 0x1000 on a 64-bit target, if two hardware watchpoint
registers were used to track these separately, one of them may not be
hit. Or if you have one Watchpoint on a variable with a condition set,
and another Watchpoint on that same variable with a command defined or
different condition, or ignorecount, both of those Watchpoints need to
evaluate their criteria/commands when their WatchpointResource has been
hit.
There's a bit of code movement to rearrange things in the direction I'll
need for implementing this feature, so I want to start with reviewing &
landing this mostly NFC patch and we can focus on the algorithmic
choices about how WatchpointResources are shared and handled as they're
triggeed, separately.
This patch also stops printing "Watchpoint <n> hit: old value: <x>, new
vlaue: <y>" for Read watchpoints. I could make an argument for print
"Watchpoint <n> hit: current value <x>" but the current output doesn't
make any sense, and the user can print the value if they are
particularly interested. Read watchpoints are used primarily to
understand what code is reading a variable.
This patch adds more fallbacks for how to print the objects being
watched if we have types, instead of assuming they are all integral
values, so a struct will print its elements. As large watchpoints are
added, we'll be doing a lot more of those.
To track the WatchpointSP in the WatchpointResources, I changed the
internal API which took a WatchpointSP and devolved it to a Watchpoint*,
which meant touching several different Process files. I removed the
watchpoint code in ProcessKDP which only reported that watchpoints
aren't supported, the base class does that already.
I haven't yet changed how we receive a watchpoint to identify the
WatchpointResource responsible for the trigger, and identify all
Watchpoints that are using this Resource to evaluate their conditions
etc. This is the same work that a BreakpointSite needs to do when it has
been tiggered, where multiple Breakpoints may be at the same address.
There is not yet any printing of the Resources that a Watchpoint is
implemented in terms of ("watchpoint list", or
SBWatchpoint::GetDescription).
"watchpoint set var" and "watchpoint set expression" take a size
argument which was previously 1, 2, 4, or 8 (an enum). I've changed this
to an unsigned int. Most hardware implementations can only watch 1, 2,
4, 8 byte ranges, but with Resources we'll allow a user to ask for
different sized watchpoints and set them in hardware-expressble terms
soon.
I've annotated areas where I know there is work still needed with
LWP_TODO that I'll be working on once this is landed.
I've tested this on aarch64 macOS, aarch64 Linux, and Intel macOS.
https://discourse.llvm.org/t/rfc-large-watchpoint-support-in-lldb/72116
(cherry picked from commit fc6b725)
) This patch is rearranging code a bit to add WatchpointResources to Process. A WatchpointResource is meant to represent a hardware watchpoint register in the inferior process. It has an address, a size, a type, and a list of Watchpoints that are using this WatchpointResource. This current patch doesn't add any of the features of WatchpointResources that make them interesting -- a user asking to watch a 24 byte object could watch this with three 8 byte WatchpointResources. Or a Watchpoint on 1 byte at 0x1002 and a second watchpoint on 1 byte at 0x1003, these must both be served by a single WatchpointResource on that doubleword at 0x1000 on a 64-bit target, if two hardware watchpoint registers were used to track these separately, one of them may not be hit. Or if you have one Watchpoint on a variable with a condition set, and another Watchpoint on that same variable with a command defined or different condition, or ignorecount, both of those Watchpoints need to evaluate their criteria/commands when their WatchpointResource has been hit. There's a bit of code movement to rearrange things in the direction I'll need for implementing this feature, so I want to start with reviewing & landing this mostly NFC patch and we can focus on the algorithmic choices about how WatchpointResources are shared and handled as they're triggeed, separately. This patch also stops printing "Watchpoint <n> hit: old value: <x>, new vlaue: <y>" for Read watchpoints. I could make an argument for print "Watchpoint <n> hit: current value <x>" but the current output doesn't make any sense, and the user can print the value if they are particularly interested. Read watchpoints are used primarily to understand what code is reading a variable. This patch adds more fallbacks for how to print the objects being watched if we have types, instead of assuming they are all integral values, so a struct will print its elements. As large watchpoints are added, we'll be doing a lot more of those. To track the WatchpointSP in the WatchpointResources, I changed the internal API which took a WatchpointSP and devolved it to a Watchpoint*, which meant touching several different Process files. I removed the watchpoint code in ProcessKDP which only reported that watchpoints aren't supported, the base class does that already. I haven't yet changed how we receive a watchpoint to identify the WatchpointResource responsible for the trigger, and identify all Watchpoints that are using this Resource to evaluate their conditions etc. This is the same work that a BreakpointSite needs to do when it has been tiggered, where multiple Breakpoints may be at the same address. There is not yet any printing of the Resources that a Watchpoint is implemented in terms of ("watchpoint list", or SBWatchpoint::GetDescription). "watchpoint set var" and "watchpoint set expression" take a size argument which was previously 1, 2, 4, or 8 (an enum). I've changed this to an unsigned int. Most hardware implementations can only watch 1, 2, 4, 8 byte ranges, but with Resources we'll allow a user to ask for different sized watchpoints and set them in hardware-expressble terms soon. I've annotated areas where I know there is work still needed with LWP_TODO that I'll be working on once this is landed. I've tested this on aarch64 macOS, aarch64 Linux, and Intel macOS. https://discourse.llvm.org/t/rfc-large-watchpoint-support-in-lldb/72116 (cherry picked from commit fc6b725) (cherry picked from commit c73a3f1)
In `[lldb] [mostly NFC] Large WP foundation: WatchpointResources (#68845)` I added a new template StopPointSiteList to combine WatchpointResourceList and BreakpointSiteList. But I didn't remove the now-unused WatchpointResourceList class. This patch fixes that.
In `[lldb] [mostly NFC] Large WP foundation: WatchpointResources (llvm#68845)` I added a new template StopPointSiteList to combine WatchpointResourceList and BreakpointSiteList. But I didn't remove the now-unused WatchpointResourceList class. This patch fixes that. (cherry picked from commit bddeef5)
In `[lldb] [mostly NFC] Large WP foundation: WatchpointResources (llvm#68845)` I added a new template StopPointSiteList to combine WatchpointResourceList and BreakpointSiteList. But I didn't remove the now-unused WatchpointResourceList class. This patch fixes that. (cherry picked from commit bddeef5)
This patch is rearranging code a bit to add WatchpointResources to Process. A WatchpointResource is meant to represent a hardware watchpoint register in the inferior process. It has an address, a size, a type, and a list of Watchpoints that are using this WatchpointResource.
This current patch doesn't add any of the features of WatchpointResources that make them interesting -- a user asking to watch a 24 byte object could watch this with three 8 byte WatchpointResources. Or a Watchpoint on 1 byte at 0x1002 and a second watchpoint on 1 byte at 0x1003, these must both be served by a single WatchpointResource on that doubleword at 0x1000 on a 64-bit target, if two hardware watchpoint registers were used to track these separately, one of them may not be hit. Or if you have one Watchpoint on a variable with a condition set, and another Watchpoint on that same variable with a command defined or different condition, or ignorecount, both of those Watchpoints need to evaluate their criteria/commands when their WatchpointResource has been hit.
There's a bit of code movement to rearrange things in the direction I'll need for implementing this feature, so I want to start with reviewing & landing this mostly NFC patch and we can focus on the algorithmic choices about how WatchpointResources are shared and handled as they're triggeed, separately.
This patch also stops printing "Watchpoint hit: old value: , new vlaue: " for Read watchpoints. I could make an argument for print "Watchpoint hit: current value " but the current output doesn't make any sense, and the user can print the value if they are particularly interested. Read watchpoints are used primarily to understand what code is reading a variable.
This patch adds more fallbacks for how to print the objects being watched if we have types, instead of assuming they are all integral values, so a struct will print its elements. As large watchpoints are added, we'll be doing a lot more of those.
To track the WatchpointSP in the WatchpointResources, I changed the internal API which took a WatchpointSP and devolved it to a Watchpoint*, which meant touching several different Process files. I removed the watchpoint code in ProcessKDP which only reported that watchpoints aren't supported, the base class does that already.
I haven't yet changed how we receive a watchpoint to identify the WatchpointResource responsible for the trigger, and identify all Watchpoints that are using this Resource to evaluate their conditions etc. This is the same work that a BreakpointSite needs to do when it has been tiggered, where multiple Breakpoints may be at the same address.
There is not yet any printing of the Resources that a Watchpoint is implemented in terms of ("watchpoint list", or
SBWatchpoint::GetDescription).
"watchpoint set var" and "watchpoint set expression" take a size argument which was previously 1, 2, 4, or 8 (an enum). I've changed this to an unsigned int. Most hardware implementations can only watch 1, 2, 4, 8 byte ranges, but with Resources we'll allow a user to ask for different sized watchpoints and set them in hardware-expressble terms soon.
I've annotated areas where I know there is work still needed with LWP_TODO that I'll be working on once this is landed.
I've tested this on aarch64 macOS, aarch64 Linux, and Intel macOS.
https://discourse.llvm.org/t/rfc-large-watchpoint-support-in-lldb/72116