lldb can know architecture-dependent watchpoint behaviors, instead of depending on the remote stub

[jasonmolenda]

Summary

Some CPU architectures will give lldb a watchpoint exception before the instruction has executed (it is rolled back when the address matches), or after the instruction has executed. Currently, lldb learns which type of CPU this is from the gdb remote serial protocol stub though a key in the lldb-extension qHostInfo packet. This is fixed for most architectures, and lldb needs to interop with non-debugserver/non-lldb-server stubs, so moving this into lldb simplifies a lot. This patch still allows the remote stub to override the behavior if it needs to.

There's another lldb extended gdb RSP packet to retrieve the number of watchpoints that this target supports. This needs to be calculated on the target system to be correct, so the stub is the right place to retrieve this knowledge. But lldb needs to interoperate with remote stubs that do not implement this extended packet; we can optionally print the number to the user, but we should not prevent watchpoints from attempting to be set based on this number.

This patch changes lldb to expect these pieces of information -- whether the exceptions are received before/after, and the number of watchpoints -- to not be available from the remote stub, and behave correctly.

The bug driving this was when connected to a gdb RSP stub that did not implement one of these extended packets, and debugging on a target where watchpoint exceptions are received before the instruction executes. lldb would try to fetch whether this machine is a before/after machine, and also fetch the number of watchpoints. The latter packet was not implemented, and Process::GetWatchpointSupportInfo would return error and not provide whether watchpoint exceptions are before/after.

[jasonmolenda]

Ah, I wasn't paying close enough attention and got the logic for MIPS/PPC64 watchpoint exceptions backwards in my update to GDBRemoteCommunicationClient::GetWatchpointReportedAfter, update patch for that.

[jasonmolenda]

tl;dr: watchpoints don't work on things like arm targets with a JTAG gdb stub that doesn't support qWatchpointSupportInfo: or qHostInfo with the watchpoint_exceptions_received key.

Nearly all of this patch is mechanical, and IMO the question is whether I should have stuck with Process:GetWatchpointSupportInfo but added some way to indicate that we cannot fetch the number of watchpoints, but we can report whether the exceptions come before/after.

Also this seems a little weak to have it encoded down in GDBRemoteCommunicationClient; it seems like something we could ask the ArchSpec about tbh. I didn't go that far, but maybe that would be better to do while I'm here.

[DavidSpickett]

I like removing a return by ref and being explicit about what info is wanted where.

So say the stub reports the "watchpoints received" but not the number of slots.

Does lldb just send the watchpoint set packets and rely on the stub to error if it can't handle it (presumably you get a less than helpful error)? As opposed to if you had a number of slots lldb could track how many it had used and give you a nice error instead.

lldb/include/lldb/Target/Process.h

1822–1825

This is a bit muddled.

I guess your point is that number of watchpoints the user can set may be less than the number that the hardware supports. And that this method returns the latter not the former. So perhaps:

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.

1830

I would wave my optional flag here but defining some error value is the way of the land so I will keep it under wraps :)

[jasonmolenda]

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.

This patch separates these into two different Process methods, and sets a default of "before" for arm targets if the qHostInfo packet didn't provide any hints (e.g. watchpoint_exceptions_received:after;), otherwise it returns nullopt.

I removed the before/after calculation from the ProcessWindows plugin because it always returned after; the concrete method in Process already behaves that way.

I need to review & test this patch - it's closer to a WIP at this instant, but I wanted to checkpoint the updates I made so far, and where I'm heading with it.

[jasonmolenda]

I phabracated wrong and the end of the last msg was truncated. I meant to end it with this:

If the number of watchpoints could not be fetched with an lldb-extension packet, Process:GetWatchpointSupportInfo would not report whether breakpoints are received before or after the instruction has executed. This latter is important to know to handle watchpoints; the former doesn't really matter. So when we connect to a non-lldb-server/debugserver stub, and we are targetting an arm cpu (where watchpoint exceptions are received before the instruction executes), lldb would not correctly disable the watchpoint/insn-step/re-enable the watchpoint (v. StopInfoWatchpoint::ShouldStopSynchronous in StopInfo.cpp), you'd hit the watchpoint exception over and over without advancing.

This patch separates these into two different Process methods, and sets a default of "before" for arm targets if the qHostInfo packet didn't provide any hints (e.g. watchpoint_exceptions_received:after;), otherwise it returns nullopt.

I removed the before/after calculation from the ProcessWindows plugin because it always returned after; the concrete method in Process already behaves that way for intel.

