[lldb-dap] Added "port" property to vscode "attach" command. #91570

santhoshe447 · 2024-05-09T08:22:56Z

Adding a "port" property to the VsCode "attach" command likely extends the functionality of the debugger configuration to allow attaching to a process using PID or PORT number.
Currently, the "Attach" configuration lets the user specify a pid. We tell the user to use the attachCommands property to run "gdb-remote ".
Followed the below conditions for "attach" command with "port" and "pid"
We should add a "port" property. If port is specified and pid is not, use that port to attach. If both port and pid are specified, return an error saying that the user can't specify both pid and port.

Ex - launch.json
{
"version": "0.2.0",
"configurations": [
{
"name": "lldb-dap Debug",
"type": "lldb-dap",
"request": "attach",
"gdb-remote-port":1234,
"program": "${workspaceFolder}/a.out",
"args": [],
"stopOnEntry": false,
"cwd": "${workspaceFolder}",
"env": [],

]
}

… summary.

Adding a "port" property to the VsCode "attach" command likely extends the functionality of the debugger configuratiuon to allow attaching to a process using PID or PORT number. Currently, the "Attach" configuration lets the user specify a pid. We tell the user to use the attachCommands property to run "gdb-remote <port>". Followed the below conditions for "attach" command with "port" and "pid" We should add a "port" property. If port is specified and pid is not, use that port to attach. If both port and pid are specified, return an error saying that the user can't specify both pid and port. Ex - launch.json { "version": "0.2.0", "configurations": [ { "name": "lldb-dap Debug", "type": "lldb-dap", "request": "attach", "port":1234, "program": "${workspaceFolder}/a.out", "args": [], "stopOnEntry": false, "cwd": "${workspaceFolder}", "env": [], } ] }

Adding a "port" property to the VsCode "attach" command likely extends the functionality of the debugger configuration to allow attaching to a process using PID or PORT number. Currently, the "Attach" configuration lets the user specify a pid. We tell the user to use the attachCommands property to run "gdb-remote ". Followed the below conditions for "attach" command with "port" and "pid" We should add a "port" property. If port is specified and pid is not, use that port to attach. If both port and pid are specified, return an error saying that the user can't specify both pid and port. Ex - launch.json { "version": "0.2.0", "configurations": [ { "name": "lldb-dap Debug", "type": "lldb-dap", "request": "attach", "port":1234, "program": "${workspaceFolder}/a.out", "args": [], "stopOnEntry": false, "cwd": "${workspaceFolder}", "env": [], } ] } In this patch we have resolved code formatting issues and fixed "test_by_name" failure.

github-actions · 2024-05-09T08:26:34Z

✅ With the latest revision this PR passed the Python code formatter.

Resolved code format issues.

lldb/packages/Python/lldbsuite/test/tools/lldb-dap/lldbdap_testcase.py

lldb/test/API/tools/lldb-dap/attach/TestDAP_attachByPortNum.py

lldb/tools/lldb-dap/lldb-dap.cpp

santhoshe447 · 2024-06-04T23:31:55Z

FWIW, I think this feature would be more useful and easier to implement if there was just a single setting called "connection url" or something, which accepted all of the usual lldb connection urls (unix-abstract-connect://, listen://, ...).

Since a simple tcp port connection is definitely going to be the most commonly used connection, you can, if you want, treat a simple number (or :number) as an alias to connect://localhost:number. lldb-server does something similar.

PS: I'm clicking request changes because I don't believe the test for the feature can go in in this form. I don't consider myself an owner for the rest, so you can consider my comments as suggestions.

I apologize for missing your earlier comment. To clarify.
Using the URL "connect://localhost:12345" or "connect://:12345" with the "connect" scheme helps invoke the appropriate function to establish a connection.
Pls ref: https://github.com/llvm/llvm-project/blob/main/lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp#L148
$gdb-remote number - would work from the command-line, because lldb is taking care of adding "connect://".

Please let me know if my understating is incorrect.

labath · 2024-06-05T08:10:35Z

FWIW, I think this feature would be more useful and easier to implement if there was just a single setting called "connection url" or something, which accepted all of the usual lldb connection urls (unix-abstract-connect://, listen://, ...).
Since a simple tcp port connection is definitely going to be the most commonly used connection, you can, if you want, treat a simple number (or :number) as an alias to connect://localhost:number. lldb-server does something similar.
PS: I'm clicking request changes because I don't believe the test for the feature can go in in this form. I don't consider myself an owner for the rest, so you can consider my comments as suggestions.

I apologize for missing your earlier comment. To clarify. Using the URL "connect://localhost:12345" or "connect://:12345" with the "connect" scheme helps invoke the appropriate function to establish a connection. Pls ref: https://github.com/llvm/llvm-project/blob/main/lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp#L148 $gdb-remote number - would work from the command-line, because lldb is taking care of adding "connect://".

Please let me know if my understating is incorrect.

Your understanding of the code is correct. What I am suggesting is basically to emulate this command line behavior in vscode.

Basically, instead of two host+port settings, you could have one setting with a sufficiently vague name ("connection url", "remote address", ...) to cover the expanded scope. Then, you implementation could take the user-provided string and fill in the blanks:

if the user types "1234", you'd convert it to connect://localhost:1234
if the user types "my-host:1234", you'd convert it to connect://localhost:1234
if the user types "unix-abstract-connect://..." (or any other scheme supported by lldb), you'd pass the value unmodified

The advantage of this is that you'd automatically support every connection protocol that lldb understands with almost no effort. If we stick to the current implementation, then if someone later needs to connect through unix sockets (or whatever), he would have to go back and add another setting to support connecting through that.

Now, I do see the appeal of having an explicit host/port fields (they're much easier to understand for the average user), which is why I'm not insisting on this approach. Nonetheless, I think this is an option worth considering, and I think we could avoid most of the confusion by carefully crafting the description of this field to guide the user to the most common scenarios.

lldb/test/API/tools/lldb-dap/attach/TestDAP_attachByPortNum.py

clayborg

The code looks good, a few nits in comments.

@labath: did we resolve the port race condition testing issue? We really don't want this to be a flaky test on overloaded systems. It would be nice to make sure this is solid before getting this in.

clayborg · 2024-06-07T20:56:56Z

lldb/tools/lldb-dap/lldb-dap.cpp

+  const auto gdb_remote_port =
+      GetUnsigned(arguments, "gdb-remote-port", invalid_port);
+  llvm::StringRef gdb_remote_hostname =
+      GetString(arguments, "gdb-remote-hostname", "localhost");


I know in the past we have used 127.0.0.1 instead of localhost. The reason is people can modify their config files in their environment and localhost can be aliased to something else. Not sure if this is an issue

Using 127.0.0.1 can provide slight advantage in terms of consistency and compatibility in specific scenarios.
Any suggestions from others?

There are environments (Google's (default) test environment is like that) that don't have an IPv4 stack configured (just v6), so localhost would be an advantage there. That said, this definitely wouldn't be the first instance of `127.0.0.1 in lldb.

FWIW, I'm of the opinion that anyone who configures localhost to point to something other than their local host deserves whatever is coming their way,

lldb/tools/lldb-dap/lldb-dap.cpp

labath · 2024-06-10T07:09:25Z

The code looks good, a few nits in comments.

@labath: did we resolve the port race condition testing issue? We really don't want this to be a flaky test on overloaded systems. It would be nice to make sure this is solid before getting this in.

The current version of the code should be fine. We just need to sort out the code placement issue.

…review comments.

…d review comments.

I have shifted "Pipe" class to common location "lldb/packages/Python/lldbsuite/test/tools/lldb-server/lldbgdbserverutils.py"

labath

My concerns have been addressed (*), so I'm removing my request for changes. It looks like the only way to do that is to Approve the PR, but I'd suggest getting another approval from a lldb-dap owner as well.

(*) There is still the question of the single "url" setting (to rule them all), but I don't consider myself an authority there.

labath · 2024-06-11T08:23:31Z

lldb/tools/lldb-dap/lldb-dap.cpp

+  const auto gdb_remote_port =
+      GetUnsigned(arguments, "gdb-remote-port", invalid_port);
+  llvm::StringRef gdb_remote_hostname =
+      GetString(arguments, "gdb-remote-hostname", "localhost");


There are environments (Google's (default) test environment is like that) that don't have an IPv4 stack configured (just v6), so localhost would be an advantage there. That said, this definitely wouldn't be the first instance of `127.0.0.1 in lldb.

FWIW, I'm of the opinion that anyone who configures localhost to point to something other than their local host deserves whatever is coming their way,

walter-erquinigo · 2024-06-11T12:42:12Z

I'll review this today or tomorrow. Thanks for all the activity!

walter-erquinigo

This LGTM

omjavaid · 2024-06-29T12:31:19Z

This breaks lldb-api :: tools/lldb-server/commandline/TestGdbRemoteConnection.py on lldb-aarch64-windows bot.

https://lab.llvm.org/buildbot/#/builders/141/builds/376

tedwoodward · 2024-06-29T15:41:40Z

This breaks lldb-api :: tools/lldb-server/commandline/TestGdbRemoteConnection.py on lldb-aarch64-windows bot.

https://lab.llvm.org/buildbot/#/builders/141/builds/376

File "C:\Users\tcwg\llvm-worker\lldb-aarch64-windows\llvm-project\lldb\packages\Python\lldbsuite\test\tools\lldb-server\lldbgdbserverutils.py", line 1045, in init
self.name = "lldb-" + str(random.randrange(1e10))
^^^^^^
NameError: name 'random' is not defined

Looks like "import random" should have been added to lldbgdbserverutils.py when the pipe class was moved from TestGDBRemoteConnection.py.

This fixes TestGdbRemoteConnection.py failing after PR #91570 on AArch64 Windows LLDB buildbot. https://lab.llvm.org/buildbot/#/builders/141/builds/376

) Adding a "port" property to the VsCode "attach" command likely extends the functionality of the debugger configuration to allow attaching to a process using PID or PORT number. Currently, the "Attach" configuration lets the user specify a pid. We tell the user to use the attachCommands property to run "gdb-remote ". Followed the below conditions for "attach" command with "port" and "pid" We should add a "port" property. If port is specified and pid is not, use that port to attach. If both port and pid are specified, return an error saying that the user can't specify both pid and port. Ex - launch.json { "version": "0.2.0", "configurations": [ { "name": "lldb-dap Debug", "type": "lldb-dap", "request": "attach", "gdb-remote-port":1234, "program": "${workspaceFolder}/a.out", "args": [], "stopOnEntry": false, "cwd": "${workspaceFolder}", "env": [], } ] } --------- Co-authored-by: Santhosh Kumar Ellendula <sellendu@hu-sellendu-hyd.qualcomm.com> Co-authored-by: Santhosh Kumar Ellendula <sellendu@hu-sellendu-lv.qualcomm.com>

This fixes TestGdbRemoteConnection.py failing after PR llvm#91570 on AArch64 Windows LLDB buildbot. https://lab.llvm.org/buildbot/#/builders/141/builds/376

Added "gdb-remote-port" and "gdb-remote-hostname" attach properties usage in README.md. This was missed in PR llvm#91570

Added "gdb-remote-port" and "gdb-remote-hostname" attach properties usage in README.md and updated package.json version to 0.2.3 This was missed in PR #91570 --------- Co-authored-by: Santhosh Kumar Ellendula <sellendu@hu-sellendu-hyd.qualcomm.com> Co-authored-by: Santhosh Kumar Ellendula <sellendu@hu-sellendu-lv.qualcomm.com>

…#99926) Added "gdb-remote-port" and "gdb-remote-hostname" attach properties usage in README.md and updated package.json version to 0.2.3 This was missed in PR llvm#91570 --------- Co-authored-by: Santhosh Kumar Ellendula <sellendu@hu-sellendu-hyd.qualcomm.com> Co-authored-by: Santhosh Kumar Ellendula <sellendu@hu-sellendu-lv.qualcomm.com> (cherry picked from commit 7345025)

Santhosh Kumar Ellendula and others added 19 commits November 17, 2023 15:09

[lldb][test] Add the ability to extract the variable value out of the…

960351c

… summary.

Merge branch 'llvm:main' into main

94c7680

Merge branch 'llvm:main' into main

3235090

Merge branch 'llvm:main' into main

ff9077f

Merge branch 'llvm:main' into main

fa762d2

Merge branch 'llvm:main' into main

09ff158

Merge branch 'llvm:main' into main

ef4f016

Merge branch 'llvm:main' into main

35ba7f7

Merge branch 'llvm:main' into main

bd38f88

Merge branch 'llvm:main' into main

f65a580

Merge branch 'llvm:main' into main

5763225

Merge branch 'llvm:main' into main

bb18d6d

Merge branch 'llvm:main' into main

1b1ae2b

Merge branch 'llvm:main' into main

a2e4580

Merge branch 'llvm:main' into main

3795dcd

Merge branch 'llvm:main' into main

628fa40

Merge branch 'llvm:main' into main

4774f58

santhoshe447 requested a review from JDevlieghere as a code owner May 9, 2024 08:22

Santhosh Kumar Ellendula and others added 4 commits May 9, 2024 01:46

[lldb-dap] Added "port" property to vscode "attach" command.

175f1d3

Resolved code format issues.

Merge branch 'llvm:main' into main

a6e9f66

Resolved code format issue.

e5c0aed

Resolved code format errors.

c502203

JDevlieghere requested review from walter-erquinigo and labath May 14, 2024 18:35

walter-erquinigo reviewed May 14, 2024

View reviewed changes

lldb/packages/Python/lldbsuite/test/tools/lldb-dap/lldbdap_testcase.py Outdated Show resolved Hide resolved

lldb/test/API/tools/lldb-dap/attach/TestDAP_attachByPortNum.py Outdated Show resolved Hide resolved

lldb/tools/lldb-dap/lldb-dap.cpp Outdated Show resolved Hide resolved

walter-erquinigo requested a review from clayborg May 14, 2024 21:36

Merge branch 'llvm:main' into main

89800a8

labath reviewed Jun 5, 2024

View reviewed changes

clayborg reviewed Jun 7, 2024

View reviewed changes

Merge branch 'llvm:main' into main

7d1319c

santhoshe447 requested a review from labath June 7, 2024 21:29

santhoshe447 and others added 4 commits June 10, 2024 14:55

Merge branch 'llvm:main' into main

2a0eb9d

[lldb-dap] Added "port" property to vscode "attach" command - fixed …

08f40aa

…review comments.

[lldb-dap] Added "port" property to vscode "attach" command - resolve…

2a0bc63

…d review comments.

[lldb-dap] Added "port" property to vscode "attach" command.

656c34a

I have shifted "Pipe" class to common location "lldb/packages/Python/lldbsuite/test/tools/lldb-server/lldbgdbserverutils.py"

labath removed their request for review June 11, 2024 08:20

labath approved these changes Jun 11, 2024

View reviewed changes

walter-erquinigo approved these changes Jun 17, 2024

View reviewed changes

tedwoodward merged commit a52be0c into llvm:main Jun 28, 2024
6 checks passed

omjavaid added a commit that referenced this pull request Jul 1, 2024

[LLDB] Fix TestGdbRemoteConnection.py on Windows

781ba3c

This fixes TestGdbRemoteConnection.py failing after PR #91570 on AArch64 Windows LLDB buildbot. https://lab.llvm.org/buildbot/#/builders/141/builds/376

santhoshe447 mentioned this pull request Jul 22, 2024

[lldb-dap] Updated README.md for newly added attach properties. #99926

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[lldb-dap] Added "port" property to vscode "attach" command. #91570

[lldb-dap] Added "port" property to vscode "attach" command. #91570

santhoshe447 commented May 9, 2024 •

edited

Loading

github-actions bot commented May 9, 2024 •

edited

Loading

santhoshe447 commented Jun 4, 2024

labath commented Jun 5, 2024

clayborg left a comment

clayborg Jun 7, 2024

santhoshe447 Jun 10, 2024

labath Jun 11, 2024

labath commented Jun 10, 2024

labath left a comment

labath Jun 11, 2024

walter-erquinigo commented Jun 11, 2024

walter-erquinigo left a comment

omjavaid commented Jun 29, 2024

tedwoodward commented Jun 29, 2024

[lldb-dap] Added "port" property to vscode "attach" command. #91570

[lldb-dap] Added "port" property to vscode "attach" command. #91570

Conversation

santhoshe447 commented May 9, 2024 • edited Loading

github-actions bot commented May 9, 2024 • edited Loading

santhoshe447 commented Jun 4, 2024

labath commented Jun 5, 2024

clayborg left a comment

Choose a reason for hiding this comment

clayborg Jun 7, 2024

Choose a reason for hiding this comment

santhoshe447 Jun 10, 2024

Choose a reason for hiding this comment

labath Jun 11, 2024

Choose a reason for hiding this comment

labath commented Jun 10, 2024

labath left a comment

Choose a reason for hiding this comment

labath Jun 11, 2024

Choose a reason for hiding this comment

walter-erquinigo commented Jun 11, 2024

walter-erquinigo left a comment

Choose a reason for hiding this comment

omjavaid commented Jun 29, 2024

tedwoodward commented Jun 29, 2024

santhoshe447 commented May 9, 2024 •

edited

Loading

github-actions bot commented May 9, 2024 •

edited

Loading