iframe-proxy

xisui-MSFT · 2022-01-21T22:30:16Z

As discussed in microsoft/vscode-cpptools#8497, this PR introduces a way to attach to a process after making remote connection.

A launch option field "useExtendedRemote" is added, which when being set to true, extended-remote mode will be used.

A known limitation is we can't guarantee the specified PID exists. We'll probably need to add a remote process picker in the future.

WardenGnaw · 2022-01-22T00:19:29Z

xisui-MSFT · 2022-01-22T00:38:29Z

Is switching from remote to extended-remote going to break the scenarios where users already have gdbserver already attached to a PID and users are expecting GDB to disconnect from it?

I tried a scenario where only miDebuggerServerAddress needs to be specified, and didn't notice any behavior change.

Also checked the log, MIEngine sends target detach, gdb-exit, done and exit to gdb at the end, and those completely cuts off gdb connection. On the other hand, I think gdb would stay connected if the debugee exits, so the user will need to press 'stop debugging' button again. I'll double check.

chuckries · 2022-01-22T00:43:54Z

@xisui-MSFT it sounds like you did some testing against this already, which gdbserver scenarios did you test exactly? A few questions:

is -target-select extended-remote supported in gdb-mi? I'm familliar with it as a CLI option, but not an MI option.
Have you tested lldbserver? Does it support extended-remote?
As I said in the code comments, extended-remote works when gdbserver is launched with --attach. What happens when we issues the -target-attach command if gdbserver is launched with --attach?

chuckries

This needs more testing:

Applicable Android and iOS scenarios.
lldbserver (@WardenGnaw do we have any known lldbserver scnearios?)
lldb in general as I'm not sure if we know if lldb-mi supports -target-select extended-remote
gdbserver when it is launched with --attach

chuckries · 2022-01-22T00:29:52Z

            }

-            string targetCommand = string.Format(CultureInfo.InvariantCulture, "-target-select remote localhost:{0}", _remotePorts.IDeviceDebugServerProxyPort.ToString(CultureInfo.InvariantCulture));
+            string targetCommand = string.Format(CultureInfo.InvariantCulture, "-target-select extended-remote localhost:{0}", _remotePorts.IDeviceDebugServerProxyPort.ToString(CultureInfo.InvariantCulture));


IIRC, iOS would use some form of lldbserver on the device. We have no idea if that supports extended-remote or not. We would need to test it against iOS scenarios, and those haven't been run in some time.

chuckries · 2022-01-22T00:31:21Z

                    if (!string.IsNullOrWhiteSpace(destination))
                    {
-                        commands.Add(new LaunchCommand("-target-select remote " + destination, string.Format(CultureInfo.CurrentCulture, ResourceStrings.ConnectingMessage, destination)));
+                        commands.Add(new LaunchCommand("-target-select extended-remote " + destination, string.Format(CultureInfo.CurrentCulture, ResourceStrings.ConnectingMessage, destination)));


I believe this would get used in android scenarios. We would need to test the android scenarios that we still support in order to ensure those versions of gdbserver support extended-remote.

chuckries · 2022-01-22T00:39:34Z

                    }
-                    else // gdbserver is already attached when using LocalLaunchOptions
+
+                    string processId = localLaunchOptions?.ProcessId.Value.ToString(CultureInfo.InvariantCulture);


dropping the else here seems problemeatic.

Just because gdb is connected to the remote with extended-remote doesn't mean the server is not already attached. i.e. the current code always expects gdbserver to be launched with --attach for an attach and then connect to it with -target-select remote. extended-remote can still be used in conjuction with gdbserver --attach. Therefore it is not correct to blindly issue an attach command just because we connected with extended-remote, we would need to know exactly how gdbserver was launched.

Tried this, but not sure how valid is this scenario.

For attach mode, a processId must be specified (so there isn't a case where midebuggerserveraddress is specified but processID isn't). And if gdbserver is lauched with --attach option, while both processId and midebuggerserveraddress are specified, attach will fail with message failed to attach to process ....

Therefore, launch mode should always be used for this case. And launch mode isn't compatible with processId.

So what do you think?

