Page MenuHomePhabricator

Make ConnectionFileDescription work with all sockets

Authored by aadsm on May 17 2019, 3:51 PM.



My main goal here is to make lldb-server work with Android Studio.

This is currently not the case because lldb-server is started in platform mode listening on a domain socket. When Android Studio connects to it lldb-server crashes because even though it's listening on a domain socket as soon as it gets a connection it asserts that it's a TCP connection, which will obviously fails for any non-tcp connection.

To do this I came up with a new method called GetConnectURI() in Socket that returns the URI needed to connect to the connected portion of the socket.

Diff Detail


Event Timeline

aadsm created this revision.May 17 2019, 3:51 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 17 2019, 3:51 PM
aadsm added a comment.May 17 2019, 3:54 PM

I'm not really sure which source version lldb-server is being shipped with Android Studio today (since their version works just fine obviously)...

aadsm updated this revision to Diff 200126.May 17 2019, 6:55 PM

Avoid parsing the URL when starting the GDB server if it's not a TCP connection.

The patch looks fine, modulo some inline nits. I am somewhat doubtful about the name of the new function though. When I first saw GetConnectURI, i thought this would be the URI used to connect to this socket, but in reality it is the URI of the other end (right?). In that case, I think GetRemoteURI would be better. Or maybe, borrowing from the BSD socket vocabulary, GetPeerURI ? WDYT?

I'm not really sure which source version lldb-server is being shipped with Android Studio today (since their version works just fine obviously)...

I'm not sure either, but I'd expect it's a pretty old one (>1yr) at this point. I guess this behavior regressed somehow since then..

147–151 ↗(On Diff #200126)

You could just write something like llvm::formatv("{0}://{1}", GetNameOffset() == 0 ? ... , GetSocketName())

165–166 ↗(On Diff #200126)

The same issue as in the previous patch. You'll need to store the GetConnectURI result in a local variable.

167 ↗(On Diff #200126)

EXPECT_EQ("connect", scheme); is enough.

aadsm marked an inline comment as done.May 20 2019, 6:30 AM

Glad you brought the naming up, I was also not happy with it. I guess I kind of convinced myself in the end with "it's the url the socket used to create its connection". The other name I tinkered with was GetConnectRemoteURI, I wanted to have Connect in the name because of the naming scheme that it's being using, it seems something specifically to be used with ConnectionFileDescriptor::Connect (it's not clear to me why though). Actually now that I re read that function maybe GetRemoteConnectionURI would be better?

165–166 ↗(On Diff #200126)

Ah! thanks!

GetRemoteConnectionURI sounds fine.

130 ↗(On Diff #200126)

This should also be GetRemoteSocketName or something similar (though the word remote looks a bit weird when it comes to domain sockets).

aadsm marked an inline comment as done.May 20 2019, 2:18 PM
aadsm added inline comments.
130 ↗(On Diff #200126)

uhm.. I'm a bit skeptical on this one 😄
As far as I understand domain sockets don't really have endpoints, there's just a single point of consumption (the inode). Here's what I see in my head when comparing TCP to Domain sockets:


consumer o---------o provider


consumer -----o----- provider

While on TCP we have 2 things that we can name "remote" and "local". On Domain there's only one thing to name, so I think it would be odd calling it "remote" given there's no parallel with the tcp model.

I don't have strong feelings on this though, and I'm happy to change to land the patch.

aadsm updated this revision to Diff 200403.May 20 2019, 10:31 PM
  • Change GetConnectURI to GetRemoteConnectionURI
  • Fixes tests when it comes to using dangling pointers on ConstRefs
  • Better usage of the EXPECT_* macros.

It looks like diff now contains the changes from D61833 as well. It would be better to upload just the changes relative to the previous patch in the series, otherwise things get confusing about what is being changed where. It's kind of obvious here, but you can use the "parent revision" field in phabricator to indicate that one diff depends on another.

Other than that, I *think* this is fine.

1–23 ↗(On Diff #200403)

Delete this.

130 ↗(On Diff #200126)

Oops, sorry, you're totally right. I just noticed this name in drive-by and didn't stop to think about it. Please ignore this comment.

1–16 ↗(On Diff #200403)

Delete this.

aadsm updated this revision to Diff 200594.May 21 2019, 2:57 PM

Fix the mess I created by including parts of the previous diff

aadsm updated this revision to Diff 200597.May 21 2019, 3:01 PM

Add missing bits from previous revision

labath accepted this revision.May 22 2019, 12:16 AM
labath added inline comments.
172–178 ↗(On Diff #200597)

You have a bunch of unused variables here.

This revision is now accepted and ready to land.May 22 2019, 12:16 AM
aadsm updated this revision to Diff 200800.May 22 2019, 11:27 AM

Remove unused variables

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMay 30 2019, 4:32 PM