Open brownts opened 2 years ago
I totally second that and would favor a PR.
Just one thing to keep in mind: extended-remote
may also "launch" a new process (you just need the executable and if wanted args after connecting, then use start
as common. If I remember correctly the current way to do that is to attach
with extended-remote server:port
then do the file and args setup along with start manually in autorun commands...
Related (but outdated): #195 - I'd like to see a clean launch also for extended-remote
, not sure if we really need more than one function on the debug implementation or if it wouldn't be enough to just pass a boolean / enum for the "types" (normal/remote/extended-remote) and (attach/launch).
I'm also not sure if this actually works but what should work is also ssh
to the gdb binary ("ssh-remote"), then extended-remote from there (either to the same or to another machine).
@WebFreak001 Are you ok with changes here?
The main part of this issue is a design one. While possible changes here should possibly be moved to the next version (#332 and at least inspecting #335 may come into the current one) it would be good to get feedback from @WebFreak001 about this issue.
Yes, this will be a larger change. I can see this as an opportunity to likely unify MI2::attach, MI2::connect and LLDB_MI2::attach into a single implementation. Furthermore, it appears that SSH only supports the "normal" launch and attach to PID. It doesn't appear to have any support for the remote versions of these. I don't see any reason that couldn't be addressed as well and further reduce the amount of code duplication while broadening the extension's capabilities.
I would agree that moving this to the next version makes sense due to the amount of changes involved. I'd really like to see a new official release of this extension so that users can benefit from the recent enhancements and fixes that have been made.
I agree the configuration would be cleaner after this change. Not sure about unifying the methods yet. The idea is great, but I don't know if they share enough code to be worth it. Also the LLDB attach one overrides as lldb-mi behaves differently from the GDB MI implementation.
Overriding usually is cleaner than having all the different cases in one big implementation. I would be for combining duplicated code into single functions, but where LLDB differs from GDB it might make more sense to have isolated functions which can be overridden. (e.g. extract the GDB-LLDB differing parts into separate functions which are called from the connect/attach method)
Also the LLDB attach one overrides as lldb-mi behaves differently from the GDB MI implementation.
The difference I see here is that lldb-mi
doesn't support supplying the PID on the command line, thus LLDB_MI2::attach uses the target-attach
command instead.
In order to fix #329, I was planning to make the same change in MI2::attach (and MI2::ssh) where attaching to a local PID is done via the target-attach
command instead of supplying the PID on the command line. This way, we can make sure the initialization commands are sent prior to attaching. Once that change is done, the attach to local PID would look the same between the GDB and LLDB implementations, which would both use target-attach
.
Additionally, lldb-mi
also supports the target-select remote
command, but this extension currently doesn't provide any capability to use it since the LLDB::attachRequest only uses LLDB_MI2::attach, which doesn't provide the same functionality that currently resides in MI2::connect (which is only called by the GDB::attachRequest). So the functionality for handling the "remote" connection that currently resides in MI2::connect should also be applicable to LLDB too.
I haven't looked at the "extended remote" case for LLDB, but there might be something there as well. However, with commonality between the "attach to local PID", and "attach to remote" being pretty much the same between LLDB and GDB, I believe these could be unified.
With regards to SSH, the LLDB::attachRequest currently doesn't support this...probably because MI2::ssh currently supplies the PID as part of the command line, but the proposed change for #329 would also update MI2::ssh to use the target-attach
approach, which should open up the ability to attach to a local PID over SSH with lldb-mi
.
One thing highlighted in #290: extended-remote may want to attach to a running process on the server. So far the only way to do this is to use autorun commands. I suggest to add a new field "pid" which can be used for this, in which case "target" would be left empty for "local PID" and set to the remote address for extended-remote.
OK !!!!
After a fair bit of experimentation, I have finally managed to get my scenario working. My particular requirements are as follows:
1.) Remote required to debug a variety of applications 2.) Remote therefore runs gdbserver in "--multi" mode, and is always running. (e.g. ME@remote:~$ gdbserver --multi :1234 ) no app or arguments provided 3.) Local builds and deploys the debug app and libraries to remote. 4.) Local gdb session attaches to remote and determines which app is debugged, and what arguments are required.
So, here is launch.json entry (for a particular application) which seems to work just fine.
I supply it here in the hopes that this helps anyone struggling in the way I did.
Please note, the comments must be removed, launch.json doesn't support them.
{
"configurations": [
{
"type": "gdb",
"request": "attach",
"name": "Attach and Debug MYAPP",
"executable": "./build/MYAPP",
"cwd": "${workspaceRoot}",
"target": "extended-remote worker:1234",
"remote": false,
"stopAtEntry": "main",
"autorun": [
"file ${workspaceRoot}/build/MYAPP", # unclear - isn't this "executable" ?
"set sysroot ./lib", # My libraries for MYAPP are built into this relative path on Local
"set remote exec-file /home/ME/PROJECT/MYAPP", # This is the full path on Remote to the deployed application
"set environment LD_LIBRARY_PATH /home/ME/PROJECT/MYAPP/libs", # Set library path on remote
"set solib-search-path ./lib:/lib64:/usr/lib:/usr/lib64", # Set library path on local
"set auto-solib-add on", # automatically load symbols for shared libraries
"set args /dev/TTYACM0", # Set arguments for the app to use
"break main", # Set initial breakpoint (this is upon entering main, but you could specify file and line)
"run", # Run the app - B U T ! ! !
"interrupt" # Immediately interrupt it, so that gdb and gdbserver have time to do *SOMETHING*
]
}
]
}
The current way to configure the gdbserver "remote" debugging is to:
remote
boolean totrue
target
field (e.g.,1.2.3.4:6789
).The current way to configure the gdbserver "extended remote" debugging is to:
remote
boolean tofalse
(or not at all)...which seems counter-intuitivetarget
field includingextended-remote
(e.g.,extended-remote 1.2.3.4:6789
)From a user perspective, it seems odd that for
extended-remote
debugging, you would not set theremote
boolean. Additionally, having thetarget
commands be different (where one requires theextended-remote
, but the other doesn't require theremote
) at the beginning is also inconsistent.It would seem that the
remote
boolean could go away (i.e., deprecate the option) and that the regular remote debugging could just be specified in the same way that the extended remote is, with aremote
prefix in thetarget
property. I think, at least from a user perspective, this would seem more consistent.Additionally, internally the paths seem to needlessly diverge since the
extended-remote
is processed viaMI2::attach
, whereas the regularremote
is processed viaMI2::connect
. It would seem the logic for both of these could be very similar internally.Furthermore, standardizing on this might better position the extension to support other capabilities in the future, such as loading a core file (i.e.,
target-select core <file>
).