This is an archive of the discontinued LLVM Phabricator instance.

Fix IPv6 support on lldb-server platform
ClosedPublic

Authored by aadsm on May 12 2019, 12:39 PM.

Details

Summary

This is a general fix for the ConnectionFileDescriptor class but my main motivation was to make lldb-server working with IPv6.
The connect URI can use square brackets ([]) to wrap the interface part of the URI (e.g.: <scheme>://[<interface>]:<port>). For IPv6 addresses this is a must since its ip can include colons and it will overlap with the port colon otherwise. The URIParser class parses the square brackets correctly but the ConnectionFileDescriptor doesn't generate them for IPv6 addresses making it impossible to connect to the gdb server when using this protocol.

How to reproduce the issue:

$ lldb-server p --server --listen [::1]:8080
...
$ lldb
(lldb) platform select remote-macosx
(lldb) platform connect connect://[::1]:8080
(lldb) platform process -p <pid>
error: unable to launch a GDB server on 'computer'

The server was actually launched we were just not able to connect to it. With this fix lldb will correctly connect. I fixed this by wrapping the ip portion with [].

Diff Detail

Repository
rLLDB LLDB

Event Timeline

aadsm created this revision.May 12 2019, 12:39 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 12 2019, 12:39 PM

The actual change looks obviously correct. Below are my comments on the test situation.

I wanted to reuse the CreateConnectedSockets part of the SocketTests. Initially I thought about creating a few functions in a test socket utility file, but then I realized the tests need to initialize the Socket plugin for these functions to work. In the end I created a new class (SocketBasedTest) so I can ensure this plugin is correctly initialized and teared down. However, I'm not a fan of subclassing as a means of code shared (as I prefer composition or delegation). What are your thoughts on this?

I don't think that the necessity of the Socket::Initialize call should prevent you from creating free functions. Everything else in lldb dealing with sockets already assumes that Socket::Initialize has been called, so I don't see why this couldn't be an implicit prerequisite of these functions (or you can even document it explicitly if you want). Though I generally also prefer free functions, I'm fairly ambivalent in this case, so I'll leave it up to you whether you want to leave this as a separate class, or convert it into a bunch of free functions. (If you do leave it as-is, I think you should at least remove the `setUp/tearDown overrides in the derived classes as they're pretty useless now - you're just overriding the methods with identical implementations.).

Another question I have (and this is more a n00b question as I don't have prior c++ experience), the CreateConnectedSockets functions that exist use unique pointers, and after reading the documentation about this I decided to use .release() so I could safely pass them to the ConnectionFileDescriptor in the test I created for it. Should I have changed the unique pointers to shared pointers instead?

Using release is the correct thing to do here as ConnectionFileDescriptor takes ownership of the passed-in object. What would be a nice improvement here would be to change the interface of ConnectionFileDescriptor constructor so that it takes a unique_ptr instead of a raw pointer. That way, you would just say ConnectionFileDescriptor(std::move(whatever_up)). That way the ownership part would be explicitly documented (now, I've had to look at the source to check the ownership semantics). If you want to do that please make a separate patch for that.

lldb/unittests/Host/ConnectionFileDescriptorTest.cpp
40 ↗(On Diff #199173)

You can just do EXPECT_EQ(ip, hostname)

lldb/unittests/Host/SocketBasedTest.cpp
63–68 ↗(On Diff #199173)

The fact that you felt the need to place // this does foo comments everywhere is a signal that this behavior is unintuitive. I think it would be better to split this part into a separate function, and name it something like IsAddressFamilySupported. Then each test can do something like:

if (!IsAddressFamilySupported(...)) {
  GTEST_LOG_(WARNING) << "Skipping test due to missing ??? support.";
  return;
}
aadsm updated this revision to Diff 199331.May 13 2019, 2:38 PM

Use functions instead of subclassing

labath accepted this revision.May 13 2019, 11:50 PM

Looks fine to me. Thank you for fixing this.

lldb/unittests/Host/SocketTestUtilities.cpp
16 ↗(On Diff #199331)

LLVM prefers static functions over ones in anonymous namespaces.

This revision is now accepted and ready to land.May 13 2019, 11:50 PM
aadsm updated this revision to Diff 199619.May 15 2019, 8:26 AM

Move function from anonymous namespace to const

aadsm edited the summary of this revision. (Show Details)May 17 2019, 10:24 AM
aadsm edited the summary of this revision. (Show Details)

Landed -- r361079

This revision was automatically updated to reflect the committed changes.

This broke windows buildbots. I tried to fix with svn r361083 but there were other things that got broken. @aadsm: We should revisit this patch next week and see what we need to fix up before it goes in.

labath added inline comments.May 20 2019, 12:18 AM
unittests/Host/ConnectionFileDescriptorTest.cpp
42

Judging by Davide's email, this looks like the cause of your problem. GetURI() returns a temporary string, which means that the StringRefs UriParser parses it into become dangling after this statement.

aadsm reopened this revision.May 20 2019, 1:02 PM
This revision is now accepted and ready to land.May 20 2019, 1:02 PM
aadsm updated this revision to Diff 200353.May 20 2019, 1:02 PM
aadsm edited the summary of this revision. (Show Details)
  • Fixed function from const to static (mea culpa)
  • Fixed inet_pton usage on windows
  • Fixed issue with GetURL return creating a temp object
labath accepted this revision.May 20 2019, 11:45 PM
labath added inline comments.May 20 2019, 11:48 PM
lldb/unittests/Host/CMakeLists.txt
12–13 ↗(On Diff #200353)

Please sort this list before committing.

aadsm updated this revision to Diff 200562.May 21 2019, 11:56 AM

A-Z order the CMakeLists files

This revision was automatically updated to reflect the committed changes.