[vscode-lldb] Restart server when the lldb-dap binary's modification time has changed #159481
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
royitaqi
requested review from
aperez,
clayborg,
kusmour and
walter-erquinigo
|
@llvm/pr-subscribers-lldb Author: Roy Shi (royitaqi) ChangesThis helps lldb-dap (the binary) development. For example, after rebuilding the lldb-dap binary, one probably wants to restart the server so that it will use the newly built binary instead of a stale one. With this patch, the modification time will be appended to the internal "spawn info" variable, which is displayed when prompting the user about restarting the server. See screenshot below. Full diff: https://github.com/llvm/llvm-project/pull/159481.diff 4 Files Affected:
diff --git a/lldb/tools/lldb-dap/.vscode/launch.json b/lldb/tools/lldb-dap/.vscode/launch.json
index 8241a5aca0354..524fc2dd5ce7b 100644
--- a/lldb/tools/lldb-dap/.vscode/launch.json
+++ b/lldb/tools/lldb-dap/.vscode/launch.json
@@ -14,11 +14,11 @@
],
"outFiles": [
"${workspaceFolder}/out/**/*.js"
- ],
- "preLaunchTask": {
- "type": "npm",
- "script": "watch"
- }
+ ]
+ // "preLaunchTask": {
+ // "type": "npm",
+ // "script": "watch"
+ // }
}
]
}
diff --git a/lldb/tools/lldb-dap/package-lock.json b/lldb/tools/lldb-dap/package-lock.json
index 26db1ce6df2fd..92143f7f37268 100644
--- a/lldb/tools/lldb-dap/package-lock.json
+++ b/lldb/tools/lldb-dap/package-lock.json
@@ -8,7 +8,11 @@
"name": "lldb-dap",
"version": "0.2.16",
"license": "Apache 2.0 License with LLVM exceptions",
+ "dependencies": {
+ "fs-extra": "^11.3.2"
+ },
"devDependencies": {
+ "@types/fs-extra": "^11.0.4",
"@types/node": "^18.19.41",
"@types/tabulator-tables": "^6.2.10",
"@types/vscode": "1.75.0",
@@ -835,6 +839,27 @@
"@jridgewell/sourcemap-codec": "^1.4.14"
}
},
+ "node_modules/@types/fs-extra": {
+ "version": "11.0.4",
+ "resolved": "https://registry.npmjs.org/@types/fs-extra/-/fs-extra-11.0.4.tgz",
+ "integrity": "sha512-yTbItCNreRooED33qjunPthRcSjERP1r4MqCZc7wv0u2sUkzTFp45tgUfS5+r7FrZPdmCCNflLhVSP/o+SemsQ==",
+ "dev": true,
+ "license": "MIT",
+ "dependencies": {
+ "@types/jsonfile": "*",
+ "@types/node": "*"
+ }
+ },
+ "node_modules/@types/jsonfile": {
+ "version": "6.1.4",
+ "resolved": "https://registry.npmjs.org/@types/jsonfile/-/jsonfile-6.1.4.tgz",
+ "integrity": "sha512-D5qGUYwjvnNNextdU59/+fI+spnwtTFmyQP0h+PfIOSkNfpU6AOICUOkm4i0OnSk+NyjdPJrxCDro0sJsWlRpQ==",
+ "dev": true,
+ "license": "MIT",
+ "dependencies": {
+ "@types/node": "*"
+ }
+ },
"node_modules/@types/node": {
"version": "18.19.75",
"resolved": "https://registry.npmjs.org/@types/node/-/node-18.19.75.tgz",
@@ -1748,6 +1773,20 @@
"dev": true,
"optional": true
},
+ "node_modules/fs-extra": {
+ "version": "11.3.2",
+ "resolved": "https://registry.npmjs.org/fs-extra/-/fs-extra-11.3.2.tgz",
+ "integrity": "sha512-Xr9F6z6up6Ws+NjzMCZc6WXg2YFRlrLP9NQDO3VQrWrfiojdhS56TzueT88ze0uBdCTwEIhQ3ptnmKeWGFAe0A==",
+ "license": "MIT",
+ "dependencies": {
+ "graceful-fs": "^4.2.0",
+ "jsonfile": "^6.0.1",
+ "universalify": "^2.0.0"
+ },
+ "engines": {
+ "node": ">=14.14"
+ }
+ },
"node_modules/function-bind": {
"version": "1.1.2",
"resolved": "https://registry.npmjs.org/function-bind/-/function-bind-1.1.2.tgz",
@@ -1877,6 +1916,12 @@
"url": "https://github.com/sponsors/ljharb"
}
},
+ "node_modules/graceful-fs": {
+ "version": "4.2.11",
+ "resolved": "https://registry.npmjs.org/graceful-fs/-/graceful-fs-4.2.11.tgz",
+ "integrity": "sha512-RbJ5/jmFcNNCcDV5o9eTnBLJ/HszWV0P73bc+Ff4nS/rJj+YaS6IGyiOL0VoBYX+l1Wrl3k63h/KrH+nhJ0XvQ==",
+ "license": "ISC"
+ },
"node_modules/has-flag": {
"version": "3.0.0",
"resolved": "https://registry.npmjs.org/has-flag/-/has-flag-3.0.0.tgz",
@@ -2094,6 +2139,18 @@
"integrity": "sha512-gfFQZrcTc8CnKXp6Y4/CBT3fTc0OVuDofpre4aEeEpSBPV5X5v4+Vmx+8snU7RLPrNHPKSgLxGo9YuQzz20o+w==",
"dev": true
},
+ "node_modules/jsonfile": {
+ "version": "6.2.0",
+ "resolved": "https://registry.npmjs.org/jsonfile/-/jsonfile-6.2.0.tgz",
+ "integrity": "sha512-FGuPw30AdOIUTRMC2OMRtQV+jkVj2cfPqSeWXv1NEAJ1qZ5zb1X6z1mFhbfOB/iy3ssJCD+3KuZ8r8C3uVFlAg==",
+ "license": "MIT",
+ "dependencies": {
+ "universalify": "^2.0.0"
+ },
+ "optionalDependencies": {
+ "graceful-fs": "^4.1.6"
+ }
+ },
"node_modules/jsonwebtoken": {
"version": "9.0.2",
"resolved": "https://registry.npmjs.org/jsonwebtoken/-/jsonwebtoken-9.0.2.tgz",
@@ -3182,6 +3239,15 @@
"integrity": "sha512-JlCMO+ehdEIKqlFxk6IfVoAUVmgz7cU7zD/h9XZ0qzeosSHmUJVOzSQvvYSYWXkFXC+IfLKSIffhv0sVZup6pA==",
"dev": true
},
+ "node_modules/universalify": {
+ "version": "2.0.1",
+ "resolved": "https://registry.npmjs.org/universalify/-/universalify-2.0.1.tgz",
+ "integrity": "sha512-gptHNQghINnc/vTGIk0SOFGFNXw7JVrlRUtConJRlvaw6DuX0wO5Jeko9sWrMBhh+PsYAZ7oXAiOnf/UKogyiw==",
+ "license": "MIT",
+ "engines": {
+ "node": ">= 10.0.0"
+ }
+ },
"node_modules/url-join": {
"version": "4.0.1",
"resolved": "https://registry.npmjs.org/url-join/-/url-join-4.0.1.tgz",
diff --git a/lldb/tools/lldb-dap/package.json b/lldb/tools/lldb-dap/package.json
index 6566ba3bdee13..3633e292a6264 100644
--- a/lldb/tools/lldb-dap/package.json
+++ b/lldb/tools/lldb-dap/package.json
@@ -27,7 +27,11 @@
"categories": [
"Debuggers"
],
+ "dependencies": {
+ "fs-extra": "^11.0.4"
+ },
"devDependencies": {
+ "@types/fs-extra": "^11.0.4",
"@types/node": "^18.19.41",
"@types/tabulator-tables": "^6.2.10",
"@types/vscode": "1.75.0",
diff --git a/lldb/tools/lldb-dap/src-ts/lldb-dap-server.ts b/lldb/tools/lldb-dap/src-ts/lldb-dap-server.ts
index 774be50053a17..59dd8a7a6b250 100644
--- a/lldb/tools/lldb-dap/src-ts/lldb-dap-server.ts
+++ b/lldb/tools/lldb-dap/src-ts/lldb-dap-server.ts
@@ -1,3 +1,4 @@
+import * as fs from 'fs-extra';
import * as child_process from "node:child_process";
import { isDeepStrictEqual } from "util";
import * as vscode from "vscode";
@@ -54,7 +55,7 @@ export class LLDBDapServer implements vscode.Disposable {
return this.serverInfo;
}
- this.serverInfo = new Promise((resolve, reject) => {
+ this.serverInfo = new Promise(async (resolve, reject) => {
const process = child_process.spawn(dapPath, dapArgs, options);
process.on("error", (error) => {
reject(error);
@@ -82,7 +83,7 @@ export class LLDBDapServer implements vscode.Disposable {
}
});
this.serverProcess = process;
- this.serverSpawnInfo = this.getSpawnInfo(dapPath, dapArgs, options?.env);
+ this.serverSpawnInfo = await this.getSpawnInfo(dapPath, dapArgs, options?.env);
});
return this.serverInfo;
}
@@ -104,13 +105,13 @@ export class LLDBDapServer implements vscode.Disposable {
return true;
}
- const newSpawnInfo = this.getSpawnInfo(dapPath, args, env);
+ const newSpawnInfo = await this.getSpawnInfo(dapPath, args, env);
if (isDeepStrictEqual(this.serverSpawnInfo, newSpawnInfo)) {
return true;
}
const userInput = await vscode.window.showInformationMessage(
- "The arguments to lldb-dap have changed. Would you like to restart the server?",
+ "The lldb-dap binary and/or the arguments to it have changed. Would you like to restart the server?",
{
modal: true,
detail: `An existing lldb-dap server (${this.serverProcess.pid}) is running with different arguments.
@@ -131,8 +132,7 @@ Restarting the server will interrupt any existing debug sessions and start a new
switch (userInput) {
case "Restart":
this.serverProcess.kill();
- this.serverProcess = undefined;
- this.serverInfo = undefined;
+ this.cleanUp(this.serverProcess);
return true;
case "Use Existing":
return true;
@@ -149,27 +149,40 @@ Restarting the server will interrupt any existing debug sessions and start a new
this.cleanUp(this.serverProcess);
}
- cleanUp(process: child_process.ChildProcessWithoutNullStreams) {
+ private cleanUp(process: child_process.ChildProcessWithoutNullStreams) {
// If the following don't equal, then the fields have already been updated
// (either a new process has started, or the fields were already cleaned
// up), and so the cleanup should be skipped.
if (this.serverProcess === process) {
this.serverProcess = undefined;
this.serverInfo = undefined;
+ this.serverSpawnInfo = undefined;
}
}
- getSpawnInfo(
+ private async getSpawnInfo(
path: string,
args: string[],
env: NodeJS.ProcessEnv | { [key: string]: string } | undefined,
- ): string[] {
+ ): Promise<string[]> {
return [
path,
...args,
...Object.entries(env ?? {}).map(
(entry) => String(entry[0]) + "=" + String(entry[1]),
),
+ `(${await this.getFileModifiedTimestamp(path)})`,
];
}
+
+ private async getFileModifiedTimestamp(file: string): Promise<string | null> {
+ try {
+ if (!(await fs.pathExists(file))) {
+ return null;
+ }
+ return (await fs.promises.stat(file)).mtime.toLocaleString();
+ } catch (error) {
+ return null;
+ }
+ }
}
|
walter-erquinigo
requested changes
Sep 18, 2025
| "type": "npm", | ||
| "script": "watch" | ||
| } | ||
| ] |
There was a problem hiding this comment.
Thanks for the good catch. I appreciate it.
These are unintentionally done during debug.
Comment on lines
+177
to
+187
|
|
||
| private async getFileModifiedTimestamp(file: string): Promise<string | null> { | ||
| try { | ||
| if (!(await fs.pathExists(file))) { | ||
| return null; | ||
| } | ||
| return (await fs.promises.stat(file)).mtime.toLocaleString(); | ||
| } catch (error) { | ||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
You should use the builtin method for this: vscode.workspace.createFileSystemWatcher
that will be more reliable
There was a problem hiding this comment.
(The following is probably more for my learning.)
I did a Google search and it said this:
While vscode.createFileSystemWatcher is reliable, file system watching is not always perfect and can be affected by the operating system. According to the VS Code team, the OS may decide to drop file events at any time, so there is no 100% guarantee that an event will be detected.
It seems it's because the underlying fs.watch is "not always reliable".
--
FWIW, I have a local implementation using the createFileSystemWatcher but somehow it's not triggering when I touch the lldb-dap binary. Still trying to figure out why.
The code is very simple:
this.serverFileWatcher = vscode.workspace.createFileSystemWatcher(absoluteDapPath);
this.serverFileWatcher.onDidChange(() => {
this.serverFileChanged = true;
});
The callback is never executed when I do touch <absoluteDapPath>.
It seems there is a bunch of reasons where createFileSystemWatcher might not work.
--
What reliability issue did you have in mind about the "modification time" approach?
I might have been naive and missed something important. Currently, it appears to me that checking the file modification time is better in terms of simplicity (only depend on the FS supporting modification time), debuggability (user can verify the modification time), and consistency (as long as the modification time changed, it should be detected).
There was a problem hiding this comment.
Interesting. I used createFileSystemWatcher for a few extensions that are being shipped in the marketplace already and never got a complaint.
Can you share with me the correct link for a bunch of reasons and "not always reliable".? The links seem to take me to google and not actual pages where you got the info you mention
Also, could you share the snippet of your own local copy of createFileSystemWatcher?
What I believed so far is that createFileSystemWatcher is used internally by vscode to identify when files change, and because it's a crucial feature of the IDE, I assumed it was thoroughly tested in all platforms.
There was a problem hiding this comment.
I was wrong. What my extensions were using is https://www.npmjs.com/package/chokidar
Try using that one
|
Reverted the unintended changes in the "modification time" approach by 4b989d8. cc @walter-erquinigo, in case you think the "modification time" approach is okay to go after reading my last comment. -- Still trying to figure out why my local |
royitaqi
added a commit
that referenced
this pull request
Sep 23, 2025
# Motiviation This helps the development of the lldb-dap binary. For example, when testing a locally built lldb-dap binary, one probably wants to restart the server after each build in order to use the latest binary. Not doing so leads to confusion like "why the new lldb-dap isn't doing what I just changed?". This patch adds dependency to the `chokidar` package. The reason is that we need something to detect changes to the `lldb-dap` binary file and `chokidar` appears to be the most reliable package to do so. An alternative solution which doesn't require adding dependencies is discussed below (solution 1). # Two different solutions considered Solution 1: Restart server when lldb-dap binary's modification time changes. #159481 implements solution 1. Solution 2: Restart server when lldb-dap binary has changed (as detected by a file system watcher). This patch implements solution 2 (using `chokidar`). # This patch (solution 2) If the lldb-dap binary has changed, the next time the user start a debug session, a dialog box will show up and prompt the user to restart the server. Depend on what has changed, the dialog box will show different content (see below) * When both the lldb-dap binary and the arguments have changed: <img width="520" height="357" alt="diff_args_and_binary" src="https://github.com/%3Ca%20href="https://github.com/user-attachments/assets/6580e05f-05c3-4d80-8c1a-9c3abf279218">https://github.com/user-attachments/assets/6580e05f-05c3-4d80-8c1a-9c3abf279218" /> * When only the lldb-dap binary has changed: <img width="260" height="384" alt="diff_binary" src="https://github.com/%3Ca%20href="https://github.com/user-attachments/assets/933b8987-9c21-44f3-ad68-57b8eeb58525">https://github.com/user-attachments/assets/933b8987-9c21-44f3-ad68-57b8eeb58525" /> * When only the arguments have changed (existing): <img width="520" height="343" alt="diff_args" src="https://github.com/%3Ca%20href="https://github.com/user-attachments/assets/c0e6771c-ad32-4fb8-b5df-b34cce48ed1a">https://github.com/user-attachments/assets/c0e6771c-ad32-4fb8-b5df-b34cce48ed1a" />
llvm-sync bot
pushed a commit
to arm/arm-toolchain
that referenced
this pull request
Sep 23, 2025
…ged (#159797) # Motiviation This helps the development of the lldb-dap binary. For example, when testing a locally built lldb-dap binary, one probably wants to restart the server after each build in order to use the latest binary. Not doing so leads to confusion like "why the new lldb-dap isn't doing what I just changed?". This patch adds dependency to the `chokidar` package. The reason is that we need something to detect changes to the `lldb-dap` binary file and `chokidar` appears to be the most reliable package to do so. An alternative solution which doesn't require adding dependencies is discussed below (solution 1). # Two different solutions considered Solution 1: Restart server when lldb-dap binary's modification time changes. llvm/llvm-project#159481 implements solution 1. Solution 2: Restart server when lldb-dap binary has changed (as detected by a file system watcher). This patch implements solution 2 (using `chokidar`). # This patch (solution 2) If the lldb-dap binary has changed, the next time the user start a debug session, a dialog box will show up and prompt the user to restart the server. Depend on what has changed, the dialog box will show different content (see below) * When both the lldb-dap binary and the arguments have changed: <img width="520" height="357" alt="diff_args_and_binary" src="https://github.com/%3Ca%20href="https://github.com/user-attachments/assets/6580e05f-05c3-4d80-8c1a-9c3abf279218">https://github.com/user-attachments/assets/6580e05f-05c3-4d80-8c1a-9c3abf279218" /> * When only the lldb-dap binary has changed: <img width="260" height="384" alt="diff_binary" src="https://github.com/%3Ca%20href="https://github.com/user-attachments/assets/933b8987-9c21-44f3-ad68-57b8eeb58525">https://github.com/user-attachments/assets/933b8987-9c21-44f3-ad68-57b8eeb58525" /> * When only the arguments have changed (existing): <img width="520" height="343" alt="diff_args" src="https://github.com/%3Ca%20href="https://github.com/user-attachments/assets/c0e6771c-ad32-4fb8-b5df-b34cce48ed1a">https://github.com/user-attachments/assets/c0e6771c-ad32-4fb8-b5df-b34cce48ed1a" />
JDevlieghere
pushed a commit
to swiftlang/llvm-project
that referenced
this pull request
Oct 14, 2025
…59797) # Motiviation This helps the development of the lldb-dap binary. For example, when testing a locally built lldb-dap binary, one probably wants to restart the server after each build in order to use the latest binary. Not doing so leads to confusion like "why the new lldb-dap isn't doing what I just changed?". This patch adds dependency to the `chokidar` package. The reason is that we need something to detect changes to the `lldb-dap` binary file and `chokidar` appears to be the most reliable package to do so. An alternative solution which doesn't require adding dependencies is discussed below (solution 1). # Two different solutions considered Solution 1: Restart server when lldb-dap binary's modification time changes. llvm#159481 implements solution 1. Solution 2: Restart server when lldb-dap binary has changed (as detected by a file system watcher). This patch implements solution 2 (using `chokidar`). # This patch (solution 2) If the lldb-dap binary has changed, the next time the user start a debug session, a dialog box will show up and prompt the user to restart the server. Depend on what has changed, the dialog box will show different content (see below) * When both the lldb-dap binary and the arguments have changed: <img width="520" height="357" alt="diff_args_and_binary" src="https://github.com/%3Ca%20href="https://github.com/user-attachments/assets/6580e05f-05c3-4d80-8c1a-9c3abf279218">https://github.com/user-attachments/assets/6580e05f-05c3-4d80-8c1a-9c3abf279218" /> * When only the lldb-dap binary has changed: <img width="260" height="384" alt="diff_binary" src="https://github.com/%3Ca%20href="https://github.com/user-attachments/assets/933b8987-9c21-44f3-ad68-57b8eeb58525">https://github.com/user-attachments/assets/933b8987-9c21-44f3-ad68-57b8eeb58525" /> * When only the arguments have changed (existing): <img width="520" height="343" alt="diff_args" src="https://github.com/%3Ca%20href="https://github.com/user-attachments/assets/c0e6771c-ad32-4fb8-b5df-b34cce48ed1a">https://github.com/user-attachments/assets/c0e6771c-ad32-4fb8-b5df-b34cce48ed1a" /> (cherry picked from commit 6007a4d)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
NOTE: This patch is discarded in favor of #159797.
Motiviation
This helps the development of the lldb-dap binary. For example, when testing a locally built lldb-dap binary, one probably wants to restart the server after each build in order to use the latest binary. Not doing so leads to confusion like "why the new lldb-dap isn't doing what I just changed?".
Two different solutions considered
Solution 1: Restart server when lldb-dap binary's modification time changes. This patch implements solution 1.
Solution 2: Restart server when lldb-dap binary has changed (as detected by a file system watcher). #159797 implements solution 2.
This patch (solution 1)
If the lldb-dap binary's modification time has changed, the next time the user start a debug session, the existing dialog box will show up and prompt the user to restart the server.
In that dialog box, the new modification time will be appended to the internal "spawn info" variable. It will then be compared to the existing "spawn info", and prompt for restart if they are different. The modification time is also displayed in the user prompt (see screenshot below).