[LLDB] Expose several methods in SBWatchpoint

[delcypher]

Summary

[LLDB] Expose several methods in SBWatchpoint

This patch adds the following methods:

• GetType()
• GetWatchValueKind()
• GetWatchSpec()
• IsWatchingReads()
• IsWatchingWrites()

These mostly expose methods that lldb_private::Watchpoint already
had. Tests are included that exercise these new methods.

The motivation for exposing these are as follows:

• GetType() - With this information and the address from a watchpoint it is now possible to construct an SBValue from an SBWatchpoint. Previously this wasn't possible. The included test case illustrates doing this.
• GetWatchValueKind() - This allows the caller to determine whether the watchpoint is a variable watchpoint or an expression watchpoint. A new enum (WatchpointValueKind) has been introduced to represent the return values. Unfortunately the name WatchpointKind was already taken.
• GetWatchSpec() - This allows (at least for variable watchpoints) to use a sensible name for SBValues created from an SBWatchpoint.
• IsWatchingReads() - This allow checking if a watchpoint is monitoring read accesses.
• IsWatchingWRites() - This allow checking if a watchpoint is monitoring write accesses.

rdar://105606978

[delcypher]

Fix reviewers

[delcypher]

Fix typo

[mib]

Most of this looks fine to me, besides the WatchSpec cache string in the SB class. I don't think the spec changes after the creation of the watchpoint, so may be this WatchSpec object should be a lldb_private::ConstString and from there you'll be able to get your c_string.

[mib]

lldb/test/API/python_api/watchpoint/TestSetWatchpoint.py

86

+1, please file an enhancement request :)

88

runCmd doesn't check if the command succeeded, so you probably want to add an asset to check the number of watchpoint on the target is not zero.

103

unrelated to this patch, there is seems we don't initialize the watch spec for expression watchpoint, we should probably file a bug report for that.

[mib]

lldb/test/API/python_api/watchpoint/TestSetWatchpoint.py

103

it seems that *

[jasonmolenda]

Hi Dan, I hadn't looked this over very closely but one thing that jumped out is that you're adding a member to SBWatchpoint, and we can't do that, it's an API breaking change. In cases where we need to store additional information than a weak pointer to an lldb private object, we traditionally add an Impl class which has the additional member(s) and a shared pointer to the lldb private object that backs the class. e.g. see SBValue's ValueImpl, the definition is in SBValue.cpp.

[jasonmolenda]

In D144937#4159324, @jasonmolenda wrote:

In cases where we need to store additional information than a weak pointer to an lldb private object, we traditionally add an Impl class which has the additional member(s) and a shared pointer to the lldb private object that backs the class. e.g. see SBValue's ValueImpl, the definition is in SBValue.cpp.

I was being a little sloppy in the ownership. The SBValue has to keep the underlying ValueObject alive, so it has a shared pointer to its ValueImpl, which has a shared pointer to the ValueObject. Running the SBValue dtor will decr the count on the ValueImpl causing it to be removed, and decrementing the ValueObjectSP count.

SBWatchpoint would use a shared pointer to its WatchpointImpl objct, and that would have a weak pointer to the lldb_private Watchpoint object; it is not responsible for keeping this watchpoint alive.

[JDevlieghere]

lldb/source/API/SBWatchpoint.cpp

327–339

As Ismail and Jason pointed out, the way to do this is wrapping it into a ConstString:

return ConstString(watchpoint_sp->GetWatchSpec()).AsCString();

[delcypher]

In D144937#4159324, @jasonmolenda wrote:

Hi Dan, I hadn't looked this over very closely but one thing that jumped out is that you're adding a member to SBWatchpoint, and we can't do that, it's an API breaking change. In cases where we need to store additional information than a weak pointer to an lldb private object, we traditionally add an Impl class which has the additional member(s) and a shared pointer to the lldb private object that backs the class. e.g. see SBValue's ValueImpl, the definition is in SBValue.cpp.

I talked to @jasonmolenda offline about this. He meant ABI (broken by the patch in its current form) not API (not broken by this patch in its current form).

[bulbazord]

Seems like you've already got feedback about breaking ABI. Small nit:

lldb/bindings/interface/SBWatchpointDocstrings.i

30–31

[delcypher]

lldb/source/API/SBWatchpoint.cpp

317

@bulbazord What I don't like about my implementation here is that the user can't tell the difference between

1. there's no shared pointer

AND

2. watchpoint is an expression watchpoint.

Is 1, common? Feels like this function should really return a std:optional<bool> but I'm not sure how to make that play well with SWIG.

