Page MenuHomePhabricator

Use accept instead of accept4 for Android.
ClosedPublic

Authored by chaoren on Jul 1 2015, 3:45 PM.

Details

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.

Diff Detail

Event Timeline

chaoren updated this revision to Diff 28911.Jul 1 2015, 3:45 PM
chaoren retitled this revision from to Use accept instead of accept4 for Android..
chaoren updated this object.
chaoren edited the test plan for this revision. (Show Details)
chaoren added reviewers: tberghammer, vharron.
chaoren added a subscriber: Unknown Object (MLST).
chaoren updated this revision to Diff 28918.Jul 1 2015, 4:51 PM
  • It's only a problem on arm.
labath added a subscriber: labath.Jul 2 2015, 1:49 AM

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 requested changes to this revision.Jul 2 2015, 2:33 AM
tberghammer edited edge metadata.

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.

This revision now requires changes to proceed.Jul 2 2015, 2:33 AM

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 requested changes to this revision.Jul 2 2015, 8:12 AM
labath added a reviewer: 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.

  • 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 added a comment.Jul 2 2015, 8:32 AM
  • 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 added a comment.Jul 2 2015, 8:41 AM

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....

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 added a comment.Jul 2 2015, 8:47 AM

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.

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.

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

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 added a subscriber: ovyalov.Jul 2 2015, 9:41 PM

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 abandoned this revision.Jul 6 2015, 10:28 AM

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

chaoren reclaimed this revision.Jul 15 2015, 10:05 AM

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

This revision now requires changes to proceed.Jul 15 2015, 10:05 AM
chaoren edited edge metadata.
  • Add explanatory comment.
labath accepted this revision.Jul 15 2015, 10:10 AM
labath edited edge metadata.

I withdraw my objections.

ovyalov added inline comments.Jul 15 2015, 10:45 AM
source/Host/common/Socket.cpp
77

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

82

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 added inline comments.Jul 15 2015, 10:49 AM
source/Host/common/Socket.cpp
77

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

chaoren edited edge metadata.
  • Address review comments.
  • Address review comments.
chaoren marked 2 inline comments as done.Jul 15 2015, 12:03 PM
ovyalov accepted this revision.Jul 15 2015, 12:05 PM
ovyalov added a reviewer: ovyalov.

LGTM

This revision was automatically updated to reflect the committed changes.