Skip to content
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

Add support for using extended-remote with launch configurations #195

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

esben
Copy link

@esben esben commented Nov 21, 2019

This extends gdb launch configurations to support extended-remote.
It can be used for remote debugging where code is compiled on host, and
transferred and debugged on target using a remote gdbserver (or something
else speaking GDB/MI).

Note, this is not touching the code added in commit
318ece4 ("Added special commands for extended-remote fix #91"),
as that is only related to gdb attach configuration, although the
example in package.json is confusingly put with launch, which is
opposite of the code change.
I did not fixup that change, as I am not sure exactly what to put
instead.

@esben
Copy link
Author

esben commented Nov 21, 2019

This is my first Node.js code ever, so please bear with me.
I did not manage to run the test code, so I have no idea if I needed to do any changes in that.

@esben esben force-pushed the extended-remote-launch branch 2 times, most recently from 2921f7a to bca7a61 Compare November 22, 2019 07:13
@esben
Copy link
Author

esben commented Nov 22, 2019

Changed string quoting to backticks for better readability and rebased to master.
Figured out how to run the test code :)

Copy link
Owner

@WebFreak001 WebFreak001 left a comment

Choose a reason for hiding this comment

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

There is support for extended-remote already. See https://github.com/WebFreak001/code-debug/blob/master/package.json#L428 and #91

So I would remove the remote & executable arguments from LaunchRequestArgument again and instead write an example configuration using the existing syntax.

For remote debugging copying the executable and changing it is a great change though. It would be great if this was done in the attach function in mi2.ts line 243. Best to add a "copyExecutable" boolean value for this so it doesn't change behavior of previous configurations and possibly overrides some files on the remote system.

@esben
Copy link
Author

esben commented Nov 23, 2019

Please take a look at 318ece4.
It adds support for extended-remote for AttachRequest, although confusingly, it shows it for launch in package.json, which is clearly an error.
This PR adds support for extended-remote in LaunchRequest, without making any changes to the existing support for extended-remote in AttachRequest.

Why do you want to have this in attach instead of launch? Logically (and practically), attach is for attaching to an existing process, and launch is for starting a new process and debugging that. What I do here is copying the executable to the remote, launching it and debugging it from there. I believe doing this as an AttachRequest will be confusing, and also complicate the code, as attach will then have to do the same as launch already does.

@WebFreak001
Copy link
Owner

I see, launch does make sense for actually starting the application, but it feels like it could be confusing adding an additional "executable" field to the launch settings. Connecting to a remote and executing the launch commands there also feels a little bit like it's not actually launching. Using attach will also show a stop button with some kind of cable icon in the stop button which I think fits extended remote more. Displaying it with the attach UI also implies that it is not running locally or that the extension is not in direct control over the executable (as the remote is), which would be a little bit more clear I think.

Adding the file copy commands, which are a great addition for remote debugging, to the attach code instead would make the code changes extremely minimal. See the isExtendedRemote code in mi2.ts for this which would just need the changed commands. Attach also already has separate target and executable properties in the JSON schema, which would easily be usable. I think it would just be best to add an optional "copy files to remote" boolean to this.

WebFreak001 added a commit that referenced this pull request Nov 23, 2019
@esben
Copy link
Author

esben commented Nov 25, 2019

While I do agree with you that the executable argument added to
LauncRequestArguments is likely to cause some confusion, I belive
it is directly linked to the way that the target argument is
already dfined for launch and attach. The launch target argument
basically has the same meaning as the attach argument.

Looking forward, it might be best to fix this now, renaming the
launch target argument to executable, which allow for a much
clearer way to add support for extended-remote. A normal (local)
launch request would simply specify the executable, where as a
remote (extended-remote) launch would specify both executable and
a-new-to-be-named argument specifying the remote to connect to.
Logically, that argument should probably be named "remote", or
maybe better "extendedRemote".

I don't share your feeling that a remote launch does not feel
like an actually launch. Yes, seen from a network centric point
of view, extended-remote launch is an "attach" action, but from a
debug point of view, extended-remote launch is a "launch" action.

