Add a `bytes` range to the DataBreakpointInfo Request about debug-adapter-protocol HOT 14 CLOSED

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;

from debug-adapter-protocol.

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

from debug-adapter-protocol.

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?

from debug-adapter-protocol.

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?

from debug-adapter-protocol.

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

from debug-adapter-protocol.

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.

from debug-adapter-protocol.

This was finished in #461

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

from debug-adapter-protocol.

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.

from debug-adapter-protocol.

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

from debug-adapter-protocol.

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.

from debug-adapter-protocol.

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.

from debug-adapter-protocol.

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?

from debug-adapter-protocol.

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.

from debug-adapter-protocol.

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.

from debug-adapter-protocol.