Page MenuHomePhabricator

Add reverse-connect functionality to the gdb-remote command
AbandonedPublic

Authored by fjricci on Feb 10 2016, 2:17 PM.

Details

Summary

Add an optional "reverse" option to the gdb-remote command,
which causes lldb to listen on the specified [host:]port,
waiting for the remote server to reverse-connect back to lldb.

Diff Detail

Event Timeline

fjricci updated this revision to Diff 47523.Feb 10 2016, 2:17 PM
fjricci updated this revision to Diff 47524.
fjricci retitled this revision from to Add reverse-connect functionality to the gdb-remote command.
fjricci updated this object.
fjricci added subscribers: lldb-commits, sas.

Accidentally uploaded the wrong commit. Sorry about that

Looks like there's a few inconsistencies with our coding style. Can you please run clang-format on this patch?

zturner requested changes to this revision.Feb 10 2016, 2:32 PM
zturner added a reviewer: zturner.
This revision now requires changes to proceed.Feb 10 2016, 2:32 PM
fjricci updated this revision to Diff 47534.Feb 10 2016, 2:44 PM
fjricci edited edge metadata.

Apply clang-format

Perhaps I ran clang-format in the wrong configuration, but it looks like it made the function declaration style in include/lldb/Target/Platform.h a bit inconsistent. Is this the desired behavior?

Perhaps I ran clang-format in the wrong configuration, but it looks like it made the function declaration style in include/lldb/Target/Platform.h a bit inconsistent. Is this the desired behavior?

The way it formatted the code is consistent with our rules, so it looks fine to me.

(The problem is that surrounding code was not clang-formatted)

labath added a subscriber: labath.Feb 11 2016, 1:43 AM

I think this would be better if you could specify the connection direction in the url. Also, a test for this functionality would be great.

source/Interpreter/CommandInterpreter.cpp
638

how about just using listen://... to mean reverse connection?

source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.h
222 ↗(On Diff #47534)

I don't think we need to pass this reverse=false around everywhere. We already support listen://host:port url scheme (see e.g. ConnectionFileDescriptor::Connect`). Could you use that to specify connection direction?

Would it be better to add a test in the same commit or a separate one?

source/Interpreter/CommandInterpreter.cpp
638

My concern with doing it this way was that ProcessGDBRemote::ConnectToDebugserver() seems to implicitly require a connect://... url, and I didn't want to risk breaking future changes. However, it still works correctly if I pass a listen://... url, so it seems like a reasonable change.

source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.h
222 ↗(On Diff #47534)

I had considered that, but wasn't sure whether parsing the direction out of the url string every time it was needed would be better or worse than passing it around as a bool. I'll make the change.

fjricci updated this revision to Diff 47681.Feb 11 2016, 10:35 AM
fjricci edited edge metadata.

Refactor to use listen urls

clayborg accepted this revision.Feb 11 2016, 11:29 AM
clayborg edited edge metadata.

I like this more, but I'd like to take it a bit further. I think we won't have to parse the url string at all (see comments).

I believe the best way to avoid breaking (and being broken by) other changes is to make sure this fits in nicely in the existing architecture. Putting this logic into ConnectToDebugServer seems like the logical place.

You can wait with the test until we have settled on the final implementation, but I'd like the test to go in together with the change.

source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
726 ↗(On Diff #47681)

If we can get ConnectToDebugserver working with a listen url then we will get rid of this url parsing hack...

1088 ↗(On Diff #47681)

ConnectionFileDescriptor::Connect seems to be able to handle listen:// urls. What would happen if you just passed the listen url to ConnectToDebugserver

It looks like passing a listen:// url to ConnectToDebugserver actually works "out of the box". Thanks! I'll work on writing a unit test.

fjricci updated this revision to Diff 47806.Feb 12 2016, 8:04 AM
fjricci edited edge metadata.

Remove unnecessary special-case logic for reverse connecting

fjricci abandoned this revision.Mar 14 2016, 8:54 AM

This functionality can already be accessed by using the "process connect" command with a listen url directly.