[LLDB] add arch-specific watchpoint behavior defaults to lldb

lldb was originally designed to get the watchpoint exception behavior
from the gdb remote serial protocol stub -- exceptions are either
received before the instruction executes, or after the instruction
has executed.  This behavior was reported via two lldb extensions
to gdb RSP, so generic remote stubs like gdbserver or a JTAG stub,
would not tell lldb which behavior was correct, and it would default
to "exceptions are received after the instruction has executed".
Two architectures hard coded their correct "exceptions before
instruction" behavior, to work around this issue.

Most architectures have a fixed behavior of watchpoint exceptions,
and we can center that information in lldb.  We can allow a remote
stub to override the default behavior via our packet extensions
if it's needed on a specific target.

This patch also separates the fetching of the number of watchpoints
from whether exceptions are before/after the insn.  Currently if
lldb couldn't fetch the number of watchpoints (not really needed), it
also wouldn't get when exceptions are received, and watchpoint
handling would fail.  lldb doesn't actually use the number of
watchpoints for anything beyond printing it to the user.

Differential Revision: https://reviews.llvm.org/D143215
rdar://101426626

(cherry picked from commit eaeb8dd)

Author: jasonmolenda

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>
@@ -1834,20 +1835,35 @@ class Process : public std::enable_shared_from_this<Process>,
   virtual Status
   GetMemoryRegions(lldb_private::MemoryRegionInfos &region_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", except for certain CPU architectures.
+  /// Process subclasses may override this if they have additional
+  /// information.
+  ///
+  /// \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,
@@ -2698,6 +2714,24 @@ void PruneThreadPlans();
     return Status("Process::DoGetMemoryRegionInfo() not supported");
   }
 
+  /// Provide an override value in the subclass for lldb's
+  /// CPU-based logic for whether watchpoint exceptions are
+  /// received before or after an instruction executes.
+  ///
+  /// If a Process subclass needs to override this architecture-based
+  /// result, it may do so by overriding this method.
+  ///
+  /// \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/source/API/SBProcess.cpp

@@ -967,7 +967,12 @@ SBProcess::GetNumSupportedHardwareWatchpoints(lldb::SBError &sb_error) const {
   if (process_sp) {
     std::lock_guard<std::recursive_mutex> guard(
         process_sp->GetTarget().GetAPIMutex());
-    sb_error.SetError(process_sp->GetWatchpointSupportInfo(num));
+    std::optional<uint32_t> actual_num = process_sp->GetWatchpointSlotCount();
+    if (actual_num) {
+      num = *actual_num;
+    } else {
+      sb_error.SetErrorString("Unable to determine number of watchpoints");
+    }
   } else {
     sb_error.SetErrorString("SBProcess is invalid");
   }

lldb/source/Commands/CommandObjectWatchpoint.cpp

@@ -209,13 +209,13 @@ class CommandObjectWatchpointList : public CommandObjectParsed {
     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();

lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp

@@ -835,15 +835,8 @@ void ProcessWindows::OnDebuggerError(const Status &error, uint32_t type) {
   }
 }
 
-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) {

lldb/source/Plugins/Process/Windows/Common/ProcessWindows.h

@@ -95,8 +95,7 @@ class ProcessWindows : public Process, public ProcessDebugger {
   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;
 

lldb/source/Plugins/Process/Windows/Common/RegisterContextWindows.h

@@ -38,7 +38,6 @@ class RegisterContextWindows : public lldb_private::RegisterContext {
   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);

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp

@@ -1773,77 +1773,51 @@ Status GDBRemoteCommunicationClient::LoadQXferMemoryMap() {
   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) ==
         PacketResult::Success) {
       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 {
       m_supports_watchpoint_support_info = eLazyBoolNo;
     }
   }
 
-  if (m_supports_watchpoint_support_info == eLazyBoolNo) {
-    error.SetErrorString("qWatchpointSupportInfo is not supported");
-  }
-  return error;
-}
-
-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;
+  return num;
 }
 
-lldb_private::Status
-GDBRemoteCommunicationClient::GetWatchpointsTriggerAfterInstruction(
-    bool &after, const ArchSpec &arch) {
-  Status error;
-  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;
+std::optional<bool> GDBRemoteCommunicationClient::GetWatchpointReportedAfter() {
+  if (m_qHostInfo_is_valid == eLazyBoolCalculate)
+    GetHostInfo();
 
-    after = (m_watchpoints_trigger_after_instruction != eLazyBoolNo);
+  // Process determines this by target CPU, but allow for the
+  // remote stub to override it via the qHostInfo
+  // watchpoint_exceptions_received key, if it is present.
+  if (m_qHostInfo_is_valid == eLazyBoolYes) {
+    if (m_watchpoints_trigger_after_instruction == eLazyBoolNo)
+      return false;
+    if (m_watchpoints_trigger_after_instruction == eLazyBoolYes)
+      return true;
   }
-  return error;
+
+  return std::nullopt;
 }
 
 int GDBRemoteCommunicationClient::SetSTDIN(const FileSpec &file_spec) {

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h

@@ -194,13 +194,9 @@ class GDBRemoteCommunicationClient : public GDBRemoteClientBase {
 
   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 &GetHostArchitecture();
 

lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp

@@ -2830,16 +2830,12 @@ Status ProcessGDBRemote::DoGetMemoryRegionInfo(addr_t load_addr,
   return error;
 }
 
-Status ProcessGDBRemote::GetWatchpointSupportInfo(uint32_t &num) {
-
-  Status error(m_gdb_comm.GetWatchpointSupportInfo(num));
-  return error;
+std::optional<uint32_t> ProcessGDBRemote::GetWatchpointSlotCount() {
+  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();
 }
 
 Status ProcessGDBRemote::DoDeallocateMemory(lldb::addr_t addr) {

lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h

@@ -158,7 +158,7 @@ class ProcessGDBRemote : public Process,
 
   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;
 
@@ -171,7 +171,7 @@ class ProcessGDBRemote : public Process,
   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;