Reland "[lldb] Add 'modify' type watchpoints, make it default (llvm#66308)"

This reverts commit a7b78ca.

With updates to the tests.

TestWatchTaggedAddress.py: Updated the expected watchpoint types,
though I'm not sure there should be a differnt default for the two
ways of setting them, that needs to be confirmed.

TestStepOverWatchpoint.py: Skipped this everywhere because I think
what used to happen is you couldn't put 2 watchpoints on the same
address (after alignment). I guess that this is now allowed because
modify watchpoints aren't accounted for, but likely should be.
Needs investigating.

(cherry picked from commit 75e8620)

Author: DavidSpickett

lldb/bindings/headers.swig

@@ -77,4 +77,5 @@
 #include "lldb/API/SBValueList.h"
 #include "lldb/API/SBVariablesOptions.h"
 #include "lldb/API/SBWatchpoint.h"
+#include "lldb/API/SBWatchpointOptions.h"
 %}

lldb/bindings/interface/SBWatchpointOptionsDocstrings.i

@@ -0,0 +1,12 @@
+%feature("docstring",
+"A container for options to use when creating watchpoints."
+) lldb::SBWatchpointOptions;
+
+%feature("docstring", "Sets whether the watchpoint should stop on read accesses."
+) lldb::SBWatchpointOptions::SetWatchpointTypeRead;
+%feature("docstring", "Gets whether the watchpoint should stop on read accesses."
+) lldb::SBWatchpointOptions::GetWatchpointTypeRead;
+%feature("docstring", "Sets whether the watchpoint should stop on write accesses. eWatchpointWriteTypeOnModify is the most commonly useful mode, where lldb will stop when the watched value has changed. eWatchpointWriteTypeAlways will stop on any write to the watched region, even if it's the value is the same."
+) lldb::SBWatchpointOptions::SetWatchpointTypeWrite;
+%feature("docstring", "Gets whether the watchpoint should stop on write accesses, returning WatchpointWriteType to indicate the type of write watching that is enabled, or eWatchpointWriteTypeDisabled."
+) lldb::SBWatchpointOptions::GetWatchpointTypeWrite;

lldb/bindings/interfaces.swig

@@ -80,6 +80,7 @@
 %include "./interface/SBValueListDocstrings.i"
 %include "./interface/SBVariablesOptionsDocstrings.i"
 %include "./interface/SBWatchpointDocstrings.i"
+%include "./interface/SBWatchpointOptionsDocstrings.i"
 
 /* API headers */
 %include "lldb/API/SBAddress.h"
@@ -152,6 +153,7 @@
 %include "lldb/API/SBValueList.h"
 %include "lldb/API/SBVariablesOptions.h"
 %include "lldb/API/SBWatchpoint.h"
+%include "lldb/API/SBWatchpointOptions.h"
 
 /* Extensions for SB classes */
 %include "./interface/SBAddressExtensions.i"

lldb/include/lldb/API/SBTarget.h

@@ -21,6 +21,7 @@
 #include "lldb/API/SBType.h"
 #include "lldb/API/SBValue.h"
 #include "lldb/API/SBWatchpoint.h"