As for the code structure, handling extended-remote launch in
attachRequest() would mean that start() must be called
there (only when extended-remote launch style attachRequest is
done of-course). Implementing extended-remote launch in
launchRequest() is simply the least intrusive solution.

What should we do?

@WebFreak001
Copy link
Owner

I too think a new executable argument with backwards compatibility from target (and deprecation message) would be very nice for the user indeed.

With a new clear key for this I agree this change makes sense and would make a good addition.

However I think if we add the copy files commands, they should also be added to attach for consistency because that does seem like a very useful command for debugging.

For a plan I would now suggest:

  • add executable value in launch, deprecate old target value, add new key for possible other functionality
  • add new attributes needed for this PR
  • add the copy commands to attach (under a configuration flag)

Also please add the new configuration attributes to the JSON schema of the GDB section and change the keys in LLDB and Mago-MI if you rename executable. See https://json-schema.org/

@esben
Copy link
Author

esben commented Nov 25, 2019

Should we leave the attach arguments as they are now? The target and remote arguments there are also kind of tricky/confusing.
Maybe deprecate the target argument in attach also, adding new arguments for pid and remote. Unfortunately, it would be really nice if they could be named "pid" and "remote", but the "remote" argument is already defined as a boolean.

@WebFreak001
Copy link
Owner

hm for attach I don't think "target" is such a bad name

As the target argument for launch configurations is an executable, let's name
it as such to avoid confusion.  The target argument is deprecated but kept for
backwards compatibility.
This extends gdb launch configurations to support extended-remote.
It can be used for remote debugging where code is compiled on host, and
transferred and debugged on target using a remote gdbserver (or something
else speaking GDB/MI).

Note, this is not touching the code added in commit
318ece4 ("Added special commands for extended-remote fix WebFreak001#91"),
as that is only related to gdb attach configuration.
@esben
Copy link
Author

esben commented Jan 7, 2020

Ok, I have updated PR with the changes regarding launch target/executable argument. I will not be adding generic copy commands in this PR. I believe it is better handled in another PR (and I am not sure how the configuration parameter for that should look like).

@ghost
Copy link

ghost commented Jul 13, 2020

Is there any progress in this PR? Can it be merged? Would be very helpful to have this feature.

@retif
Copy link

retif commented Jun 6, 2024

Removed previous comment.
I was able to use extended-debug argument in target parameter, it works.

@Smit-tay
Copy link

What is the status of this PR ?

I am facing a problem which I think might be fixed by this PR.
I wish to accomplish the equiv. to this series of steps, but within VSCode

Step 1 - copy the localApp from builder to worker
Console 1: cd /home/ME/dev/MYPROJ/build
Console 1: scp ./my_app ./lib/my_lib.so ME@worker:/home/ME/MYPROJ

Step 2 - log into worker and start gdbserver

Console 1: ssh ME@worker
Console 1: gdbserver --multi :1234

Console 1 now indicates:
Listening on port 1234

Step 2 - Launch gdb and prepare it to debug 

Console 2: gdb target extended-remote worker:1234
Remote debugging using worker:1234

Console 2: (gdb) file ./my_app
Reading symbols from ./my_app...

Console 2: (gdb) set remote exec-file /home/ME/MYPROJ/my_app
Console 2: (gdb) set solib-search-path ./lib
Console 2: (gdb) set args -a ARG1
Console 2: (gdb) break main
Breakpoint 1 at 0x4059b3: file /home/ME/dev/MYPROJ/example/my_app_main.cpp, line 388.
Console 2: (gdb) run
Starting program: /home/ME/dev/MYPROJ/build/my_app -a ARG1

Unfortunately attempts at making this work fail with errors similar to :

VSCode shows me a dialog box with the following message:

Unable to start debugging. Unexpected GDB output from the command "-target-select remote worker:1234". The target is not running (try extended-remote?)

There doesn't appear to be a way to use "extended-remote"

@GitMensch
Copy link
Collaborator

@Smit-tay wrote:

There doesn't appear to be a way to use "extended-remote"

please have a look at #330 which includes samples how to do that and has the discussion about how this should be adjusted

@Smit-tay
Copy link

Smit-tay commented Sep 14, 2024 via email

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

Successfully merging this pull request may close these issues.

Enhancement for extended-remote target
5 participants