[vscode-lldb] Support lldb-dap environment in debug configuration

[royitaqi]

Changes

1. Add a new debug configuration field called debugAdapterEnv. It accepts a string-valued dictionary or array.
   1. This input format is consistent with the (program's) env debug configuration.
2. In the adapter descriptor factory, honor the said field before looking at the VS Code settings Lldb-dap: Environment .
   1. This order is consistent with how things are done for other debug configuration fields, e.g. lldb-dap path and args.
3. In the lldb-dap server, note down the environment entries as a part of the adapter spawn info (now it becomes "path + args + env"), and prompt the user to restart server if such info has changed.

Motivation

1. Adapter environment can be set in launch.json.
2. Other debugger extensions can invoke the lldb-dap extension with adapter environment (via debug configuration).

Tests

Manually verified the following.

• [ v ] Test 1: Environment entries in settings are effective (no regress).
• [ v ] Test 2: Ill-formatted environment entries in launch.json are rejected.
• [ v ] Test 3: Environment entries in launch.json are effective.
• [ v ] Test 4: When debug configuration has environment entries (from launch.json), the ones in the settings are ignored.
• [ v ] Test 5: In server mode, changing environment entries will prompt the user to restart server.
• [ v ] Test 6: If the user choose to keep the existing server, environment entries shouldn't change.
• [ v ] Test 7: If the user choose to restart server, environment entries should reflect new value.

I have test videos as proof, but not sure if it's needed or where to upload.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: None (royitaqi)

Changes

Changes

1. Add a new debug configuration field called debugAdapterEnv, which accepts a string-valued dictionary.
2. In the adapter descriptor factory, honor the said field before looking at the VS Code settings Lldb-dap: Environment .
   1. This order is consistent with how things are done for other debug configuration fields, e.g. lldb-dap path and args.
3. In the lldb-dap server, note down the environment entries as a part of the dap spawn info (now it becomes "path + args + env"), and prompt the user to restart server if such info has changed.

Motivation

1. lldb-dap environment can be set in launch.json.
2. Other debugger extensions can invoke lldb-dap extension with lldb-dap environment (via debug configuration).

Tests

Manually verified the following.

• [ v ] Test 1: Environment entries in settings are effective (no regress)
• [ v ] Test 2: Bad formatted environment entries in launch.json are rejected: non-dict. E.g. array, string, true, 1
• [ v ] Test 3: Bad formatted environment entries in launch.json are rejected: non-string values in dict. E.g. true, 1, dict, array
• [ v ] Test 4: Environment entries in launch.json are effective
• [ v ] Test 5: When debug configuration has environment entries (from launch.json), the ones in the settings are ignored
• [ v ] Test 6: In server mode, changing environment entries will prompt the user to restart server
• [ v ] Test 7: If the user choose to keep the existing server, environment entries shouldn't change
• [ v ] Test 8: If the user choose to restart server, environment entries should reflect new value

I have a video as proof, but not sure if it's needed or where to upload.

---

Full diff: https://github.com/llvm/llvm-project/pull/153536.diff

3 Files Affected:

• (modified) lldb/tools/lldb-dap/package.json (+9)
• (modified) lldb/tools/lldb-dap/src-ts/debug-adapter-factory.ts (+40-2)
• (modified) lldb/tools/lldb-dap/src-ts/lldb-dap-server.ts (+22-4)

diff --git a/lldb/tools/lldb-dap/package.json b/lldb/tools/lldb-dap/package.json
index d677a81cc7974..3e6928cf4327f 100644
--- a/lldb/tools/lldb-dap/package.json
+++ b/lldb/tools/lldb-dap/package.json
@@ -370,6 +370,15 @@
                 },
                 "markdownDescription": "The list of additional arguments used to launch the debug adapter executable. Overrides any user or workspace settings."
               },