+#include "lldb/API/SBWatchpointOptions.h"
 
 namespace lldb_private {
 namespace python {
@@ -839,8 +840,13 @@ class LLDB_API SBTarget {
 
   lldb::SBWatchpoint FindWatchpointByID(lldb::watch_id_t watch_id);
 
+  LLDB_DEPRECATED("WatchAddress deprecated, use WatchpointCreateByAddress")
   lldb::SBWatchpoint WatchAddress(lldb::addr_t addr, size_t size, bool read,
-                                  bool write, SBError &error);
+                                  bool modify, SBError &error);
+
+  lldb::SBWatchpoint
+  WatchpointCreateByAddress(lldb::addr_t addr, size_t size,
+                            lldb::SBWatchpointOptions options, SBError &error);
 
   bool EnableAllWatchpoints();
 

lldb/include/lldb/API/SBWatchpointOptions.h

@@ -0,0 +1,43 @@
+//===-- SBWatchpointOptions.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_API_SBWATCHPOINTOPTIONS_H
+#define LLDB_API_SBWATCHPOINTOPTIONS_H
+
+#include "lldb/API/SBDefines.h"
+
+class WatchpointOptionsImpl;
+
+namespace lldb {
+
+class LLDB_API SBWatchpointOptions {
+public:
+  SBWatchpointOptions();
+
+  SBWatchpointOptions(const lldb::SBWatchpointOptions &rhs);
+
+  ~SBWatchpointOptions();
+
+  const SBWatchpointOptions &operator=(const lldb::SBWatchpointOptions &rhs);
+
+  /// Stop when the watched memory region is read.
+  void SetWatchpointTypeRead(bool read);
+  bool GetWatchpointTypeRead() const;
+
+  /// Stop when the watched memory region is written to/modified
+  void SetWatchpointTypeWrite(lldb::WatchpointWriteType write_type);
+  lldb::WatchpointWriteType GetWatchpointTypeWrite() const;
+
+private:
+  // This auto_pointer is made in the constructor and is always valid.
+  mutable std::unique_ptr<WatchpointOptionsImpl> m_opaque_up;
+};
+
+} // namespace lldb
+
+#endif // LLDB_API_SBWATCHPOINTOPTIONS_H

lldb/include/lldb/Breakpoint/Watchpoint.h

@@ -76,12 +76,14 @@ class Watchpoint : public std::enable_shared_from_this<Watchpoint>,
 
   bool WatchpointRead() const;
   bool WatchpointWrite() const;
+  bool WatchpointModify() const;
   uint32_t GetIgnoreCount() const;
   void SetIgnoreCount(uint32_t n);
   void SetWatchpointType(uint32_t type, bool notify = true);
   void SetDeclInfo(const std::string &str);
   std::string GetWatchSpec();
   void SetWatchSpec(const std::string &str);
+  bool WatchedValueReportable(const ExecutionContext &exe_ctx);
 
   // Snapshot management interface.
   bool IsWatchVariable() const;
@@ -212,7 +214,8 @@ class Watchpoint : public std::enable_shared_from_this<Watchpoint>,
   // again, we check the count, if it is more than 1, it means the user-
   // supplied actions actually want the watchpoint to be disabled!
   uint32_t m_watch_read : 1, // 1 if we stop when the watched data is read from
-      m_watch_write : 1;     // 1 if we stop when the watched data is written to
+      m_watch_write : 1,     // 1 if we stop when the watched data is written to
+      m_watch_modify : 1;    // 1 if we stop when the watched data is changed
   uint32_t m_ignore_count;      // Number of times to ignore this watchpoint
   std::string m_decl_str;       // Declaration information, if any.
   std::string m_watch_spec_str; // Spec for the watchpoint.

lldb/include/lldb/Interpreter/OptionGroupWatchpoint.h

@@ -30,13 +30,15 @@ class OptionGroupWatchpoint : public OptionGroup {
 
   void OptionParsingStarting(ExecutionContext *execution_context) override;
 
-  // Note:
-  // eWatchRead == LLDB_WATCH_TYPE_READ; and
-  // eWatchWrite == LLDB_WATCH_TYPE_WRITE
+  /// eWatchRead == LLDB_WATCH_TYPE_READ
+  /// eWatchWrite == LLDB_WATCH_TYPE_WRITE
+  /// eWatchModify == LLDB_WATCH_TYPE_MODIFY
+  /// eWatchReadWrite == LLDB_WATCH_TYPE_READ | LLDB_WATCH_TYPE_WRITE
   enum WatchType {
     eWatchInvalid = 0,
     eWatchRead,
     eWatchWrite,
+    eWatchModify,
     eWatchReadWrite
   };
 

lldb/include/lldb/lldb-defines.h

@@ -44,8 +44,10 @@
 #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_MODIFY (1u << 2)
 #define LLDB_WATCH_TYPE_IS_VALID(type)                                         \
-  ((type & LLDB_WATCH_TYPE_READ) || (type & LLDB_WATCH_TYPE_WRITE))
+  ((type & LLDB_WATCH_TYPE_READ) || (type & LLDB_WATCH_TYPE_WRITE) ||          \
+   (type & LLDB_WATCH_TYPE_MODIFY))
 
 // Generic Register Numbers
 #define LLDB_REGNUM_GENERIC_PC 0    // Program Counter

lldb/include/lldb/lldb-enumerations.h

@@ -431,6 +431,21 @@ FLAGS_ENUM(WatchpointEventType){
     eWatchpointEventTypeThreadChanged = (1u << 11),
     eWatchpointEventTypeTypeChanged = (1u << 12)};
 
+enum WatchpointWriteType {
+  /// Don't stop when the watched memory region is written to.
+  eWatchpointWriteTypeDisabled,
+  /// Stop on any write access to the memory region, even if
+  /// the value doesn't change.  On some architectures, a write
+  /// near the memory region may be falsely reported as a match,
+  /// and notify this spurious stop as a watchpoint trap.
+  eWatchpointWriteTypeAlways,
+  /// Stop on a write to the memory region that changes its value.
+  /// This is most likely the behavior a user expects, and is the
+  /// behavior in gdb.  lldb can silently ignore writes near the
+  /// watched memory region that are reported as accesses to lldb.
+  eWatchpointWriteTypeOnModify
+};
+
 /// Programming language type.
 ///
 /// These enumerations use the same language enumerations as the DWARF

lldb/include/lldb/lldb-forward.h

@@ -286,6 +286,7 @@ class VariableList;
 class Watchpoint;
 class WatchpointList;
 class WatchpointOptions;
+class WatchpointSetOptions;
 struct CompilerContext;
 struct LineEntry;
 struct PropertyDefinition;

lldb/source/API/CMakeLists.txt

@@ -101,6 +101,7 @@ add_lldb_library(liblldb SHARED ${option_framework}
   SBValueList.cpp
   SBVariablesOptions.cpp
   SBWatchpoint.cpp
+  SBWatchpointOptions.cpp
   SBUnixSignals.cpp
   SystemInitializerFull.cpp
   ${lldb_python_wrapper}

lldb/source/API/SBTarget.cpp

@@ -1358,27 +1358,39 @@ SBWatchpoint SBTarget::FindWatchpointByID(lldb::watch_id_t wp_id) {
 }
 
 lldb::SBWatchpoint SBTarget::WatchAddress(lldb::addr_t addr, size_t size,
-                                          bool read, bool write,
+                                          bool read, bool modify,
                                           SBError &error) {
   LLDB_INSTRUMENT_VA(this, addr, size, read, write, error);
 
+  SBWatchpointOptions options;
+  options.SetWatchpointTypeRead(read);
+  options.SetWatchpointTypeWrite(eWatchpointWriteTypeOnModify);
+  return WatchpointCreateByAddress(addr, size, options, error);
+}
+
+lldb::SBWatchpoint
+SBTarget::WatchpointCreateByAddress(lldb::addr_t addr, size_t size,
+                                    SBWatchpointOptions options,
+                                    SBError &error) {
+  LLDB_INSTRUMENT_VA(this, addr, size, options, error);
+
   SBWatchpoint sb_watchpoint;
   lldb::WatchpointSP watchpoint_sp;
   TargetSP target_sp(GetSP());
-  if (target_sp && (read || write) && addr != LLDB_INVALID_ADDRESS &&
-      size > 0) {
+  uint32_t watch_type = 0;
+  if (options.GetWatchpointTypeRead())
+    watch_type |= LLDB_WATCH_TYPE_READ;
+  if (options.GetWatchpointTypeWrite() == eWatchpointWriteTypeAlways)
+    watch_type |= LLDB_WATCH_TYPE_WRITE;
+  if (options.GetWatchpointTypeWrite() == eWatchpointWriteTypeOnModify)
+    watch_type |= LLDB_WATCH_TYPE_MODIFY;
+  if (watch_type == 0) {
+    error.SetErrorString("Can't create a watchpoint that is neither read nor "
+                         "write nor modify.");
+    return sb_watchpoint;
+  }
+  if (target_sp && addr != LLDB_INVALID_ADDRESS && size > 0) {
     std::lock_guard<std::recursive_mutex> guard(target_sp->GetAPIMutex());
-    uint32_t watch_type = 0;
-    if (read)
-      watch_type |= LLDB_WATCH_TYPE_READ;
-    if (write)
-      watch_type |= LLDB_WATCH_TYPE_WRITE;
-    if (watch_type == 0) {
-      error.SetErrorString(
-          "Can't create a watchpoint that is neither read nor write.");
-      return sb_watchpoint;
-    }
-
     // Target::CreateWatchpoint() is thread safe.
     Status cw_error;
     // This API doesn't take in a type, so we can't figure out what it is.

lldb/source/API/SBValue.cpp

@@ -1434,10 +1434,17 @@ lldb::SBWatchpoint SBValue::Watch(bool resolve_location, bool read, bool write,
       return sb_watchpoint;
 
     uint32_t watch_type = 0;
-    if (read)
+    if (read) {
       watch_type |= LLDB_WATCH_TYPE_READ;
-    if (write)
-      watch_type |= LLDB_WATCH_TYPE_WRITE;
+      // read + write, the most likely intention
+      // is to catch all writes to this, not just
+      // value modifications.
+      if (write)
+        watch_type |= LLDB_WATCH_TYPE_WRITE;
+    } else {
+      if (write)
+        watch_type |= LLDB_WATCH_TYPE_MODIFY;
+    }
 
     Status rc;
     CompilerType type(value_sp->GetCompilerType());

lldb/source/API/SBWatchpoint.cpp

@@ -355,7 +355,8 @@ bool SBWatchpoint::IsWatchingWrites() {
     std::lock_guard<std::recursive_mutex> guard(
         watchpoint_sp->GetTarget().GetAPIMutex());
 
-    return watchpoint_sp->WatchpointWrite();
+    return watchpoint_sp->WatchpointWrite() ||
+           watchpoint_sp->WatchpointModify();
   }
 
   return false;

lldb/source/API/SBWatchpointOptions.cpp

@@ -0,0 +1,73 @@
+//===-- SBWatchpointOptions.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/API/SBWatchpointOptions.h"
+#include "lldb/Breakpoint/Watchpoint.h"
+#include "lldb/Utility/Instrumentation.h"
+
+#include "Utils.h"
+
+using namespace lldb;
+using namespace lldb_private;
+
+class WatchpointOptionsImpl {
+public:
+  bool m_read = false;
+  bool m_write = false;
+  bool m_modify = false;
+};
+
+
+SBWatchpointOptions::SBWatchpointOptions()
+    : m_opaque_up(new WatchpointOptionsImpl()) {
+  LLDB_INSTRUMENT_VA(this);
+}
+
+SBWatchpointOptions::SBWatchpointOptions(const SBWatchpointOptions &rhs) {
+  LLDB_INSTRUMENT_VA(this, rhs);
+
+  m_opaque_up = clone(rhs.m_opaque_up);
+}
+
+const SBWatchpointOptions &
+SBWatchpointOptions::operator=(const SBWatchpointOptions &rhs) {
+  LLDB_INSTRUMENT_VA(this, rhs);
+
+  if (this != &rhs)
+    m_opaque_up = clone(rhs.m_opaque_up);
+  return *this;
+}
+
+SBWatchpointOptions::~SBWatchpointOptions() = default;
+
+void SBWatchpointOptions::SetWatchpointTypeRead(bool read) {
+  m_opaque_up->m_read = read;
+}
+bool SBWatchpointOptions::GetWatchpointTypeRead() const {
+  return m_opaque_up->m_read;
+}
+
+void SBWatchpointOptions::SetWatchpointTypeWrite(
+    WatchpointWriteType write_type) {
+  if (write_type == eWatchpointWriteTypeOnModify) {
+    m_opaque_up->m_write = false;
+    m_opaque_up->m_modify = true;
+  } else if (write_type == eWatchpointWriteTypeAlways) {
+    m_opaque_up->m_write = true;
+    m_opaque_up->m_modify = false;
+  } else
+    m_opaque_up->m_write = m_opaque_up->m_modify = false;
+}
+
+WatchpointWriteType SBWatchpointOptions::GetWatchpointTypeWrite() const {
+  if (m_opaque_up->m_modify)
+    return eWatchpointWriteTypeOnModify;
+  if (m_opaque_up->m_write)
+    return eWatchpointWriteTypeAlways;
+  return eWatchpointWriteTypeDisabled;
+}