[Lldb-commits] [lldb] d347c56 - Revert "[lldb] Add support for large watchpoints in lldb (#79962)"

Jason Molenda via lldb-commits lldb-commits at lists.llvm.org
Wed Jan 31 12:26:43 PST 2024


Author: Jason Molenda
Date: 2024-01-31T12:22:43-08:00
New Revision: d347c564299eeb8ad1fcb58c06914473d6a789d8

URL: https://github.com/llvm/llvm-project/commit/d347c564299eeb8ad1fcb58c06914473d6a789d8
DIFF: https://github.com/llvm/llvm-project/commit/d347c564299eeb8ad1fcb58c06914473d6a789d8.diff

LOG: Revert "[lldb] Add support for large watchpoints in lldb (#79962)"

This reverts commit 57c66b35a885b571f9897d75d18f1d974c29e533.

Added: 
    

Modified: 
    lldb/include/lldb/lldb-enumerations.h
    lldb/packages/Python/lldbsuite/test/concurrent_base.py
    lldb/source/Breakpoint/CMakeLists.txt
    lldb/source/Breakpoint/Watchpoint.cpp
    lldb/source/Breakpoint/WatchpointResource.cpp
    lldb/source/Commands/CommandObjectWatchpoint.cpp
    lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp
    lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
    lldb/source/Target/StopInfo.cpp
    lldb/test/API/functionalities/watchpoint/large-watchpoint/TestLargeWatchpoint.py
    lldb/unittests/Breakpoint/CMakeLists.txt

Removed: 
    lldb/include/lldb/Breakpoint/WatchpointAlgorithms.h
    lldb/source/Breakpoint/WatchpointAlgorithms.cpp
    lldb/test/API/functionalities/watchpoint/unaligned-large-watchpoint/Makefile
    lldb/test/API/functionalities/watchpoint/unaligned-large-watchpoint/TestUnalignedLargeWatchpoint.py
    lldb/test/API/functionalities/watchpoint/unaligned-large-watchpoint/main.c
    lldb/unittests/Breakpoint/WatchpointAlgorithmsTests.cpp


################################################################################
diff  --git a/lldb/include/lldb/Breakpoint/WatchpointAlgorithms.h b/lldb/include/lldb/Breakpoint/WatchpointAlgorithms.h
deleted file mode 100644
index 9b5bb4e10f8fa..0000000000000
--- a/lldb/include/lldb/Breakpoint/WatchpointAlgorithms.h
+++ /dev/null
@@ -1,109 +0,0 @@
-//===-- WatchpointAlgorithms.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_BREAKPOINT_WATCHPOINTALGORITHMS_H
-#define LLDB_BREAKPOINT_WATCHPOINTALGORITHMS_H
-
-#include "lldb/Breakpoint/WatchpointResource.h"
-#include "lldb/Utility/ArchSpec.h"
-#include "lldb/lldb-public.h"
-
-#include <vector>
-
-namespace lldb_private {
-
-class WatchpointAlgorithms {
-
-public:
-  /// Convert a user's watchpoint request into an array of memory
-  /// regions that can be watched by one hardware watchpoint register
-  /// on the current target.
-  ///
-  /// \param[in] addr
-  ///     The start address specified by the user.
-  ///
-  /// \param[in] size
-  ///     The number of bytes the user wants to watch.
-  ///
-  /// \param[in] read
-  ///     True if we are watching for read accesses.
-  ///
-  /// \param[in] write
-  ///     True if we are watching for write accesses.
-  ///     \a read and \a write may both be true.
-  ///     There is no "modify" style for WatchpointResources -
-  ///     WatchpointResources are akin to the hardware watchpoint
-  ///     registers which are either in terms of read or write.
-  ///     "modify" distinction is done at the Watchpoint layer, where
-  ///     we check the actual range of bytes the user requested.
-  ///
-  /// \param[in] supported_features
-  ///     The bit flags in this parameter are set depending on which
-  ///     WatchpointHardwareFeature enum values the current target supports.
-  ///     The eWatchpointHardwareFeatureUnknown bit may be set if we
-  ///     don't have specific information about what the remote stub
-  ///     can support, and a reasonablec default will be used.
-  ///
-  /// \param[in] arch
-  ///     The ArchSpec of the current Target.
-  ///
-  /// \return
-  ///     A vector of WatchpointResourceSP's, one per hardware watchpoint
-  ///     register needed.  We may return more WatchpointResources than the
-  ///     target can watch at once; if all resources cannot be set, the
-  ///     watchpoint cannot be set.
-  static std::vector<lldb::WatchpointResourceSP> AtomizeWatchpointRequest(
-      lldb::addr_t addr, size_t size, bool read, bool write,
-      lldb::WatchpointHardwareFeature supported_features, ArchSpec &arch);
-
-  struct Region {
-    lldb::addr_t addr;
-    size_t size;
-  };
-
-protected:
-  /// Convert a user's watchpoint request into an array of addr+size that
-  /// can be watched with power-of-2 style hardware watchpoints.
-  ///
-  /// This is the default algorithm if we have no further information;
-  /// most watchpoint implementations can be assumed to be able to watch up
-  /// to pointer-size regions of memory in power-of-2 sizes and alingments.
-  ///
-  /// \param[in] user_addr
-  ///     The user's start address.
-  ///
-  /// \param[in] user_size
-  ///     The user's specified byte length.
-  ///
-  /// \param[in] min_byte_size
-  ///     The minimum byte size supported on this target.
-  ///     In most cases, this will be 1.  AArch64 MASK watchpoints can
-  ///     watch a minimum of 8 bytes (although Byte Address Select watchpoints
-  ///     can watch 1 to pointer-size bytes in a pointer-size aligned granule).
-  ///
-  /// \param[in] max_byte_size
-  ///     The maximum byte size supported for one watchpoint on this target.
-  ///
-  /// \param[in] address_byte_size
-  ///     The address byte size on this target.
-  static std::vector<Region> PowerOf2Watchpoints(lldb::addr_t user_addr,
-                                                 size_t user_size,
-                                                 size_t min_byte_size,
-                                                 size_t max_byte_size,
-                                                 uint32_t address_byte_size);
-};
-
-// For the unittests to have access to the individual algorithms
-class WatchpointAlgorithmsTest : public WatchpointAlgorithms {
-public:
-  using WatchpointAlgorithms::PowerOf2Watchpoints;
-};
-
-} // namespace lldb_private
-
-#endif // LLDB_BREAKPOINT_WATCHPOINTALGORITHMS_H

diff  --git a/lldb/include/lldb/lldb-enumerations.h b/lldb/include/lldb/lldb-enumerations.h
index 50cbccba4d7c5..392d333c23a44 100644
--- a/lldb/include/lldb/lldb-enumerations.h
+++ b/lldb/include/lldb/lldb-enumerations.h
@@ -448,32 +448,6 @@ enum WatchpointWriteType {
   eWatchpointWriteTypeOnModify
 };
 
-/// The hardware and native stub capabilities for a given target,
-/// for translating a user's watchpoint request into hardware
-/// capable watchpoint resources.
-FLAGS_ENUM(WatchpointHardwareFeature){
-    /// lldb will fall back to a default that assumes the target
-    /// can watch up to pointer-size power-of-2 regions, aligned to
-    /// power-of-2.
-    eWatchpointHardwareFeatureUnknown = (1u << 0),
-
-    /// Intel systems can watch 1, 2, 4, or 8 bytes (in 64-bit targets),
-    /// aligned naturally.
-    eWatchpointHardwareX86 = (1u << 1),
-
-    /// ARM systems with Byte Address Select watchpoints
-    /// can watch any consecutive series of bytes up to the
-    /// size of a pointer (4 or 8 bytes), at a pointer-size
-    /// alignment.
-    eWatchpointHardwareArmBAS = (1u << 2),
-
-    /// ARM systems with MASK watchpoints can watch any power-of-2
-    /// sized region from 8 bytes to 2 gigabytes, aligned to that
-    /// same power-of-2 alignment.
-    eWatchpointHardwareArmMASK = (1u << 3),
-};
-LLDB_MARK_AS_BITMASK_ENUM(WatchpointHardwareFeature)
-
 /// Programming language type.
 ///
 /// These enumerations use the same language enumerations as the DWARF

