Add a "type" field for SourceBreakpoints

[connor4312]

Splitting off from #445

The original authors requested the capability to present hard vs soft breakpoints.

Hardware breakpoints are useful in a variety of low-level and embedded computing scenarios. They are supported on major architectures and by debugging such as gdb (the hbreak family of commands).

Unlike software breakpoints, they are usually limited in number. I do not think we need any new way to express this limit, as DA's can already return a message explaining why a Breakpoint may not have been verified. This I propose the following additions:

interface Capabilities {
  // ... in addition to exisitng properties:
  /**
   * Types of breakpoints supported by the debug adapter. If present, the client
   * may allow the user to select a type and include it in its `setBreakpoints` request.
   */
  breakpointTypes?: BreakpointType[];
  // ...
}

// borrowing properties from the `ExceptionBreakpointsFilter`:
interface BreakpointType {
  /**
   * The internal ID of the breakpoint type. This value is passed to the
   * `setBreakpoints` request.
   */
  type: string;

  /**
   * The name of the breakpoint type. This is shown in the UI.
   */
  label: string;

  /**
   * A help text providing additional information about the breakpoint type.
   * This string is typically shown as a hover and can be translated.
   */
  description?: string;
}

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

  /**
   * The type of this breakpoint. This must be one of the `breakpointTypes` the
   * debug adapter advertised in its Capabilities.
   */
  type?: string;
}

[gregg-miskelly]

Questions:

1. 'Type' feels a bit overloaded since there is also 'breakpoint type' in the sense of 'File/Line' vs 'Function' vs 'Data', etc, as well as 'breakpoint type' in the sense of regular breakpoint vs log point. How about CustomBreakpointMode?
2. Don't we need to add 'Type' to all of the breakpoint objects? (FunctionBreakpoint, DataBreakpoint, InstructionBreakpoint)?

[connor4312]

1. Good point. I think renaming "type" to "mode" is good.
2. From cursory reading of the x86/arm docs, I don't think so, but others from the original thread may be able to weigh in. Instructions breakpoints may be set either as hardware or software breakpoints are both architectures, but data breakpoints are always hardware-based. GDB also only differentiates hardware and software when set on an instruction/location.

[gregg-miskelly]

Agreed that, for most implementations at least, we don't need 'hardware vs. software' for data breakpoints. But since this is for a custom extension point that doesn't need to be used for just hardware vs. software maybe we want it anyway? Or maybe we just wait for a scenario? I still think we want it for all the instruction breakpoint types.

[roblourens]

So if you provide breakpointTypes, does the default "Add Breakpoint" option go away, or are these in addition to that?

[connor4312]

I think we should suggest that clients take the first-presented mode as the "default" and offer alternative actions for the other modes. Adapters should also treat breakpoints without a mode (sent from clients who don't support breakpointModes) as being set at the first mode.

Update with these discussions:

interface Capabilities {
  // ... in addition to existing properties:
  /**
   * Modes of breakpoints supported by the debug adapter. If present, the client
   * may allow the user to select a mode and include it in its `setBreakpoints`
   * request. 
   * 
   * Clients may present the first mode in this array as the 'default' mode
   * in gestures that set breakpoints.
   */
  breakpointModes?: BreakpointMode[];
  // ...
}

// borrowing properties from the `ExceptionBreakpointsFilter`:
interface BreakpointMode {
  /**
   * The internal ID of the breakpoint mode. This value is passed to the
   * `setBreakpoints` request.
   */
  mode: string;

  /**
   * The name of the breakpoint mode. This is shown in the UI.
   */
  label: string;

  /**
   * A help text providing additional information about the breakpoint mode.
   * This string is typically shown as a hover and can be translated.
   */
  description?: string;
}

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

  /**
   * The mode of this breakpoint. If defined, this must be one of the
   * `breakpointModes` the debug adapter advertised in its Capabilities.
   */
  mode?: string;
}

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

  /**
   * The mode of this breakpoint. If defined, this must be one of the
   * `breakpointModes` the debug adapter advertised in its Capabilities.
   */
  mode?: string;
}

