-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[lldb-dap] Adding server mode support to lldb-dap VSCode extension. #128957
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
e95459a
e5f1861
9cca26e
88095b9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -4,6 +4,8 @@ import * as vscode from "vscode"; | |||
| import * as child_process from "child_process"; | ||||
| import * as fs from "node:fs/promises"; | ||||
|
|
||||
| const exec = util.promisify(child_process.execFile); | ||||
|
|
||||
| export async function isExecutable(path: string): Promise<Boolean> { | ||||
| try { | ||||
| await fs.access(path, fs.constants.X_OK); | ||||
|
|
@@ -16,15 +18,14 @@ export async function isExecutable(path: string): Promise<Boolean> { | |||
| async function findWithXcrun(executable: string): Promise<string | undefined> { | ||||
| if (process.platform === "darwin") { | ||||
| try { | ||||
| const exec = util.promisify(child_process.execFile); | ||||
| let { stdout, stderr } = await exec("/usr/bin/xcrun", [ | ||||
| "-find", | ||||
| executable, | ||||
| ]); | ||||
| if (stdout) { | ||||
| return stdout.toString().trimEnd(); | ||||
| } | ||||
| } catch (error) {} | ||||
| } catch (error) { } | ||||
| } | ||||
| return undefined; | ||||
| } | ||||
|
|
@@ -97,8 +98,15 @@ async function getDAPExecutable( | |||
| * depending on the session configuration. | ||||
| */ | ||||
| export class LLDBDapDescriptorFactory | ||||
| implements vscode.DebugAdapterDescriptorFactory | ||||
| { | ||||
| implements vscode.DebugAdapterDescriptorFactory, vscode.Disposable { | ||||
| private server?: Promise<{ process: child_process.ChildProcess, host: string, port: number }>; | ||||
|
|
||||
| dispose() { | ||||
| this.server?.then(({ process }) => { | ||||
| process.kill(); | ||||
| }); | ||||
| } | ||||
|
|
||||
| async createDebugAdapterDescriptor( | ||||
| session: vscode.DebugSession, | ||||
| executable: vscode.DebugAdapterExecutable | undefined, | ||||
|
|
@@ -115,41 +123,71 @@ export class LLDBDapDescriptorFactory | |||
| } | ||||
| const configEnvironment = | ||||
| config.get<{ [key: string]: string }>("environment") || {}; | ||||
| const dapPath = await getDAPExecutable(session); | ||||
| const dapPath = (await getDAPExecutable(session)) ?? executable?.command; | ||||
|
|
||||
| if (!dapPath) { | ||||
| LLDBDapDescriptorFactory.showLLDBDapNotFoundMessage(); | ||||
| return undefined; | ||||
| } | ||||
|
|
||||
| if (!(await isExecutable(dapPath))) { | ||||
| LLDBDapDescriptorFactory.showLLDBDapNotFoundMessage(dapPath); | ||||
| return; | ||||
| } | ||||
|
|
||||
| const dbgOptions = { | ||||
| env: { | ||||
| ...executable?.options?.env, | ||||
| ...configEnvironment, | ||||
| ...env, | ||||
| }, | ||||
| }; | ||||
| if (dapPath) { | ||||
| if (!(await isExecutable(dapPath))) { | ||||
| LLDBDapDescriptorFactory.showLLDBDapNotFoundMessage(dapPath); | ||||
| return undefined; | ||||
| } | ||||
| return new vscode.DebugAdapterExecutable(dapPath, [], dbgOptions); | ||||
| } else if (executable) { | ||||
| if (!(await isExecutable(executable.command))) { | ||||
| LLDBDapDescriptorFactory.showLLDBDapNotFoundMessage(executable.command); | ||||
| return undefined; | ||||
| } | ||||
| return new vscode.DebugAdapterExecutable( | ||||
| executable.command, | ||||
| executable.args, | ||||
| dbgOptions, | ||||
| ); | ||||
| const dbgArgs = executable?.args ?? []; | ||||
|
|
||||
| const serverMode = config.get<boolean>('serverMode', false); | ||||
| if (serverMode) { | ||||
| const { host, port } = await this.startServer(dapPath, dbgArgs, dbgOptions); | ||||
| return new vscode.DebugAdapterServer(port, host); | ||||
| } | ||||
| return undefined; | ||||
|
|
||||
| return new vscode.DebugAdapterExecutable(dapPath, dbgArgs, dbgOptions); | ||||
| } | ||||
|
|
||||
| startServer(dapPath: string, args: string[], options: child_process.CommonSpawnOptions): Promise<{ host: string, port: number }> { | ||||
| if (this.server) return this.server; | ||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. afaict, this does not work correctly in combination with #126803. We might end up reusing the wrong lldb-dap binary Not sure what this means for us, though... We also can't just kill the old server when we receive a different lldb-dap path, because we could have two separate debugging sessions running at the same time, each one using a different server binary. I guess the most robust solution would be to use a
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think a simple solution could be to just ask the user if we can kill the running server or if they want to reuse the existing server. I don't expect that users will be changing lldb-dap paths often, I hope.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. discussion continued in #129262 (comment) |
||||
|
|
||||
| this.server = new Promise(resolve => { | ||||
| args.push( | ||||
| '--connection', | ||||
| 'connect://localhost:0' | ||||
| ); | ||||
| const server = child_process.spawn(dapPath, args, options); | ||||
| server.stdout!.setEncoding('utf8').once('data', (data: string) => { | ||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this actually "race-free" (for lack of a better term)? Looking at https://nodejs.org/api/stream.html#event-data I am not sure if we have a guarantee that the complete stdout content will be delivered in a single
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A single 'data' event usually corresponds to a single 'write' event which llvm-project/lldb/tools/lldb-dap/lldb-dap.cpp Line 298 in cef6dbb
|
||||
| const connection = /connection:\/\/\[([^\]]+)\]:(\d+)/.exec(data); | ||||
| if (connection) { | ||||
| const host = connection[1]; | ||||
| const port = Number(connection[2]); | ||||
| resolve({ process: server, host, port }); | ||||
| } | ||||
| }); | ||||
| server.on('exit', () => { | ||||
| this.server = undefined; | ||||
| }) | ||||
| }); | ||||
| return this.server; | ||||
| } | ||||
|
|
||||
| /** | ||||
| * Shows a message box when the debug adapter's path is not found | ||||
| */ | ||||
| static async showLLDBDapNotFoundMessage(path: string) { | ||||
| static async showLLDBDapNotFoundMessage(path?: string) { | ||||
| const message = | ||||
| path | ||||
| ? `Debug adapter path: ${path} is not a valid file.` | ||||
| : "Unable to find the path to the LLDB debug adapter executable."; | ||||
| const openSettingsAction = "Open Settings"; | ||||
| const callbackValue = await vscode.window.showErrorMessage( | ||||
| `Debug adapter path: ${path} is not a valid file`, | ||||
| message, | ||||
| openSettingsAction, | ||||
| ); | ||||
|
|
||||
|
|
||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,3 @@ | ||
| import * as path from "path"; | ||
| import * as util from "util"; | ||
| import * as vscode from "vscode"; | ||
|
|
||
| import { | ||
|
|
@@ -15,13 +13,14 @@ import { DisposableContext } from "./disposable-context"; | |
| export class LLDBDapExtension extends DisposableContext { | ||
| constructor() { | ||
| super(); | ||
| const factory = new LLDBDapDescriptorFactory(); | ||
| this.pushSubscription(factory); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What does this do?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I made the factory itself implement |
||
| this.pushSubscription( | ||
| vscode.debug.registerDebugAdapterDescriptorFactory( | ||
| "lldb-dap", | ||
| new LLDBDapDescriptorFactory(), | ||
| ), | ||
| factory, | ||
| ) | ||
| ); | ||
|
|
||
| this.pushSubscription( | ||
| vscode.workspace.onDidChangeConfiguration(async (event) => { | ||
| if (event.affectsConfiguration("lldb-dap.executable-path")) { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we have plans to enable this by default in the future? I think we should. In my experience, users rarely fiddle with the default settings, and to allow the widest possible user base to benefit from your great work, I think we should aim for making server-mode the default. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should have more testing of the setting first. I know the unit tests are failing on Windows right now and I'm not sure why at the moment. Right now this is fairly basic, I have #129283 filed to look into improving server management operations in the extension code.
We may be able to enable it on macOS and linux sooner than Windows, we'd just want to do some more testing before making it the default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 sounds good to me