diff  --git a/lldb/packages/Python/lldbsuite/test/concurrent_base.py b/lldb/packages/Python/lldbsuite/test/concurrent_base.py
index 39eb27fd99747..72e04bbb20a04 100644
--- a/lldb/packages/Python/lldbsuite/test/concurrent_base.py
+++ b/lldb/packages/Python/lldbsuite/test/concurrent_base.py
@@ -166,12 +166,7 @@ def do_thread_actions(
 
         # Initialize the (single) watchpoint on the global variable (g_watchme)
         if num_watchpoint_threads + num_delay_watchpoint_threads > 0:
-            # The concurrent tests have multiple threads modifying a variable
-            # with the same value.  The default "modify" style watchpoint will
-            # only report this as 1 hit for all threads, because they all wrote
-            # the same value.  The testsuite needs "write" style watchpoints to
-            # get the correct number of hits reported.
-            self.runCmd("watchpoint set variable -w write g_watchme")
+            self.runCmd("watchpoint set variable g_watchme")
             for w in self.inferior_target.watchpoint_iter():
                 self.thread_watchpoint = w
                 self.assertTrue(

diff  --git a/lldb/source/Breakpoint/CMakeLists.txt b/lldb/source/Breakpoint/CMakeLists.txt
index 2fa659f803c28..3b39189e52587 100644
--- a/lldb/source/Breakpoint/CMakeLists.txt
+++ b/lldb/source/Breakpoint/CMakeLists.txt
@@ -21,7 +21,6 @@ add_lldb_library(lldbBreakpoint NO_PLUGIN_DEPENDENCIES
   StoppointSite.cpp
   StopPointSiteList.cpp
   Watchpoint.cpp
-  WatchpointAlgorithms.cpp
   WatchpointList.cpp
   WatchpointOptions.cpp
   WatchpointResource.cpp

diff  --git a/lldb/source/Breakpoint/Watchpoint.cpp b/lldb/source/Breakpoint/Watchpoint.cpp
index 7728fd09a07ae..1a8ef87c1e67a 100644
--- a/lldb/source/Breakpoint/Watchpoint.cpp
+++ b/lldb/source/Breakpoint/Watchpoint.cpp
@@ -45,16 +45,10 @@ Watchpoint::Watchpoint(Target &target, lldb::addr_t addr, uint32_t size,
       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
+      if (auto ts = *type_system_or_err)
+        m_type =
+            ts->GetBuiltinTypeForEncodingAndBitSize(eEncodingUint, 8 * size);
+      else
         LLDB_LOG_ERROR(GetLog(LLDBLog::Watchpoints), std::move(err),
                        "Failed to set type: Typesystem is no longer live: {0}");
     }
@@ -358,20 +352,6 @@ void Watchpoint::DumpWithLevel(Stream *s,
       s->Printf("\n    declare @ '%s'", m_decl_str.c_str());
     if (!m_watch_spec_str.empty())
       s->Printf("\n    watchpoint spec = '%s'", m_watch_spec_str.c_str());
-    if (IsEnabled()) {
-      if (ProcessSP process_sp = m_target.GetProcessSP()) {
-        auto &resourcelist = process_sp->GetWatchpointResourceList();
-        size_t idx = 0;
-        s->Printf("\n    watchpoint resources:");
-        for (WatchpointResourceSP &wpres : resourcelist.Sites()) {
-          if (wpres->ConstituentsContains(this)) {
-            s->Printf("\n       #%zu: ", idx);
-            wpres->Dump(s);
-          }
-          idx++;
-        }
-      }
-    }
 
     // Dump the snapshots we have taken.
     DumpSnapshots(s, "    ");

diff  --git a/lldb/source/Breakpoint/WatchpointAlgorithms.cpp b/lldb/source/Breakpoint/WatchpointAlgorithms.cpp
deleted file mode 100644
index 95a978a17cbfd..0000000000000
--- a/lldb/source/Breakpoint/WatchpointAlgorithms.cpp
+++ /dev/null
@@ -1,142 +0,0 @@
-//===-- WatchpointAlgorithms.cpp ------------------------------------------===//
-//
-// 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
-//
-//===----------------------------------------------------------------------===//
-
-#include "lldb/Breakpoint/WatchpointAlgorithms.h"
-#include "lldb/Breakpoint/WatchpointResource.h"
-#include "lldb/Target/Process.h"
-#include "lldb/Utility/ArchSpec.h"
-
-#include <utility>
-#include <vector>
-
-using namespace lldb;
-using namespace lldb_private;
-
-std::vector<WatchpointResourceSP>
-WatchpointAlgorithms::AtomizeWatchpointRequest(
-    addr_t addr, size_t size, bool read, bool write,
-    WatchpointHardwareFeature supported_features, ArchSpec &arch) {
-
-  std::vector<Region> entries;
-
-  if (supported_features &
-      WatchpointHardwareFeature::eWatchpointHardwareArmMASK) {
-    entries =
-        PowerOf2Watchpoints(addr, size,
-                            /*min_byte_size*/ 1,
-                            /*max_byte_size*/ INT32_MAX,
-                            /*address_byte_size*/ arch.GetAddressByteSize());
-  } else {
-    // As a fallback, assume we can watch any power-of-2
-    // number of bytes up through the size of an address in the target.
-    entries =
-        PowerOf2Watchpoints(addr, size,
-                            /*min_byte_size*/ 1,
-                            /*max_byte_size*/ arch.GetAddressByteSize(),
-                            /*address_byte_size*/ arch.GetAddressByteSize());
-  }
-
-  std::vector<WatchpointResourceSP> resources;
-  for (Region &ent : entries) {
-    WatchpointResourceSP wp_res_sp =
-        std::make_shared<WatchpointResource>(ent.addr, ent.size, read, write);
-    resources.push_back(wp_res_sp);
-  }
-
-  return resources;
-}
-
-/// Convert a user's watchpoint request (\a user_addr and \a user_size)
-/// into hardware watchpoints, for a target that can watch a power-of-2
-/// region of memory (1, 2, 4, 8, etc), aligned to that same power-of-2
-/// memory address.
-///
-/// If a user asks to watch 4 bytes at address 0x1002 (0x1002-0x1005
-/// inclusive) we can implement this with two 2-byte watchpoints
-/// (0x1002 and 0x1004) or with an 8-byte watchpoint at 0x1000.
-/// A 4-byte watchpoint at 0x1002 would not be properly 4 byte aligned.
-///
-/// If a user asks to watch 16 bytes at 0x1000, and this target supports
-/// 8-byte watchpoints, we can implement this with two 8-byte watchpoints
-/// at 0x1000 and 0x1008.
-std::vector<WatchpointAlgorithms::Region>
-WatchpointAlgorithms::PowerOf2Watchpoints(addr_t user_addr, size_t user_size,
-                                          size_t min_byte_size,
-                                          size_t max_byte_size,
-                                          uint32_t address_byte_size) {
-
-  // Can't watch zero bytes.
-  if (user_size == 0)
-    return {};
-
-  // The aligned watch region will be less than/equal to the size of
-  // an address in this target.
-  const int address_bit_size = address_byte_size * 8;
-
-  size_t aligned_size = std::max(user_size, min_byte_size);
-  /// Round up \a user_size to the next power-of-2 size
-  /// user_size == 8   -> aligned_size == 8
-  /// user_size == 9   -> aligned_size == 16
-  /// user_size == 15  -> aligned_size == 16
-  /// user_size == 192 -> aligned_size == 256
-  /// Could be `std::bit_ceil(aligned_size)` when we build with C++20?
-
-  aligned_size = 1ULL << (address_bit_size - __builtin_clzll(aligned_size - 1));
-
-  addr_t aligned_start = user_addr & ~(aligned_size - 1);
-
-  // Does this power-of-2 memory range, aligned to power-of-2 that the
-  // hardware can watch, completely cover the requested region.
-  if (aligned_size <= max_byte_size &&
-      aligned_start + aligned_size >= user_addr + user_size)
-    return {{aligned_start, aligned_size}};
-
-  // If the maximum region we can watch is larger than the aligned
-  // size, try increasing the region size by one power of 2 and see
-  // if aligning to that amount can cover the requested region.
-  //
-  // Increasing the aligned_size repeatedly instead of splitting the
-  // watchpoint can result in us watching large regions of memory
-  // unintentionally when we could use small two watchpoints.  e.g.
-  //    user_addr 0x3ff8 user_size 32
-  // can be watched with four 8-byte watchpoints or if it's done with one
-  // MASK watchpoint, it would need to be a 32KB watchpoint (a 16KB
-  // watchpoint at 0x0 only covers 0x0000-0x4000).  A user request
-  // at the end of a power-of-2 region can lead to these undesirably
-  // large watchpoints and many false positive hits to ignore.
-  if (max_byte_size >= (aligned_size << 1)) {
-    aligned_size <<= 1;
-    aligned_start = user_addr & ~(aligned_size - 1);
-    if (aligned_size <= max_byte_size &&
-        aligned_start + aligned_size >= user_addr + user_size)
-      return {{aligned_start, aligned_size}};
-
-    // Go back to our original aligned size, to try the multiple
-    // watchpoint approach.
-    aligned_size >>= 1;
-  }
-
-  // We need to split the user's watchpoint into two or more watchpoints
-  // that can be monitored by hardware, because of alignment and/or size
-  // reasons.
-  aligned_size = std::min(aligned_size, max_byte_size);
-  aligned_start = user_addr & ~(aligned_size - 1);
-
-  std::vector<Region> result;
-  addr_t current_address = aligned_start;
-  const addr_t user_end_address = user_addr + user_size;
-  while (current_address + aligned_size < user_end_address) {
-    result.push_back({current_address, aligned_size});
-    current_address += aligned_size;
-  }
-
-  if (current_address < user_end_address)
-    result.push_back({current_address, aligned_size});
-
-  return result;
-}

diff  --git a/lldb/source/Breakpoint/WatchpointResource.cpp b/lldb/source/Breakpoint/WatchpointResource.cpp
index 8f15fc7c49583..c1eb50c0358b3 100644
--- a/lldb/source/Breakpoint/WatchpointResource.cpp
+++ b/lldb/source/Breakpoint/WatchpointResource.cpp
@@ -9,7 +9,6 @@
 #include <assert.h>
 
 #include "lldb/Breakpoint/WatchpointResource.h"
-#include "lldb/Utility/Stream.h"
 
 #include <algorithm>
 
@@ -114,8 +113,7 @@ bool WatchpointResource::ShouldStop(StoppointCallbackContext *context) {
 }
 
 void WatchpointResource::Dump(Stream *s) const {
-  s->Printf("addr = 0x%8.8" PRIx64 " size = %zu", m_addr, m_size);
-  return;
+  return; // LWP_TODO
 }
 
 wp_resource_id_t WatchpointResource::GetNextID() {

diff  --git a/lldb/source/Commands/CommandObjectWatchpoint.cpp b/lldb/source/Commands/CommandObjectWatchpoint.cpp
index 438a16c50bd67..c80868d33905e 100644
--- a/lldb/source/Commands/CommandObjectWatchpoint.cpp
+++ b/lldb/source/Commands/CommandObjectWatchpoint.cpp
@@ -1139,22 +1139,9 @@ class CommandObjectWatchpointSetExpression : public CommandObjectRaw {
 
     // Fetch the type from the value object, the type of the watched object is
     // the pointee type
-    /// of the expression, so convert to that if we found a valid type.
+    /// of the expression, so convert to that if we  found a valid type.
     CompilerType compiler_type(valobj_sp->GetCompilerType());
 
-    std::optional<uint64_t> valobj_size = valobj_sp->GetByteSize();
-    // Set the type as a uint8_t array if the size being watched is
-    // larger than the ValueObject's size (which is probably the size
-    // of a pointer).
-    if (valobj_size && size > *valobj_size) {
-      auto type_system = compiler_type.GetTypeSystem();
-      if (type_system) {
-        CompilerType clang_uint8_type =
-            type_system->GetBuiltinTypeForEncodingAndBitSize(eEncodingUint, 8);
-        compiler_type = clang_uint8_type.GetArrayType(size);
-      }
-    }
-
     Status error;
     WatchpointSP watch_sp =
         target->CreateWatchpoint(addr, size, &compiler_type, watch_type, error);

diff  --git a/lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp b/lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp
index d756354f9bd27..565941d3168f1 100644
--- a/lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp
+++ b/lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp
@@ -491,13 +491,14 @@ 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.
-    WatchpointResourceSP wp_rsrc_sp =
-        target->GetProcessSP()->GetWatchpointResourceList().FindByAddress(
-            (addr_t)exc_sub_code);
-    if (wp_rsrc_sp && wp_rsrc_sp->GetNumberOfConstituents() > 0) {
-      return StopInfo::CreateStopReasonWithWatchpointID(
-          thread, wp_rsrc_sp->GetConstituentAtIndex(0)->GetID());
+    lldb::WatchpointSP wp_sp =
+        target->GetWatchpointList().FindByAddress((lldb::addr_t)exc_sub_code);
+    if (wp_sp && wp_sp->IsEnabled()) {
+      return StopInfo::CreateStopReasonWithWatchpointID(thread, wp_sp->GetID());
     }
   }
 

diff  --git a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
index 4e3447e767c35..fd724350b1554 100644
--- a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
+++ b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
@@ -24,7 +24,6 @@
 #include <sys/types.h>
 
 #include "lldb/Breakpoint/Watchpoint.h"
-#include "lldb/Breakpoint/WatchpointAlgorithms.h"
 #include "lldb/Breakpoint/WatchpointResource.h"
 #include "lldb/Core/Debugger.h"
 #include "lldb/Core/Module.h"
@@ -3154,22 +3153,23 @@ Status ProcessGDBRemote::EnableWatchpoint(WatchpointSP wp_sp, bool notify) {
   bool write = wp_sp->WatchpointWrite() || wp_sp->WatchpointModify();
   size_t size = wp_sp->GetByteSize();
 
-  ArchSpec target_arch = GetTarget().GetArchitecture();
-  WatchpointHardwareFeature supported_features =
-      eWatchpointHardwareFeatureUnknown;
-
-  // LWP_TODO: enable MASK watchpoint for arm64 debugserver
-  // when it reports that it supports them.
-  if (target_arch.GetTriple().getOS() == llvm::Triple::MacOSX &&
-      target_arch.GetTriple().getArch() == llvm::Triple::aarch64) {
-#if 0
-       supported_features |= eWatchpointHardwareArmMASK;
-#endif
-  }
-
-  std::vector<WatchpointResourceSP> resources =
-      WatchpointAlgorithms::AtomizeWatchpointRequest(
-          addr, size, read, write, supported_features, target_arch);
+  // New WatchpointResources needed to implement this Watchpoint.
+  std::vector<WatchpointResourceSP> resources;
+
+  // LWP_TODO: Break up the user's request into pieces that can be watched
+  // given the capabilities of the target cpu / stub software.
+  // As a default, breaking the watched region up into target-pointer-sized,
+  // aligned, groups.
+  //
+  // Beyond the default, a stub can / should inform us of its capabilities,
+  // e.g. a stub that can do AArch64 power-of-2 MASK watchpoints.
+  //
+  // And the cpu may have unique capabilities. AArch64 BAS watchpoints
+  // can watch any sequential bytes in a doubleword, but Intel watchpoints
+  // can only watch 1, 2, 4, 8 bytes within a doubleword.
+  WatchpointResourceSP wp_res_sp =
+      std::make_shared<WatchpointResource>(addr, size, read, write);
+  resources.push_back(wp_res_sp);
 
   // LWP_TODO: Now that we know the WP Resources needed to implement this
   // Watchpoint, we need to look at currently allocated Resources in the

diff  --git a/lldb/source/Target/StopInfo.cpp b/lldb/source/Target/StopInfo.cpp
index 95f78056b1644..3b65d661c1abd 100644
--- a/lldb/source/Target/StopInfo.cpp
+++ b/lldb/source/Target/StopInfo.cpp
@@ -987,10 +987,8 @@ class StopInfoWatchpoint : public StopInfo {
 
         // Don't stop if the watched region value is unmodified, and
         // this is a Modify-type watchpoint.
-        if (m_should_stop && !wp_sp->WatchedValueReportable(exe_ctx)) {
-          wp_sp->UndoHitCount();
+        if (m_should_stop && !wp_sp->WatchedValueReportable(exe_ctx))
           m_should_stop = false;
-        }
 
         // Finally, if we are going to stop, print out the new & old values:
         if (m_should_stop) {

diff  --git a/lldb/test/API/functionalities/watchpoint/large-watchpoint/TestLargeWatchpoint.py b/lldb/test/API/functionalities/watchpoint/large-watchpoint/TestLargeWatchpoint.py
index f7ceb47c0b615..c5e161497e628 100644
--- a/lldb/test/API/functionalities/watchpoint/large-watchpoint/TestLargeWatchpoint.py
+++ b/lldb/test/API/functionalities/watchpoint/large-watchpoint/TestLargeWatchpoint.py
@@ -25,11 +25,6 @@ def continue_and_report_stop_reason(self, process, iter_str):
     @skipIf(archs=no_match(["arm64", "arm64e", "aarch64"]))
     @skipUnlessDarwin
 
-    # LWP_TODO: until debugserver advertises that it supports
-    # MASK watchpoints, this test can't be enabled, lldb won't
-    # try to send watchpoints larger than 8 bytes.
-    @skipIfDarwin
-
     # debugserver only gained the ability to watch larger regions
     # with this patch.
     @skipIfOutOfTreeDebugserver

diff  --git a/lldb/test/API/functionalities/watchpoint/unaligned-large-watchpoint/Makefile b/lldb/test/API/functionalities/watchpoint/unaligned-large-watchpoint/Makefile
deleted file mode 100644
index 10495940055b6..0000000000000
--- a/lldb/test/API/functionalities/watchpoint/unaligned-large-watchpoint/Makefile
+++ /dev/null
@@ -1,3 +0,0 @@
-C_SOURCES := main.c
-
-include Makefile.rules

diff  --git a/lldb/test/API/functionalities/watchpoint/unaligned-large-watchpoint/TestUnalignedLargeWatchpoint.py b/lldb/test/API/functionalities/watchpoint/unaligned-large-watchpoint/TestUnalignedLargeWatchpoint.py
deleted file mode 100644
index fea8f62b4be9c..0000000000000
--- a/lldb/test/API/functionalities/watchpoint/unaligned-large-watchpoint/TestUnalignedLargeWatchpoint.py
+++ /dev/null
@@ -1,101 +0,0 @@
-"""
-Watch a large unaligned memory region that
-lldb will need multiple hardware watchpoints
-to cover.
-"""
-
-
-import lldb
-from lldbsuite.test.decorators import *
-from lldbsuite.test.lldbtest import *
-from lldbsuite.test import lldbutil
-
-
-class UnalignedLargeWatchpointTestCase(TestBase):
-    def continue_and_report_stop_reason(self, process, iter_str):
-        if self.TraceOn():
-            self.runCmd("script print('continue')")
-        process.Continue()
-        self.assertIn(
-            process.GetState(), [lldb.eStateStopped, lldb.eStateExited], iter_str
-        )
-        thread = process.GetSelectedThread()
-        return thread.GetStopReason()
-
-    NO_DEBUG_INFO_TESTCASE = True
-
-    # Test on 64-bit targets where we probably have
-    # four watchpoint registers that can watch doublewords (8-byte).
-    @skipIf(archs=no_match(["arm64", "arm64e", "aarch64", "x86_64"]))
-    def test_unaligned_large_watchpoint(self):
-        """Test watching an unaligned region of memory that requires multiple watchpoints."""
-        self.build()
-        self.main_source_file = lldb.SBFileSpec("main.c")
-        (target, process, thread, bkpt) = lldbutil.run_to_source_breakpoint(
-            self, "break here", self.main_source_file
-        )
-        self.runCmd("break set -p done")
-        self.runCmd("break set -p exiting")
-
-        frame = thread.GetFrameAtIndex(0)
-
-        array_addr = frame.GetValueForVariablePath("array").GetValueAsUnsigned()
-
-        # Don't assume that the heap allocated array is aligned
-        # to a 1024 byte boundary to begin with, force alignment.
-        # wa_addr = (array_addr + 1024) & ~(1024 - 1)
-        wa_addr = array_addr
-
-        # Now make the start address unaligned.
-        wa_addr = wa_addr + 7
-
-        err = lldb.SBError()
-        wp_opts = lldb.SBWatchpointOptions()
-        wp_opts.SetWatchpointTypeWrite(lldb.eWatchpointWriteTypeOnModify)
-        wp = target.WatchpointCreateByAddress(wa_addr, 22, wp_opts, err)
-        self.assertTrue(wp.IsValid())
-        self.assertSuccess(err)
-        if self.TraceOn():
-            self.runCmd("watch list -v")
-
-        c_count = 0
-        reason = self.continue_and_report_stop_reason(process, "continue #%d" % c_count)
-        while reason == lldb.eStopReasonWatchpoint:
-            c_count = c_count + 1
-            reason = self.continue_and_report_stop_reason(
-                process, "continue #%d" % c_count
-            )
-            self.assertLessEqual(c_count, 22)
-
-        self.assertEqual(c_count, 22)
-        self.expect("watchpoint list -v", substrs=["hit_count = 22"])
-        self.assertEqual(wp.GetHitCount(), 22)
-
-        target.DeleteWatchpoint(wp.GetID())
-
-        # Now try watching a 16 byte variable
-        # (not unaligned, but a good check to do anyway)
-        #
-        frame = thread.GetFrameAtIndex(0)
-        err = lldb.SBError()
-        wp = frame.locals["variable"][0].Watch(True, False, True, err)
-        self.assertSuccess(err)
-        if self.TraceOn():
-            self.runCmd("frame select 0")
-            self.runCmd("watchpoint list")
-
-        c_count = 0
-        reason = self.continue_and_report_stop_reason(process, "continue #%d" % c_count)
-        while reason == lldb.eStopReasonWatchpoint:
-            c_count = c_count + 1
-            reason = self.continue_and_report_stop_reason(
-                process, "continue #%d" % c_count
-            )
-            self.assertLessEqual(c_count, 4)
-
-        if self.TraceOn():
-            self.runCmd("frame select 0")
-
-        self.assertEqual(c_count, 4)
-        self.expect("watchpoint list -v", substrs=["hit_count = 4"])
-        self.assertEqual(wp.GetHitCount(), 4)

diff  --git a/lldb/test/API/functionalities/watchpoint/unaligned-large-watchpoint/main.c b/lldb/test/API/functionalities/watchpoint/unaligned-large-watchpoint/main.c
deleted file mode 100644
index 4c8c66bfc6762..0000000000000
--- a/lldb/test/API/functionalities/watchpoint/unaligned-large-watchpoint/main.c
+++ /dev/null
@@ -1,35 +0,0 @@
-#include <stdint.h>
-#include <stdio.h>
-#include <stdlib.h>
-#include <string.h>
-
-struct obj {
-  uint32_t one;
-  uint32_t two;
-  uint32_t three;
-  uint32_t four;
-};
-
-int main() {
-  const int count = 16776960;
-  uint8_t *array = (uint8_t *)malloc(count);
-  memset(array, 0, count);
-  struct obj variable;
-  variable.one = variable.two = variable.three = variable.four = 0;
-
-  puts("break here");
-
-  for (int i = 0; i < count; i++)
-    array[i]++;
-
-  puts("done iterating");
-
-  variable.one = 1;
-  variable.two = 2;
-  variable.three = 3;
-  variable.four = 4;
-
-  printf("variable value is %d\n",
-         variable.one + variable.two + variable.three + variable.four);
-  puts("exiting.");
-}

diff  --git a/lldb/unittests/Breakpoint/CMakeLists.txt b/lldb/unittests/Breakpoint/CMakeLists.txt
index 757c2da1a4d9d..3164af237a772 100644
--- a/lldb/unittests/Breakpoint/CMakeLists.txt
+++ b/lldb/unittests/Breakpoint/CMakeLists.txt
@@ -1,6 +1,5 @@
 add_lldb_unittest(LLDBBreakpointTests
   BreakpointIDTest.cpp
-  WatchpointAlgorithmsTests.cpp
 
   LINK_LIBS
     lldbBreakpoint

diff  --git a/lldb/unittests/Breakpoint/WatchpointAlgorithmsTests.cpp b/lldb/unittests/Breakpoint/WatchpointAlgorithmsTests.cpp
deleted file mode 100644
index 6617d36f1d9bc..0000000000000
--- a/lldb/unittests/Breakpoint/WatchpointAlgorithmsTests.cpp
+++ /dev/null
@@ -1,156 +0,0 @@
-//===-- WatchpointAlgorithmsTests.cpp -------------------------------------===//
-//
-// 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
-//
-//===----------------------------------------------------------------------===//
-
-#include "gtest/gtest.h"
-
-#include "lldb/Breakpoint/WatchpointAlgorithms.h"
-
-#include <utility>
-#include <vector>
-
-using namespace lldb;
-using namespace lldb_private;
-
-struct testcase {
-  WatchpointAlgorithms::Region user; // What the user requested
-  std::vector<WatchpointAlgorithms::Region>
-      hw; // The hardware watchpoints we'll use
-};
-
-void check_testcase(testcase test,
-                    std::vector<WatchpointAlgorithms::Region> result,
-                    size_t min_byte_size, size_t max_byte_size,
-                    uint32_t address_byte_size) {
-
-  EXPECT_EQ(result.size(), test.hw.size());
-  for (size_t i = 0; i < result.size(); i++) {
-    EXPECT_EQ(result[i].addr, test.hw[i].addr);
-    EXPECT_EQ(result[i].size, test.hw[i].size);
-  }
-}
-
-TEST(WatchpointAlgorithmsTests, PowerOf2Watchpoints) {
-
-  // clang-format off
-  std::vector<testcase> doubleword_max = {
-    {
-      {0x1012, 8},
-      {{0x1010, 8}, {0x1018, 8}}
-    },
-    {
-      {0x1002, 4},
-      {{0x1000, 8}}
-    },
-    {
-      {0x1006, 4},
-      {{0x1004, 4}, {0x1008, 4}}
-    },
-    {
-      {0x1006, 8},
-      {{0x1000, 8}, {0x1008, 8}}
-    },
-    {
-      {0x1000, 24},
-      {{0x1000, 8}, {0x1008, 8}, {0x1010, 8}}
-    },
-    {
-      {0x1014, 26},
-      {{0x1010, 8}, {0x1018, 8}, {0x1020, 8}, {0x1028, 8}}
-    },
-  };
-  // clang-format on
-  for (testcase test : doubleword_max) {
-    addr_t user_addr = test.user.addr;
-    size_t user_size = test.user.size;
-    size_t min_byte_size = 1;
-    size_t max_byte_size = 8;
-    size_t address_byte_size = 8;
-    auto result = WatchpointAlgorithmsTest::PowerOf2Watchpoints(
-        user_addr, user_size, min_byte_size, max_byte_size, address_byte_size);
-
-    check_testcase(test, result, min_byte_size, max_byte_size,
-                   address_byte_size);
-  }
-
-  // clang-format off
-  std::vector<testcase> word_max = {
-    {
-      {0x1002, 4},
-      {{0x1000, 4}, {0x1004, 4}}
-    },
-  };
-  // clang-format on
-  for (testcase test : word_max) {
-    addr_t user_addr = test.user.addr;
-    size_t user_size = test.user.size;
-    size_t min_byte_size = 1;
-    size_t max_byte_size = 4;
-    size_t address_byte_size = 4;
-    auto result = WatchpointAlgorithmsTest::PowerOf2Watchpoints(
-        user_addr, user_size, min_byte_size, max_byte_size, address_byte_size);
-
-    check_testcase(test, result, min_byte_size, max_byte_size,
-                   address_byte_size);
-  }
-
-  // clang-format off
-  std::vector<testcase> twogig_max = {
-    {
-      {0x1010, 16},
-      {{0x1010, 16}}
-    },
-    {
-      {0x1010, 24},
-      {{0x1000, 64}}
-    },
-
-    // We increase 36 to the aligned 64 byte size, but
-    // 0x1000-0x1040 doesn't cover the requested region.  Then
-    // we expand to 128 bytes starting at 0x1000 that does
-    // cover it.  Is this a good tradeoff for a 36 byte region?
-    {
-      {0x1024, 36},
-      {{0x1000, 128}}
-    },
-    {
-      {0x1000, 192},
-      {{0x1000, 256}}
-    },
-    {
-      {0x1080, 192},
-      {{0x1000, 512}}
-    },
-
-    // In this case, our aligned size is 128, and increasing it to 256
-    // still can't watch the requested region.  The algorithm
-    // falls back to using two 128 byte watchpoints.  
-    // The alternative would be to use a 1024B watchpoint 
-    // starting at 0x1000, to watch this 120 byte user request.
-    //
-    // This still isn't ideal.  The user is asking to watch 0x12e0-1358
-    // and could be optimally handled by a 
-    // 16-byte watchpoint at 0x12e0 and a 128-byte watchpoint at 0x1300
-    {
-      {0x12e0, 120},
-      {{0x1280, 128}, {0x1300, 128}}
-    },
-  };
-  // clang-format on
-  for (testcase test : twogig_max) {
-    addr_t user_addr = test.user.addr;
-    size_t user_size = test.user.size;
-    size_t min_byte_size = 1;
-    size_t max_byte_size = INT32_MAX;
-    size_t address_byte_size = 8;
-    auto result = WatchpointAlgorithmsTest::PowerOf2Watchpoints(
-        user_addr, user_size, min_byte_size, max_byte_size, address_byte_size);
-
-    check_testcase(test, result, min_byte_size, max_byte_size,
-                   address_byte_size);
-  }
-}


        


More information about the lldb-commits mailing list