If we add this and later need modes to apply to different types of breakpoints, we could add some appliesTo: ('source' | 'instruction' | 'exception' | ...)[] property to the BreakpointMode.

[roblourens]

Seems reasonable to me. Definitely a niche usecase but this makes sense.

[kkistm]

As one of the authors of the original request, I would like to confirm that the proposition with the recent addition to have a default breakpoint mode to be the first one in the list looks fine for me.
What about to make the default mode to have an empty type by definition? And if there is any non-empty type for the default mode, it would be ignored by the clients?

[gregg-miskelly]

Rather than the first one in the list, why don't we make the default value as unspecified, and then the debug adapter is free to choose whatever is the appropriate default?

Example where I think this would be helpful: suppose we add mode to all breakpoint types, the debug adapter wants to have a mode for hardware vs software breakpoints, and suppose only hardware breakpoints are supported for data breakpoints. The debug adapter could then ignore mode for data breakpoints, but fail if the user explicitly tried to set the mode to software.

[connor4312]

What about to make the default mode to have an empty type by definition? And if there is any non-empty type for the default mode, it would be ignored by the clients? / Rather than the first one in the list, why don't we make the default value as unspecified, and then the debug adapter is free to choose whatever is the appropriate default?

It sounds like both of these are getting at the same point.

I think defining the default is useful for clients when implementing the feature. In VS Code, we would not want to open a picker when a user clicks in the glyph margin to add a breakpoint for a line, so we'd have that in the "default mode". But if we then have an option to "change breakpoint mode" for a given breakpoint, we would show a select box listing the modes, and would want to show the current mode as the default value in the box. If the default value is unspecified, would we just show "unknown" there? That seems like a poor experience.

That same problem is faced for other UI indicator of breakpoint mode such as tooltips, icons, or groupings.

Example where I think this would be helpful: suppose we add mode to all breakpoint types, the debug adapter wants to have a mode for hardware vs software breakpoints, and suppose only hardware breakpoints are supported for data breakpoints.

If this comes up I think that's a good use case for adding a appliesTo? property like I mentioned above -- that would avoid a failure state entirely.

[markgoodchild]

Hi there, thank you for looking into this.

When determining hardware breakpoints it is important to clarify the overall numbers available. In some IDE you see the available resources shown in a view somewhere. Will the debug adaptor have this kind of interface?

Some of the complexity with this feature comes back to how the IDE will handle it. So while the DAP interface is simple the feature is a bit less so. But that is not so important here.

For example users can set breakpoints when they are not connected to the hardware and it is not until the code is downloaded that the exact locations and numbers of breakpoints can be determined in some cases.
I think this problem is less to do with DAP and more to do with the IDE and how the underlying debugger handles these cases.

[connor4312]

When determining hardware breakpoints it is important to clarify the overall numbers available. In some IDE you see the available resources shown in a view somewhere. Will the debug adaptor have this kind of interface?

From my comment in the other issue: I think the easiest way to handle this in the existing protocol, without introducing a lot of complexity around counts, is returning further breakpoints as verified: false with a descriptive message once the limit gets hit.

[markgoodchild]

Thank you, I understand how it would work when trying to set a breakpoint.

But this method does not inform users how many resources they have available until they hit the limit. Probably not hyper important from a priority point of view but something which we have now in our tools.

[connor4312]

I'm not sure how/if I would implement it in VS Code since it's gnarly when we have multiple concurrent debug sessions/types, but I think the need is sensible since all hardware generally has a breakpoint limit. We could add some property for that for clients who can support it:

interface BreakpointMode {
  /**
   * An positive integer indicating the number of breakpoints of this mode the
   * adapter supports.
   */
  limit?: number;
// ...

...but this is sent before the "launch" request so I'm not sure if that's sufficient for your use case

[markgoodchild]

Thinking about it this also introduces the problem of multicore context as well. The facilities can differ depending on the core you are setting it on. I don't think that BreakpointMode interface is sufficiently complex.

[connor4312]

DAP does not have any representation of a breakpoint set only for a specific thread or core, so I'm not sure whether that would be in-scope for this change.

[connor4312]

For participants, I have adopted appliesTo and added a mode option for exception and data breakpoints in the PR discussion #460 (comment)