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

[Greg Clayton via lldb-commits]

clayborg wrote:

> > > Just to be clear, I need to find a solution for SBTarget::CreateWatchpoint before this PR is ready to land. I'm inclined towards adding a third bool parameter 'modify', but I'm not sure that's the best choice.
> > 
> > 
> > Anytime we keep adding more options to an API call we have two options:
> 
> Yeah exactly, I wasn't restating the options as clearly in that last comment, that's exactly what I meant. I can't think of other options we would want to build in to a Watchpoint at creation time - but the argument for SBWatchpointOptions is that ten-years-in-the-future-Jason may want another flag.

The SBTarget::Launch(SBLaunchInfo) and SBTarget::Attach(SBAttachInfo) are some of the best example of using an options class. Breakpoints never tried to use an options class, we just kept adding new APIs which makes our API a bit messier, but we must keep APIs after they are added. So I would vote for the SBWatchpointOptions method. 

In the future we might have move complex ways to track watchpoints or want to track multiple address ranges with a single watchpoint? 
> 
> Probably the best thing is to look at the SBTarget BreakpointCreate* methods; there are a dozen different methods for the different types of breakpoints you might want to create (address breakpoint, file & line breakpoint, symbol name breakpoint etc). An interesting aside is that none of the SB API methods take a flag for whether breakpoint should be set using a software or hardware breakpoint. Jonas added that feature to debugserver a few years ago for x86_64 and aarch64, and I think he added the `target.require-hardware-breakpoint` setting then. Otherwise the only way to set a hardware breakpoint is through the commandline `breakpoint set` command.

Yeah, we probably didn't want to add yet another API call to just add access to the flag.
> 
> For a watchpoint, we only have `SBTarget::WatchAddress`, which takes and address and size. If we were trying to follow the breakpoint API naming style, we would add `SBTarget::WatchpointCreateByAddress`, `SBTarget::WatchpointCreateByVariable`, and `SBTarget::WatchpointCreateByExpression` methods. All of them would take the same read/write/modify flags, which might be the strongest argument for an options class even if it seems a little bit much for a few bools.

Yeah, it isn't too hard and it allows us great flexibility in the future. I have a patch coming for improving the saving of core files that allows users to specify a custom memory region list. I am creating a new options class named SBProcessSaveCoreOptions for the SBProcess::SaveCore(SBProcessSaveCoreOptions options) function as we can always add more flags to this class. 



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