Update NDK to r15b and add some missing symbols #634

ndusart · 2017-06-29T21:46:28Z

Use the new unified headers of the NDK and add some missing symbols for Android.

Fixes #632

alexcrichton · 2017-06-30T05:42:46Z

Thanks for the PR! I think a number of the changes here are breaking changes, right? Unfortunately we won't be able to land those at that this time, but mind detailing what's breaking?

ndusart · 2017-06-30T09:52:33Z

Yes, there are some breaking changes, but unfortunately, according to Google:

Before NDK r14, we had a set of libc headers for each API version. In many cases these headers were incorrect. Many exposed APIs that didn‘t exist, and others didn’t expose APIs that did.

So the current binding is based on partially broken headers.
The incompatibilities we have here are mainly just types with wrong signedness. Here is the list:

strerror_r is changed from int strerror_r(int, char*, size_t) to char* strerror_r(int, char*, size_t). It seems just to be an addition of a new function, we could force to use the old definition to preserve compatibility with all android versions. As asked in #632, I need to know if it is acceptable to modify the headers seen by the CI for accepting the old symbol.

===

stat struct has changed. These fields

  long st_atime; \
  unsigned long st_atime_nsec; \
  long st_mtime; \
  unsigned long st_mtime_nsec; \
  long st_ctime; \
  unsigned long st_ctime_nsec; \

as changed to:

  struct timespec st_atim; \
  struct timespec st_mtim; \
  struct timespec st_ctim; \

where timespec is defined as:

struct timespec {
  time_t tv_sec;
  long tv_nsec;
};

There are some #define to present the st_atime_nsec field:

/* Compatibility with older versions of POSIX. */
#define st_atime st_atim.tv_sec
#define st_mtime st_mtim.tv_sec
#define st_ctime st_ctim.tv_sec
/* Compatibility with glibc. */
#define st_atimensec st_atim.tv_nsec
#define st_mtimensec st_mtim.tv_nsec
#define st_ctimensec st_ctim.tv_nsec

Notice the change from st_atime_nsec to st_atimnsec. I made this change using r14b, since r15b, they added another #define so that st_atime_nsec is available again. (commit). I will fall back to using st_atime_nsec for not breaking that but there is still a breaking change as the *_nsec variables change from unsigned long to long.

===

Some functions have an argument that changed from const void* to void*. It's the case of madvise, msync, mprotect, and recvfrom.

Maybe for these, we could let the const but we need to change the headers in the CI too.

===

get/setpriority has a signedness change for their who argument:

int getpriority(int, int);

changed to

int getpriority(int, id_t);

where id_t is uint32_t

We could still keep the old type, as they have the same size.

===

personality has changed from int personality (unsigned long persona) to int personality(unsigned int persona)

In Android 32bits, these are exactly the same ABI but the test fail because it's not the same C API.
If we can change the headers, this can easily be changed to keep using the unsigned long.

So, we could avoid making a breaking change for all these but it requires to keep the binding to broken definitions. For example, the getpriority never used the int argument in the bionic source.

I don't mind waiting for the crate going to release a breaking change if that allow the change to be cleaner. Is that planned in a foreseeable future ?

roblabla · 2017-06-30T11:50:45Z

Worth noting that, AFAICT, 015f637 doesn't contain any breaking changes. Only 4d9b179 does. Maybe it'd be a good idea to split this PR so that the non-breaking commit can get in libc sooner ?

ndusart · 2017-06-30T12:29:33Z

Unfortunately no, 015f637 need the changes made by 4d9b179.
pty.h is only present in the new unified headers, CLONE_NEWCGROUP as well.

I think we shouldn't try to fix it as soon as possible.
I think we cannot avoid to make breaking changes to do it correctly (while preserving backward compatibility with older android version).

ndusart · 2017-06-30T22:00:34Z

I set strerror_r back to its old definition.
Needed to modify header for the test to pass, please tell me if you find it as a bad practice.
@alexcrichton, maybe I should add strerror_r in the cfg.skip_fn function in the build.rs instead ?

