diff --git a/src/breakpointManager.ts b/src/breakpointManager.ts index 4cca440..a279941 100644 --- a/src/breakpointManager.ts +++ b/src/breakpointManager.ts @@ -52,8 +52,18 @@ interface ManagedBreakpoint extends BreakpointSpec { reported: boolean; /** The last DAP breakpoint sent to VS Code, to avoid sending unchanged ones. */ lastReported?: string; + /** The line last sent to VS Code, which VS Code uses as the breakpoint's line. */ + reportedLine?: number; + /** Disabled in the agent: VS Code left it out of the last request, but still has it. */ + disabled?: boolean; } +/** A breakpoint that VS Code left out of a set*Breakpoints request. */ +export type OmittedBreakpoint = + | { kind: "source"; path: string; lines: number[]; column?: number } + | { kind: "function"; name: string } + | { kind: "data"; address: bigint }; + export interface BreakpointHost { sendEvent(event: DebugProtocol.Event): void; /** Maps an address in a context to a source location on the host, if known. */ @@ -69,6 +79,11 @@ export interface BreakpointHost { processContexts(contextId: string): string[]; /** The context of instruction references without one. */ defaultContext(): string | undefined; + /** + * True if VS Code still has an omitted breakpoint, which is then disabled + * (or all breakpoints deactivated) rather than deleted. + */ + isDisabled?(bp: OmittedBreakpoint): boolean; } export interface BreakpointHits { @@ -89,9 +104,9 @@ const ACCESS_TYPES: Record = { /** * Keeps the TCF agent's breakpoint table in sync with the breakpoints in VS - * Code. VS Code always sends the full list of breakpoints of one kind (or one - * source file), which is diffed against what is planted to send Breakpoints - * add/change/remove commands. + * Code. VS Code always sends the full list of enabled breakpoints of one kind + * (or one source file), which is diffed against what is planted to send + * Breakpoints add/change/enable/disable/remove commands. */ export class BreakpointManager { private readonly idPrefix = `vscode-tcf-${randomUUID()}`; @@ -454,18 +469,23 @@ export class BreakpointManager { const toChange: ManagedBreakpoint[] = []; const toRemove: string[] = []; + const toEnable: ManagedBreakpoint[] = []; + const toDisable: ManagedBreakpoint[] = []; + for (const spec of specs) { const old = unused.get(spec.key); if (old) { unused.delete(spec.key); const propsChanged = JSON.stringify(old.props) !== JSON.stringify(spec.props); + const wasDisabled = old.disabled === true; Object.assign(old, spec); old.error = spec.error; old.logMessage = spec.logMessage; old.scopeContext = spec.scopeContext; - if (propsChanged) { + old.disabled = false; + if (propsChanged || wasDisabled) { if (old.tcfId && spec.props) { - toChange.push(old); + (propsChanged ? toChange : toEnable).push(old); } else if (old.tcfId) { toRemove.push(this.forget(old)); } else if (spec.props) { @@ -481,8 +501,21 @@ export class BreakpointManager { result.push(bp); } } + // Kept after the requested ones, which report() returns in request order. for (const bp of unused.values()) { - if (bp.tcfId) { + if (!bp.tcfId) { + continue; + } + const left = omitted(bp); + if (left && this.host.isDisabled?.(left)) { + if (!bp.disabled) { + bp.disabled = true; + toDisable.push(bp); + } + // VS Code no longer knows the DAP ID. + bp.reported = false; + result.push(bp); + } else { toRemove.push(this.forget(bp)); } } @@ -497,10 +530,36 @@ export class BreakpointManager { if (toRemove.length > 0) { await this.service.remove(toRemove).catch(() => undefined); } - await Promise.all([...toAdd.map((bp) => this.plant(bp, false)), ...toChange.map((bp) => this.plant(bp, true))]); + await Promise.all([ + this.setEnabled(toDisable, false), + this.setEnabled(toEnable, true), + ...toAdd.map((bp) => this.plant(bp, false)), + ...toChange.map((bp) => this.plant(bp, true)), + ]); return result; } + private async setEnabled(bps: ManagedBreakpoint[], enabled: boolean): Promise { + if (bps.length === 0) { + return; + } + const service = this.service!; + const ids = bps.map((bp) => bp.tcfId!); + try { + await (enabled ? service.enable(ids) : service.disable(ids)); + } catch { + await Promise.all(bps.map((bp) => this.plant(bp, true))); + return; + } + if (enabled) { + await Promise.all( + bps.map(async (bp) => { + bp.status = await service.getStatus(bp.tcfId!).catch(() => bp.status); + }), + ); + } + } + private async plant(bp: ManagedBreakpoint, change: boolean): Promise { const service = this.service!; if (!change) { @@ -509,11 +568,12 @@ export class BreakpointManager { } const id = bp.tcfId!; bp.status = undefined; + const props: BreakpointProperties = { ...bp.props, ID: id, ...(bp.disabled && { Enabled: false }) }; try { if (change) { - await service.change({ ...bp.props, ID: id }); + await service.change(props); } else { - await service.add({ ...bp.props, ID: id }); + await service.add(props); } } catch (err) { this.forget(bp); @@ -537,11 +597,13 @@ export class BreakpointManager { return id; } - private async report(bps: ManagedBreakpoint[]): Promise { + private async report(all: ManagedBreakpoint[]): Promise { + const bps = all.filter((bp) => !bp.disabled); const result = await Promise.all(bps.map((bp) => this.toDap(bp))); bps.forEach((bp, i) => { bp.reported = true; bp.lastReported = JSON.stringify(result[i]); + bp.reportedLine = result[i].line; }); return result; } @@ -552,6 +614,7 @@ export class BreakpointManager { const json = JSON.stringify(dap); if (bp.reported && json !== bp.lastReported) { bp.lastReported = json; + bp.reportedLine = dap.line; this.host.sendEvent(new BreakpointEvent("changed", dap)); } } @@ -646,6 +709,24 @@ export function parseHitCondition(hitCondition: string): number | undefined { return Math.max(Number(m[1]) - 1, 0); } +function omitted(bp: ManagedBreakpoint): OmittedBreakpoint | undefined { + switch (bp.kind) { + case "source": { + const lines = [bp.line, bp.reportedLine].filter((l): l is number => l !== undefined); + return { kind: "source", path: bp.source?.path ?? bp.source?.name ?? "", lines, column: bp.column }; + } + case "function": + return { kind: "function", name: bp.key }; + case "data": { + const target = parseDataId(bp.key.slice(0, bp.key.lastIndexOf("|"))); + return target && { kind: "data", address: target.address }; + } + default: + // VS Code does not show instruction breakpoints to extensions. + return undefined; + } +} + function parseDataId(dataId: string): { address: bigint; size: number } | undefined { const m = /^(0x[0-9a-f]+):(\d+)$/i.exec(dataId); return m ? { address: BigInt(m[1]), size: Number(m[2]) } : undefined; diff --git a/src/extension.ts b/src/extension.ts index ab5d0ce..8a1dcf8 100644 --- a/src/extension.ts +++ b/src/extension.ts @@ -2,6 +2,7 @@ import * as fs from "fs"; import * as path from "path"; import * as vscode from "vscode"; import { DebugProtocol } from "@vscode/debugprotocol"; +import { OmittedBreakpoint } from "./breakpointManager"; import { parseCallStackContext } from "./callStackFrame"; import { parseInstructionReference } from "./disassembly"; import { registerFunctionHeaderFileSystem } from "./functionHeaderFileSystem"; @@ -45,7 +46,7 @@ export function activate(context: vscode.ExtensionContext): void { vscode.debug.registerDebugConfigurationProvider(DEBUG_TYPE, new TcfConfigurationProvider()), vscode.debug.registerDebugAdapterDescriptorFactory(DEBUG_TYPE, { createDebugAdapterDescriptor: (session) => { - const adapter = new TcfDebugSession(sessionLog(tcfLog, session.name)); + const adapter = new TcfDebugSession(sessionLog(tcfLog, session.name), hasBreakpoint); adapters.set(session.id, adapter); return new vscode.DebugAdapterInlineImplementation(adapter); }, @@ -125,6 +126,52 @@ export function deactivate(): void { // Nothing to clean up, sessions are disposed by VS Code. } +/** + * True if VS Code still has a breakpoint, enabled or not, that it left out of + * a set*Breakpoints request. VS Code updates this list before it sends the + * request. + */ +function hasBreakpoint(omitted: OmittedBreakpoint): boolean { + return vscode.debug.breakpoints.some((bp) => { + switch (omitted.kind) { + case "source": { + if (!(bp instanceof vscode.SourceBreakpoint)) { + return false; + } + const { uri, range } = bp.location; + const file = uri.scheme === "file" ? uri.fsPath : uri.toString(); + const column = range.start.character + 1; + return ( + path.normalize(file) === path.normalize(omitted.path) && + omitted.lines.includes(range.start.line + 1) && + (omitted.column === undefined || column === 1 || column === omitted.column) + ); + } + case "function": + return bp instanceof vscode.FunctionBreakpoint && bp.functionName === omitted.name; + case "data": + return dataBreakpointAddress(bp) === omitted.address; + } + }); +} + +/** + * The address of a data breakpoint. vscode.DataBreakpoint is newer than the + * API version used here; its dataId is ours ("
:") or the + * address entered in VS Code. + */ +function dataBreakpointAddress(bp: vscode.Breakpoint): bigint | undefined { + const dataId: unknown = (bp as { dataId?: unknown }).dataId; + if (typeof dataId !== "string") { + return undefined; + } + try { + return BigInt(dataId.split(":")[0].trim()); + } catch { + return undefined; + } +} + /** * Copies the address of a Call Stack frame: the PC for the top frame, the * return address for callers. diff --git a/src/tcf/services/breakpoints.ts b/src/tcf/services/breakpoints.ts index 7d1da04..fe8780b 100644 --- a/src/tcf/services/breakpoints.ts +++ b/src/tcf/services/breakpoints.ts @@ -76,6 +76,16 @@ export class BreakpointsService { await this.channel.command(BreakpointsService.NAME, "change", bp); } + /** Sets only the Enabled property of the breakpoints to true. */ + async enable(ids: string[]): Promise { + await this.channel.command(BreakpointsService.NAME, "enable", ids); + } + + /** Sets only the Enabled property of the breakpoints to false. */ + async disable(ids: string[]): Promise { + await this.channel.command(BreakpointsService.NAME, "disable", ids); + } + async remove(ids: string[]): Promise { await this.channel.command(BreakpointsService.NAME, "remove", ids); } diff --git a/src/tcfDebugSession.ts b/src/tcfDebugSession.ts index b152e81..7b39777 100644 --- a/src/tcfDebugSession.ts +++ b/src/tcfDebugSession.ts @@ -36,7 +36,7 @@ import { } from "./disassembly"; import { FunctionInfo, FunctionRanges } from "./functionHeaders"; import { MemoryAccessMode, MemoryService } from "./tcf/services/memory"; -import { BreakpointHits, BreakpointManager } from "./breakpointManager"; +import { BreakpointHits, BreakpointManager, OmittedBreakpoint } from "./breakpointManager"; import { VariableNode, VariablesProvider } from "./variables"; import { PathMapper, PathMapRule, parsePathMapConfig } from "./pathMapper"; import { PathMapService } from "./tcf/services/pathMap"; @@ -220,7 +220,11 @@ export class TcfDebugSession extends DebugSession { private readonly regContextCache = new Map>(); private readonly regChildrenCache = new Map>(); - constructor(private readonly tcfLog?: TcfLog) { + constructor( + private readonly tcfLog?: TcfLog, + /** See BreakpointHost.isDisabled. */ + private readonly isBreakpointDisabled?: (bp: OmittedBreakpoint) => boolean, + ) { super(); this.setDebuggerLinesStartAt1(true); this.setDebuggerColumnsStartAt1(true); @@ -397,6 +401,7 @@ export class TcfDebugSession extends DebugSession { toTargetPath: (hostPath, contextId) => pathMapper.toTargetLocation(hostPath, contextId), processContexts: (ctxId) => this.processContexts(ctxId), defaultContext: () => this.suspendedThreadContext(), + isDisabled: (bp) => this.isBreakpointDisabled?.(bp) ?? false, }, ); diff --git a/src/test/breakpointManager.test.ts b/src/test/breakpointManager.test.ts index 1bf7b78..f3d993a 100644 --- a/src/test/breakpointManager.test.ts +++ b/src/test/breakpointManager.test.ts @@ -1,6 +1,6 @@ import { test } from "node:test"; import assert from "node:assert/strict"; -import { BreakpointHost, BreakpointManager, parseHitCondition } from "../breakpointManager"; +import { BreakpointHost, BreakpointManager, OmittedBreakpoint, parseHitCondition } from "../breakpointManager"; import { instructionReference } from "../disassembly"; import { formatFunctionBreakpointName, parseFunctionBreakpointName } from "../functionBreakpoint"; import { PathMapper } from "../pathMapper"; @@ -51,6 +51,12 @@ function fakeAgent(): FakeAgent { remove: async (ids: string[]) => { agent.commands.push(["remove", ids]); }, + enable: async (ids: string[]) => { + agent.commands.push(["enable", ids]); + }, + disable: async (ids: string[]) => { + agent.commands.push(["disable", ids]); + }, getStatus: async (): Promise => ({ Instances: [{ LocationContext: "thread1", Address: 0x1010 }], }), @@ -121,6 +127,88 @@ test("scoped function breakpoints never fall back to global planting", async () assert.equal(agent.commands.length, 0); }); +// VS Code leaves disabled breakpoints out of setBreakpoints, like deleted +// ones; only the host can tell them apart. +test("breakpoints that VS Code still has are disabled, not removed", async () => { + const agent = fakeAgent(); + const host = fakeHost(["thread1"]); + let kept = true; + const asked: OmittedBreakpoint[] = []; + host.isDisabled = (bp) => { + asked.push(bp); + return kept; + }; + const manager = new BreakpointManager(agent.service, host); + const source = { path: "/host/main.c" }; + const [before] = await manager.setSourceBreakpoints(source, [{ line: 12 }]); + const id = (agent.commands[0][1] as BreakpointProperties).ID; + + assert.deepEqual(await manager.setSourceBreakpoints(source, []), []); + assert.deepEqual(agent.commands[1], ["disable", [id]]); + assert.deepEqual(asked, [{ kind: "source", path: "/host/main.c", lines: [12, 12], column: undefined }]); + + const [after] = await manager.setSourceBreakpoints(source, [{ line: 12 }]); + assert.deepEqual(agent.commands[2], ["enable", [id]]); + assert.equal(after.id, before.id); + assert.equal(after.verified, true); + + await manager.setSourceBreakpoints(source, []); + kept = false; + await manager.setSourceBreakpoints(source, []); + assert.deepEqual(agent.commands.slice(3), [ + ["disable", [id]], + ["remove", [id]], + ]); +}); + +test("a disabled breakpoint stays disabled when it is changed", async () => { + const agent = fakeAgent(); + const host = fakeHost(["thread1"]); + let prefix = "/old"; + host.toTargetPath = async () => `${prefix}/main.c`; + host.isDisabled = () => true; + const manager = new BreakpointManager(agent.service, host); + const source = { path: "/host/main.c" }; + await manager.setSourceBreakpoints(source, [{ line: 12 }]); + await manager.setSourceBreakpoints(source, []); + prefix = "/new"; + await manager.updateSourcePaths(); + const [command, props] = agent.commands[2] as [string, BreakpointProperties]; + assert.equal(command, "change"); + assert.equal(props.File, "/new/main.c"); + assert.equal(props.Enabled, false); + assert.equal(host.events.length, 0); + + await manager.setSourceBreakpoints(source, [{ line: 12, condition: "x" }]); + const [again, enabled] = agent.commands[3] as [string, BreakpointProperties]; + assert.equal(again, "change"); + assert.equal(enabled.Enabled, true); + assert.equal(enabled.Condition, "x"); +}); + +test("function and data breakpoints are disabled too", async () => { + const agent = fakeAgent(); + const host = fakeHost(["thread1"]); + const asked: OmittedBreakpoint[] = []; + host.isDisabled = (bp) => { + asked.push(bp); + return true; + }; + const manager = new BreakpointManager(agent.service, host); + await manager.setFunctionBreakpoints([{ name: "main" }]); + await manager.setFunctionBreakpoints([]); + await manager.setDataBreakpoints([{ dataId: "0x1000:4", accessType: "write" }]); + await manager.setDataBreakpoints([]); + assert.deepEqual(asked, [ + { kind: "function", name: "main" }, + { kind: "data", address: 0x1000n }, + ]); + assert.deepEqual( + agent.commands.map(([c]) => c), + ["add", "disable", "add", "disable"], + ); +}); + test("path map edits remap planted source breakpoints without changing IDs", async () => { const agent = fakeAgent(); const host = fakeHost(["thread1"]);