Remote vs Extended-Remote Configuration Inconsistent

[brownts]

The current way to configure the gdbserver "remote" debugging is to:

• set the remote boolean to true
• add necessary information in the target field (e.g., 1.2.3.4:6789).

•       "request": "attach",
        "executable": "./bin/executable",
        "target": "1.2.3.4:6789",
        "cwd": "${workspaceRoot}",
        "remote": true
  
  

The current way to configure the gdbserver "extended remote" debugging is to:

• set the remote boolean to false (or not at all)...which seems counter-intuitive
• add necessary information in the target field including extended-remote (e.g., extended-remote 1.2.3.4:6789)

•       "request": "attach",
        "executable": "./bin/executable",
        "target": "extended-remote 1.2.3.4:6789",
        "cwd": "${workspaceRoot}",
        "remote": false
  
  

From a user perspective, it seems odd that for extended-remote debugging, you would not set the remote boolean. Additionally, having the target commands be different (where one requires the extended-remote, but the other doesn't require the remote) 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 a remote prefix in the target 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 via MI2::attach, whereas the regular remote is processed via MI2::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>).

[GitMensch]

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...

[GitMensch]

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).

[GitMensch]

@WebFreak001 Are you ok with changes here?

[GitMensch]

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.

[brownts]

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.

[WebFreak001]

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)

[brownts]

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.

[GitMensch]

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.