chuckries · 2022-01-22T00:40:32Z

                        if (!string.IsNullOrWhiteSpace(destination))
                        {
-                            commands.Add(new LaunchCommand("-target-select remote " + destination, string.Format(CultureInfo.CurrentCulture, ResourceStrings.ConnectingMessage, destination)));
+                            commands.Add(new LaunchCommand("-target-select extended-remote " + destination, string.Format(CultureInfo.CurrentCulture, ResourceStrings.ConnectingMessage, destination)));


same as previous, not sure if its safe to do this at all times.

xisui-MSFT · 2022-01-22T01:00:44Z

@xisui-MSFT it sounds like you did some testing against this already, which gdbserver scenarios did you test exactly? A few questions:

is -target-select extended-remote supported in gdb-mi? I'm familliar with it as a CLI option, but not an MI option.

Have you tested lldbserver? Does it support extended-remote?

As I said in the code comments, extended-remote works when gdbserver is launched with --attach. What happens when we issues the -target-attach command if gdbserver is launched with --attach?

I only tested with some basic gdb cases. Trying to answer your questions:

What is gdb-mi? Is it what's used by MIEngine? If so, then extended-remote is supported since I've verified this change works for our scenario.
Never tested with lldb.
That's a good question. I'll test this as well.

chuckries · 2022-01-24T18:24:41Z

@xisui-MSFT MI is GDB's "Machine Interface" (this is where the MI in MIEngine comes from). For most versions of gdb, we invoke it with --interpreter=mi and we are in MI mode instead of the standard GDB CLI. For LLDB, the interpreter flag is not available and typically MI is only available through another executable called lldb-mi. @WardenGnaw can speak more to lldb-mi. In my experience, the MI frontends do not always support everything the CLI frontends do, so we just need to test the use of new commands/parameters.

WardenGnaw · 2022-01-24T18:45:16Z

Because lldb-mi is under Apache License v2.0, we can look at the source which shows it does not support extended-remote.

  if (rRemoteType != "remote") {
    SetError(CMIUtilString::Format(MIRSRC(IDS_CMD_ERR_INVALID_TARGET_TYPE),
                                   m_cmdData.strMiCmd.c_str(),
                                   rRemoteType.c_str()));
    return MIstatus::failure;
  }

From the source

We would need to not change it to extended-remote when targeting lldb.

chuckries · 2022-01-24T18:51:40Z

@WardenGnaw are there any supported scenarios where we are not picking up the lldb-mi that we ship ourselves? one thing to check would be if

lldb + lldbserver supports extended-remote
if so, the mi frontend condition might just be superficial, and we could pass through extended-remote without issue.

gregg-miskelly · 2022-01-24T19:49:59Z

According to this the start of the documentation that was linked to (https://sourceware.org/gdb/onlinedocs/gdb/Connecting.html):

GDB supports two types of remote connections, target remote mode and target extended-remote mode. Note that many remote targets support only target remote mode

It sounds like we need a new option to indicate if extended-remote or remote should be used instead of always specifying extended-remote.

gregg-miskelly · 2022-01-24T19:54:09Z

I guess the other option is: only use extended-remote if we know we are in a scenario that requires it (like attach). But I think that is too magical.

xisui-MSFT · 2022-01-25T00:19:08Z

Looks like most discuss are around how compatible are remote and extended-remote, and lldb. While I didn't find a case where remote and extended-remote are incompatible, I agree that we should provide the option to use remote or extended remote. So I'll change this PR to add a useExtendedRemote option, and add some checks around lldb. Thanks for the advises!

xisui-MSFT · 2022-01-25T22:33:40Z

@WardenGnaw @chuckries @gregg-miskelly I've added an option "useExtendedRemote" to control when to use extended-remote mode, and modified the title and description of this PR. Can someone please re-review? Thanks!

gregg-miskelly

Otherwise LGTM

fbs2016 · 2022-06-22T11:28:35Z

Replace target remote with extended-remote

e58d0d0

WardenGnaw requested review from WardenGnaw, gregg-miskelly and paulmaybee January 22, 2022 00:18

WardenGnaw requested a review from chuckries January 22, 2022 00:19

chuckries requested changes Jan 22, 2022

View reviewed changes

Added option useExtendedRemote

198853c

xisui-MSFT changed the title ~~Replace target remote with extended-remote~~ Added extended-remote support Jan 25, 2022

gregg-miskelly reviewed Jan 26, 2022

View reviewed changes

Comment thread src/MIDebugEngine/Engine.Impl/DebuggedProcess.cs Outdated

gregg-miskelly approved these changes Jan 26, 2022

View reviewed changes

Resolving comments

51ee85b

WardenGnaw approved these changes Jan 26, 2022

View reviewed changes

WardenGnaw requested a review from chuckries January 26, 2022 00:56

WardenGnaw mentioned this pull request Jan 26, 2022

Extended remote #1115

Closed

gregg-miskelly approved these changes Jan 26, 2022

View reviewed changes

chuckries approved these changes Jan 27, 2022

View reviewed changes

Comment thread src/MIDebugEngine/Engine.Impl/DebuggedProcess.cs

WardenGnaw merged commit e8851dc into microsoft:main Jan 27, 2022

puremourning mentioned this pull request Mar 25, 2022

[Feature Request]: Extended remote support puremourning/vimspector#562

Closed

WardenGnaw mentioned this pull request Apr 7, 2022

Feature Request: GDB extended-remote Support microsoft/vscode-cpptools#2118

Closed

WardenGnaw mentioned this pull request Apr 18, 2022

cppdbg miDebuggerServerAddress parameter generates warning microsoft/vscode-cpptools#9195

Closed

xisui-MSFT mentioned this pull request Jul 11, 2022

Extended remote support microsoft/vscode-cpptools#9569

Merged

Sunbelt Computer Software

PL/B Language Development and Support

Uh oh!

Conversation

xisui-MSFT commented Jan 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

WardenGnaw commented Jan 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xisui-MSFT commented Jan 22, 2022

Uh oh!

chuckries commented Jan 22, 2022

Uh oh!

chuckries left a comment

Choose a reason for hiding this comment

Uh oh!

chuckries Jan 22, 2022

Choose a reason for hiding this comment

Uh oh!

chuckries Jan 22, 2022

Choose a reason for hiding this comment

Uh oh!

chuckries Jan 22, 2022

Choose a reason for hiding this comment

Uh oh!

xisui-MSFT Jan 25, 2022

Choose a reason for hiding this comment

Uh oh!

chuckries Jan 22, 2022

Choose a reason for hiding this comment

Uh oh!

xisui-MSFT commented Jan 22, 2022

Uh oh!

chuckries commented Jan 24, 2022

Uh oh!

WardenGnaw commented Jan 24, 2022

Uh oh!

chuckries commented Jan 24, 2022

Uh oh!

gregg-miskelly commented Jan 24, 2022

Uh oh!

gregg-miskelly commented Jan 24, 2022

Uh oh!

xisui-MSFT commented Jan 25, 2022

Uh oh!

xisui-MSFT commented Jan 25, 2022

Uh oh!

Uh oh!

gregg-miskelly left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fbs2016 commented Jun 22, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

xisui-MSFT commented Jan 21, 2022 •

edited

Loading

WardenGnaw commented Jan 22, 2022 •

edited

Loading