Skip to content

Fix lldb-dap pickProcess command for selecting process for debugger attachment #75342

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Dodzey
Copy link

@Dodzey Dodzey commented Dec 13, 2023

Hi,

I see that the documentation for lldb-dap refers to a ${command.pickMyProcess} which can be used for interactive process selection within the VS Code IDE. It appears that this functionality no longer works, perhaps due to internal changes in VS Code.

I can get interactive process selection if I add the following snippet to the lldp-dap extension package.json:

"variables": {
	"pickProcess": "extension.pickNativeProcess"
},

and then use

"pid": "${command:pickProcess}",

within my launch.json configuration for lldb-dap.

However, referencing extension.pickNativeProcess here is only valid if the Microsoft VSCode CPPTools extension is installed & enabled, as that is the extension that registers a native process picker under the name 'extension.pickNativeProcess'.
I'm not sure if it's desirable behaviour for the lldb-dap extension to be dependent on the presence of the Microsoft CPPTools extension?

It appears (although I am not familiar with VSCode extension development) that any solution to this that was fully integrated in the lldp-dap extension would require typescript source to be added to the extension in order to register a private native process picker under a custom name - something like extension.lldb-dap.pickNativeProcess. I would assume that this implementation would look very similar, if not identical, to the command currently exposed by the Microsoft CPPTools extension.

In the PR I have included the minimal changes required to make the process picker work when the Microsoft VSCode CPPTools extension is also installed.

What are the recommendations on how we could proceed here?

Thanks!

@Dodzey Dodzey requested a review from JDevlieghere as a code owner December 13, 2023 14:14
Copy link

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be
notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write
permissions for the repository. In which case you can instead tag reviewers by
name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review
by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate
is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

@llvmbot llvmbot added the lldb label Dec 13, 2023
@llvmbot
Copy link
Member

llvmbot commented Dec 13, 2023

@llvm/pr-subscribers-lldb

Author: None (Dodzey)

Changes

Hi,

I see that the documentation for lldb-dap refers to a ${command.pickMyProcess} which can be used for interactive process selection within the VS Code IDE. It appears that this functionality no longer works, perhaps due to internal changes in VS Code.

I can get interactive process selection if I add the following snippet to the lldp-dap extension package.json:

"variables": {
	"pickProcess": "extension.pickNativeProcess"
},

However, referencing extension.pickNativeProcess here is only valid if the Microsoft VSCode CPPTools extension is installed, as that is the extension that registers a native process picker under the name 'extension.pickNativeProcess' I'm not sure if it's desirable behaviour for the lldb-dap extension to be dependent on the presence of the Microsoft CPPTools extension?

It appears (although I am not familiar with VSCode extension development) that any solution to this that was fully integrated in the lldp-dap extension would require typescript source to be added to the extension in order to register a private native process picker under a custom name - something like extension.lldb-dap.pickNativeProcess. I would assume that this implementation would look very similar, if not identical to the command currently exposed by the Microsoft CPPTools extension.

In the PR I have included the minimal changes required to seemingly make the process picker work when the Microsoft VSCode CPPTools extension is also installed.

What are the recommendations on how we could proceed here?

Thanks!


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

2 Files Affected:

  • (modified) lldb/tools/lldb-dap/README.md (+1-1)
  • (modified) lldb/tools/lldb-dap/package.json (+3)
diff --git a/lldb/tools/lldb-dap/README.md b/lldb/tools/lldb-dap/README.md
index 00ceb0bedc40a4..d341a136293a87 100644
--- a/lldb/tools/lldb-dap/README.md
+++ b/lldb/tools/lldb-dap/README.md
@@ -125,7 +125,7 @@ The JSON configuration file can contain the following `lldb-dap` specific launch
 |**type**           |string  |Y| Must be "lldb-dap".
 |**request**        |string  |Y| Must be "attach".
 |**program**        |string  | | Path to the executable to attach to. This value is optional but can help to resolve breakpoints prior the attaching to the program.
-|**pid**            |number  | | The process id of the process you wish to attach to. If **pid** is omitted, the debugger will attempt to attach to the program by finding a process whose file name matches the file name from **porgram**. Setting this value to `${command:pickMyProcess}` will allow interactive process selection in the IDE.
+|**pid**            |number  | | The process id of the process you wish to attach to. If **pid** is omitted, the debugger will attempt to attach to the program by finding a process whose file name matches the file name from **program**. Setting this value to `${command:pickProcess}` will allow interactive process selection in the IDE.
 |**stopOnEntry**    |boolean| | Whether to stop program immediately after launching.
 |**waitFor**        |boolean | | Wait for the process to launch.
 |**initCommands**   |[string]| | LLDB commands executed upon debugger startup prior to creating the LLDB target. Commands and command output will be sent to the debugger console when they are executed.