327–339

Sure. Based on ConstString's documentation looks like it uses a pool of strings that lives for ever. So we'll be leaking memory here. I guess that's acceptable because this method is unlikely to be called often, and even if it is we only waste memory per unique watchpoint spec.

This sounds a lot simpler than implementing a WatchpointImpl.

[bulbazord]

lldb/source/API/SBWatchpoint.cpp

317

I'm not sure if 1 is common. To distinguish between 1 and 2, clients will also want to also check the validity of the watchpoint with IsValid or operator bool since those just get the shared pointer and see if its valid. We could make a std::optional work through a swig type map but I'm not sure I like the idea of a yes-or-no-question API being able to return 'None'. If it's not a watch variable, then 'false' makes sense for an invalid watchpoint.

Another option could be to change the interface a bit. Maybe create an enum called WatchpointKind containing 3 values, "Variable", "Expression", and "Invalid". You could then change this method to be GetWatchpointKind and return the right value depending on a combination of GetSP() and IsWatchVariable.

[delcypher]

lldb/test/API/python_api/watchpoint/TestSetWatchpoint.py

86

Will do. TBH I'm actually surprised that calling SBValue::Watch() doesn't create a variable watchpoint.

88

@mib Did I misunderstand something? runCmd has this in the end of its implementation.

if check:
    output = ""
    if self.res.GetOutput():
        output += "\nCommand output:\n" + self.res.GetOutput()
    if self.res.GetError():
        output += "\nError output:\n" + self.res.GetError()
    if msg:
        msg += output
    if cmd:
        cmd += output
    self.assertTrue(self.res.Succeeded(),
                    msg if (msg) else CMD_MSG(cmd))

and check is set to True by default.

So it looks like runCmd does check that the command succeeded.

103

Yes. I plan to file a report about that.

[mib]

lldb/test/API/python_api/watchpoint/TestSetWatchpoint.py

88

In that case, it's fine. Otherwise, I'd have expected to have self.assertGreater(target.GetNumWatchpoints(), 0) before calling target.GetWatpointAtIndex(0)

[delcypher]

• Add WatchpointValueKind enum
• Use WatchpointValueKind for new GetWatchValueKind() method (previously named SBWatchpoint::IsWatchVariable)
• Add IsWatchingsRead() method
• Add IsWatchingWrites() method
• Remove m_cached_watch_spec to avoid breaking ABI.
• In GetWatchSpec() use ConstString rather than returning a pointer to owned by the now removed m_cached_watched_spec.

[delcypher]

lldb/source/API/SBWatchpoint.cpp

317

Done. I ended up giving it a different name because I discovered there's already a WatchpointKind enum and I thought it best to not confuse the two.

327–339

Done.

lldb/test/API/python_api/watchpoint/TestSetWatchpoint.py

86

Actually I'm even more confident this is a bug. If value.Watch(...) created an expression watchpoint you would expect the type to be a int32_t* (because the watched expression would need to have been &global), not int32_t.

However, I think this bug should be fixed in a different commit.

[delcypher]

@JDevlieghere @mib @bulbazord Thanks for all the feedback. This patch is ready for another review pass.

[mib]

LGTM! Thanks for your patch @delcypher

[JDevlieghere]

LGTM with two small nits.

lldb/include/lldb/API/SBWatchpoint.h

14

No longer necessary?

lldb/include/lldb/lldb-enumerations.h

1224–1228

With the 80-col limit it helps readability to put the Doxygen comments on the line above. Also comments should start with a capital.

[delcypher]

• Fix nits

[delcypher]

Thanks for the review and approval. I've the fixed the nits so I'm going to land.

[jingham]

lldb/test/API/python_api/watchpoint/TestSetWatchpoint.py

86

There's no guarantee that an SBValue represents a variable. An SBValue backed by a ValueObjectVariable (or a ValueObjectChild whose root object is a ValueObjectVariable) could create a Variable watchpoint. But for instance a typed memory address isn't a variable, nor is an expression result. They don't have scopes, which is the main thing "variable" watchpoints have over expression ones, for instance.

lldb/bindings/interface/SBWatchpointDocstrings.i

 
 %feature("docstring", "
     Get the condition expression for the watchpoint."
 ) lldb::SBWatchpoint::GetCondition;
 
 %feature("docstring", "
     The watchpoint stops only if the condition expression evaluates to true."
 ) lldb::SBWatchpoint::SetCondition;