I need to review & test this patch - it's closer to a WIP at this instant, but I wanted to checkpoint the updates I made so far, and where I'm heading with it.

[jasonmolenda]

Went over the rewrite one more time, fixing up comments, fixing a couple of logic bugs I wrote, and having tested it with a debugserver I hacked to remove the watchpoint_exceptions_received:before; key in the qHostInfo reply packet, so lldb had to use its knowledge of the target system to determine this.

I have changed ProcessWindows and can't build that to ensure it's correct, but the changes are tiny and I'll keep an eye on the bots when I land it.

I think this is ready for review now.

[jasonmolenda]

If I was going to describe this patch briefly at this point, I would say: "Change lldb from depending on the remote gdb stub to tell it whether watchpoints are received before or after the instruction, to knowing the architectures where watchpoint exceptions are received before the instruction executes, and allow the remote gdb stub to override this behavior in its qHostInfo response". The rest is mechanical change.

[DavidSpickett]

Change lldb from depending on the remote gdb stub to tell it whether watchpoints are received before or after the instruction, to knowing the architectures where watchpoint exceptions are received before the instruction executes, and allow the remote gdb stub to override this behavior in its qHostInfo response.

Sounds good to me. Can you simplify the commit message some now that you've got the changes you want?

Seems like the main things are:

• Split up querying watchpoint reporting style, and number of watchpoint slots.
• Have process determine the reporting style by target, with the option for the remote to override. Meaning this query never fails.
• Only conclude that watchpoints are unsupported if the remote tells us that it has 0 slots.

(the last one I think we always did but good for context)

lldb/source/Target/Process.cpp

2360 ↗

