Add a bytes range to the DataBreakpointInfo Request

[connor4312]

Splitting off from #445

One feature from the authors of the request include the ability to set breakpoints for a range of memory:

The "event points" map pretty well onto DAP data breakpoints, except data breakpoints can only be set for singular variables in the protocol.

To allow a range to be set, we need a corresponding capability

interface Capability {
  // ... in addition to exisitng properties:

  /**
   * The debug adapter supports padding `length` in the `DataBreakpoint` request.
   */
  supportsDataBreakpointBytes?: boolean;

Then there may be an additional field in the info request, which specifies the number of bytes following the address in which the debugger should stop:

interface DataBreakpointInfoArguments {
  // ... in addition to exisitng properties:

  /**
   * Clients may set this variable only if the `supportsDataBreakpointBytes`
   * capability is true.
   * 
   * Clients may pass this to request a reference to a range of memory rather
   * than a single variable or expression. Data breakpoints set using the
   * resulting data ID request that the debugger pauses within the range of
   * memory starting at address of the variable or expression,
   * and extending `bytes` exclusive bytes.
   * 
   * Treated as a hex value if prefixed with `0x`, or as a decimal value
   * otherwise.
   */
  bytes?: string;

[gregg-miskelly]

dataBreakpointInfo is really about asking a debug adapter to provide a description of how to set a data breakpoint on a variable. In this scenario, it doesn't seem like there is a variable involved -- it seems like the user is really inputting an address, size, and access settings.

Questions:

1. Can we come up with a standard set of properties for 'access settings'? Or do we really need a custom request (like launch arguments)?
2. Instead of extending dataBreakpointInfo, should we just extend DataBreakpoint so that when a new capability is present, dataId can have some special value (empty string?) and then there is a new field that carries additional properties about the address range?

[kkistm]

I agree with @gregg-miskelly about DataBreakpointInfo usage. Two fields from this interface, variableReference and frameId, are not useful for data breakpoints.
What about to introduce DataRangeBreakpoint or MemoryRangeBreakpoing interfaces, inspired by DataBreakpoint but tailored to the specific case? With a corresponding set*() method?

[markgoodchild]

I agree with @kkistm that making the interface more precise makes the purpose easier to understand and its usage cleaner.

[connor4312]

Thanks for the feedback. How about we introduce an alternative request, like DataAddressBreakpointInfo, with arguments

interface DataAddressBreakpointInfoArguments {
  /**
   * The address of the data for which to obtain breakpoint information.
   * Treated as a hex value if prefixed with `0x`, or as a decimal value
   * otherwise.
   */
  address: string;

  /**
   * If passed, requests breakpoint information for an exclusive byte range
   * rather than a single address. The range extends the given number of `bytes`
   * from the start `address`.
   * 
   * Treated as a hex value if prefixed with `0x`, or as a decimal value
   * otherwise.
   */
  bytes?: string
}

The response is identical to the DataBreakpointInfo response, allowing the resulting dataId to interop with the existing SetDataBreakpoints request.

This covers the 'access settings' menu with the exception of the "mask" field, though that could be added as a property.

Of course this comes with a capability.

interface Capability {
  // ... in addition to existing properties:

  /**
   * The debug adapter supports the `dataAdressBreakpointInfo` request.
   */
  supportsDataAddressInfo?: boolean;

[kkistm]

@connor4312, the proposition about DataAddressBreakpointInfo sounds good for me, keeping in mind that additional properties could be added as needed.

[ZequanWu]

https://microsoft.github.io/debug-adapter-protocol/specification#Requests_DataBreakpointInfo says "If variablesReference isn't specified, this can be an expression." in DataBreakpointInfoArguments's name field. If this request is added, does this mean the name could not be an expression anymore? If no, we still need the number of bytes to watch from expression.

[connor4312]

This was finished in #461

@ZequanWu see that PR for what we ended up doing, let us know if you have any feedback. Thanks!

[ZequanWu]

Oh, I saw the new request DataAddressBreakpointInfo has two fields: address and bytes. Then my question might be another issue: For name in DataBreakpointInfoArguments, it could be an expression if variablesReference is not specified, like ${variable} + ${offset}. How would the debugger know how many bytes to watch from the starting address evaluated from the expression in this case?

I prefer the original change by adding bytes to DataBreakpointInfoArguments as this will solve both problem.

[connor4312]

We don't specify that an adapter should treat a numeric return value from an expression in name as a pointer to that address. A common implementation may be expecting some pointer-type result from the name and breaking either on that specific address or size of the data that was pointed to. An adapter or debugger could implement this in any number of ways. In a JavaScript runtime, there may be no notion of memory addresses at all and only object identity.

While a little roundabout it's possible to get the address of a variable via its memoryReference, which one could use to use the new range request to set a custom range of bytes for a variable

[ZequanWu]

A common implementation may be expecting some pointer-type result from the name and breaking either on that specific address or size of the data that was pointed to.

I think it's common the pointer-type result from the name is a void* type which doesn't tell the underlying pointee's type information and size information in C/C++. In this case, the best we can do is to set the size to watch to the pointer size which might not always be the case.

[connor4312]

Reopening for discussion.

If we were to do it that way, we'd need some additional flagging for the debugger to treat the name as a numeric value. It's a bit of an overload, but we cannot make it optional as that would be backwards-incompatible.

interface DataBreakpointInfoArguments {
  /**
   * Reference to the variable container if the data breakpoint is requested for
   * a child of the container. The `variablesReference` must have been obtained
   * in the current suspended state. See 'Lifetime of Object References' in the
   * Overview section for details.
   */
  variablesReference?: number;

  /**
   * The name of the variable's child to obtain data breakpoint information for.
   * If `variablesReference` isn't specified, this can be an expression.
   * If `asAddress` is specified, this is a memory address: interpreted as a
   * hex value if prefixed with `0x`, or a decimal value otherwise.
   */
  name: string;

  /**
   * Clients may set this variable only if the `supportsDataBreakpointBytes`
   * capability is true.
   * 
   * Clients may pass this to request a reference to a range of memory rather
   * than a single address. Data breakpoints set using the resulting data ID
   * request that the debugger pauses within the range of memory extending
   * `bytes` from the address or variable specified by `name`.
   * 
   * Treated as a hex value if prefixed with `0x`, or as a decimal value
   * otherwise.
   */
  bytes?: number;

  /**
   * If `true`, the `name` is a memory address and the debugger should interpret.
   * 
   * Clients may set this variable only if the `supportsDataBreakpointBytes`
   * capability is true.
   */
  asAddress?: boolean;

  /**
   * When `name` is an expression, evaluate it in the scope of this stack frame.
   * If not specified, the expression is evaluated in the global scope. When
   * `variablesReference` is specified, this property has no effect.
   */
  frameId?: number;
}

Technically asAddress and bytes could be two different capabilities, but both require an adapter able to address memory regions, so they go hand-in-hand.

The downside of this is that there is less support for "address-only" properties, but otherwise it's also a nice solution and solves the issue you mention -- and practically I don't expect adapters have special behavior applicable only for a data breakpoint on a variable versus a range of addresses or vise versa.

[ZequanWu]

Do we need the extra field asAddress?

Given "If variablesReference isn't specified, this can be an expression." for name, an expression could be a numeric value (decimal or hex) representing the address, right?

[connor4312]

I think asAddress is preferable so that DA's don't have to try to pattern match what they're being given. In common lisp for example, 0x123 is also a valid variable name, so it would be impossible to differentiate when taking input from a client.

[connor4312]

After playing around with this a bit, I prefer the new proposal in #455 (comment). I'm going to back my previous changes out of the upcoming DAP release and put a PR in with that proposal for next month's release to allow time for additional discussion.