+              "debugAdapterEnv": {
+                "type": "object",
+                "patternProperties": {
+                  ".*": {
+                    "type": "string"
+                  }
+                },
+                "markdownDescription": "Additional environment variables to set when launching the debug adapter executable. E.g. `{ \"FOO\": \"1\" }`"
+              },
               "program": {
                 "type": "string",
                 "description": "Path to the program to debug."
diff --git a/lldb/tools/lldb-dap/src-ts/debug-adapter-factory.ts b/lldb/tools/lldb-dap/src-ts/debug-adapter-factory.ts
index 157aa2ac76a1f..e7bdd835894c0 100644
--- a/lldb/tools/lldb-dap/src-ts/debug-adapter-factory.ts
+++ b/lldb/tools/lldb-dap/src-ts/debug-adapter-factory.ts
@@ -157,6 +157,42 @@ async function getDAPArguments(
     .get<string[]>("arguments", []);
 }
 
+/**
+ * Retrieves the environment that will be provided to lldb-dap either from settings or the provided
+ * {@link vscode.DebugConfiguration}.
+ *
+ * @param workspaceFolder The {@link vscode.WorkspaceFolder} that the debug session will be launched within
+ * @param configuration The {@link vscode.DebugConfiguration} that will be launched
+ * @throws An {@link ErrorWithNotification} if something went wrong
+ * @returns The environment that will be provided to lldb-dap
+ */
+async function getDAPEnvironment(
+  workspaceFolder: vscode.WorkspaceFolder | undefined,
+  configuration: vscode.DebugConfiguration,
+): Promise<string[]> {
+  const debugConfigEnv = configuration.debugAdapterEnv;
+  if (debugConfigEnv) {
+    if (
+      Array.isArray(debugConfigEnv) ||
+      typeof debugConfigEnv !== "object" ||
+      Object.values(debugConfigEnv).findIndex(
+        (entry) => typeof entry !== "string",
+      ) !== -1
+    ) {
+      throw new ErrorWithNotification(
+        "The debugAdapterEnv property must be a dictionary with string values. Please update your launch configuration",
+        new ConfigureButton(),
+      );
+    }
+    return debugConfigEnv;
+  }
+
+  const config = vscode.workspace.workspaceFile
+    ? vscode.workspace.getConfiguration("lldb-dap")
+    : vscode.workspace.getConfiguration("lldb-dap", workspaceFolder);
+  return config.get<{ [key: string]: string }>("environment") || {};
+}
+
 /**
  * Creates a new {@link vscode.DebugAdapterExecutable} based on the provided workspace folder and
  * debug configuration. Assumes that the given debug configuration is for a local launch of lldb-dap.
@@ -186,8 +222,10 @@ export async function createDebugAdapterExecutable(
   ) {
     env["LLDBDAP_LOG"] = logFilePath.get(LogType.DEBUG_SESSION);
   }
-  const configEnvironment =
-    config.get<{ [key: string]: string }>("environment") || {};
+  const configEnvironment = await getDAPEnvironment(
+    workspaceFolder,
+    configuration,
+  );
   const dapPath = await getDAPExecutable(workspaceFolder, configuration);
 
   const dbgOptions = {
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 5f9d8efdcb3a3..7d37c695c5a0a 100644
--- a/lldb/tools/lldb-dap/src-ts/lldb-dap-server.ts
+++ b/lldb/tools/lldb-dap/src-ts/lldb-dap-server.ts
@@ -11,6 +11,7 @@ import * as vscode from "vscode";
 export class LLDBDapServer implements vscode.Disposable {
   private serverProcess?: child_process.ChildProcessWithoutNullStreams;
   private serverInfo?: Promise<{ host: string; port: number }>;
+  private serverSpawnInfo?: string[];
 
   constructor() {
     vscode.commands.registerCommand(
@@ -34,7 +35,7 @@ export class LLDBDapServer implements vscode.Disposable {
     options?: child_process.SpawnOptionsWithoutStdio,
   ): Promise<{ host: string; port: number } | undefined> {
     const dapArgs = [...args, "--connection", "listen://localhost:0"];
-    if (!(await this.shouldContinueStartup(dapPath, dapArgs))) {
+    if (!(await this.shouldContinueStartup(dapPath, dapArgs, options?.env))) {
       return undefined;
     }
 
@@ -70,6 +71,7 @@ export class LLDBDapServer implements vscode.Disposable {
         }
       });
       this.serverProcess = process;
+      this.serverSpawnInfo = this.getSpawnInfo(dapPath, dapArgs, options?.env);
     });
     return this.serverInfo;
   }
@@ -85,12 +87,14 @@ export class LLDBDapServer implements vscode.Disposable {
   private async shouldContinueStartup(
     dapPath: string,
     args: string[],
+    env: NodeJS.ProcessEnv | { [key: string]: string } | undefined,
   ): Promise<boolean> {
     if (!this.serverProcess || !this.serverInfo) {
       return true;
     }
 
-    if (isDeepStrictEqual(this.serverProcess.spawnargs, [dapPath, ...args])) {
+    const newSpawnInfo = this.getSpawnInfo(dapPath, args, env);
+    if (isDeepStrictEqual(this.serverSpawnInfo, newSpawnInfo)) {
       return true;
     }
 
@@ -102,11 +106,11 @@ export class LLDBDapServer implements vscode.Disposable {
 
 The previous lldb-dap server was started with:
 
-${this.serverProcess.spawnargs.join(" ")}
+${this.serverSpawnInfo.join(" ")}
 
 The new lldb-dap server will be started with:
 
-${dapPath} ${args.join(" ")}
+${newSpawnInfo.join(" ")}
 
 Restarting the server will interrupt any existing debug sessions and start a new server.`,
       },
@@ -143,4 +147,18 @@ Restarting the server will interrupt any existing debug sessions and start a new
       this.serverInfo = undefined;
     }
   }
+
+  getSpawnInfo(
+    path: string,
+    args: string[],
+    env: NodeJS.ProcessEnv | { [key: string]: string } | undefined,
+  ): string[] {
+    return [
+      path,
+      ...args,
+      ...Object.entries(env ?? {}).map(
+        (entry) => String(entry[0]) + "=" + String(entry[1]),
+      ),
+    ];
+  }
 }

[royitaqi]

@JDevlieghere / @walter-erquinigo: Gentle ping: Do you guys want to review before I merge, or later? (I know I can already merge with one approval. Just want to be a good citizen and check with you guys, at least for this time.)

[walter-erquinigo]

just a minor thing and then good to go

[walter-erquinigo]

move this to a helper function called validateEnv or something like that. Otherwise it's very hard to read.
Also, that regex is sadly very difficult to read, can you do a simpler check? It doesn't need to be perfect perfect

[royitaqi]

Will move to a helper function. I see no dedicated source file for util functions, so I'm thinking just put into the same file.

Re. the regex, I actually feel the other way around, that the condition here should be exactly as what's specified in the package.json, so as to avoid user confusion like "why this env is accepted by launch.json, but not when I use it in vscode.debug.startDebugging". Here I assume user experience is more important than development experience (of us). Happy to learn more your thoughts.

[royitaqi]

@walter-erquinigo I have moved the logic into a util function validateDAPEnv(). See latest commits
(0f91d37, 34a9980).

For the above, I have manually tested that it can catch the following cases:

  const ret1 = validateDAPEnv({
    999: 888,
  }); // false

  const ret2 = validateDAPEnv({
    "999": 888,
  }); // false

--

Waiting to learn more about your thoughts regarding the regex (see my prev comment).

[walter-erquinigo]

well, this looks good to me. Thanks!