jasonmolenda updated this revision to Diff 495385.
jasonmolenda edited the summary of this revision.
jasonmolenda added a comment.
David's feedback was spot on, overhaul patch to address.
Change Process::GetWatchpointSlotCount to return an optional count, for when it
is available, instead of depending on a magic number.
I've been unhappy with how we calculate if a target reports watchpoint
exceptions before or after the instruction has executed. I believe it is
target arch specific, I don't think we have any processors that change behavior
at runtime. We originally homed this bit of information in debugserver and had
it report it in qHostInfo, but it was one of those early decisions that was not
correct IMO, and I'm not in favor of perpetuating it. This update to the patch
makes a concrete Process::GetWatchpointReportedAfter method that uses the
Target architecture to determine it. It also has a call to a
`DoGetWatchpointReportedAfter` method that subclasses can override if they are
an environment where the target arch determination is not sufficient - who
knows.
I changed the gdb-remote plugin to only return the override when the qHostInfo
key is
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D143215/new/
https://reviews.llvm.org/D143215
Files:
lldb/include/lldb/Target/Process.h
lldb/source/API/SBProcess.cpp
lldb/source/Commands/CommandObjectWatchpoint.cpp
lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp
lldb/source/Plugins/Process/Windows/Common/ProcessWindows.h
lldb/source/Plugins/Process/Windows/Common/RegisterContextWindows.h
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
lldb/source/Target/Process.cpp
lldb/source/Target/StopInfo.cpp
lldb/source/Target/Target.cpp
Index: lldb/source/Target/Target.cpp
===================================================================
--- lldb/source/Target/Target.cpp
+++ lldb/source/Target/Target.cpp
@@ -790,19 +790,18 @@
}
static bool CheckIfWatchpointsSupported(Target *target, Status &error) {
- uint32_t num_supported_hardware_watchpoints;
- Status rc = target->GetProcessSP()->GetWatchpointSupportInfo(
- num_supported_hardware_watchpoints);
+ std::optional<uint32_t> num_supported_hardware_watchpoints =
+ target->GetProcessSP()->GetWatchpointSlotCount();
// If unable to determine the # of watchpoints available,
// assume they are supported.
- if (rc.Fail())
+ if (!num_supported_hardware_watchpoints)
return true;
if (num_supported_hardware_watchpoints == 0) {
error.SetErrorStringWithFormat(
"Target supports (%u) hardware watchpoint slots.\n",
- num_supported_hardware_watchpoints);
+ *num_supported_hardware_watchpoints);
return false;
}
return true;
Index: lldb/source/Target/StopInfo.cpp
===================================================================
--- lldb/source/Target/StopInfo.cpp
+++ lldb/source/Target/StopInfo.cpp
@@ -823,16 +823,8 @@
// stop
ProcessSP process_sp = exe_ctx.GetProcessSP();
- uint32_t num;
- bool wp_triggers_after;
+ bool wp_triggers_after = process_sp->GetWatchpointReportedAfter();
- if (!process_sp->GetWatchpointSupportInfo(num, wp_triggers_after)
- .Success()) {
- m_should_stop_is_valid = true;
- m_should_stop = true;
- return m_should_stop;
- }
-
if (!wp_triggers_after) {
// We have to step over the watchpoint before we know what to do:
StopInfoWatchpointSP me_as_siwp_sp
Index: lldb/source/Target/Process.cpp
===================================================================
--- lldb/source/Target/Process.cpp
+++ lldb/source/Target/Process.cpp
@@ -2355,6 +2355,24 @@
return error;
}
+bool Process::GetWatchpointReportedAfter() {
+ std::optional<bool> subclass_override = DoGetWatchpointReportedAfter();
+ if (subclass_override) {
+ return *subclass_override;
+ }
+ bool reported_after = true;
+ const ArchSpec &arch = GetTarget().GetArchitecture();
+ if (!arch.IsValid()) {
+ return reported_after;
+ }
+ llvm::Triple triple = arch.GetTriple();
+ if (triple.isMIPS() || triple.isPPC64())
+ reported_after = false;
+ if (triple.isAArch64() || triple.isArmMClass() || triple.isARM())
+ reported_after = false;
+ return reported_after;
+}
+
ModuleSP Process::ReadModuleFromMemory(const FileSpec &file_spec,
lldb::addr_t header_addr,
size_t size_to_read) {
Index: lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
===================================================================
--- lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
+++ lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
@@ -159,7 +159,7 @@
Status DisableWatchpoint(Watchpoint *wp, bool notify = true) override;
- Status GetWatchpointSupportInfo(uint32_t &num) override;
+ std::optional<uint32_t> GetWatchpointSlotCount() override;
llvm::Expected<TraceSupportedResponse> TraceSupported() override;
@@ -172,7 +172,7 @@
llvm::Expected<std::vector<uint8_t>>
TraceGetBinaryData(const TraceGetBinaryDataRequest &request) override;
- Status GetWatchpointSupportInfo(uint32_t &num, bool &after) override;
+ std::optional<bool> DoGetWatchpointReportedAfter() override;
bool StartNoticingNewThreads() override;
Index: lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
===================================================================
--- lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
+++ lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
@@ -2818,16 +2818,13 @@
return error;
}
-Status ProcessGDBRemote::GetWatchpointSupportInfo(uint32_t &num) {
+std::optional<uint32_t> ProcessGDBRemote::GetWatchpointSlotCount() {
- Status error(m_gdb_comm.GetWatchpointSupportInfo(num));
- return error;
+ return m_gdb_comm.GetWatchpointSlotCount();
}
-Status ProcessGDBRemote::GetWatchpointSupportInfo(uint32_t &num, bool &after) {
- Status error(m_gdb_comm.GetWatchpointSupportInfo(
- num, after, GetTarget().GetArchitecture()));
- return error;
+std::optional<bool> ProcessGDBRemote::DoGetWatchpointReportedAfter() {
+ return m_gdb_comm.GetWatchpointReportedAfter(GetTarget().GetArchitecture());
}
Status ProcessGDBRemote::DoDeallocateMemory(lldb::addr_t addr) {
Index: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
===================================================================
--- lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
+++ lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
@@ -194,13 +194,9 @@
Status GetMemoryRegionInfo(lldb::addr_t addr, MemoryRegionInfo &range_info);
- Status GetWatchpointSupportInfo(uint32_t &num);
+ std::optional<uint32_t> GetWatchpointSlotCount();
- Status GetWatchpointSupportInfo(uint32_t &num, bool &after,
- const ArchSpec &arch);
-
- Status GetWatchpointsTriggerAfterInstruction(bool &after,
- const ArchSpec &arch);
+ std::optional<bool> GetWatchpointReportedAfter(const ArchSpec &arch);
const ArchSpec &GetHostArchitecture();
Index: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
===================================================================
--- lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
+++ lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
@@ -1780,16 +1780,12 @@
return error;
}
-Status GDBRemoteCommunicationClient::GetWatchpointSupportInfo(uint32_t &num) {
- Status error;
-
+std::optional<uint32_t> GDBRemoteCommunicationClient::GetWatchpointSlotCount() {
if (m_supports_watchpoint_support_info == eLazyBoolYes) {
- num = m_num_supported_hardware_watchpoints;
- return error;
+ return m_num_supported_hardware_watchpoints;
}
- // Set num to 0 first.
- num = 0;
+ std::optional<uint32_t> num;
if (m_supports_watchpoint_support_info != eLazyBoolNo) {
StringExtractorGDBRemote response;
if (SendPacketAndWaitForResponse("qWatchpointSupportInfo:", response) ==
@@ -1797,15 +1793,13 @@
m_supports_watchpoint_support_info = eLazyBoolYes;
llvm::StringRef name;
llvm::StringRef value;
- bool found_num_field = false;
while (response.GetNameColonValue(name, value)) {
if (name.equals("num")) {
value.getAsInteger(0, m_num_supported_hardware_watchpoints);
num = m_num_supported_hardware_watchpoints;
- found_num_field = true;
}
}
- if (!found_num_field) {
+ if (!num) {
m_supports_watchpoint_support_info = eLazyBoolNo;
}
} else {
@@ -1813,44 +1807,24 @@
}
}
- if (m_supports_watchpoint_support_info == eLazyBoolNo) {
- error.SetErrorString("qWatchpointSupportInfo is not supported");
- }
- return error;
+ return num;
}
-lldb_private::Status GDBRemoteCommunicationClient::GetWatchpointSupportInfo(
- uint32_t &num, bool &after, const ArchSpec &arch) {
- Status error(GetWatchpointSupportInfo(num));
- if (error.Success())
- error = GetWatchpointsTriggerAfterInstruction(after, arch);
- return error;
-}
-
-lldb_private::Status
-GDBRemoteCommunicationClient::GetWatchpointsTriggerAfterInstruction(
- bool &after, const ArchSpec &arch) {
- Status error;
+std::optional<bool>
+GDBRemoteCommunicationClient::GetWatchpointReportedAfter(const ArchSpec &arch) {
llvm::Triple triple = arch.GetTriple();
// we assume watchpoints will happen after running the relevant opcode and we
// only want to override this behavior if we have explicitly received a
// qHostInfo telling us otherwise
- if (m_qHostInfo_is_valid != eLazyBoolYes) {
- // On targets like MIPS and ppc64, watchpoint exceptions are always
- // generated before the instruction is executed. The connected target may
- // not support qHostInfo or qWatchpointSupportInfo packets.
- after = !(triple.isMIPS() || triple.isPPC64());
- } else {
- // For MIPS and ppc64, set m_watchpoints_trigger_after_instruction to
- // eLazyBoolNo if it is not calculated before.
- if (m_watchpoints_trigger_after_instruction == eLazyBoolCalculate &&
- (triple.isMIPS() || triple.isPPC64()))
- m_watchpoints_trigger_after_instruction = eLazyBoolNo;
-
- after = (m_watchpoints_trigger_after_instruction != eLazyBoolNo);
+ if (m_qHostInfo_is_valid == eLazyBoolYes) {
+ if (m_watchpoints_trigger_after_instruction == eLazyBoolNo)
+ return false;
+ else
+ return true;
}
- return error;
+
+ return std::nullopt;
}
int GDBRemoteCommunicationClient::SetSTDIN(const FileSpec &file_spec) {
Index: lldb/source/Plugins/Process/Windows/Common/RegisterContextWindows.h
===================================================================
--- lldb/source/Plugins/Process/Windows/Common/RegisterContextWindows.h
+++ lldb/source/Plugins/Process/Windows/Common/RegisterContextWindows.h
@@ -38,7 +38,6 @@
static constexpr uint32_t GetNumHardwareBreakpointSlots() {
return NUM_HARDWARE_BREAKPOINT_SLOTS;
}
- static constexpr bool DoHardwareBreakpointsTriggerAfter() { return true; }
bool AddHardwareBreakpoint(uint32_t slot, lldb::addr_t address, uint32_t size,
bool read, bool write);
Index: lldb/source/Plugins/Process/Windows/Common/ProcessWindows.h
===================================================================
--- lldb/source/Plugins/Process/Windows/Common/ProcessWindows.h
+++ lldb/source/Plugins/Process/Windows/Common/ProcessWindows.h
@@ -95,8 +95,7 @@
void OnDebugString(const std::string &string) override;
void OnDebuggerError(const Status &error, uint32_t type) override;
- Status GetWatchpointSupportInfo(uint32_t &num) override;
- Status GetWatchpointSupportInfo(uint32_t &num, bool &after) override;
+ std::optional<uint32_t> GetWatchpointSlotCount() override;
Status EnableWatchpoint(Watchpoint *wp, bool notify = true) override;
Status DisableWatchpoint(Watchpoint *wp, bool notify = true) override;
Index: lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp
===================================================================
--- lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp
+++ lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp
@@ -835,15 +835,8 @@
}
}
-Status ProcessWindows::GetWatchpointSupportInfo(uint32_t &num) {
- num = RegisterContextWindows::GetNumHardwareBreakpointSlots();
- return {};
-}
-
-Status ProcessWindows::GetWatchpointSupportInfo(uint32_t &num, bool &after) {
- num = RegisterContextWindows::GetNumHardwareBreakpointSlots();
- after = RegisterContextWindows::DoHardwareBreakpointsTriggerAfter();
- return {};
+std::optional<uint32_t> ProcessWindows::GetWatchpointSlotCount() {
+ return RegisterContextWindows::GetNumHardwareBreakpointSlots();
}
Status ProcessWindows::EnableWatchpoint(Watchpoint *wp, bool notify) {
Index: lldb/source/Commands/CommandObjectWatchpoint.cpp
===================================================================
--- lldb/source/Commands/CommandObjectWatchpoint.cpp
+++ lldb/source/Commands/CommandObjectWatchpoint.cpp
@@ -209,13 +209,13 @@
Target *target = &GetSelectedTarget();
if (target->GetProcessSP() && target->GetProcessSP()->IsAlive()) {
- uint32_t num_supported_hardware_watchpoints;
- Status error = target->GetProcessSP()->GetWatchpointSupportInfo(
- num_supported_hardware_watchpoints);
- if (error.Success())
+ std::optional<uint32_t> num_supported_hardware_watchpoints =
+ target->GetProcessSP()->GetWatchpointSlotCount();
+
+ if (num_supported_hardware_watchpoints)
result.AppendMessageWithFormat(
"Number of supported hardware watchpoints: %u\n",
- num_supported_hardware_watchpoints);
+ *num_supported_hardware_watchpoints);
}
const WatchpointList &watchpoints = target->GetWatchpointList();
Index: lldb/source/API/SBProcess.cpp
===================================================================
--- lldb/source/API/SBProcess.cpp
+++ lldb/source/API/SBProcess.cpp
@@ -962,16 +962,20 @@
SBProcess::GetNumSupportedHardwareWatchpoints(lldb::SBError &sb_error) const {
LLDB_INSTRUMENT_VA(this, sb_error);
- uint32_t num = 0;
+ std::optional<uint32_t> num;
ProcessSP process_sp(GetSP());
if (process_sp) {
std::lock_guard<std::recursive_mutex> guard(
process_sp->GetTarget().GetAPIMutex());
- sb_error.SetError(process_sp->GetWatchpointSupportInfo(num));
+ num = process_sp->GetWatchpointSlotCount();
+ if (!num) {
+ sb_error.SetErrorString("Unable to determine number of watchpoints");
+ num = 0;
+ }
} else {
sb_error.SetErrorString("SBProcess is invalid");
}
- return num;
+ return *num;
}
uint32_t SBProcess::LoadImage(lldb::SBFileSpec &sb_remote_image_spec,
Index: lldb/include/lldb/Target/Process.h
===================================================================
--- lldb/include/lldb/Target/Process.h
+++ lldb/include/lldb/Target/Process.h
@@ -17,6 +17,7 @@
#include <list>
#include <memory>
#include <mutex>
+#include <optional>
#include <string>
#include <unordered_set>
#include <vector>
@@ -1817,20 +1818,34 @@
virtual Status
GetMemoryRegions(lldb_private::MemoryRegionInfos ®ion_list);
- virtual Status GetWatchpointSupportInfo(uint32_t &num) {
- Status error;
- num = 0;
- error.SetErrorString("Process::GetWatchpointSupportInfo() not supported");
- return error;
+ /// Get the number of watchpoints supported by this target.
+ ///
+ /// We may be able to determine the number of watchpoints available
+ /// on this target; retrieve this value if possible.
+ ///
+ /// This number may be less than the number of watchpoints a user
+ /// can specify. This is because a single user watchpoint may require
+ /// multiple watchpoint slots to implement. Due to the size
+ /// and/or alignment of objects.
+ ///
+ /// \return
+ /// Returns the number of watchpoints, if available.
+ virtual std::optional<uint32_t> GetWatchpointSlotCount() {
+ return std::nullopt;
}
- virtual Status GetWatchpointSupportInfo(uint32_t &num, bool &after) {
- Status error;
- num = 0;
- after = true;
- error.SetErrorString("Process::GetWatchpointSupportInfo() not supported");
- return error;
- }
+ /// Whether lldb will be notified about watchpoints after
+ /// the instruction has completed executing, or if the
+ /// instruction is rolled back and it is notified before it
+ /// executes.
+ /// The default behavior is "exceptions received after instruction
+ /// has executed"; Process subclasses should override this to
+ /// detect if an ISA where this isn't true is in use.
+ ///
+ /// \return
+ /// Returns true for targets where lldb is notified after
+ /// the instruction has completed executing.
+ bool GetWatchpointReportedAfter();
lldb::ModuleSP ReadModuleFromMemory(const FileSpec &file_spec,
lldb::addr_t header_addr,
@@ -2663,6 +2678,26 @@
return Status("Process::DoGetMemoryRegionInfo() not supported");
}
+ /// Whether lldb will be notified about watchpoints after
+ /// the instruction has completed executing, or if the
+ /// instruction is rolled back and it is notified before it
+ /// executes.
+ ///
+ /// This is normally determined by architecture, but this
+ /// method allows Process subclasses to override the default
+ /// behavior as needed.
+ ///
+ /// \return
+ /// No boolean returned means there is no override of the
+ /// default architecture-based behavior.
+ /// true is returned for targets where watchpoints are reported
+ /// after the instruction has completed.
+ /// false is returned for targets where watchpoints are reported
+ /// before the instruction executes.
+ virtual std::optional<bool> DoGetWatchpointReportedAfter() {
+ return std::nullopt;
+ }
+
lldb::StateType GetPrivateState();
/// The "private" side of resuming a process. This doesn't alter the state
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
