[Lldb-commits] [lldb] [lldb-dap] Allow providing debug adapter arguments in the extension (PR #129262)

[Adrian Vogelsgesang via lldb-commits]

================
@@ -137,53 +157,59 @@ export class LLDBDapDescriptorFactory
 
     const dbgOptions = {
       env: {
-        ...executable?.options?.env,
         ...configEnvironment,
         ...env,
       },
     };
-    const dbgArgs = executable?.args ?? [];
+    const dbgArgs = getDAPArguments(session);
 
-    const serverMode = config.get<boolean>('serverMode', false);
+    const serverMode = config.get<boolean>("serverMode", false);
     if (serverMode) {
-      const { host, port } = await this.startServer(dapPath, dbgArgs, dbgOptions);
+      const { host, port } = await this.startServer(
+        dapPath,
+        dbgArgs,
+        dbgOptions,
+      );
       return new vscode.DebugAdapterServer(port, host);
     }
 
     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;
+  startServer(
+    dapPath: string,
+    args: string[],
+    options: child_process.CommonSpawnOptions,
+  ): Promise<{ host: string; port: number }> {
+    if (this.server) {
+      return this.server;
+    }
----------------
vogelsgesang wrote:

This does not interact correctly with the new `session.configuration.debugAdapterArgs` settings. We might be reusing a lldb-dap instance which was launched with different arguments, effectively ignoring the `debugAdapterArgs` specified in the launch config.

We already have the same issue with the `dapPath`, where we might end up reusing the wrong lldb-dap binary (see my other comment in https://github.com/llvm/llvm-project/pull/128957#discussion_r1976226941).

But I think this commit is unfortunately further into the direction of incorrectly caching the server processes. Not sure how we want to proceed here...

WDYT, @ashgti and @JDevlieghere ? Is it fine merging this PR as is? Or should we first fix our logic on when to reuse the server and when not?

https://github.com/llvm/llvm-project/pull/129262