+
+%feature("docstring", "
+    Returns the type recorded when the watchpoint was created. For variable
+    watchpoints it is the type of the watched variable. For expression
+    watchpoints it is the type of the provided expression."
+) lldb::SBWatchpoint::GetType;
+
+%feature("docstring", "
+    Returns the kind of value that was watched when the watchpoint was created.
+    Returns one of the following eWatchPointValueKindVariable,
+    eWatchPointValueKindExpression, eWatchPointValueKindInvalid.
+    "
+) lldb::SBWatchpoint::GetWatchValueKind;
+
+%feature("docstring", "
+    Get the spec for the watchpoint. For variable watchpoints this is the name
+    of the variable. For expression watchpoints it is empty
+    (may change in the future)."
+) lldb::SBWatchpoint::GetWatchSpec;
+
+%feature("docstring", "
+    Returns true if the watchpoint is watching reads. Returns false otherwise."
+) lldb::SBWatchpoint::IsWatchingReads;
+
+%feature("docstring", "
+    Returns true if the watchpoint is watching writes. Returns false otherwise."
+) lldb::SBWatchpoint::IsWatchingWrites;

lldb/docs/python_api_enums.rst

 .. py:data:: eCommandInterpreterResultCommandError
 
    Stopped because the corresponding option was set and a command returned
    an error.
 
 .. py:data:: eCommandInterpreterResultQuitRequested
 
    Stopped because quit was requested.
+
+
+.. _WatchPointValueKind:
+
+WatchPointValueKind
+-------------------
+
+The type of value that the watchpoint was created to monitor.
+
+.. py:data:: eWatchPointValueKindInvalid
+
+   Invalid kind.
+
+.. py:data:: eWatchPointValueKindVariable
+
+   Watchpoint was created watching a variable
+
+.. py:data:: eWatchPointValueKindExpression
+
+   Watchpoint was created watching the result of an expression that was
+   evaluated at creation time.

lldb/include/lldb/API/SBType.h

protected:
   friend class SBTarget;
   friend class SBTypeEnumMember;
   friend class SBTypeEnumMemberList;
   friend class SBTypeNameSpecifier;
   friend class SBTypeMember;
   friend class SBTypeMemberFunction;
   friend class SBTypeList;
   friend class SBValue;
+  friend class SBWatchpoint;
 
   SBType(const lldb_private::CompilerType &);
   SBType(const lldb::TypeSP &);
 };
 
 class SBTypeList {
 public:
   SBTypeList();

lldb/include/lldb/API/SBWatchpoint.h

 //===-- SBWatchpoint.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_SBWATCHPOINT_H
 #define LLDB_API_SBWATCHPOINT_H
 
 #include "lldb/API/SBDefines.h"
+#include "lldb/API/SBType.h"
 
 namespace lldb {
 
 class LLDB_API SBWatchpoint {
 public:
   SBWatchpoint();
 
   SBWatchpoint(const lldb::SBWatchpoint &rhs);
 
#endif
 
   static bool EventIsWatchpointEvent(const lldb::SBEvent &event);
 
   static lldb::WatchpointEventType
   GetWatchpointEventTypeFromEvent(const lldb::SBEvent &event);
 
   static lldb::SBWatchpoint GetWatchpointFromEvent(const lldb::SBEvent &event);
 
+  lldb::SBType GetType();
+
+  WatchpointValueKind GetWatchValueKind();
+
+  const char *GetWatchSpec();
+
+  bool IsWatchingReads();
+
+  bool IsWatchingWrites();
+
 private:
   friend class SBTarget;
   friend class SBValue;
 
   std::weak_ptr<lldb_private::Watchpoint> m_opaque_wp;
 };
 
 } // namespace lldb
 
 #endif // LLDB_API_SBWATCHPOINT_H

lldb/include/lldb/lldb-enumerations.h

enum DWIMPrintVerbosity {
   eDWIMPrintVerbosityNone,
   /// Print a message when `dwim-print` uses `expression` evaluation.
   eDWIMPrintVerbosityExpression,
   /// Always print a message indicating how `dwim-print` is evaluating its
   /// expression.
   eDWIMPrintVerbosityFull,
 };
 
+enum WatchpointValueKind {
+  eWatchPointValueKindInvalid = 0,
+  ///< Watchpoint was created watching a variable
+  eWatchPointValueKindVariable = 1,
+  ///< Watchpoint was created watching the result of an expression that was
+  ///< evaluated at creation time.
+  eWatchPointValueKindExpression = 2,
+};
+
 } // namespace lldb
 
 #endif // LLDB_LLDB_ENUMERATIONS_H

