Clean up libc-test/build.rs for FreeBSD and enable testing FreeBSD12 on CI

[gnzlbg]

No description provided.

[rust-highfive]

@gnzlbg: no appropriate reviewer found, use r? to override

[gnzlbg]

@asomers @myfreeweb it appears that EVFILT_EMPTY is not available in FreeBSD 11, but the comment in the build.rs says that the constant was added in that FreeBSD version. Is the comment incorrect?

[asomers]

Yep, that's wrong EVFILT_EMPTY was added by r312277 and made it into 12.0, but was never merged to stable/11. The erroneous comment was added by 2aeb382 .

https://svnweb.freebsd.org/base?view=revision&revision=312277

[asomers]

The title of this PR seems misleading. Tests were never disabled on FreeBSD, were they? It seems like what this PR actually does is enable nightly tests, --no-default-features tests, and --features extra_traits tests for FreeBSD. Perhaps the PR should be titled "enable full testing on FreeBSD"?

[gnzlbg]

There are many tests for many constants, functions, structs, etc. that were disabled at some point or another. Some of them were disabled in #1337 to unbreak CI. This PR goes through all disabled tests, and re-enables all that can be made to work.

[gnzlbg]

The latest logs show the failures that occur. Most of them appear upgradeable or easily "ignored".

[asomers]

Nonetheless, until #570 is fixed we should be targetting FreeBSD 11 (though we can update the image from 11.1 to 11.2). Upgrading the test image to 12.0 will cause a net reduction in test coverage.

[asomers]

Please revert those latest changes! They'll break all Rust programs on FreeBSD 11. Prior to those changes, Rust could run on both FreeBSD 11 and FreeBSD 12, it just couldn't use some of 12's newer features.

[asomers]

I suspect that this will fail. Because while FreeBSD amd64 can run i386 binaries, it doesn't include i386 headers, and libc's CI tests need to interrogate those headers. Testing FreeBSD i386 in CI will require an actual i386 environment, either a VM or a jail on an amd64 host.

[gnzlbg]

Ideally we would use qemu-user for this, we need something like that to test aarch64-unknown-freebsd and friends as well.

[asomers]

qemu isn't necessary for running i386 binaries on an amd64 host; that capability is builtin. All you need is an i386 jail, or even a chroot. See https://github.com/nix-rust/nix/blob/master/.cirrus.yml for an example of testing i386 binaries for a project that doesn't need headers.

[gnzlbg]

We do need headers, so that .cirrus.yml does not help much.

[asomers]

This PR can't be merged until the FreeBSD 12-specific changes are reverted, because they'll break Rust on FreeBSD 11.

[gnzlbg]

This PR can't be merged until the FreeBSD 12-specific changes are reverted, because they'll break Rust on FreeBSD 11.

Not all changes break Rust on FreeBSD 11 (e.g. mprotect pointer mutability), and some changes seem to imply that using Rust on FreeBSD 12 will invoke UB (e.g. the dirent changes).

It would be more useful if you could be more specific about which precise changes break what.

[gnzlbg]

FWIW "WIP" means work in progress. No need to panic about this getting merged.

[asomers]

This PR can't be merged until the FreeBSD 12-specific changes are reverted, because they'll break Rust on FreeBSD 11.

Not all changes break Rust on FreeBSD 11 (e.g. mprotect pointer mutability), and some changes seem to imply that using Rust on FreeBSD 12 will invoke UB (e.g. the dirent changes).

It would be more useful if you could be more specific about which precise changes break what.

There's no UB here; that's a compiler concept. Currently, using libc on FreeBSD 12 is safe thanks to ELF symbol versioning. For example, libc sets readdir's link_name to readdir@FBSD_1.0. That means that whether the OS is 11 or 12, the program will link to a version of readdir that returns FreeBSD 11's version of dirent. So using Rust's libc on FreeBSD 12 works, but it's limited to FreeBSD 11's capabilities. For example, if the running OS happens to have a file system mounted on a directory that has more than 88 characters, then the Rust program won't be able to see the full mountpoint path.

[gnzlbg]

Part of re-enabling all FreeBSD tests is figuring out what's broken. The code used to figure out what is broken does not necessarily have to be merged into libc, I don't know how you are jumping to the conclusion that the --cfg hack will be merged. Again: WIP means work-in-progress.

[gnzlbg]

There's no UB here;

