This is an archive of the discontinued LLVM Phabricator instance.

[LLDB] Expose several methods in SBWatchpoint
ClosedPublic

Authored by delcypher on Feb 27 2023, 6:13 PM.

Details

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

Diff Detail

Event Timeline

delcypher created this revision.Feb 27 2023, 6:13 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 27 2023, 6:13 PM
delcypher requested review of this revision.Feb 27 2023, 6:13 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 27 2023, 6:13 PM
delcypher updated this revision to Diff 500993.Feb 27 2023, 6:15 PM

Fix reviewers

delcypher edited the summary of this revision. (Show Details)Feb 27 2023, 6:17 PM
mib added a comment.Feb 28 2023, 9:51 AM

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 added inline comments.Feb 28 2023, 10:04 AM
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 added inline comments.Feb 28 2023, 10:08 AM
lldb/test/API/python_api/watchpoint/TestSetWatchpoint.py
83

it seems that *

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.

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 requested changes to this revision.Feb 28 2023, 12:49 PM
JDevlieghere added inline comments.
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();
This revision now requires changes to proceed.Feb 28 2023, 12:49 PM

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

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

lldb/bindings/interface/SBWatchpointDocstrings.i
30–31
delcypher added inline comments.Feb 28 2023, 1:05 PM
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

  1. 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 added inline comments.Feb 28 2023, 1:19 PM
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 added inline comments.Feb 28 2023, 2:49 PM
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 added inline comments.Feb 28 2023, 3:00 PM
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 updated this revision to Diff 501356.Feb 28 2023, 6:51 PM
  • 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 marked an inline comment as done.Feb 28 2023, 6:54 PM
delcypher added inline comments.
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 edited the summary of this revision. (Show Details)Feb 28 2023, 6:58 PM
delcypher marked an inline comment as done.

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

mib accepted this revision.Feb 28 2023, 7:10 PM

LGTM! Thanks for your patch @delcypher

JDevlieghere accepted this revision.Feb 28 2023, 9:29 PM

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.

This revision is now accepted and ready to land.Feb 28 2023, 9:29 PM
delcypher updated this revision to Diff 501582.Mar 1 2023, 10:52 AM
delcypher edited the summary of this revision. (Show Details)
  • Fix nits
delcypher marked 2 inline comments as done.Mar 1 2023, 10:53 AM

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

This revision was automatically updated to reflect the committed changes.
jingham added inline comments.Mar 1 2023, 1:51 PM
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.