Use accept instead of accept4 for Android.

[chaoren]

Summary

The accept4 syscall is missing on older ARM Android kernels, and the accept()
call is implemented with the accept4 syscall, so we'll need to call the accept
syscall directly.

[chaoren]

• It's only a problem on arm.

[labath]

Just to confirm: Which toolchain are you using to compile this? It sounds like the android-9 toolchain has serious problems if it produces executables that do not run on an android-9 device...

Also, I know that this file already has a number of #ifdefs, but this is by far the most complicated one. It would be good if we could bury this detail somewhere deep inside Host/linux or Host/android. For example, we could define our own Accept4 call, which normally just calls accept4, but under the right conditions, it would fall back to the hack you did here. Something similar to what we do for process_vm_readv already...

[tberghammer]

I agree with Pavel that the toolchain shouldn't generate any source code what is using a missing syscall. I checked the implementation of accept on Gingerbread (API-9) and it is using the right syscall (https://android.googlesource.com/platform/bionic/+/gingerbread-release/libc/arch-arm/syscalls/accept.S).

I am not sure what is causing this issue, but one of my guess is that you are statically linking against a wrong version of libc.

[chaoren]

I am statically linking against API-21. There are some bugs in the old
toolchains that can be avoided by doing this. Aside from this issue, the
resulting binary seem to run fine on API 10 devices (and passes most of the
test suite).

[labath]

What kind of bugs are we talking about?

I find the idea of using an API 21-built binary and running it on an older device extremely troubling, and I would not trust it even if it did seem to work. I think we should find another way to achive this.

[chaoren]

• b.android.com/178448
• pthread_sigmask doesn't work at all in the older toolchains

The only possible problems I can see right now are missing syscalls, and we
can easily get a diff of those between API 21 and API 9.
labath requested changes to this revision.
labath added a reviewer: labath.
labath added a comment.

What kind of bugs are we talking about?

I find the idea of using an API 21-built binary and running it on an older
device extremely troubling, and I would not trust it even if it did seem to
work. I think we should find another way to achive this.

http://reviews.llvm.org/D10887

[labath]

In D10887#198515, @chaoren wrote:

• b.android.com/178448
• pthread_sigmask doesn't work at all in the older toolchains

The only possible problems I can see right now are missing syscalls, and we
can easily get a diff of those between API 21 and API 9.

The sentence "in general, statically linked binaries are compatible with neither older nor newer releases, especially where networking is concerned" from the bug leads me to believe that we should reconsider the static linking approach. I would normally expect these to be at least forward compatible, but if they don't guarantee even that, I think we should go back to the idea of using a non-pie shim process to load the lldb-server on old devices. Otherwise, we can encounter subtle and annoying breakages, which I would prefer to avoid.

[labath]

As for the pthread_sigmask and signalfd problems. If I understand correctly, the problem is the broken implementation of syscall on i386, right? If that is the case we can just import a working implementation of syscall() for i386 -- it's just a couple of instructions anyway. If that doesn't work, we can even get rid of pthread_sigmask and signalfd calls completely....

[chaoren]

For reference, the gdbserver binary currently in the toolchain is
statically linked.

Using a shim results in about a 5M increase in the lldb-server binary
because of the need to export all symbols dynamically. And still has those
two bugs (which would be in the system libs, if linked dynamically).

[labath]

In D10887#198526, @chaoren wrote:

Using a shim results in about a 5M increase in the lldb-server binary
because of the need to export all symbols dynamically. And still has those
two bugs (which would be in the system libs, if linked dynamically).

Couldn't this be avoided somehow (with some attribute magic or something). In reality, we just need one symbol, "lldb_main", or such).

And if that proves unfeasible and we have to statically link, I would prefer patching bugs in older libc over patching "features" in newer ones.

[tberghammer]

In D10887#198532, @labath wrote:

In D10887#198526, @chaoren wrote:

Using a shim results in about a 5M increase in the lldb-server binary
because of the need to export all symbols dynamically. And still has those
two bugs (which would be in the system libs, if linked dynamically).

Couldn't this be avoided somehow (with some attribute magic or something). In reality, we just need one symbol, "lldb_main", or such).

And if that proves unfeasible and we have to statically link, I would prefer patching bugs in older libc over patching "features" in newer ones.

I think we can avoid the size increase if we compile lldb-server as we do it now and on older target launch it with calling main through dlopen.

[chaoren]

That's what the shim does. You need to export the main symbol to call it
with dlopen.

[chaoren]

We can't patch bugs on the system libc of shipped devices, while with
static linking, we can avoid all but the latest bugs in the toolchain
(which I think the toolchain guys would be more receptive to fixing).
labath added a comment.

In http://reviews.llvm.org/D10887#198526, @chaoren wrote:

Using a shim results in about a 5M increase in the lldb-server binary
because of the need to export all symbols dynamically. And still has

those

two bugs (which would be in the system libs, if linked dynamically).

Couldn't this be avoided somehow (with some attribute magic or
something). In reality, we just need one symbol, "lldb_main", or such).

And if that proves unfeasible and we have to statically link, I would
prefer patching bugs in older libc over patching "features" in newer ones.

http://reviews.llvm.org/D10887

[ovyalov]

Potentially, there is no one size fits all solution.
Since statically linked binaries pose a significant threat due their incompatibility we might use dynamic linkage of lldb-server for majority of devices.
If there is need for hotfix which covers specific configuration - create a custom build of statically linked lldb-server which includes patches for affected platform and is built by toolchain for this platform. The same pattern may be applicable for devices which don't support -pie - like API-9.
Such split will increase size of package that user has to download - to mitigate this we may download lldb components on-demand when user knows which api level is needed.

Zach sometimes ago proposed to split Socket.cpp into platform-specific pieces to reduce conditional compilation nesting - like PosixSocket.cpp, WindowsSocket.cpp,.. This idea can be taken further with finer granularity - AndroidSocket.cpp with some specialization on api level. But I'm not sure that in this particular case we need such workaround.

[chaoren]

Won't be necessary if we make lldb-server single threaded and use the oldest supported API level (9).

[chaoren]

Whelp, it looks like we're going back to using the latest API. So this'll be temporarily necessary.

[chaoren]

• Add explanatory comment.

[labath]

I withdraw my objections.

[ovyalov]

source/Host/common/Socket.cpp

75–80

Is there any define that indicates that we're building statically?

80

Could you use such pattern for updating flags?

int flags = ::fcntl(fd, F_GETFD);
    if (flags == -1)
        return false;
    return (::fcntl(fd, F_SETFD, flags | FD_CLOEXEC) == 0);

[chaoren]

source/Host/common/Socket.cpp

75–80

I don't think so, but we can probably pass our own in the cmake file.

[chaoren]

• Address review comments.

[chaoren]

• Address review comments.

[ovyalov]

LGTM

source/Host/common/Socket.cpp

 #include "lldb/Host/TimeValue.h"
 
 #ifdef __ANDROID_NDK__
 #include <linux/tcp.h>
 #include <bits/error_constants.h>
 #include <asm-generic/errno-base.h>
 #include <errno.h>
 #include <arpa/inet.h>
+#include <unistd.h>
+#include <sys/syscall.h>
 #endif
 
 #ifndef LLDB_DISABLE_POSIX
 #include <arpa/inet.h>
 #include <netdb.h>
 #include <netinet/in.h>
 #include <netinet/tcp.h>
 #include <sys/socket.h>
if (!child_processes_inherit) {
         socketType |= SOCK_CLOEXEC;
     }
 #endif
     return ::socket (domain, socketType, protocol);
 }
 
 NativeSocket Accept(NativeSocket sockfd, struct sockaddr *addr, socklen_t *addrlen, bool child_processes_inherit)
 {
-#ifdef SOCK_CLOEXEC
+#if defined(__ANDROID_NDK__)
+  int fd = syscall(__NR_accept, sockfd, addr, addrlen);
+  if (fd >= 0 && !child_processes_inherit)
+    fcntl(fd, F_SETFD, FD_CLOEXEC);
+  return fd;
+#elif defined(SOCK_CLOEXEC)
     int flags = 0;
     if (!child_processes_inherit) {
         flags |= SOCK_CLOEXEC;
     }
     return ::accept4 (sockfd, addr, addrlen, flags);
 #else
     return ::accept (sockfd, addr, addrlen);
 #endif