lldb/source/API/SBWatchpoint.cpp

 #include "lldb/API/SBDefines.h"
 #include "lldb/API/SBEvent.h"
 #include "lldb/API/SBStream.h"
 #include "lldb/Utility/Instrumentation.h"
 
 #include "lldb/Breakpoint/Watchpoint.h"
 #include "lldb/Breakpoint/WatchpointList.h"
 #include "lldb/Core/StreamFile.h"
+#include "lldb/Symbol/CompilerType.h"
 #include "lldb/Target/Process.h"
 #include "lldb/Target/Target.h"
 #include "lldb/Utility/Stream.h"
 #include "lldb/lldb-defines.h"
 #include "lldb/lldb-types.h"
 
 using namespace lldb;
 using namespace lldb_private;
SBWatchpoint SBWatchpoint::GetWatchpointFromEvent(const lldb::SBEvent &event) {
   LLDB_INSTRUMENT_VA(event);
 
   SBWatchpoint sb_watchpoint;
   if (event.IsValid())
     sb_watchpoint =
         Watchpoint::WatchpointEventData::GetWatchpointFromEvent(event.GetSP());
   return sb_watchpoint;
 }
+
+lldb::SBType SBWatchpoint::GetType() {
+  LLDB_INSTRUMENT_VA(this);
+
+  lldb::WatchpointSP watchpoint_sp(GetSP());
+  if (watchpoint_sp) {
+    std::lock_guard<std::recursive_mutex> guard(
+        watchpoint_sp->GetTarget().GetAPIMutex());
+    const CompilerType &type = watchpoint_sp->GetCompilerType();
+    return lldb::SBType(type);
+  }
+  return lldb::SBType();
+}
+
+WatchpointValueKind SBWatchpoint::GetWatchValueKind() {
+  LLDB_INSTRUMENT_VA(this);
+
+  lldb::WatchpointSP watchpoint_sp(GetSP());
+  if (watchpoint_sp) {
+    std::lock_guard<std::recursive_mutex> guard(
+        watchpoint_sp->GetTarget().GetAPIMutex());
+    if (watchpoint_sp->IsWatchVariable())
+      return WatchpointValueKind::eWatchPointValueKindVariable;
+    return WatchpointValueKind::eWatchPointValueKindExpression;
+  }
+  return WatchpointValueKind::eWatchPointValueKindInvalid;
+}
+
+const char *SBWatchpoint::GetWatchSpec() {
+  LLDB_INSTRUMENT_VA(this);
+
+  lldb::WatchpointSP watchpoint_sp(GetSP());
+  if (watchpoint_sp) {
+    std::lock_guard<std::recursive_mutex> guard(
+        watchpoint_sp->GetTarget().GetAPIMutex());
+    // Store the result of `GetWatchSpec()` as a ConstString
+    // so that the C string we return has a sufficiently long
+    // lifetime. Note this a memory leak but should be fairly
+    // low impact.
+    return ConstString(watchpoint_sp->GetWatchSpec()).AsCString();
+  }
+  return nullptr;
+}
+
+bool SBWatchpoint::IsWatchingReads() {
+  LLDB_INSTRUMENT_VA(this);
+  lldb::WatchpointSP watchpoint_sp(GetSP());
+  if (watchpoint_sp) {
+    std::lock_guard<std::recursive_mutex> guard(
+        watchpoint_sp->GetTarget().GetAPIMutex());
+
+    return watchpoint_sp->WatchpointRead();
+  }
+
+  return false;
+}
+
+bool SBWatchpoint::IsWatchingWrites() {
+  LLDB_INSTRUMENT_VA(this);
+  lldb::WatchpointSP watchpoint_sp(GetSP());
+  if (watchpoint_sp) {
+    std::lock_guard<std::recursive_mutex> guard(
+        watchpoint_sp->GetTarget().GetAPIMutex());
+
+    return watchpoint_sp->WatchpointWrite();
+  }
+
+  return false;
+}

lldb/test/API/python_api/watchpoint/TestSetWatchpoint.py

class SetWatchpointAPITestCase(TestBase):
     def setUp(self):
         # Call super's setUp().
         TestBase.setUp(self)
         # Our simple source filename.
         self.source = 'main.c'
         # Find the line number to break inside main().
         self.line = line_number(
             self.source, '// Set break point at this line.')
+        self.build()
 
     # Read-write watchpoints not supported on SystemZ
     @expectedFailureAll(archs=['s390x'])
     def test_watch_val(self):
         """Exercise SBValue.Watch() API to set a watchpoint."""
