0.2.61 regression

[carllerche]

The 0.2.61 release causes the Tokio CI to fail on FreeBSD:

Failure: https://cirrus-ci.com/task/5089534616797184

I tested the CI run with 0.2.60 and it passes: tokio-rs/tokio#1434

I checked the diff, but did not see anything that would obviously cause this. I will continue to investigate, but any advise would be appreciated.

[semarie]

I suspect #1440 to have changed some "default" values when LIB_CI isn't used (the standard case, because it should be used for CI only).

Some code changed from "defined when freebsd12 is absent" to "defined when freebsd11 is present" (which assume LIB_CI to be set). So the default behaviour isn't tested anymore.

@asomers @pizzamig I think build.rs should provide cargo:rustc-cfg=freebsd11 by default now, and not only when LIB_CI is defined.

[gnzlbg]

@semarie I also suspect #1440 introduced the issue.

I haven't looked at this into great detail, but a couple of things are clear (1) libc builds for the target, (2) the libc APIs that tokio uses are part of the built library (otherwise tokio would have failed to compile), and (3) the types of these APIs did not change substantially (otherwise tokio would have failed to compile).

That CI build job is using FreeBSD 12.0, so maybe this is caused by an ABI difference that's leading to undefined behavior ?

[semarie]

build.rs defines freebsd11 only when LIBC_CI is used:

    if env::var("LIBC_CI").is_ok() {
        if let Some(11) = which_freebsd() {
            println!("cargo:rustc-cfg=freebsd11");
        }
        if let Some(12) = which_freebsd() {
            println!("cargo:rustc-cfg=freebsd12");
        }
        if let Some(13) = which_freebsd() {
            println!("cargo:rustc-cfg=freebsd13");
        }
    }

so in normal build, there is no frebsd11 defined.

Now in src/unix/bsd/freebsdlike/mod.rs :

    #[cfg_attr(
        all(target_os = "freebsd", freebsd11),
        link_name = "kevent@FBSD_1.0"
    )]
    pub fn kevent(kq: ::c_int,
                  changelist: *const ::kevent,
                  nchanges: ::c_int,
                  eventlist: *mut ::kevent,
                  nevents: ::c_int,
                  timeout: *const ::timespec) -> ::c_int;

    #[cfg_attr(
        all(target_os = "freebsd", freebsd11),
        link_name = "mknodat@FBSD_1.1"
    )]
    pub fn mknodat(dirfd: ::c_int, pathname: *const ::c_char,
                  mode: ::mode_t, dev: dev_t) -> ::c_int;

before it was all(target_os = "freebsd", not(freebsd12)), so when no freebsd12 the link_name was used. so at least kevent() and mknodat() doesn't have the right link_name on freebsd11 now.

[gnzlbg]

@semarie Thanks, I agree that this is the bug, but I still don't understand why that is causing the tokio build jobs to fail.

Tokio is using FreeBSD12 on their CI, and when building libc without freebsd11 defined, kevent gets the kevent symbol. That would be bad, if the ABI of kevent and kevent@FBSD_1.0 differ. However, we test FreeBSD12 in our CI without freebsd11 and the tests for kevent pass. This would mean that FreeBSD12 kevent symbol being picked is ABI compatible with what we define in libc. That is, AFAICT, even though libc has an error, the symbol being called is at least ABI compatible with libc.

For this to be an issue, the semantics of the kevent API must have changed from FreeBSD11 to FreeBSD12, and what the tokio error means is that tokio requires FreeBSD11 semantics.

I think it would be worth it to exactly pin-point why the tokio tests are failing. Knowing this might be useful for the discussion about how to let users choose the FreeBSD version that libc targets (e.g. changing semantics of some ABI-compatible symbols is something that they should be aware of).

[pizzamig]

I guess that I introduced an error in the logic how things are handled.
@semarie is right, a freebsd11 or freebsd12 has to be always defined, not only when LIBC_CI is defined.

The struct kevent passed to kevent() is changed from 11 to 12. Hence:

• in FreeBSD 11 you have kevent@FBSD_1.0
• in FreeBSD 12 you have kevent@FBSD_1.0 and kevent@FBSD_1.5, to support the new feature, while providing retro-compatibility.

For what I can see, if freebsd12 is defined, the new definition of struct kevent is picked up (src/unix/bsd/freebsdlike/freebsd/freebsd12/mod.rs:20) while there is only one defintion for the kevent() system call (src/unix/bsd/freebsdlike/mod.rs:1156).
I've spot those kind of error with libc_test, however, I guess that the missing LIBC_CI can cause the confusion.
I'll work on a patch immediately

[gnzlbg]

@pizzamig there is a patch for the LIBC_CI issue already: #1467

I've spot those kind of error with libc_test

If the kevent struct is wrong for freebsd12 and up, it would be worth it to investigate why CI does not fail for it, and fix that and the struct definition.

[pizzamig]

@gnzlbg
The struct kevent is correctly defined in libc.
In FreeBSD 12 there is the extra field pub ext: [u64; 4]

In the mio crate (sys/unix/kqueue.rs) I see a macro creates a struct libc::kevent without the extra field. The compiler should complain about the missing field in the initializer.
I wonder how mio can use the freebsd11 struct kevent while the freebsd12 fn kevent is used,

[pizzamig]

I was able to successfully test tokio against a patched libc and a patched mio, where I added the additional field ext: [64; 4] to the mio macro kevent in sys/unix/kqueue.rs.

However, I don't know how to make the field ext: [64; 4] optional with the version of FreeBSD in the mio crate.
Any idea?

[DoumanAsh]

@pizzamig Can mio actually detect when the field present? I think without some dirty hacks it is unlikely, unless we somehow can easily spot freebsd 11 and 12?

But the fact that it compiled on 12 without problem indicates that field was missing in libc too?

Also which path will you take for kevent function on >=12?

[gnzlbg]

In the mio crate (sys/unix/kqueue.rs) I see a macro creates a struct libc::kevent without the extra field.

The problem probably is that, because cfg(freebsd11) is not true, the kevent function from FreeBSD12 is being called. However, because cfg(freebsd12) is not true either, the kevent struct from FreeBSD11 is being used, which lacks the extra field.

The kevent function of FreeBSD12 is probably invoking undefined behavior, because it is attempting to read or write from a struct field that the struct definition being used by tokio does not have.

The fix for this is to remove this catch all else block here:

https://github.com/rust-lang/libc/blob/master/src/unix/bsd/freebsdlike/freebsd/mod.rs#L1337

and replace it with an else if cfg(freebsd11) { ...}.

That way, if the appropriate cfg(freebsdXY) is not defined, instead of getting the definitions for freebsd11 by default, you'll get a compile-time error saying that these are not available.

[pizzamig]

@gnzlbg that's exactly what is happening. Nice catch!
Do you know how if there is a way to "export" this cfg from the libc crate, in such a way that it can be used by other crates?
In this case, mio has to know how kevent looks like (with or without the extra field).
The worst case scenario, is to duplicate the detection performed by build.rs in libc in every crate using kevent on FreeBSD, quite inconvenient..

[gnzlbg]

Do you know how if there is a way to "export" this cfg from the libc crate, in such a way that it can be used by other crates?

The libc crate only supports freebsd11. Other freebsd versions are not supported.

What you want is probably some sort of target-platform versioning, see #570 .

[DoumanAsh]

If freebsd11 is only supported version does it mean that kevent should be linked with kevent@FBSD_1.0 always?

Right now kevent link_name is kevent@FBSD_1.0 only on 11, but what about 12?
If it is not supported, is it going to be ignored?

In this case, mio has to know how kevent looks like (with or without the extra field).

As work-around it would be good to have ctor that could allow initialize common fields, and the rest could be done by user, if necessary

[gnzlbg]

If freebsd11 is only supported version does it mean that kevent should be linked with kevent@FBSD_1.0 always?

Yes.

but what about 12?
If it is not supported, is it going to be ignored?

Yes.