(On Diff #495686)

Can do if (std::optional<bool> subclass....) {... here.

Also no {} for single line if.

2367 ↗

(On Diff #495686)

No {} needed here too.

2373 ↗

(On Diff #495686)

Would this be any clearer with multiple returns? Or one giant return, but the logic is a bit cryptic then.

const ArchSpec &arch = GetTarget().GetArchitecture();
if (!arch.IsValid())
  return true;

llvm::Triple triple = arch.GetTriple();
return !(triple.isMIPS() || triple.isPPC64() || triple.isAArch64() || triple.isArmMClass() || triple.isARM());

Better as:

const ArchSpec &arch = GetTarget().GetArchitecture();
if (!arch.IsValid())
  return false;

llvm::Triple triple = arch.GetTriple();
if (triple.isMIPS() || triple.isPPC64() || triple.isAArch64() || triple.isArmMClass() || triple.isARM())
  return false;

return true;

Also, do we know what RISC-V does?

lldb/source/Target/StopInfo.cpp

833

This is no longer needed because GetWatchpointReportedAfter will fall back to the process' defaults if the GDB layer does not/can not provide a value, correct?

lldb/source/Target/Target.cpp

804

This should just append "Target supports 0 hardware...".

In theory a compiler could figure that out but also it's a bit confusing because it makes the reader wonder what value other than 0 is supposed to end up here.

[DavidSpickett]

lldb/source/Target/Process.cpp

2373 ↗

(On Diff #495686)

@Emmmer any idea?

[Emmmer]

lldb/source/Target/Process.cpp

2373 ↗

(On Diff #495686)

It seems standard RISC-V uses the before mode, that is

The action for this trigger will be taken just before the instruction that triggered it is committed, but after all preceding instructions are committed.

If I understand this code correctly (correct me if I was wrong), we should return false for RISC-V.

But in the debug-spec [1], RISC-V seems to allow both before and after modes, which can be detected through CSRs. When debug-spec lands, we may change the code accordingly.

[1]: https://github.com/riscv/riscv-debug-spec/blob/6309972901417a0fa72d812d2ffe89e432f00dff/xml/hwbp_registers.xml#L365

[jasonmolenda]

lldb/source/Target/Process.cpp

2373 ↗

(On Diff #495686)

Thanks! I'll add isRISCV() to the architectures we default to "watchpoints received before instruction executes". This can be overridden by a gdb remote stub which adds the lldb-extension key to qHostInfo response. although it sounds like it could technically be per *process*??? in which case the key should really be sent in qProcessInfo. The remote stub should be able to send qHostInfo before it has attached to any specific process.

lldb/source/Target/StopInfo.cpp

833

Previously, this would fetch the number of watchpoints available (if the lldb-extended packet was provided) and it would fetch whether watchpoints are received before or after (if the qHostInfo includes the lldb-extended key), and you would get an error if the former was unavailable. (I think the latter would default to "watchpoints are after"). This m_should_stop = true is the behavior when watchpoints are received after the instruction has executed; this is the crux of the bug I was trying to fix, where lldb would not instruction step over the instruction and re-add it, when the packet declaring the number of watchpoints was unimplemented.

lldb/source/Target/Target.cpp

804

It is a bit confusing. The layers that return the optional<uint32_t> will return the actual number of hardware watchpoints available, including 0 if that is the correct value. Or a nullopt to indicate that it could not be retrieved. So in this method, we got back the number of watchpoints available, and it was said to be 0. lldb won't actually check against this number if you try to set one, so even if we got a bogus value here, you could try to set a watchpoint and see if it works. SBProcess::GetNumSupportedHardwareWatchpoints() takes an SBError out arg & returns a uint32_t. It can return 0 to mean "could not get number of watchpoints" or "there are zero watchpoints" but the SBError tells you whether it was the failure case or not.

[jasonmolenda]

Update to fix David's second round of comments, rewrite the Title/Summary to more accurately reflect what I'm doing in this patch. I think we're about done here, but open to any comments.

[DavidSpickett]

One minor comment otherwise LGTM.

[LLDB] tag in the commit title please :)

lldb/source/Target/Process.cpp

2371 ↗

(On Diff #495976)

Combine these into one and if you feel like it, return false instead.

lldb/include/lldb/Target/Process.h

   ///     A vector to contain MemoryRegionInfo objects for all mapped
   ///     ranges.
   ///
   /// \return
   ///     An error value.
   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.
+  ///
+  /// One user specified watchpoint may require multiple hardware
+  /// watchpoints, e.g. a larger object, or an unaligned object.
+  /// A target stub may not allow the user to set that many distinct
+  /// watchpoints if one of these is true.
+  ///
+  /// \return
+  ///     Returns the number of watchpoints, or LLDB_INVALID_WATCHPOINT_SLOTS
+  ///     if unknown.
+  virtual uint32_t GetWatchpointSlotCount() {
+    return LLDB_INVALID_WATCHPOINT_SLOTS;
   }
 
-  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.
+  ///
+  /// \return
+  ///     Returns true for targets where lldb is notified after
+  ///     the instruction has completed executing.
+  virtual bool GetWatchpointReportedAfter() { return true; }
 
   lldb::ModuleSP ReadModuleFromMemory(const FileSpec &file_spec,
                                       lldb::addr_t header_addr,
                                       size_t size_to_read = 512);
 
   /// Attempt to get the attributes for a region of memory in the process.
   ///
   /// It may be possible for the remote debug server to inspect attributes for

lldb/include/lldb/lldb-defines.h

 
 // Breakpoints
 #define LLDB_INVALID_BREAK_ID 0
 #define LLDB_DEFAULT_BREAK_SIZE 0
 #define LLDB_BREAK_ID_IS_VALID(bid) ((bid) != (LLDB_INVALID_BREAK_ID))
 #define LLDB_BREAK_ID_IS_INTERNAL(bid) ((bid) < 0)
 
 // Watchpoints
+#define LLDB_INVALID_WATCHPOINT_SLOTS UINT32_MAX
 #define LLDB_INVALID_WATCH_ID 0
 #define LLDB_WATCH_ID_IS_VALID(uid) ((uid) != (LLDB_INVALID_WATCH_ID))
 #define LLDB_WATCH_TYPE_READ (1u << 0)
 #define LLDB_WATCH_TYPE_WRITE (1u << 1)
 #define LLDB_WATCH_TYPE_IS_VALID(type)                                         \
   ((type & LLDB_WATCH_TYPE_READ) || (type & LLDB_WATCH_TYPE_WRITE))
 
 // Generic Register Numbers

lldb/source/API/SBProcess.cpp

 SBProcess::GetNumSupportedHardwareWatchpoints(lldb::SBError &sb_error) const {
   LLDB_INSTRUMENT_VA(this, sb_error);
 
   uint32_t num = 0;
   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 == LLDB_INVALID_WATCHPOINT_SLOTS) {
+      sb_error.SetErrorString("Unable to determine number of watchpoints");
+      num = 0;
+    }
   } else {
     sb_error.SetErrorString("SBProcess is invalid");
   }
   return num;
 }
 
 uint32_t SBProcess::LoadImage(lldb::SBFileSpec &sb_remote_image_spec,
                               lldb::SBError &sb_error) {

lldb/source/Commands/CommandObjectWatchpoint.cpp

public:
     lldb::DescriptionLevel m_level = lldb::eDescriptionLevelBrief;
   };
 
 protected:
   bool DoExecute(Args &command, CommandReturnObject &result) override {
     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())
+      uint32_t num_supported_hardware_watchpoints =
+          target->GetProcessSP()->GetWatchpointSlotCount();
+
+      if (num_supported_hardware_watchpoints != LLDB_INVALID_WATCHPOINT_SLOTS)
         result.AppendMessageWithFormat(
             "Number of supported hardware watchpoints: %u\n",
             num_supported_hardware_watchpoints);
     }
 
     const WatchpointList &watchpoints = target->GetWatchpointList();
 
     std::unique_lock<std::recursive_mutex> lock;

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

public:
   void OnCreateThread(const HostThread &thread) override;
   void OnExitThread(lldb::tid_t thread_id, uint32_t exit_code) override;
   void OnLoadDll(const ModuleSpec &module_spec,
                  lldb::addr_t module_addr) override;
   void OnUnloadDll(lldb::addr_t module_addr) override;
   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;
+  uint32_t GetWatchpointSlotCount() override;
+  bool GetWatchpointReportedAfter() override;
   Status EnableWatchpoint(Watchpoint *wp, bool notify = true) override;
   Status DisableWatchpoint(Watchpoint *wp, bool notify = true) override;
 
 protected:
   ProcessWindows(lldb::TargetSP target_sp, lldb::ListenerSP listener_sp);
 
   Status DoGetMemoryRegionInfo(lldb::addr_t vm_addr,
                                MemoryRegionInfo &info) override;

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

     LLDB_LOG(
         log,
         "Error {0} occurred launching the process before the initial stop. {1}",
         error.GetError(), error);
     return;
   }
 }
 
-Status ProcessWindows::GetWatchpointSupportInfo(uint32_t &num) {
-  num = RegisterContextWindows::GetNumHardwareBreakpointSlots();
-  return {};
+uint32_t ProcessWindows::GetWatchpointSlotCount() {
+  return RegisterContextWindows::GetNumHardwareBreakpointSlots();
 }
 
-Status ProcessWindows::GetWatchpointSupportInfo(uint32_t &num, bool &after) {
-  num = RegisterContextWindows::GetNumHardwareBreakpointSlots();
-  after = RegisterContextWindows::DoHardwareBreakpointsTriggerAfter();
-  return {};
+bool ProcessWindows::GetWatchpointReportedAfter() {
+  return RegisterContextWindows::DoHardwareBreakpointsTriggerAfter();
 }
 
 Status ProcessWindows::EnableWatchpoint(Watchpoint *wp, bool notify) {
   Status error;
 
   if (wp->IsEnabled()) {
     wp->SetEnabled(true, notify);
     return error;

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

public:
   lldb::addr_t AllocateMemory(size_t size, uint32_t permissions);
 
   bool DeallocateMemory(lldb::addr_t addr);
 
   Status Detach(bool keep_stopped, lldb::pid_t pid = LLDB_INVALID_PROCESS_ID);
 
   Status GetMemoryRegionInfo(lldb::addr_t addr, MemoryRegionInfo &range_info);
 
-  Status GetWatchpointSupportInfo(uint32_t &num);
+  uint32_t GetWatchpointSlotCount();
 
-  Status GetWatchpointSupportInfo(uint32_t &num, bool &after,
-                                  const ArchSpec &arch);
-
-  Status GetWatchpointsTriggerAfterInstruction(bool &after,
-                                               const ArchSpec &arch);
+  bool GetWatchpointReportedAfter(const ArchSpec &arch);
 
   const ArchSpec &GetHostArchitecture();
 
   std::chrono::seconds GetHostDefaultPacketTimeout();
 
   const ArchSpec &GetProcessArchitecture();
 
   bool GetProcessStandaloneBinary(UUID &uuid, lldb::addr_t &value,

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

     return true;
   });
 
   m_qXfer_memory_map_loaded = true;
 
   return error;
 }
 
-Status GDBRemoteCommunicationClient::GetWatchpointSupportInfo(uint32_t &num) {
-  Status error;
-
+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;
+  uint32_t num = LLDB_INVALID_WATCHPOINT_SLOTS;
   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) {
         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;
+  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;
+bool GDBRemoteCommunicationClient::GetWatchpointReportedAfter(
+    const ArchSpec &arch) {
   llvm::Triple triple = arch.GetTriple();
+  bool after = true;
+
+  if (triple.isMIPS() || triple.isPPC64())
+    after = true;
+  if (triple.isAArch64() || triple.isArmMClass() || triple.isARM())
+    after = false;
 
   // 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;
+  if (m_qHostInfo_is_valid == eLazyBoolYes)
+    if (m_watchpoints_trigger_after_instruction == eLazyBoolNo)
+      after = false;
 
-    after = (m_watchpoints_trigger_after_instruction != eLazyBoolNo);
-  }
-  return error;
+  return after;
 }
 
 int GDBRemoteCommunicationClient::SetSTDIN(const FileSpec &file_spec) {
   if (file_spec) {
     std::string path{file_spec.GetPath(false)};
     StreamString packet;
     packet.PutCString("QSetSTDIN:");
     packet.PutStringAsRawHex8(path);

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

public:
 
   Status DisableBreakpointSite(BreakpointSite *bp_site) override;
 
   // Process Watchpoints
   Status EnableWatchpoint(Watchpoint *wp, bool notify = true) override;
 
   Status DisableWatchpoint(Watchpoint *wp, bool notify = true) override;
 
-  Status GetWatchpointSupportInfo(uint32_t &num) override;
+  uint32_t GetWatchpointSlotCount() override;
 
   llvm::Expected<TraceSupportedResponse> TraceSupported() override;
 
   llvm::Error TraceStop(const TraceStopRequest &request) override;
 
   llvm::Error TraceStart(const llvm::json::Value &request) override;
 
   llvm::Expected<std::string> TraceGetState(llvm::StringRef type) override;
 
   llvm::Expected<std::vector<uint8_t>>
   TraceGetBinaryData(const TraceGetBinaryDataRequest &request) override;
 
-  Status GetWatchpointSupportInfo(uint32_t &num, bool &after) override;
+  bool GetWatchpointReportedAfter() override;
 
   bool StartNoticingNewThreads() override;
 
   bool StopNoticingNewThreads() override;
 
   GDBRemoteCommunicationClient &GetGDBRemote() { return m_gdb_comm; }
 
   Status SendEventData(const char *data) override;

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

 
 Status ProcessGDBRemote::DoGetMemoryRegionInfo(addr_t load_addr,
                                                MemoryRegionInfo &region_info) {
 
   Status error(m_gdb_comm.GetMemoryRegionInfo(load_addr, region_info));
   return error;
 }
 
-Status ProcessGDBRemote::GetWatchpointSupportInfo(uint32_t &num) {
+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;
+bool ProcessGDBRemote::GetWatchpointReportedAfter() {
+  return m_gdb_comm.GetWatchpointReportedAfter(GetTarget().GetArchitecture());
 }
 
 Status ProcessGDBRemote::DoDeallocateMemory(lldb::addr_t addr) {
   Status error;
   LazyBool supported = m_gdb_comm.SupportsAllocDeallocMemory();
 
   switch (supported) {
   case eLazyBoolCalculate:

lldb/source/Target/StopInfo.cpp

       return m_should_stop;
     }
     // If this is a system where we need to execute the watchpoint by hand
     // after the hit, queue a thread plan to do that, and then say not to stop.
     // Otherwise, let the async action figure out whether the watchpoint should
     // 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 
           = std::static_pointer_cast<StopInfoWatchpoint>(shared_from_this());
       ThreadPlanSP step_over_wp_sp(new ThreadPlanStepOverWatchpoint(
           *(thread_sp.get()), me_as_siwp_sp, wp_sp));
       Status error;
       error = thread_sp->QueueThreadPlan(step_over_wp_sp, false);

lldb/source/Target/Target.cpp

   llvm::sort(names);
 }
 
 bool Target::ProcessIsValid() {
   return (m_process_sp && m_process_sp->IsAlive());
 }
 
 static bool CheckIfWatchpointsSupported(Target *target, Status &error) {
-  uint32_t num_supported_hardware_watchpoints;
-  Status rc = target->GetProcessSP()->GetWatchpointSupportInfo(
-      num_supported_hardware_watchpoints);
+  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 == LLDB_INVALID_WATCHPOINT_SLOTS)
     return true;
 
   if (num_supported_hardware_watchpoints == 0) {
     error.SetErrorStringWithFormat(
         "Target supports (%u) hardware watchpoint slots.\n",
         num_supported_hardware_watchpoints);
     return false;
   }
   return true;
 }
 
 // See also Watchpoint::SetWatchpointType(uint32_t type) and the
 // OptionGroupWatchpoint::WatchType enum type.
 WatchpointSP Target::CreateWatchpoint(lldb::addr_t addr, size_t size,