[lldb] refactor watchpoint functionality #159807
Conversation
|
@llvm/pr-subscribers-lldb Author: None (dlav-sc) ChangesThis patch is the first in a series of patches that add software watchpoint support to LLDB. The main purpose of this patch is to refactor watchpoint functionality in LLDB, making it easier to implement software watchpoint logic. Patch is 66.95 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/159807.diff 15 Files Affected:
diff --git a/lldb/include/lldb/Breakpoint/Watchpoint.h b/lldb/include/lldb/Breakpoint/Watchpoint.h
index ca48a0114888a..62435fe7dcce2 100644
--- a/lldb/include/lldb/Breakpoint/Watchpoint.h
+++ b/lldb/include/lldb/Breakpoint/Watchpoint.h
@@ -15,6 +15,7 @@
#include "lldb/Breakpoint/StoppointSite.h"
#include "lldb/Breakpoint/WatchpointOptions.h"
#include "lldb/Symbol/CompilerType.h"
+#include "lldb/Target/Process.h"
#include "lldb/Target/Target.h"
#include "lldb/Utility/UserID.h"
#include "lldb/lldb-private.h"
@@ -58,37 +59,92 @@ class Watchpoint : public std::enable_shared_from_this<Watchpoint>,
const WatchpointEventData &operator=(const WatchpointEventData &) = delete;
};
+ // Make sure watchpoint is properly disabled and subsequently enabled while
+ // performing watchpoint actions.
+ class WatchpointSentry {
+ public:
+ WatchpointSentry(lldb::ProcessSP p_sp, lldb::WatchpointSP w_sp)
+ : process_sp(p_sp), watchpoint_sp(w_sp) {
+ lldbassert(process_sp && watchpoint_sp && "Ill-formed WatchpointSentry!");
+
+ constexpr bool notify = false;
+ watchpoint_sp->TurnOnEphemeralMode();
+ process_sp->DisableWatchpoint(watchpoint_sp, notify);
+ process_sp->AddPreResumeAction(SentryPreResumeAction, this);
+ }
+
+ void DoReenable() {
+ bool was_disabled = watchpoint_sp->IsDisabledDuringEphemeralMode();
+ watchpoint_sp->TurnOffEphemeralMode();
+ constexpr bool notify = false;
+ if (was_disabled) {
+ process_sp->DisableWatchpoint(watchpoint_sp, notify);
+ } else {
+ process_sp->EnableWatchpoint(watchpoint_sp, notify);
+ }
+ }
+
+ ~WatchpointSentry() {
+ DoReenable();
+ process_sp->ClearPreResumeAction(SentryPreResumeAction, this);
+ }
+
+ static bool SentryPreResumeAction(void *sentry_void) {
+ WatchpointSentry *sentry = static_cast<WatchpointSentry *>(sentry_void);
+ sentry->DoReenable();
+ return true;
+ }
+
+ private:
+ lldb::ProcessSP process_sp;
+ lldb::WatchpointSP watchpoint_sp;
+ };
+
Watchpoint(Target &target, lldb::addr_t addr, uint32_t size,
- const CompilerType *type, bool hardware = true);
+ const CompilerType *type, lldb::WatchpointMode mode);
~Watchpoint() override;
bool IsEnabled() const;
- // This doesn't really enable/disable the watchpoint. It is currently just
- // for use in the Process plugin's {Enable,Disable}Watchpoint, which should
- // be used instead.
+ // This doesn't really enable/disable the watchpoint. It is currently
+ // just for use in the Process plugin's {Enable,Disable}Watchpoint, which
+ // should be used instead.
void SetEnabled(bool enabled, bool notify = true);
- bool IsHardware() const override;
+ bool IsHardware() const override {
+ return m_mode == lldb::eWatchpointModeHardware;
+ }
bool ShouldStop(StoppointCallbackContext *context) override;
- bool WatchpointRead() const;
- bool WatchpointWrite() const;
- bool WatchpointModify() const;
+ bool WatchpointRead() const { return m_watch_type & LLDB_WATCH_TYPE_READ; }
+ bool WatchpointWrite() const { return m_watch_type & LLDB_WATCH_TYPE_WRITE; }
+ bool WatchpointModify() const {
+ return m_watch_type & LLDB_WATCH_TYPE_MODIFY;
+ }
+
uint32_t GetIgnoreCount() const;
void SetIgnoreCount(uint32_t n);
void SetWatchpointType(uint32_t type, bool notify = true);
void SetDeclInfo(const std::string &str);
- std::string GetWatchSpec();
+ std::string GetWatchSpec() const;
void SetWatchSpec(const std::string &str);
+
+ // This function determines whether we should report a watchpoint value
+ // change. Specifically, it checks the watchpoint condition (if present),
+ // ignore count and so on.
+ //
+ // \param[in] exe_ctx This should represent the current execution context
+ // where execution stopped. It's used only for watchpoint condition
+ // evaluation.
+ //
+ // \return Returns true if we should report a watchpoint hit.
bool WatchedValueReportable(const ExecutionContext &exe_ctx);
// Snapshot management interface.
bool IsWatchVariable() const;
void SetWatchVariable(bool val);
- bool CaptureWatchedValue(const ExecutionContext &exe_ctx);
/// \struct WatchpointVariableContext
/// \brief Represents the context of a watchpoint variable.
@@ -124,7 +180,7 @@ class Watchpoint : public std::enable_shared_from_this<Watchpoint>,
void *baton, lldb_private::StoppointCallbackContext *context,
lldb::user_id_t break_id, lldb::user_id_t break_loc_id);
- void GetDescription(Stream *s, lldb::DescriptionLevel level);
+ void GetDescription(Stream *s, lldb::DescriptionLevel level) const;
void Dump(Stream *s) const override;
bool DumpSnapshots(Stream *s, const char *prefix = nullptr) const;
void DumpWithLevel(Stream *s, lldb::DescriptionLevel description_level) const;
@@ -186,26 +242,72 @@ class Watchpoint : public std::enable_shared_from_this<Watchpoint>,
bool IsDisabledDuringEphemeralMode();
- const CompilerType &GetCompilerType() { return m_type; }
+ CompilerType GetCompilerType() const;
private:
friend class Target;
friend class WatchpointList;
- friend class StopInfoWatchpoint; // This needs to call UndoHitCount()
+
+ lldb::ValueObjectSP CalculateWatchedValue() const;
+
+ void CaptureWatchedValue(lldb::ValueObjectSP new_value_sp) {
+ m_old_value_sp = m_new_value_sp;
+ m_new_value_sp = new_value_sp;
+ }
+
+ bool CheckWatchpointCondition(const ExecutionContext &exe_ctx) const;
+
+ // This class facilitates retrieving a watchpoint's watched value.
+ //
+ // It's used by both hardware and software watchpoints to access
+ // values stored in the process memory.
+ //
+ // To retrieve the value located in the memory, the value's memory address
+ // and its CompilerType are required. ExecutionContext in this case should
+ // contain information about current process, so CalculateWatchedValue
+ // function first of all create ExecutionContext from the process of m_target.
+ class AddressWatchpointCalculateStrategy {
+ public:
+ AddressWatchpointCalculateStrategy(Target &target, lldb::addr_t addr,
+ uint32_t size, const CompilerType *type)
+ : m_target{target}, m_addr{addr},
+ m_type{CreateCompilerType(target, size, type)} {}
+
+ lldb::ValueObjectSP CalculateWatchedValue() const;
+
+ CompilerType GetCompilerType() const { return m_type; };
+
+ private:
+ static CompilerType CreateCompilerType(Target &target, uint32_t size,
+ const CompilerType *type) {
+ if (type && type->IsValid())
+ return *type;
+ // If we don't have a known type, then we force it to unsigned int of the
+ // right size.
+ return DeriveCompilerType(target, size);
+ }
+
+ static CompilerType DeriveCompilerType(Target &target, uint32_t size);
+
+ Target &m_target;
+ lldb::addr_t m_addr;
+ CompilerType m_type;
+ };
+
+ using WatchpointCalculateStrategy =
+ std::variant<AddressWatchpointCalculateStrategy>;
void ResetHistoricValues() {
m_old_value_sp.reset();
m_new_value_sp.reset();
}
- void UndoHitCount() { m_hit_counter.Decrement(); }
-
Target &m_target;
- bool m_enabled; // Is this watchpoint enabled
- bool m_is_hardware; // Is this a hardware watchpoint
- bool m_is_watch_variable; // True if set via 'watchpoint set variable'.
- bool m_is_ephemeral; // True if the watchpoint is in the ephemeral mode,
- // meaning that it is
+ bool m_enabled; // Is this watchpoint enabled
+ lldb::WatchpointMode m_mode; // Is this hardware or software watchpoint
+ bool m_is_watch_variable; // True if set via 'watchpoint set variable'.
+ bool m_is_ephemeral; // True if the watchpoint is in the ephemeral mode,
+ // meaning that it is
// undergoing a pair of temporary disable/enable actions to avoid recursively
// triggering further watchpoint events.
uint32_t m_disabled_count; // Keep track of the count that the watchpoint is
@@ -213,15 +315,19 @@ class Watchpoint : public std::enable_shared_from_this<Watchpoint>,
// At the end of the ephemeral mode when the watchpoint is to be enabled
// again, we check the count, if it is more than 1, it means the user-
// supplied actions actually want the watchpoint to be disabled!
- uint32_t m_watch_read : 1, // 1 if we stop when the watched data is read from
- m_watch_write : 1, // 1 if we stop when the watched data is written to
- m_watch_modify : 1; // 1 if we stop when the watched data is changed
+ uint32_t m_watch_type;
uint32_t m_ignore_count; // Number of times to ignore this watchpoint
- std::string m_decl_str; // Declaration information, if any.
- std::string m_watch_spec_str; // Spec for the watchpoint.
+ std::string m_watch_spec_str; // Spec for the watchpoint. It is optional for a
+ // hardware watchpoint, in which it is used only
+ // for dumping, but required for a software
+ // watchpoint calculation
+ WatchpointCalculateStrategy m_calculate_strategy;
+ std::string m_decl_str; // Declaration information, if any.
lldb::ValueObjectSP m_old_value_sp;
lldb::ValueObjectSP m_new_value_sp;
- CompilerType m_type;
+ lldb::ValueObjectSP
+ m_previous_hit_value_sp; // Used in software watchpoints to ensure proper
+ // ignore count behavior
Status m_error; // An error object describing errors associated with this
// watchpoint.
WatchpointOptions m_options; // Settable watchpoint options, which is a
diff --git a/lldb/include/lldb/Interpreter/OptionGroupWatchpoint.h b/lldb/include/lldb/Interpreter/OptionGroupWatchpoint.h
index 527a2612b189b..67d5f5661d828 100644
--- a/lldb/include/lldb/Interpreter/OptionGroupWatchpoint.h
+++ b/lldb/include/lldb/Interpreter/OptionGroupWatchpoint.h
@@ -44,6 +44,7 @@ class OptionGroupWatchpoint : public OptionGroup {
WatchType watch_type;
OptionValueUInt64 watch_size;
bool watch_type_specified;
+ lldb::WatchpointMode watch_mode;
lldb::LanguageType language_type;
private:
diff --git a/lldb/include/lldb/Target/Target.h b/lldb/include/lldb/Target/Target.h
index 14a09f29094d5..bce2f178d4799 100644
--- a/lldb/include/lldb/Target/Target.h
+++ b/lldb/include/lldb/Target/Target.h
@@ -796,9 +796,11 @@ class Target : public std::enable_shared_from_this<Target>,
bool resolve_indirect_symbols);
// Use this to create a watchpoint:
- lldb::WatchpointSP CreateWatchpoint(lldb::addr_t addr, size_t size,
- const CompilerType *type, uint32_t kind,
- Status &error);
+ lldb::WatchpointSP CreateWatchpointByAddress(lldb::addr_t addr, size_t size,
+ const CompilerType *type,
+ uint32_t kind,
+ lldb::WatchpointMode mode,
+ Status &error);
lldb::WatchpointSP GetLastCreatedWatchpoint() {
return m_last_created_watchpoint;
diff --git a/lldb/include/lldb/lldb-enumerations.h b/lldb/include/lldb/lldb-enumerations.h
index fec9fdef44df9..f4604f9ac74b3 100644
--- a/lldb/include/lldb/lldb-enumerations.h
+++ b/lldb/include/lldb/lldb-enumerations.h
@@ -1081,6 +1081,8 @@ enum InstructionControlFlowKind {
FLAGS_ENUM(WatchpointKind){eWatchpointKindWrite = (1u << 0),
eWatchpointKindRead = (1u << 1)};
+enum WatchpointMode { eWatchpointModeHardware, eWatchpointModeSoftware };
+
enum GdbSignal {
eGdbSignalBadAccess = 0x91,
eGdbSignalBadInstruction = 0x92,
diff --git a/lldb/source/API/SBTarget.cpp b/lldb/source/API/SBTarget.cpp
index eb56337de3c44..4ea6f25906876 100644
--- a/lldb/source/API/SBTarget.cpp
+++ b/lldb/source/API/SBTarget.cpp
@@ -1349,8 +1349,8 @@ SBTarget::WatchpointCreateByAddress(lldb::addr_t addr, size_t size,
Status cw_error;
// This API doesn't take in a type, so we can't figure out what it is.
CompilerType *type = nullptr;
- watchpoint_sp =
- target_sp->CreateWatchpoint(addr, size, type, watch_type, cw_error);
+ watchpoint_sp = target_sp->CreateWatchpointByAddress(
+ addr, size, type, watch_type, lldb::eWatchpointModeHardware, cw_error);
error.SetError(std::move(cw_error));
sb_watchpoint.SetSP(watchpoint_sp);
}
diff --git a/lldb/source/API/SBValue.cpp b/lldb/source/API/SBValue.cpp
index e300ecee3f8ac..1670883062173 100644
--- a/lldb/source/API/SBValue.cpp
+++ b/lldb/source/API/SBValue.cpp
@@ -1491,8 +1491,8 @@ lldb::SBWatchpoint SBValue::Watch(bool resolve_location, bool read, bool write,
Status rc;
CompilerType type(value_sp->GetCompilerType());
- WatchpointSP watchpoint_sp =
- target_sp->CreateWatchpoint(addr, byte_size, &type, watch_type, rc);
+ WatchpointSP watchpoint_sp = target_sp->CreateWatchpointByAddress(
+ addr, byte_size, &type, watch_type, lldb::eWatchpointModeHardware, rc);
error.SetError(std::move(rc));
if (watchpoint_sp) {
diff --git a/lldb/source/Breakpoint/Watchpoint.cpp b/lldb/source/Breakpoint/Watchpoint.cpp
index f1366ca538075..dad3155ff1348 100644
--- a/lldb/source/Breakpoint/Watchpoint.cpp
+++ b/lldb/source/Breakpoint/Watchpoint.cpp
@@ -10,6 +10,7 @@
#include "lldb/Breakpoint/StoppointCallbackContext.h"
#include "lldb/Breakpoint/WatchpointResource.h"
+#include "lldb/Core/Debugger.h"
#include "lldb/Core/Value.h"
#include "lldb/DataFormatters/DumpValueObjectOptions.h"
#include "lldb/Expression/UserExpression.h"
@@ -26,45 +27,95 @@
using namespace lldb;
using namespace lldb_private;
-Watchpoint::Watchpoint(Target &target, lldb::addr_t addr, uint32_t size,
- const CompilerType *type, bool hardware)
- : StoppointSite(0, addr, size, hardware), m_target(target),
- m_enabled(false), m_is_hardware(hardware), m_is_watch_variable(false),
- m_is_ephemeral(false), m_disabled_count(0), m_watch_read(0),
- m_watch_write(0), m_watch_modify(0), m_ignore_count(0) {
-
- if (type && type->IsValid())
- m_type = *type;
- else {
- // If we don't have a known type, then we force it to unsigned int of the
- // right size.
- auto type_system_or_err =
- target.GetScratchTypeSystemForLanguage(eLanguageTypeC);
- if (auto err = type_system_or_err.takeError()) {
- LLDB_LOG_ERROR(GetLog(LLDBLog::Watchpoints), std::move(err),
- "Failed to set type: {0}");
- } else {
- if (auto ts = *type_system_or_err) {
- if (size <= target.GetArchitecture().GetAddressByteSize()) {
- m_type =
- ts->GetBuiltinTypeForEncodingAndBitSize(eEncodingUint, 8 * size);
- } else {
- CompilerType clang_uint8_type =
- ts->GetBuiltinTypeForEncodingAndBitSize(eEncodingUint, 8);
- m_type = clang_uint8_type.GetArrayType(size);
- }
- } else
- LLDB_LOG_ERROR(GetLog(LLDBLog::Watchpoints), std::move(err),
- "Failed to set type: Typesystem is no longer live: {0}");
- }
+static bool IsValueObjectsEqual(lldb::ValueObjectSP lhs,
+ lldb::ValueObjectSP rhs) {
+ lldbassert(lhs);
+ lldbassert(rhs);
+
+ Status error;
+
+ DataExtractor lhs_data;
+ lhs->GetData(lhs_data, error);
+ if (error.Fail())
+ return false;
+
+ DataExtractor rhs_data;
+ rhs->GetData(rhs_data, error);
+ if (error.Fail())
+ return false;
+
+ return llvm::equal(
+ llvm::iterator_range(lhs_data.GetDataStart(), lhs_data.GetDataEnd()),
+ llvm::iterator_range(rhs_data.GetDataStart(), rhs_data.GetDataEnd()));
+}
+
+CompilerType Watchpoint::AddressWatchpointCalculateStrategy::DeriveCompilerType(
+ Target &target, uint32_t size) {
+ auto type_system_or_err =
+ target.GetScratchTypeSystemForLanguage(eLanguageTypeC);
+
+ auto err = type_system_or_err.takeError();
+ if (err) {
+ LLDB_LOG_ERROR(GetLog(LLDBLog::Watchpoints), std::move(err),
+ "Failed to set type: {0}");
+ return CompilerType();
}
- // Set the initial value of the watched variable:
- if (m_target.GetProcessSP()) {
- ExecutionContext exe_ctx;
- m_target.GetProcessSP()->CalculateExecutionContext(exe_ctx);
- CaptureWatchedValue(exe_ctx);
+ auto ts = *type_system_or_err;
+ if (!ts) {
+ LLDB_LOG_ERROR(GetLog(LLDBLog::Watchpoints), std::move(err),
+ "Failed to set type: Typesystem is no longer live: {0}");
+ return CompilerType();
+ }
+
+ if (size <= target.GetArchitecture().GetAddressByteSize())
+ return ts->GetBuiltinTypeForEncodingAndBitSize(eEncodingUint, 8 * size);
+
+ CompilerType clang_uint8_type =
+ ts->GetBuiltinTypeForEncodingAndBitSize(eEncodingUint, 8);
+ return clang_uint8_type.GetArrayType(size);
+}
+
+lldb::ValueObjectSP
+Watchpoint::AddressWatchpointCalculateStrategy::CalculateWatchedValue() const {
+ if (!m_type.IsValid()) {
+ // Don't know how to report new & old values, since we couldn't make a
+ // scalar type for this watchpoint. This works around an assert in
+ // ValueObjectMemory::Create.
+ // FIXME: This should not happen, but if it does in some case we care about,
+ // we can go grab the value raw and print it as unsigned.
+ return nullptr;
}
+
+ // To obtain a value that represents memory at a given address, we only need
+ // an information about process.
+ // Create here an ExecutionContext from the current process.
+ ExecutionContext exe_ctx;
+ lldb::ProcessSP process_sp = m_target.GetProcessSP();
+ if (!process_sp)
+ return nullptr;
+ process_sp->CalculateExecutionContext(exe_ctx);
+
+ ConstString g_watch_name("$__lldb__watch_value");
+ Address watch_address(m_addr);
+ auto new_value_sp = ValueObjectMemory::Create(
+ exe_ctx.GetBestExecutionContextScope(), g_watch_name.GetStringRef(),
+ watch_address, m_type);
+ new_value_sp = new_value_sp->CreateConstantValue(g_watch_name);
+ return new_value_sp;
+}
+
+Watchpoint::Watchpoint(Target &target, lldb::addr_t addr, uint32_t size,
+ const CompilerType *type, lldb::WatchpointMode mode)
+ : StoppointSite(0, addr, size,
+ mode == lldb::eWatchpointModeHardware ? true : false),
+ m_target(target), m_enabled(false), m_mode(mode),
+ m_is_watch_variable(false), m_is_ephemeral(false), m_disabled_count(0),
+ m_ignore_count(0), m_watch_spec_str{},
+ m_calculate_strategy{
+ AddressWatchpointCalculateStrategy{target, addr, size, type}} {
+ // Set the initial value of the watched variable:
+ CaptureWatchedValue(CalculateWatchedValue());
}
Watchpoint::~Watchpoint() = default;
@@ -184,72 +235,121 @@ void Watchpoint::ClearCallback() {
void Watchpoint::SetDeclInfo(const std::string &str) { m_decl_str = str; }
-std::string Watchpoint::GetWatchSpec() { return m_watch_spec_str; }
+std::string Watchpoint::GetWatchSpec() const { return m_watch_spec_str; }
void Watchpoint::SetWatchSpec(const std::string &str) {
m_watch_spec_str = str;
}
-bool Watchpoint::IsHardware() const {
- lldbassert(m_is_hardware || !HardwareRequired());
- return m_is_hardware;
-}
-
bool Watchpoint::IsWatchVariable() const { return m_is_watch_variable; }
void Watchpoint::SetWatchVariable(bool val) { m_is_watch_variable = val; }
-bool Watchpoint::CaptureWatchedValue(const ExecutionContext &exe_ctx) {
- ConstString g_watch_name("$__lldb__watch_value");
- m_old_value_sp = m_new_value_sp;
- Address watch_address(GetLoadAddress());
- if (!m_type.IsValid()) {
- // Don't know how to report new & old values, since we couldn't make a
- // scalar type for this watchpoint. This works around an assert in
- // ValueObjectMemory::Create.
- // FIXME: This should not happen, but if it does in some case we care about,
- // we can go grab the value raw and print it as unsigned.
- return false;
- }
- m_new_value_sp = ValueObjectMemory::Create(
- exe_ctx.GetBestExecutionContextScope(), g_watch_name.GetStringRef(),
- watch_address, m_type);
- m_new_value_sp = m_new_value_sp->CreateCons...
[truncated]
|
|
✅ With the latest revision this PR passed the Python code formatter. |
|
Hi @DavidSpickett, @jasonmolenda, @jimingham, I've decided to divide the software watchpoint support patch (#151195) into several smaller parts. This is the first one, which consists almost entirely of NFC, refactoring of the existing watchpoint logic in LLDB. Could you please take a look when you have a moment? |
This patch is the first in a series of patches that add software watchpoint support to LLDB. The main purpose of this patch is to refactor watchpoint functionality in LLDB, making it easier to implement software watchpoint logic.
daniilavdeev
force-pushed
the
dlav-sc/lldb_refactor_software_watchpoint
branch
from
0851eb2 to
9092e3a
Compare
| if (error.AsCString(nullptr)) | ||
| result.AppendError(error.AsCString()); | ||
| WatchpointSP watch_sp; | ||
| watch_sp = |
There was a problem hiding this comment.
Could you explain this diff?
There was a problem hiding this comment.
If you are asking about WatchpointSP watch_sp;, then I myself don't know why this line is placed separately :) It seems I changed these lines several times.
Fixed it.
|
Could you explain in broad strokes the theme of the refactoring here? This is a big PR to go in without a summary of what to expect. |
| // shouldn't push a step over breakpoint plan or do that work. | ||
| if (GetCurrentPlan()->IsVirtualStep()) | ||
| return false; | ||
| if (GetResumeState() == eStateSuspended) |
There was a problem hiding this comment.
why is a patch about watchpoints changing this method?
There was a problem hiding this comment.
I refactored this part as well because I plan to implement the logic for adding a WatchpointThreadPlan to the thread plan stack within this function. As I mentioned above, the logic for software watchpoints is planned to be implemented using a special ThreadPlan.
Essentially, the entire refactoring of this function boils down to adding a couple of early return statements. Personally, I strongly dislike cascades of nested if/else blocks. I understand that several years ago it was apparently fashionable to write functions with a single return statement, so I'm lenient towards that practice. However, early returns are much easier to read, and since I'm about to add new logic to this function anyway, I figured why not tidy it up first.
Although, if you think it's better to implement this part together with the logic for adding the thread plan, I wouldn't be opposed. We can leave this for the next patch.
Could you elaborate on the "almost entirely"? If it is not, what's stopping it from being? I'm worried about a massive change that looks NFC but that might be hiding a change in behavior. |
| }; | ||
|
|
||
| // Make sure watchpoint is properly disabled and subsequently enabled while | ||
| // performing watchpoint actions. |
There was a problem hiding this comment.
These should be doxygen comments ///
|
Thank you for breaking this change up into smaller patches, that's always a good idea and will certainly help with reviewing. Given that this is a significant new feature, could you please start an RFC on Discourse? We had a good amount of discussion in the original PR that would be good to capture in the RFC. |
Alright, I can describe a few key points: I ==== StopInfo.cpp/Watchpoint.cpp ====================================== Prior to this patch, all logic for evaluating a watchpoint hit on the host side was implemented in the These checks are also necessary for software watchpoints. However, these checks are not performed within the The reason is the difference in how hardware and software watchpoints are handled. In the case of a hardware watchpoint, we enter In contrast, for a software watchpoint, all the logic is implemented on the client side; the Furthermore, there were software design issues. For example, the These reasons led me to the idea that the watchpoint evaluation checks on the client side should be implemented directly within the This allowed me to eliminate the need for Note: I simply moved the P.S. Furthermore, I believe that checking the condition of a hardware watchpoint and its ignore count within I don't remember all the intricacies anymore, but I encountered some very rare and exceptional bugs where a watchpoint with an ignore count would break the operation of |
|
II ==== Watchpoint.cpp ================================================ I conducted significant refactoring of the internal logic of the However, I later noticed that the main differences were due to how the watchpoints calculated their watched value. How a watchpoint calculates the value depends on what we are trying to track. If we are tracking a value located in memory (e.g., a local variable), then the watchpoint simply needs to read the memory contents at the address of the tracked value. If we are tracking a register value or a complex expression like Therefore, I decided to extract the logic for calculating the watched value into separate entities: Please note that this is generally unrelated to whether the watchpoint is software or hardware. For tracking a value in memory, we can use either a software watchpoint (which must read the memory value after each step) or a hardware watchpoint (which can simply set a trigger on that memory region). However, a watchpoint on an expression not residing in memory can only be software, as there is nowhere for hardware to set a trigger in this case. Additionally, I extracted some logic into separate functions to improve code readability (e.g., |
|
III ==== StopInfo.cpp ============================================= Besides moving the logic from IV ==== lldb-enumeration.h ======================================== I added a separate enum https://github.com/llvm/llvm-project/pull/159807/files#diff-62036fb08830b445f086a17fd4be2132b4dc1da85446791d8c101574edbf58f3R1084 to describe whether a watchpoint is software or hardware. Using an V ==== Target.cpp =============================================== I renamed the |
Yes, of course. Here I am referring to three specific points, two of which, incidentally, were reflected in the tests: Firstly, I slightly modified the message displayed for watchpoints, for example by the Notice that it is now explicitly stated that Secondly, I changed the logic for setting a watchpoint on the same address. Before this patch, LLDB could silently and opaquely remove an old watchpoint and replace it with a new one https://github.com/llvm/llvm-project/pull/159807/files#diff-dcc80a369493ccabaf85b5492444457f8ce3f44c8be983dc01be9330f424e35cL1018-L1019. The fact that LLDB did this implicitly seemed very questionable to me. Instead of implicitly removing and replacing the watchpoint, I added an explicit message stating that a user cannot have multiple watchpoints monitoring the exact same address. If a user tries to set a new watchpoint on an address that is already monitored, and if any parameter of the new watchpoint (size, type, or hardware/software mode) differs from the old one, I notify the user of the impossibility and suggest they explicitly delete the old watchpoint using the watchpoint delete command before setting a new one https://github.com/llvm/llvm-project/pull/159807/files#diff-dcc80a369493ccabaf85b5492444457f8ce3f44c8be983dc01be9330f424e35cR1007-R1031. The message looks like this: In cases where all parameters of the new watchpoint are identical to the old one, I display an explicit message stating that LLDB will not create a new watchpoint but will reuse the existing, identical one: Thirdly, I changed the logic of how the Before the patch, the behavior of the ignore count in LLDB was very strange. The user would set an ignore count, and if Perhaps this implementation of the This broke one test, which I also fixed ( |
Thanks for the suggestion. I will create a topic in a day or two and will do my best to write something sensible there :) |
|
The RFC topic is here: https://discourse.llvm.org/t/rfc-software-watchpoints-support/88391 |
5 participants
This patch is the first in a series of patches that add software watchpoint support to LLDB.
The main purpose of this patch is to refactor watchpoint functionality in LLDB, making it easier to implement software watchpoint logic.