Page MenuHomePhabricator

Update LLDB Host to support IPv6 over TCP
ClosedPublic

Authored by beanz on Apr 7 2017, 11:56 AM.

Details

Summary

This patch adds IPv6 support to LLDB/Host's TCP socket implementation. Supporting IPv6 involved a few significant changes to the implementation of the socket layers, and I have performed some significant code cleanup along the way.

This patch changes the Socket constructors for all types of sockets to not create sockets until first use. This is required for IPv6 support because the socket type will vary based on the address you are connecting to. This also has the benefit of removing code that could have errors from the Socket subclass constructors (which seems like a win to me).

The patch also slightly changes the API and behaviors of the Listen/Accept pattern. Previously both Listen and Accept calls took an address specified as a string. Now only listen does. This change was made because the Listen call can result in opening more than one socket. In order to support listening for both IPv4 and IPv6 connections we need to open one AF_INET socket and one AF_INET6 socket. During the listen call we construct a map of file descriptors to addrin structures which represent the allowable incoming connection address. This map removes the need for taking an address into the Accept call.

This does have a change in functionality. Previously you could Listen for connections based on one address, and Accept connections from a different address. This is no longer supported. I could not find anywhere in LLDB where we actually used the APIs in that way. The new API does still support AnyAddr for allowing incoming connections from any address.

The Listen implementation is implemented using kqueue on FreeBSD and Darwin, WSAPoll on Windows and poll(2) everywhere else.

Diff Detail

Repository
rL LLVM

Event Timeline

beanz created this revision.Apr 7 2017, 11:56 AM
labath added a subscriber: labath.Apr 11 2017, 5:19 AM

