[Lldb-commits] [lldb] [lldb] Add 'modify' type watchpoints, make it default (PR #66308)

[via lldb-commits]

jimingham wrote:

I'm not sure I see the benefit of the SBWatchpointSpec including the way the watched region is specified.  That's just moving the proliferation of API's from SBTarget::WatchpointCreate*() to the SBWatchpointSpec constructor. We'd have to have:

SBWatchpointSpec::SBWatchpointSpec(SBAddress addr, int flags);
SBWatchpointSpec::SBWatchpointSpec(SBValue value, int flags);
SBWatchpointSpec::SBWatchpointSpec(const char *expr, int flags);

And then users would do:

wp_spec = lldb.SBWatchpointSpec("foo + 5", lldb.eWatchpointTypeModify)
wp = target.WatchpointCreate(wp_spec)

So that's really one more API plus some boilerplate to construct the options.  If an SBWatchpointSpec were the sort of thing you'd reuse, there might be some sense in making that a separate entity, but I don't see that for watchpoints.  Otherwise, I don't think it adds much.

Jim


> On Sep 14, 2023, at 3:12 PM, Jason Molenda ***@***.***> wrote:
> 
> 
>    SBWatchpoint SBTarget::WatchpointCreateByAddress(addr_t address, size_t size, uint32_t access_flags, SBError &error);
> with eWatchpointAccess{Read,Write,Modify} flags defined.
> 
> @bulbazord <https://github.com/bulbazord> what do you think about this suggestion? Would you still prefer an Options class?
> 
> If we are going to add an overload I would suggest keeping with just adding a "bool modify" as it is more clear an usable. The options does seem like overkill for just one bool I admit, it just depends on what kind of options we might want to watchpoints in the future. If this is the last change to watchpoints, then add a new API. If we think we will add more options at some point (try to think if the new fancy watchpoints Jason is about to add support for might need more options?) then do the Options class route.
> 
> You and Alex both preferred adding an Options class to pass in to this (and future WatchpointCreate API) so I'll write that up for my next revision of this PR, I didn't see your earlier message talking about your preference for that when I ping'ed Alex on their opinion.
> 
> You were talking about how you think the proliferation of BreakpointCreate API was not ideal. Do you think there should be a SBTarget::WatchpointCreate(SBWatchpointSpec, SBError &error) and you would specify (1) addr + size, (2) variable, (3) expression + size, as well as the access flags, for the Watchpoint? I don't mind the separate SBTarget methods approach for Breakpoints, it seems like six of one, half a dozen of the other to me.
> 
> It's probably best to just go with SBTarget::WatchpointCreateByAddress and add a new SBWatchpointOptions with the access flags only.
> 
> —
> Reply to this email directly, view it on GitHub <https://github.com/llvm/llvm-project/pull/66308#issuecomment-1720222526>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADUPVW4XRSYKJBWO3X4RAY3X2N6NVANCNFSM6AAAAAA4XKAUOQ>.
> You are receiving this because your review was requested.
> 



https://github.com/llvm/llvm-project/pull/66308