-        self.build()
+        self._test_watch_val(variable_watchpoint=False)
+        pass
+
+    @expectedFailureAll(archs=['s390x'])
+    def test_watch_variable(self):
+        """
+           Exercise some watchpoint APIs when the watchpoint
+           is created as a variable watchpoint.
+        """
+        self._test_watch_val(variable_watchpoint=True)
+
+    def _test_watch_val(self, variable_watchpoint):
         exe = self.getBuildArtifact("a.out")
 
         # Create a target by the debugger.
         target = self.dbg.CreateTarget(exe)
         self.assertTrue(target, VALID_TARGET)
 
         # Now create a breakpoint on main.c.
         breakpoint = target.BreakpointCreateByLocation(self.source, self.line)
def _test_watch_val(self, variable_watchpoint):
         process = target.GetProcess()
         self.assertState(process.GetState(), lldb.eStateStopped,
                          PROCESS_STOPPED)
         thread = lldbutil.get_stopped_thread(
             process, lldb.eStopReasonBreakpoint)
         frame0 = thread.GetFrameAtIndex(0)
 
         # Watch 'global' for read and write.
+        if variable_watchpoint:
+            # FIXME: There should probably be an API to create a
+            # variable watchpoint.
+            self.runCmd('watchpoint set variable -w read_write -- global')
+            watchpoint = target.GetWatchpointAtIndex(0)
+            self.assertEqual(watchpoint.GetWatchValueKind(),
+                             lldb.eWatchPointValueKindVariable)
+            self.assertEqual(watchpoint.GetWatchSpec(), 'global')
+            # Synthesize an SBValue from the watchpoint
+            watchpoint_addr = lldb.SBAddress(watchpoint.GetWatchAddress(),
+                                             target)
+            value = target.CreateValueFromAddress(
+                watchpoint.GetWatchSpec(),
+                watchpoint_addr, watchpoint.GetType())
+        else:
             value = frame0.FindValue('global', lldb.eValueTypeVariableGlobal)
             error = lldb.SBError()
             watchpoint = value.Watch(True, True, True, error)
             self.assertTrue(value and watchpoint,
                         "Successfully found the variable and set a watchpoint")
             self.DebugSBValue(value)
+            self.assertEqual(watchpoint.GetWatchValueKind(),
+                             lldb.eWatchPointValueKindExpression)
+            # FIXME: The spec should probably be '&global' given that the kind
+            # is reported as eWatchPointValueKindExpression. If the kind is
+            # actually supposed to be eWatchPointValueKindVariable then the spec
+            # should probably be 'global'.
+            self.assertEqual(watchpoint.GetWatchSpec(), None)
+
+        self.assertEqual(watchpoint.GetType().GetDisplayTypeName(), 'int32_t')
+        self.assertEqual(value.GetName(), 'global')
+        self.assertEqual(value.GetType(), watchpoint.GetType())
+        self.assertTrue(watchpoint.IsWatchingReads())
+        self.assertTrue(watchpoint.IsWatchingWrites())
 
         # Hide stdout if not running with '-t' option.
         if not self.TraceOn():
             self.HideStdout()
 
         print(watchpoint)
 
         # Continue.  Expect the program to stop due to the variable being
         # written to.
         process.Continue()
 
         if (self.TraceOn()):
             lldbutil.print_stacktraces(process)

lldb/test/API/python_api/watchpoint/watchlocation/TestSetWatchlocation.py

def test_watch_location(self):
         # Watch for write to *g_char_ptr.
         error = lldb.SBError()
         watchpoint = value.WatchPointee(True, False, True, error)
         self.assertTrue(value and watchpoint,
                         "Successfully found the pointer and set a watchpoint")
         self.DebugSBValue(value)
         self.DebugSBValue(pointee)
 
+        # Check some API calls return expected values
+        self.assertEqual(watchpoint.GetWatchValueKind(),
+                         lldb.eWatchPointValueKindExpression)
+        # FIXME: The spec should probably be 'g_char_ptr'
+        self.assertEqual(watchpoint.GetWatchSpec(), None)
+        self.assertEqual(watchpoint.GetType().GetDisplayTypeName(), 'char')
+        self.assertFalse(watchpoint.IsWatchingReads())
+        self.assertTrue(watchpoint.IsWatchingWrites())
+
         # Hide stdout if not running with '-t' option.
         if not self.TraceOn():
             self.HideStdout()
 
         print(watchpoint)
 
         # Continue.  Expect the program to stop due to the variable being
         # written to.