diff --git a/lldb/tools/lldb-dap/package.json b/lldb/tools/lldb-dap/package.json
index ebb1103d695e17..7b77b32f890a6f 100644
--- a/lldb/tools/lldb-dap/package.json
+++ b/lldb/tools/lldb-dap/package.json
@@ -124,6 +124,9 @@
 						"swift"
 					]
 				},
+				"variables": {
+					"pickProcess": "extension.pickNativeProcess"
+				},
 				"program": "./bin/lldb-dap",
 				"windows": {
 					"program": "./bin/lldb-dap.exe"

@DavidSpickett
Copy link
Collaborator

@clayborg and @walter-erquinigo are lldb-dap code owners and should be able to advise.

@walter-erquinigo
Copy link
Member

@Dodzey, thanks for submitting this PR.
@clayborg and I have recently been discussing the idea of implementing a minimal typescript extension for lldb-dap, which could host the kind of functionality that you are trying to fix.
I haven't had yet time to do it, but it's in my TODO list before the end of the year.
So, you could either wait for me do submit the minimal TS extension, or you could do it yourself if you want! Up to you :)

@Dodzey
Copy link
Author

Dodzey commented Dec 13, 2023

@walter-erquinigo
Good to hear that I'm on the right track. Would you want the minimal typescript extension to be a separate extension from the main lldb-dap one?, or would it be okay to have the typescript in the lldb-dap project/extension?
It looks to me from my inspection of how debuggers are handled in the MS CPP and node extensions etc that it might be possible to expose the commands for the 'lldb-dap' debugger type but have them reside in an entirely separate extension if this is desirable. It does make the distribution/manual installation steps for the extension a little more complicated though.

Thanks!

Copy link
Collaborator

@clayborg clayborg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me for now. We can coordinate on adding a native TypeScript layer after this, but no need to hold this up if this work!

Copy link
Member

@JDevlieghere JDevlieghere left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Since @Dodzey is a first-time contributor one of us needs to click the merge button. I'll leave that to @clayborg or @walter-erquinigo as the owners.

@Dodzey
Copy link
Author

Dodzey commented Dec 13, 2023

@clayborg Yes, it appears to. For context my workflow is C++ development, using recent versions of Clang and GCC. In VS Code I use the Microsoft CPPTools extension to allow for GDB debugging of GCC builds, with intellisense disabled. I am using clangd for auto-completion, inlay hints, automatic compiler errors/warning squiggles etc. That combination means that I have the Microsoft CPPTools extension enabled, which is enough for the 'workaround' here to be functional. I've tested launching the debugger twice within VSCode and using the process selection dropdown picker attaching to two copies of the same executable running at the same time. Both debuggers seem to be working ok and operating concurrently as you would expect.

Before this modification it wasn't possible to select two instances of the same executable, as there was no way of specifying the pids, other than hard coding in the launch.json. This made debugging MPI processes etc difficult.

I have another work in progress that adds fall back support for parsing the "pid" value as a string if the unsigned parsing fails. This allows use of the textual input boxes in VS Code - which allow you to input the pid manually - but it gets passed to the attach API as a string in the JSON object, not as unsigned, so it requires some small code changes to lldb-dap.cpp to enable that. I can put in a separate pull request for that once it is ready to get some opinions.

I'm happy to make an attempt at adding some minimal typescript elements to the extension in a future PR if I can get something working.

@Dodzey
Copy link
Author

Dodzey commented Dec 13, 2023

I've just checked one possible failure mode that came to mind. Do you get an error if you don't have the MS CPPTools extension enabled and you attempt to use a launch profile with a hardcoded pid (eg "pid": 1000) or no pid specified (so auto detecting a single running process), and that works okay. There is no error about missing extension.pickNativeProcess until you attempt to use ${command:pickProcess} as the value for pid.

@walter-erquinigo
Copy link
Member

Would you want the minimal typescript extension to be a separate extension from the main lldb-dap one?, or would it be okay to have the typescript in the lldb-dap project/extension?

It should be part of the lldb-dap folder/project and it should also be the default way to use lldb-dap. Using it directly as a binary would be discouraged but left as a possibility. The main idea behind this approach is that several companies have built their own TS wrappers for lldb-dap (at least Meta, Google and Modular), which includes features they haven't contributed back to the community. This is something I'd like to get fixed.

An additional architectural point for this extension is that it should be possible to copy/paste the TS files into another existing vscode extensions, and integrate it with possibly just one single call initLLDBDAP(settings).

@walter-erquinigo
Copy link
Member

@Dodzey , I think I'll add the minimal TS project tonight and you can review it. Then we can add a few additional features there are as follow ups

@walter-erquinigo
Copy link
Member

Here it is #75515!

@walter-erquinigo
Copy link
Member

@Dodzey, the typescript extension has been merged and, if you want, you can implement an integrated process picker for lldb-dap.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants