Undefined reference to syscalls in r15

[Mygod]

Description

clang++: error: linker command failed with exit code 1 (use -v to see invocation)
mobile/src/main/jni/../jni/libev/ev_epoll.c:249: error: undefined reference to 'epoll_create1'
make: *** [mobile/target/android/intermediates/ndk/obj/local/armeabi-v7a/libss-tunnel.so] Error 1
mobile/src/main/jni/../jni/libev/ev.c:4405: error: undefined reference to 'inotify_init1'
clang++: error: linker command failed with exit code 1 (use -v to see invocation)
make: *** [mobile/target/android/intermediates/ndk/obj/local/x86/libss-tunnel.so] Error 1
mobile/src/main/jni/../jni/libev/ev_epoll.c:249: error: undefined reference to 'epoll_create1'
mobile/src/main/jni/../jni/libev/ev.c:4405: error: undefined reference to 'inotify_init1'
clang++: error: linker command failed with exit code 1 (use -v to see invocation)
make: *** [mobile/target/android/intermediates/ndk/obj/local/x86/libss-local.so] Error 1

Travis using r14 works: https://travis-ci.org/shadowsocks/shadowsocks-android/builds/232310650

Source code: https://github.com/shadowsocks/shadowsocks-android

Environment Details

• NDK Version: 15.0.3869609-beta1
• Build sytem: ndk-build
• Host OS: Linux Mint 18.1 x64
• Compiler: clang
• ABI: armeabi-v7a, x86 (error didn't occur to arm64-v8a)
• STL: stlport_static
• NDK API level: android-19
• Device API level: Not applicable

[enh]

looks like a bug in libev:

#ifdef EPOLL_CLOEXEC
backend_fd = epoll_create1 (EPOLL_CLOEXEC);

https://github.com/BeeswaxIO/libev/blob/a92e9ffa951d26a90212a2ff207dbfafd2790e1f/ev_epoll.c#L241

they assume that if the EPOLL_CLOEXEC constant is available, epoll_create1 is available. that's not true on Android. they need either a configure-style test for epoll_create1, or they can check ANDROID_API >= 21.

they make the same incorrect assumption with inotify_init1:

#if defined IN_CLOEXEC && defined IN_NONBLOCK
int fd = inotify_init1 (IN_CLOEXEC | IN_NONBLOCK);

[Mygod]

@enh What? Why isn't that true on Android? And why did it work back in r14?

[DanAlbert]

Why isn't that true on Android?

Because there wasn't a C library wrapper for epoll_create1 until L: https://android.googlesource.com/platform/bionic/+/master/libc/libc.map.txt#308

EPOLL_CLOEXEC comes from the Linux kernel headers (which are compatible with any kernel version), whereas epoll_create1 comes from the C library. EPOLL_CLOEXEC needs to be available regardless of the existence of the C library wrapper because syscall(__NR_epoll_create1, EPOLL_CLOEXEC) is still valid.

And why did it work back in r14?

r14 defaulted to using the deprecated, waaaaaaay out of date headers. r15 has made unified headers the default.

[Mygod]

Thanks for the explanation! Btw is __ANDROID_API__ a stable API I can use so that I can write preprocessor macros as such:

#if !defined __ANDROID_API__ || __ANDROID_API__ >= 21
...
#endif

[DanAlbert]

Yep, __ANDROID_API__ will always be defined when using the NDK.

[Mygod]

Thanks. Closing this.

[alexcohn]

FWIW, this is a duplicate of #302

[alexcohn]

@DanAlbert , @enh what solution would you recommend?

int fd;
#if defined EPOLL_CLOEXEC && (!defined __ANDROID__ || __ANDROID_API__ >= 21)
  fd = epoll_create1 (EPOLL_CLOEXEC);
  if (fd < 0 && (errno == EINVAL || errno == ENOSYS))
#endif
  fd = epoll_create(epoll_size);
  if (fd != -1)
    ::fcntl(fd, F_SETFD, FD_CLOEXEC);

For boost, we are currently using another ugly workaround (note there is still no activity on https://svn.boost.org/trac/boost/ticket/12863):

#if __ANDROID_API__ < 21
  #define epoll_create1(x) -1; errno=ENOSYS
#endif

[DanAlbert]

Boost already has fallback code if epoll_create1 isn't present. I think we should just add the __ANDROID_API__ >= 21 to the check. I've opened a bug for us to send that patch, since at this point I don't think boost is going to do it themselves. I'll do the same for libev. If there are any other big open source projects that need some work for migrating here, open more bugs and we'll see what we can do. Obviously we can't fix the world, but if we can fix the big ones that should help a lot.

[alexcohn]

I was brought up on "detect features, not versions" so I am very unhappy with spreading __ANDROID_API__ >= 21 in the code that is not part of Android API, but I hear your objections to having EPOLL_CLOEXEC conditionally defined in Android headers. I hope there may be some better more robust approach to this problem.

[enh]

well, we've all lived with the alternative in the NDK for years, so we know how well that works out :-)

but in this case detecting the feature is the "try epoll_create1, fall back to epoll_create, fall back to poll/select" code. Android doesn't mandate kernel versions, so even with the legacy headers seeing EPOLL_CLOEXEC didn't mean much if anything.

[alexcohn]

What about adding to legacy_signal_inlines.h

static __inline int epoll_create1(int) { 
  errno = ENOSYS;
  return -1;
}

This will fix at least boost and libev in backwards-compatible way.

[DanAlbert]

Actually, I suppose we can do even better and just do:

static __inline int epoll_create1(int flags) {
    return syscall(__NR_epoll_create1, flags);
}

right? We've talked about trying to get libc_syscalls.a into the NDK for this kind of thing already, and this is pretty much the same thing.

[DanAlbert]

The downside of that approach is that it turns a build failure into a run time failure if the calling code isn't tolerant of the fact that syscalls with functions might not actually be available in the kernel. Boost and libev both are, but I'm not sure how common that is.

[Mygod]

Seems like a good idea to me.

[alexcohn]

The major problem with syscall approach for me is that it assumes that all kernes below API 21 will gracefully fail for this call with appropriate flags and return values. I do know that some of the ancient Linux kernels don't.

[jmgao]

I do know that some of the ancient Linux kernels don't.

😱

Do you know what they actually do instead of ENOSYS? Is it something like a device vendor using the next available syscall number instead of picking a number way out of range?

[enh]

a device vendor using the next available syscall number

we've certainly seen that happen numerous times, but there's not much we can do about it other than (a) always have CTS tests for any new syscall wrapper we add, and (b) longer term try to move to a world where we do mandate minimum kernel versions.

in this specific case, epoll_create1 and inotify_init1 are so old that there aren't any devices still running with kernels that old.

[alexcohn]

The question is which fallback is safer: to use syscall() and expect the kernel to fulfill its contract, or to force ENOSYS for API < 21 and fail to get the valid kernel answer when the kernel actually supports __NR_epoll_create1.

My opinion is that the latter is safer, because we do know that libc does not export epoll_create1() under API 21, and nobody expected it to be called on this device. E.g. a developer of NDK library could assume that epoll_create1() won't be called, because he explicitly set APP_PLATFORM=android-14.

---

Please note that epoll_create1() was used above for illustration only. Same arguments work for other syscall cases, too. And I believe that the approach should be uniform for all these. It would be really confusing to provide different workarounds for inotify_init1 and epoll_create1().

[aberaud]

Also an issue in Asio (might be the same issue as Boost since Asio is now part of Boost)
https://github.com/chriskohlhoff/asio/blob/master/asio/include/asio/detail/impl/epoll_reactor.ipp#L548

From a user (app developer) point of view if EPOLL_CLOEXEC exists then epoll_create1 should exist for consistency, or there should be a simple compile-time way to check for it (checking for specific Android versions in user code is horrible, we should check for features not versions).

[DanAlbert]

I think we need to do something about this before r15 goes to stable, but we're still talking this one over (been busy with I/O this week). Quick summary of our options:

1. Add the inline that just returns ENOSYS.

I really don't like turning compile-time errors into run-time errors. Especially because in this case you could be running on a device where epoll_create1(EPOLL_CLOEXEC) doesn't work but syscall(__NR_epoll_create1, EPOLL_CLOEXEC) does, even though both compile.

2. Add the inline that performs the syscall for you.

The scary part about this is that, as @alexcohn pointed out, the device's kernel might have used that syscall number for an extension already, so this might end up calling some bogus syscall rather than the one you want (or returning ENOSYS).

@jmgao pointed out that we could just skip calling syscall(__NR_epoll_create1, EPOLL_CLOEXEC) and instead just go straight to epoll_create() plus fcntl.

3. Keep things as they currently are and send patches upstream.

While technically correct (the best kind of correct!), this isn't great. The number of people that have landed on bugs like this from just the beta is a pretty clear indication that we probably can't stand on the ground of "but that code is wrong".

I'd really like to take this option, but I think it will be an uphill battle to convince so many third party projects that the availability of syscalls says nothing about the availability of C library wrappers and vice versa.

4. Hide EPOLL_CLOEXEC pre-21.

This choice actively restricts functionality. The existence of epoll_create1 (the function in libc) and the availability of the syscall by the same name are in no way linked. sycall(__NR_epoll_create1, EPOLL_CLOEXEC) almost certainly will work on some pre-21 devices (epoll_create1 was added to the kernel in 2.6.27). I dug up some device stats for one of Google's apps, and they have less than 400 users on 2.6.x kernels (that's all the granularity I have for kernels that old, so even some of those will have this syscall).

This might be the least bad choice in the short term. If nothing else, this is the solution which can change our minds on with the least impact (this might be a decision best left until we work out any other kinks in unified headers). This also preserves the behavior of the old headers, so at the very least this doesn't make anything worse.

---

I was completely against 4 when I started writing this, but I think I've convinced myself that it's what we should do for r15 even if we don't stick with it for future releases.

Thoughts?

[alexcohn]

I still don't fully understand the risk of hiding EPOLL_CLOEXEC. People who are smart enough to use sycall(__NR_epoll_create1, EPOLL_CLOEXEC) can set EPOLL_CLOEXEC=O_CLOEXEC themselves, can't they? And make sure that they take full responsibility for bypassing libc.

[DanAlbert]

I suppose that's true. We all are pretty settled on option 4 for the time being anyway. Odds are we'll do that and no one will ever complain about this issue, so I'm overthinking something that no one actually cares about :)

[jmgao]

I doubt there's any actual risk, but from an puritanical standpoint, hiding EPOLL_CLOEXEC and not hiding __NR_epoll_create1 is the same sort of problem as showing EPOLL_CLOEXEC and not having epoll_create1, except even worse, since __NR_epoll_create1 and EPOLL_CLOEXEC actually are a logical unit that comes from the kernel.