With the header modification, we still check if the rust libc points to the same function as the C one.

roblabla · 2017-06-30T22:32:46Z

Another idea if the current solution is inacceptable : Maybe we should check how to undefine __USE_GNU ? From sys/cdefs.h :

/*
 * With bionic, you always get all C and POSIX API.
 *
 * If you want BSD and/or GNU extensions, _BSD_SOURCE and/or _GNU_SOURCE are
 * expected to be defined by callers before *any* standard header file is
 * included.
 *
 * In our header files we test against __USE_BSD and __USE_GNU.
 */
#if defined(_GNU_SOURCE)
# define __USE_BSD 1
# define __USE_GNU 1
#endif

#if defined(_BSD_SOURCE)
# define __USE_BSD 1
#endif

So maybe we should disable _GNU_SOURCE (I don't think we're actually using any gnu extension as of yet in the android source, but I'm not 100% certain) in the tests ?

I'm not certain what currently makes _GNU_SOURCE enabled, but it should be possible to disable it ?

ndusart · 2017-06-30T23:09:11Z

It seems to be defined by the compiler itself.

#include <stdio.h>

int main() {
 #if defined(_GNU_SOURCE)
  printf("_GNU_SOURCE\n");
 #else
  printf("non GNU\n");
 #endif
}

On my machine, compiling this program with gcc outputs "non GNU", while compiling using g++ outputs "_GNU_SOURCE". Probably the arm-linux-androideabi-gcc is defining this constant, didn't try yet.

Maybe we could just add #undef _GNU_SOURCE in one of the headers included by all.c.
I do not know if it has some unwanted consequence though.

alexcrichton · 2017-07-01T16:12:03Z

Thanks for the cataloging @ndusart! I definitely agree that we should fix all these bindings at the next available chance, but for now with libc 0.1 we can't change these. In that sense I think the question is best framed as 'how do we verify these now and check them later'? It's quite a relief that nothing here is an ABI-breaking change, which is good!

In that sense, I see a few options here:

We could ignore everything that "broke" and file an issue with all known breakage. All tests for these fields and such would be ignored, and the next major release of libc would fix all these definitions.
We could add clauses in libc-test/build.rs to map everything to the correct types (maybe this is possible?) and get all tests passing but file an issue for other ignored pieces.

Or... something along those lines. Stuff like switching to timespec is ok as we expand those fields in many other stat definitions I believe, but the signedness and types changing is pretty bad. My guess is we'll just have to ignore the tests for everything here, filing an issue to update it all once we get a new major version of libc.

For strerror_r I think it's fine to ignore it as well. If the old symbol is still in libc.so then Rust programs will continue to work, and we can log in the issue what we should change to when we update.

ndusart · 2017-07-01T21:33:20Z

Beside the signedness changes, there is no ABI breaks indeed. And according to the new headers (which should apply for all Android version even older ones), these functions and stat struct were apparently always like these. It's probably not a big deal if we still use the old definition for some time as there are used for a long while without big problems obviously.

So I agree we could just ignore these tests until the next major release.
I undid the different modifications and modify build.rs for skipping the tests.

ndusart · 2017-07-01T22:04:11Z

Sorry, didn't test for android 64 bits and some functions specific to 64 bits have changed as well. I'll check for these and add them to ingoring list as the other.

I'll append them to the list of changes and report an issue for the next major version of libc.

ndusart · 2017-07-02T09:19:37Z

Funnily, the symbols that have problems now in src/unix/notbsd/android/b64/mod.rs are all in a block about some "weirdness" in Android:

// Some weirdness in Android
extern {
    // address_len should be socklen_t, but it is c_int!
    pub fn bind(socket: ::c_int, address: *const ::sockaddr,
                address_len: ::c_int) -> ::c_int;

    // the return type should be ::ssize_t, but it is c_int!
    pub fn writev(fd: ::c_int,
                  iov: *const ::iovec,
                  iovcnt: ::c_int) -> ::c_int;

    // the return type should be ::ssize_t, but it is c_int!
    pub fn readv(fd: ::c_int,
                 iov: *const ::iovec,
                 iovcnt: ::c_int) -> ::c_int;

    // the return type should be ::ssize_t, but it is c_int!
    pub fn sendmsg(fd: ::c_int,
                   msg: *const ::msghdr,
                   flags: ::c_int) -> ::c_int;

    // the return type should be ::ssize_t, but it is c_int!
    pub fn recvmsg(fd: ::c_int, msg: *mut ::msghdr, flags: ::c_int) -> ::c_int;
}

Those weirdness are specifically what are fixed by the new headers in NDK.

For bind, address_len was c_int instead of socklen_t but these two are i32, it's not either an ABI break nor a C API break. We could fix it without being a breaking change, right ? Or when pub type A = B, A and B are considered completely different types in Rust?

For the other functions, they return c_int instead of ssize_t (64 bits in 64 bits arch). So it's definitely an ABI breaking change but as told before, the implementation always used the new definition.
I think it worked because ARM are little-endian by default, then if the return value is representable by i32, it was correctly retrieved.

The solution could be also just to add them to the skipping list and wait the next major relase for the real fix.

@alexcrichton Do you agree ? And what about bind function, do we wait for it as well ?

ndusart · 2017-07-03T09:05:53Z

@alexcrichton I ignored the breaking changes and the CI is passing now. I just had to update the sys-img used for testing x86_64 from API 21 to 24, so that it contains the new functions (especially forkpty in this case).
It's then ready to merge if you think all is ok.

roblabla · 2017-07-03T11:01:22Z

src/unix/notbsd/android/mod.rs

@@ -562,6 +565,7 @@ pub const TIOCMBIC: ::c_int = 0x5417;
 pub const TIOCMSET: ::c_int = 0x5418;
 pub const FIONREAD: ::c_int = 0x541B;
 pub const TIOCCONS: ::c_int = 0x541D;
+pub const CLONE_NEWCGROUP: ::c_int = 0x02000000;


I promise this is the last nitpick :P

Maybe this should be moved along with https://github.com/rust-lang/libc/search?utf8=%E2%9C%93&q=CLONE_NEWCGROUP&type= to be with the others ?

libc/src/unix/notbsd/mod.rs

Line 689 in 83f5543

pub const CLONE_VM: ::c_int = 0x100;

It's not defined in sparc64, but I strongly believe it to be a bug since I can't find any mention of this flag not being defined on SPARC for linux.

I agree, these constants definitely should be merged.

The constant is defined on all architectures: https://github.com/torvalds/linux/blob/master/include/uapi/linux/sched.h

…ded by nix crate

alexcrichton · 2017-07-03T15:48:02Z

@ndusart

Or when pub type A = B, A and B are considered completely different types in Rust?

Correct yeah, if the typedefs all resolve to the same thing then it's fine for us to change the names here.

So it's definitely an ABI breaking change but as told before, the implementation always used the new definition.

Gee this is terrifying :(

Ok in the meantime though this looks great, thanks for the effort @ndusart! Mind also opening an issue detailing the breakage that we need to cover for the next major release of libc?

@bors: r+

bors · 2017-07-03T15:48:04Z

📌 Commit 4abc3ce has been approved by alexcrichton

bors · 2017-07-03T16:09:59Z

⌛ Testing commit 4abc3ce with merge ab78851...

Update NDK to r15b and add some missing symbols Use the new unified headers of the NDK and add some missing symbols for Android. Fixes #632

Susurrus · 2017-07-03T17:02:42Z

libc-test/build.rs

@@ -534,6 +539,16 @@ fn main() {
            // On Mac we don't use the default `close()`, instead using their $NOCANCEL variants.
            "close" if apple => true,

+            // Definition of those functions as changed since unified headers from NDK r14b


This should be "have changed", correct?

Yes, sorry for the typo. Do you think we should fix it ?

Nah, already merged. Maybe if you create a new PR for #641.

Yeah, and when fixing that issue, we'll certainly reword that comment in any way :)

bors · 2017-07-03T17:17:30Z

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing ab78851 to master...

ndusart · 2017-07-03T17:22:21Z

Mind also opening an issue detailing the breakage that we need to cover for the next major release of libc?

Done, created #641

alexcrichton mentioned this pull request Jul 1, 2017

Not up-to-date with latest Android NDK #632

Closed

ndusart force-pushed the master branch from 2c46146 to 0299161 Compare July 1, 2017 21:07

ndusart force-pushed the master branch from 0299161 to 3401b30 Compare July 3, 2017 08:42

roblabla reviewed Jul 3, 2017

View reviewed changes

Update to NDK r15b (with unified headers) and add missing symbols nee…

4abc3ce

…ded by nix crate

ndusart force-pushed the master branch from 3401b30 to 4abc3ce Compare July 3, 2017 11:29

bors added a commit that referenced this pull request Jul 3, 2017

Auto merge of #634 - ndusart:master, r=alexcrichton

ab78851

Update NDK to r15b and add some missing symbols Use the new unified headers of the NDK and add some missing symbols for Android. Fixes #632

Susurrus reviewed Jul 3, 2017

View reviewed changes

bors merged commit 4abc3ce into rust-lang:master Jul 3, 2017

roblabla mentioned this pull request Jul 3, 2017

Allow nix to compile on android nix-rust/nix#631

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update NDK to r15b and add some missing symbols #634

Update NDK to r15b and add some missing symbols #634

ndusart commented Jun 29, 2017

alexcrichton commented Jun 30, 2017

ndusart commented Jun 30, 2017 •

edited

Loading

roblabla commented Jun 30, 2017

ndusart commented Jun 30, 2017

ndusart commented Jun 30, 2017 •

edited

Loading

roblabla commented Jun 30, 2017 •

edited

Loading

ndusart commented Jun 30, 2017

alexcrichton commented Jul 1, 2017

ndusart commented Jul 1, 2017

ndusart commented Jul 1, 2017

ndusart commented Jul 2, 2017 •

edited

Loading

ndusart commented Jul 3, 2017

roblabla Jul 3, 2017

ndusart Jul 3, 2017 •

edited

Loading

alexcrichton commented Jul 3, 2017

bors commented Jul 3, 2017

bors commented Jul 3, 2017

Susurrus Jul 3, 2017

ndusart Jul 3, 2017

Susurrus Jul 3, 2017

ndusart Jul 3, 2017

bors commented Jul 3, 2017

ndusart commented Jul 3, 2017

Update NDK to r15b and add some missing symbols #634

Update NDK to r15b and add some missing symbols #634

Conversation

ndusart commented Jun 29, 2017

alexcrichton commented Jun 30, 2017

ndusart commented Jun 30, 2017 • edited Loading

roblabla commented Jun 30, 2017

ndusart commented Jun 30, 2017

ndusart commented Jun 30, 2017 • edited Loading

roblabla commented Jun 30, 2017 • edited Loading

ndusart commented Jun 30, 2017

alexcrichton commented Jul 1, 2017

ndusart commented Jul 1, 2017

ndusart commented Jul 1, 2017

ndusart commented Jul 2, 2017 • edited Loading

ndusart commented Jul 3, 2017

roblabla Jul 3, 2017

Choose a reason for hiding this comment

ndusart Jul 3, 2017 • edited Loading

Choose a reason for hiding this comment

alexcrichton commented Jul 3, 2017

bors commented Jul 3, 2017

bors commented Jul 3, 2017

Susurrus Jul 3, 2017

Choose a reason for hiding this comment

ndusart Jul 3, 2017

Choose a reason for hiding this comment

Susurrus Jul 3, 2017

Choose a reason for hiding this comment

ndusart Jul 3, 2017

Choose a reason for hiding this comment

bors commented Jul 3, 2017

ndusart commented Jul 3, 2017

ndusart commented Jun 30, 2017 •

edited

Loading

ndusart commented Jun 30, 2017 •

edited

Loading

roblabla commented Jun 30, 2017 •

edited

Loading

ndusart commented Jul 2, 2017 •

edited

Loading

ndusart Jul 3, 2017 •

edited

Loading