When I created the MainLoop class, i was hoping it would become the central place for all select-like functionality. Currently it uses pselect, but i don't see a reason we couldn't switch it to ppoll (but we actually don't have to, as the current implementation should work just fine for your use case). We also can-but-don't-have-to provide a specialized kevent-based implementation for mac&freebsd (afaict, kevent also supports listening for signals, but i would be fine with leaving that out even, as we don't need that functionality on these platforms). So all that needs to be added is a WSAPoll-based MainLoopWindows.

Then your Listen implementation would boil down to:

for (auto socket: sockets)
  handles.emplace_back(loop.RegisterReadObject(socket, [&] { accepted = accept(socket, ...); loop.RequestTermination(); });
loop.Run();

I'm not married to the current MainLoop interface in any way, so we can change it if you find the current one cumbersome, but I would like to cut down the number of selecting pieces of code.

What do you think about that?

source/Host/common/TCPSocket.cpp
284 ↗(On Diff #94551)

Why not simply use infinite timeout if that's the behaviour you desire?

zturner edited edge metadata.Apr 11 2017, 9:04 AM

Networking isn't my area of domain expertise, so these are mostly just general comments.

include/lldb/Host/common/TCPSocket.h
55 ↗(On Diff #94551)

Any particular reason you're using a std::map instead of a DenseMap or other similar LLVM structure? I was under the impression that the number of times where you should actually use std::map are pretty small.

source/Host/common/Socket.cpp
86 ↗(On Diff #94551)

Is this ever set to false anywhere? If not, just delete the parameter.

Also, since you're in this code anyway, maybe you could use llvm::make_unique.

150 ↗(On Diff #94551)

The parameters look reversed here.

260 ↗(On Diff #94551)

Makes me wish we had StringRef::shrink(int N). Oh well, this is fine.

source/Host/common/TCPSocket.cpp
29 ↗(On Diff #94551)

Can you use LLVM_ON_WIN32?

60 ↗(On Diff #94551)

!m_listen_sockets.empty()

116 ↗(On Diff #94551)

Should we perhaps assert here? Seems like creating 2 sockets back to back without cleaning up after yourself is not intended behavior. The owner of a socket should probably close it before they try to create a new one, and an assert(!IsValid()) would catch that.

194 ↗(On Diff #94551)

LLVM_ON_WIN32.

Also can you raise this up into something like this:

#if defined(LLVM_ON_WIN32)
  #define CLOSE_SOCKET ::closesocket
#else
  #define CLOSE_SOCKET ::close
#endif

Then just write CLOSE_SOCKET(fd);

217 ↗(On Diff #94551)

Use CLOSE_SOCKET macro from earlier.

250 ↗(On Diff #94551)

Can you add sock_set.reserve(m_listen_sockets.size())?

284 ↗(On Diff #94551)

Similar to before, you could add a #define POLL_SOCKET ...

318 ↗(On Diff #94551)

CLOSE_SOCKET(fd)

323–326 ↗(On Diff #94551)

llvm::errs() << formatv("error: rejecting incoming connection from {0} (expecting {1})", AcceptAddr.GetIPAddress(), AddrIn.GetIPAddress());

330 ↗(On Diff #94551)

make_unique

source/Host/common/UDPSocket.cpp
125 ↗(On Diff #94551)

This seems odd. Does return Error(g_not_supported_error) not work?

145–146 ↗(On Diff #94551)

make_unique again.

beanz added a comment.Apr 11 2017, 4:09 PM

@labath, I could adapt this into the MainLoop class, but I would actually want to change how that class hierarchy is implemented. Regardless of the event handling/polling model you use much of the code is identical between the classes. I'd rather get rid of MainLoopPosix and MainLoopBase and instead just implement a portable MainLoop class.

In particular the difference between Windows WSAPoll and posix poll(2) is the name of two functions, which makes the idea of having separate Windows and Posix implementations just seem overly complicated to me, although neither Darwin nor Windows have ppoll because it isn't POSIX, and Windows doesn't have pselect, so there isn't a remotely portable way to wait on file descriptors and signals... This is why we can't have nice things.

I'm going to dig into the MainLoop code. At first glance I do think that is a better way to fit this all together, but I'm going to need to think on it and experiment a bit.

Looks good. I'd recommend adding clayborg as a reviewer and giving him a couple days to comment on this if he has time. He wrote all of this code originally.

include/lldb/Host/common/TCPSocket.h
55 ↗(On Diff #94551)

llvm docs say, "Also, because DenseMap allocates space for a large number of key/value pairs (it starts with 64 by default), it will waste a lot of space if your keys or values are large." I think std::map is the right choice. When it's a tossup between an llvm container class and an STL container class, my opinion is always to go with the standard container class that everyone is familiar with already. If there's a distinct benefit to a given llvm container class for a specific scenario, that's fine, but it just increases the barrier to entry for people outside the llvm community to use these classes pervasively.

source/Host/common/Socket.cpp
258 ↗(On Diff #94551)

Ah, I'd originally said I thought we should just tack the :portnum on to the end of the string, search backwards to find it (because the [] characters are metacharacters in unix shells). but I see that RFC8986 says this is how things like this are specified, like http://[2001:db8:1f70::999:de8:7648:6e8]:100/ so cool with that.

zturner added inline comments.Apr 11 2017, 10:52 PM
include/lldb/Host/common/TCPSocket.h
55 ↗(On Diff #94551)

I was under the impression SocketAddress was fairly small, but I checked and it turns out it's not. That said, I *still* don't think std::map is the right choice, because there is std::unordered_map which is more efficient when iterating over keys in sorted order is not a requirement.

In fact, I'm not even sure any kind of map is needed. Isn't the point of this just to allow listening on an IPv4 interface and an IPv6 interface at the same time? If so, the most common use case is either going to be 1 item in the map or 2 items in the map, in which case perhaps a SmallVector<std::pair<int, SocketAddress>, 2> is better.

@labath, I could adapt this into the MainLoop class, but I would actually want to change how that class hierarchy is implemented. Regardless of the event handling/polling model you use much of the code is identical between the classes. I'd rather get rid of MainLoopPosix and MainLoopBase and instead just implement a portable MainLoop class.

In particular the difference between Windows WSAPoll and posix poll(2) is the name of two functions, which makes the idea of having separate Windows and Posix implementations just seem overly complicated to me, although neither Darwin nor Windows have ppoll because it isn't POSIX, and Windows doesn't have pselect, so there isn't a remotely portable way to wait on file descriptors and signals... This is why we can't have nice things.

One extra thing you should be aware of is that unlike poll(2), WSAPoll only works on sockets. We currently have pieces of code ifdef-ed out on windows because we have no way to wait on pipes, and eventually I'd like to teach the MainLoop to do that as well. I'm not saying we should do that now, just pointing out that the implementations are likely to diverge in the future.

However, if you think that for the current minimal requirements (sockets on windows, signals and file descriptors on linux and netbsd, and file descriptors everywhere else) the cleanest implementation is a single class, then that's fine with me. We can revisit that decision when the need comes up.

Adding Greg to the reviewers list.

beanz updated this revision to Diff 95355.Apr 14 2017, 3:39 PM

Updating to use MainLoop class, and refactor MainLoop class to operate on Windows.

I've added error cases to the MainLoop class for functionality that is not supported. Specifically non-socket IOObjects are not supported on Windows, and signal handling requires either kqueue or ppoll. In practice that means signal handling is not supported on Windows, older Linux OSs and some BSD variants. That is all controlled by proper configure-time checks.

labath accepted this revision.Apr 15 2017, 8:06 AM

Thank you for taking the time to do this.

It looks like ppoll was added to linux quite a long time ago, but we still get some patches supporting surprisingly old kernels. I hope it doesn't come do that, but if it does, we can easily add a pselect-based fallback as well.

looks good, assuming that the SYS_EVENT_H define can go away.

source/Host/common/MainLoop.cpp
22 ↗(On Diff #95355)

This looks wrong. Is this left over from testing ?

241 ↗(On Diff #95355)

Nit: the if could be extracted out of the #ifdef

This revision is now accepted and ready to land.Apr 15 2017, 8:06 AM
beanz updated this revision to Diff 95448.Apr 17 2017, 10:16 AM

Updating patches to reflect feedback from zturner.

beanz updated this revision to Diff 95453.Apr 17 2017, 10:39 AM

Removing code I accidentally left in that was from debugging, and moving some duplicated code that @labath spotted out of the ifdef.

clayborg accepted this revision.Apr 17 2017, 11:27 AM

I'm good if Pavel is good.

This revision was automatically updated to reflect the committed changes.

It looks like you had a fun day yesterday. :) Unfortunately, I have to add to your problems, as I've had to revert your commit due to a fairly big problem.

The problem that it can happen that the create-a-bunch-of-listening-sockets loop can sometimes only create an ipv6 socket if the respective ipv4 port is used by another process. Then the connecting side will try to connect to the IPv4 port and fail. I am not really sure what would be the best fix for this, but right now this is making our test suite completely unusable...

There was also an issue with not detecting ppoll properly, but that one was easy to fix.

I am sorry, I should have caught this earlier, but you seemed so certain of what you are doing that I did not bother trying to test the patch... :/

lldb/trunk/cmake/modules/LLDBConfig.cmake
435

This needs set(CMAKE_REQUIRED_DEFINITIONS -D_GNU_SOURCE) to detect ppoll on linux.

lldb/trunk/source/Host/common/TCPSocket.cpp
189

This has no effect now as we are not listening on the file descriptor internal to this object.

Fix from pkgsrc-wip to resurrect LLDB Standalone build in SVN r. 300654.

From 3371648af049cb4b40427ef87dcafb4745f10e8d Mon Sep 17 00:00:00 2001
From: Kamil Rytarowski <n54@gmx.com>
Date: Wed, 19 Apr 2017 06:17:30 +0200
Subject: [PATCH] lldb-netbsd: Upgrade to 300654

This code has FPR and DBR for NetBSD/amd64.

Add local patches to resurrect standalone build

Sponsored by <The NetBSD Foundation>
---
 lldb-netbsd/Makefile                                        |  2 +-
 lldb-netbsd/distinfo                                        |  3 +++
 .../patches/patch-cmake_modules_LLDBStandalone.cmake        | 13 +++++++++++++
 lldb-netbsd/patches/patch-source_Host_common_MainLoop.cpp   | 13 +++++++++++++
 lldb-netbsd/patches/patch-source_Host_common_TCPSocket.cpp  | 13 +++++++++++++
 5 files changed, 43 insertions(+), 1 deletion(-)
 create mode 100644 lldb-netbsd/patches/patch-cmake_modules_LLDBStandalone.cmake
 create mode 100644 lldb-netbsd/patches/patch-source_Host_common_MainLoop.cpp
 create mode 100644 lldb-netbsd/patches/patch-source_Host_common_TCPSocket.cpp

diff --git a/lldb-netbsd/Makefile b/lldb-netbsd/Makefile
index 6604b75afe..d92ba960cd 100644
--- a/lldb-netbsd/Makefile
+++ b/lldb-netbsd/Makefile
@@ -5,7 +5,7 @@ CATEGORIES=	lang devel
 
 SVN_REPOSITORIES=	lldb
 SVN_REPO.lldb=		http://llvm.org/svn/llvm-project/lldb/trunk
-SVN_REVISION.lldb=	299783
+SVN_REVISION.lldb=	300654
 
 MAINTAINER=	pkgsrc-users@NetBSD.org
 HOMEPAGE=	http://lldb.org/
diff --git a/lldb-netbsd/distinfo b/lldb-netbsd/distinfo
index 9e296857d3..0a1ce52470 100644
--- a/lldb-netbsd/distinfo
+++ b/lldb-netbsd/distinfo
@@ -12,6 +12,9 @@ Size (libcxx-3.6.2.src.tar.xz) = 944020 bytes
 SHA1 (llvm-3.6.2.src.tar.xz) = 7a00257eb2bc9431e4c77c3a36b033072c54bc7e
 RMD160 (llvm-3.6.2.src.tar.xz) = 521cbc5fe2925ea3c6e90c7a31f752a04045c972
 Size (llvm-3.6.2.src.tar.xz) = 12802380 bytes
+SHA1 (patch-cmake_modules_LLDBStandalone.cmake) = c68c9fbe37d7f2d118febf2914b56b551eb5e696
+SHA1 (patch-source_Host_common_MainLoop.cpp) = 66cb4d07a092caac4d271ca4a638406076b5bd81
+SHA1 (patch-source_Host_common_TCPSocket.cpp) = e18a33e66c281cbc1353787752789faf609b21f0
 SHA1 (patch-source_Plugins_ObjectFile_ELF_ObjectFileELF.cpp) = cdc5861eedcc79d3484ba011f5be595b2afcbae2
 SHA1 (patch-source_Plugins_Process_elf-core_ProcessElfCore.cpp) = e8609042fb407f3507ac7cbe00494bbad5a2ca14
 SHA1 (patch-source_Plugins_Process_elf-core_ProcessElfCore.h) = 902ce5e0187aa2649986db08c79af7291b852727
diff --git a/lldb-netbsd/patches/patch-cmake_modules_LLDBStandalone.cmake b/lldb-netbsd/patches/patch-cmake_modules_LLDBStandalone.cmake
new file mode 100644
index 0000000000..4641527762
--- /dev/null
+++ b/lldb-netbsd/patches/patch-cmake_modules_LLDBStandalone.cmake
@@ -0,0 +1,13 @@
+$NetBSD$
+
+--- cmake/modules/LLDBStandalone.cmake.orig	2017-04-19 04:06:59.854088855 +0000
++++ cmake/modules/LLDBStandalone.cmake
+@@ -4,6 +4,8 @@ if (CMAKE_SOURCE_DIR STREQUAL CMAKE_CURR
+   project(lldb)
+   cmake_minimum_required(VERSION 2.8.12.2)
+ 
++  include(CheckIncludeFile)
++
+   if (POLICY CMP0022)
+     cmake_policy(SET CMP0022 NEW) # automatic when 2.8.12 is required
+   endif()
diff --git a/lldb-netbsd/patches/patch-source_Host_common_MainLoop.cpp b/lldb-netbsd/patches/patch-source_Host_common_MainLoop.cpp
new file mode 100644
index 0000000000..747b039d09
--- /dev/null
+++ b/lldb-netbsd/patches/patch-source_Host_common_MainLoop.cpp
@@ -0,0 +1,13 @@
+$NetBSD$
+
+--- source/Host/common/MainLoop.cpp.orig	2017-04-19 03:59:22.000000000 +0000
++++ source/Host/common/MainLoop.cpp
+@@ -7,7 +7,7 @@
+ //
+ //===----------------------------------------------------------------------===//
+ 
+-#include "llvm/Config/config.h"
++#include "llvm/Config/llvm-config.h"
+ 
+ #include "lldb/Host/MainLoop.h"
+ #include "lldb/Utility/Error.h"
diff --git a/lldb-netbsd/patches/patch-source_Host_common_TCPSocket.cpp b/lldb-netbsd/patches/patch-source_Host_common_TCPSocket.cpp
new file mode 100644
index 0000000000..18f5930997
--- /dev/null
+++ b/lldb-netbsd/patches/patch-source_Host_common_TCPSocket.cpp
@@ -0,0 +1,13 @@
+$NetBSD$
+
+--- source/Host/common/TCPSocket.cpp.orig	2017-04-19 03:59:22.000000000 +0000
++++ source/Host/common/TCPSocket.cpp
+@@ -17,7 +17,7 @@
+ #include "lldb/Host/MainLoop.h"
+ #include "lldb/Utility/Log.h"
+ 
+-#include "llvm/Config/config.h"
++#include "llvm/Config/llvm-config.h"
+ #include "llvm/Support/raw_ostream.h"
+ 
+ #ifndef LLDB_DISABLE_POSIX
-- 
2.11.0

https://wip.pkgsrc.org/cgi-bin/gitweb.cgi?p=pkgsrc-wip.git;a=commitdiff;h=3371648af049cb4b40427ef87dcafb4745f10e8d

krytarowski added inline comments.Apr 19 2017, 4:25 AM
lldb/trunk/cmake/modules/LLDBConfig.cmake
435

check_symbol_exists unrecognized in LLDB Standalone build. There is need to add include(CheckIncludeFile) somewhere in generic or standalone cmake file.