lldb-dap: DataBreakpointInfoRequest handler violates specification

[Anthony-Eid]

Summary

The DataBreakpiontInforRequest handler seems to violate the DAP spec by requiring frame ID be sent in the request body. This causes two problems

1. There's no way to specify a global scope for expressions
2. The handler always responds with an error when request uses a variable reference to distinguish the scope being used

FrameId Specification

/**
   * 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;

Bug lldb-dap/Protocol/ProtocolRequest.cpp:461

The bug is caused below because frameId is not mapped as an optional. Please note that this isn't the only violation for this request handler, variablesReference, bytes, asAddresss and mode should all be optional as well (everything except the name field should be an optional).

bool fromJSON(const json::Value &Params, DataBreakpointInfoArguments &DBIA,
              json::Path P) {
  json::ObjectMapper O(Params, P);
  return O && O.map("variablesReference", DBIA.variablesReference) &&
         O.map("name", DBIA.name) && O.map("frameId", DBIA.frameId) &&
         O.map("bytes", DBIA.bytes) && O.map("asAddress", DBIA.asAddress) &&
         O.map("mode", DBIA.mode);
}

Dap logs showing error (With Zed)

{
  "type": "request",
  "seq": 21,
  "command": "dataBreakpointInfo",
  "arguments": {
    "variablesReference": 10,
    "name": "c",
    "asAddress": false
  }
}
// Receive
{
  "body": {
    "error": {
      "format": "invalid arguments for request 'dataBreakpointInfo': missing value at arguments.frameId\n/* error: missing value */\n{\n  \"asAddress\": false,\n  \"name\": \"c\",\n  \"variablesReference\": 10\n}",
      "id": 3,
      "showUser": true
    }
  },
  "command": "dataBreakpointInfo",
  "request_seq": 21,
  "seq": 0,
  "success": false,
  "type": "response"
}

[medismailben]

cc. @JDevlieghere

[Anthony-Eid]

I'll be happy to get a fix in for this after my don't reuse variables id PR gets merged btw

[JDevlieghere]

From the spec:

interface DataBreakpointInfoArguments {
  [...]

  /**
   * 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;

  [..]
}

So you're right, frameId should be optional and we're being too strict here.

[JDevlieghere]

And variablesReference, bytes, asAddress and mode are also optional, as you pointed out.

[llvmbot]

@llvm/issue-subscribers-lldb-dap

Author: Anthony Eid (Anthony-Eid)

### Summary

The DataBreakpiontInforRequest handler seems to violate the DAP spec by requiring frame ID be sent in the request body. This causes two problems

1. There's no way to specify a global scope for expressions
2. The handler always responds with an error when request uses a variable reference to distinguish the scope being used

FrameId Specification

/**
   * 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;

Bug lldb-dap/Protocol/ProtocolRequest.cpp:461

The bug is caused below because frameId is not mapped as an optional. Please note that this isn't the only violation for this request handler, variablesReference, bytes, asAddresss and mode should all be optional as well (everything except the name field should be an optional).

bool fromJSON(const json::Value &Params, DataBreakpointInfoArguments &DBIA,
              json::Path P) {
  json::ObjectMapper O(Params, P);
  return O && O.map("variablesReference", DBIA.variablesReference) &&
         O.map("name", DBIA.name) && O.map("frameId", DBIA.frameId) &&
         O.map("bytes", DBIA.bytes) && O.map("asAddress", DBIA.asAddress) &&
         O.map("mode", DBIA.mode);
}

Dap logs showing error (With Zed)

{
  "type": "request",
  "seq": 21,
  "command": "dataBreakpointInfo",
  "arguments": {
    "variablesReference": 10,
    "name": "c",
    "asAddress": false
  }
}
// Receive
{
  "body": {
    "error": {
      "format": "invalid arguments for request 'dataBreakpointInfo': missing value at arguments.frameId\n/* error: missing value */\n{\n  \"asAddress\": false,\n  \"name\": \"c\",\n  \"variablesReference\": 10\n}",
      "id": 3,
      "showUser": true
    }
  },
  "command": "dataBreakpointInfo",
  "request_seq": 21,
  "seq": 0,
  "success": false,
  "type": "response"
}

[da-viper]

The handler always responds with an error when request

Yes, this was broken In a recent commit 63f1008. but for the other fields (variablesReference, bytes, asAddress and mode) are parsed as optionals. see

llvm-project/llvm/include/llvm/Support/JSON.h

Lines 881 to 890 in e175ecf

	/// Maps a property to a field, if it exists.
	/// If the property exists and is invalid, reports an error.
	/// (Optional requires special handling, because missing keys are OK).
	template <typename T> bool map(StringLiteral Prop, std::optional<T> &Out) {
	assert(*this && "Must check this is an object before calling map()");
	if (const Value *E = O->get(Prop))
	return fromJSON(*E, Out, P.field(Prop));
	Out = std::nullopt;
	return true;
	}

llvm-project/lldb/tools/lldb-dap/Protocol/ProtocolRequests.h

Lines 709 to 742 in 428ffbd

	struct 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.
	std::optional<int64_t> variablesReference;
	/// The name of the variable's child to obtain data breakpoint information
	/// for. If `variablesReference` isn't specified, this can be an expression,
	/// or an address if `asAddress` is also true.
	std::string name;
	/// 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 `asAddress` is true, the `frameId` is ignored.
	uint64_t frameId = LLDB_DAP_INVALID_FRAME_ID;
	/// If specified, a debug adapter should return information for the range of
	/// memory extending `bytes` number of bytes from the address or variable
	/// specified by `name`. Breakpoints set using the resulting data ID should
	/// pause on data access anywhere within that range.
	/// Clients may set this property only if the `supportsDataBreakpointBytes`
	/// capability is true.
	std::optional<int64_t> bytes;
	/// If `true`, the `name` is a memory address and the debugger should
	/// interpret it as a decimal value, or hex value if it is prefixed with `0x`.
	/// Clients may set this property only if the `supportsDataBreakpointBytes`
	/// capability is true.
	std::optional<bool> asAddress;
	/// The mode of the desired breakpoint. If defined, this must be one of the
	/// `breakpointModes` the debug adapter advertised in its `Capabilities`.
	std::optional<std::string> mode;

There's no way to specify a global scope for expressions

using EvaluateExpression from lldb::SBTarget but this would most likely use the selected frame scope.

[da-viper]

fixed in #162845