freelocale in FreeBSD 12 returns void, but the FreeBSD 11 API returns int. We do not link freelocale to the FreeBSD 11 version AFAICT. That is, a user can call it, and inspect the int returned, which, if the function is called in FreeBSD 12, is uninitialized (and therefore it existing is already UB). If the FreeBSD 12 function were to actually return an int, but has a type-signature stating that it returns void, AFAICT, that would also be UB, because the int returned will overrite some part of the calling function stack.

How is the undefined behavior avoided here?

[gnzlbg]

The msgrcv function shows a similar issue, where the size of the return value changes between FreeBSD 11 and FreeBSD 12, and we do not link against the FreeBSD 11 version.

[asomers]

I don't think there's any UB issues here, since freelocale and msgrcv are external functions. There would only be UB if the compiler knew that freelocale's return value were uninitialized. But since these are FFI functions, the compiler believes that the value is initialied. It might return register garbage, but it won't do anything worse than that.

msgrcv's case is different. In that case the header in FreeBSD 11 is a mistake. The syscall was always returning the value as a ssize_t, but the prototype was wrong. In this case, we could probably change libc's definition to use ssize_t, but I might check with Ed Maste first.
https://svnweb.freebsd.org/base?view=revision&revision=303435

[gnzlbg]

From your comment in the other issue, for freelocale, we could probably just remove the int return type on FreeBSD 11 as well.

[gnzlbg]

So @asomers slowly this is taking a finished form, I'd like to hear your feedback. Basically, what I've done is split the freebsd module into two modules, one for freebsd11 and one for freebsd12. The FreeBSD11 is always used by default, and there are no direct options to compile the freebsd12 one.

In libc's CI, we define the LIBC_CI env variable, and if that's defined, and the host system is a FreeBSD 12 system, then the freebsd12 module is compiled and tested, which only triggers in the FreeBSD 12 cirrus images.

There are some cfg(freebsd12)s splittered around, mostly in the unix/mod.rs module, whether it makes sense to slowly move them to their appropriate modules or not.. can always be done later.

API wise, would it make sense to do something different for uname in FreeBSD 12 ? Also, we probably want to fix the return type of msgrcv to be ssize_t on FreeBSD 11 as well, but that can be done in a sub-sequent PR (this one should contain no breaking changes).

[gnzlbg]

So our API wrapper for lio_listio is AFAICT unsound. We do accept pointers that alias, but the C API uses restrict, which requires these pointers not to alias with anything. I'm going to leave it like this for now, but we probably want to use references & &mut ... here to imply noalias, but that would require teaching ctest to emit restrict when generating pointer types for references to fix this.

[bors]

💔 Test failed - checks-travis

[gnzlbg]

@bors: retry

[bors]

⌛ Testing commit 7437d0a with merge 9672c8d...

[bors]

💔 Test failed - checks-travis

[gnzlbg]

@bors: r+

[bors]

💡 This pull request was already approved, no need to approve it again.

• This pull request previously failed. You should add more commits to fix the bug, or use retry to trigger a build again.

[bors]

📌 Commit 7437d0a has been approved by gnzlbg

[bors]

⌛ Testing commit 7437d0a with merge e148c9d...

[bors]

💥 Test timed out

[gnzlbg]

@bors: retry

[bors]

⌛ Testing commit 7437d0a with merge 819405b...

[gnzlbg]

For some reason bors does not like this PR. It runs green on all CI, but then bors fails to receive the notification that everything succeeds, and times out. Maybe closing and re-opening the PR helps, otherwise I'll merge it manually.

[bors]

💔 Test failed - checks-travis

[asomers]

The aarch64-android build is timing out. That's probably why homu isn't merging.

[gnzlbg]

That is happening very intermittently (e.g. master is green). The problem here is that even when the travis job was green homu was still not merging this PR. I don't know why, but in the queue it went from "pending" (as in CI is running) back to "approved" (as in, CI needs to run) every time the travis job finished green =/

[gnzlbg]

I thought maybe pulling it off the queue and re-adding it (by closing and re-opening the PR) would maybe fix that, but it didn't =/

[asomers]

You changed the name of the Cirrus-CI task. Do you need to make a matching change in homu's configuration, too?

[gnzlbg]

doesn't homu pick up all tasks automatically independently of their name?(I also added a new task here)

[asomers]

It can't, because it wouldn't know that a task existed until that task started. You wouldn't want homu to merge a PR before a CI task had started.

[gnzlbg]

There is a PR in rust-central-station to fix that