[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

66

+1, please file an enhancement request :)

68

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.

83

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

83

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

329–341

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

319

@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.

329–341

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

319

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

66

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

68

@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.

83

Yes. I plan to file a report about that.

[mib]

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

68

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

319

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.

329–341

Done.

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

66

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 ↗

(On Diff #501356)

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

66

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 true if the watchpoint is a variable watchpoint. Otherwise the
+    watchpoint is an expression watchpoint or in an invalid state."
+) lldb::SBWatchpoint::IsWatchVariable;
+
+%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 if the future)."
+) lldb::SBWatchpoint::GetWatchSpec;

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"
+#include <string>
 
 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();
+
+  bool IsWatchVariable();
+
+  const char *GetWatchSpec();
+
 private:
   friend class SBTarget;
   friend class SBValue;
 
   std::weak_ptr<lldb_private::Watchpoint> m_opaque_wp;
+  std::string m_cached_watch_spec;
 };
 
 } // namespace lldb
 
 #endif // LLDB_API_SBWATCHPOINT_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() { LLDB_INSTRUMENT_VA(this); }
 
 SBWatchpoint::SBWatchpoint(const lldb::WatchpointSP &wp_sp)
-    : m_opaque_wp(wp_sp) {
+    : m_opaque_wp(wp_sp), m_cached_watch_spec() {
   LLDB_INSTRUMENT_VA(this, wp_sp);
 }
 
 SBWatchpoint::SBWatchpoint(const SBWatchpoint &rhs)
-    : m_opaque_wp(rhs.m_opaque_wp) {
+    : m_opaque_wp(rhs.m_opaque_wp),
+      m_cached_watch_spec(rhs.m_cached_watch_spec) {
   LLDB_INSTRUMENT_VA(this, rhs);
 }
 
 const SBWatchpoint &SBWatchpoint::operator=(const SBWatchpoint &rhs) {
   LLDB_INSTRUMENT_VA(this, rhs);
 
   m_opaque_wp = rhs.m_opaque_wp;
+  m_cached_watch_spec = rhs.m_cached_watch_spec;
   return *this;
 }
 
 SBWatchpoint::~SBWatchpoint() = default;
 
 watch_id_t SBWatchpoint::GetID() {
   LLDB_INSTRUMENT_VA(this);
 
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();
+}
+
+bool SBWatchpoint::IsWatchVariable() {
+  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->IsWatchVariable();
+  }
+  return false;
+}
+
+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());
+    // We can't return `watchpoint_sp->GetWatchSpec().c_str()`
+    // because the temporary std::string will be destroyed
+    // when this function finishes. Instead we store our own
+    // copy in this class and give clients the C string used
+    // by the copy.
+    if (m_cached_watch_spec.size() == 0) {
+      m_cached_watch_spec = watchpoint_sp->GetWatchSpec();
+    } else {
+      // The string should never change otherwise we'd have to
+      // update the cached value which might invalidate pointers
+      // held by clients.
+      assert(m_cached_watch_spec == watchpoint_sp->GetWatchSpec());
+    }
+    return m_cached_watch_spec.c_str();
+  }
+  return nullptr;
+}

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.assertTrue(watchpoint.IsWatchVariable())
+            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.assertFalse(watchpoint.IsWatchVariable())
+            self.assertEqual(watchpoint.GetWatchSpec(), '')
+
+        self.assertEqual(watchpoint.GetType().GetDisplayTypeName(), 'int32_t')
 
         # 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

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 for expression watchpoint
+        self.assertFalse(watchpoint.IsWatchVariable())
+        self.assertEqual(watchpoint.GetWatchSpec(), '')
+        self.assertEqual(watchpoint.GetType().GetDisplayTypeName(), 'char')
+
         # 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.