The type of eventfd's 'initval' argument should be unsigned.

[jimblandy]

This is a fix for issue #120.

I've noticed that, of the four definitions for eventfd in the tree, if this pull request is accepted, three use c_uint for the first argument, and the only remaining one is src/unix/notbsd/linux/mips.rs, which I suspect is not being tested much. If we can find anyone to test the MIPS build, we might well find that the correct fix is to simply common up the three linux definitions into a single one in src/linux/notbsd/linux.mod.rs, which would be nice.

[jimblandy]

I should point out that the unsigned initval argument matches what's on the man page, as well as the header file.

[jimblandy]

It turns out that eventfd's first argument is unsigned on Fedora (glibc), but int on Ubuntu (eglibc). Note the delightful contrast between Ubuntu's code and Ubuntu's man pages. (The CI logs tell me Trusty is the Ubuntu that Travis is using.)

[jimblandy]

This version of the push adds eventfd to the list of functions to skip in libc-test/build.rs.

Alternative approaches were:

• Create EGLIBC- and GLIBC-specialized sub-modules. But EGLIBC might fix its bug, and this would require a build.rs for libc itself, which seems like overkill.
• Instead of a direct 'extern' declaration for eventfd, add a thin wrapper that performs the syscall directly. There's no precedent for this in the crate, so I was reluctant to try this.

[jimblandy]

Those Travis-CI failures don't make a lot of sense to me. One says:

error: the crate std has been compiled with rustc 1.7.0-nightly (d4b67cd7c 2016-01-01), which is incompatible with this version of rustc [E0514]

but as far as I know, 1.7.0-nightly should be the only version of rustc involved with this build at all. Perhaps it's a conflict between targets, not rustc versions?

The MIPS build has died for reasons that don't seem like they have anything to do with my patch:

/usr/lib/gcc/mips-linux-gnu/4.4.5/../../../../mips-linux-gnu/bin/ld: Dwarf Error: found dwarf version '4', this reader only handles version 2 and 3 information.
/usr/lib/gcc/mips-linux-gnu/4.4.5/../../../../mips-linux-gnu/bin/ld: Dwarf Error: found dwarf version '0', this reader only handles version 2 and 3 information.
/home/travis/rust/lib/rustlib/mips-unknown-linux-gnu/lib/liballoc_jemalloc-17a8ccbd.rlib(jemalloc.pic.o): In function malloc_conf_init': /buildslave/rust-buildbot/slave/nightly-dist-rustc-cross-linux/build/src/jemalloc/src/jemalloc.c:(.text.malloc_conf_init+0x94): undefined reference tosecure_getenv'
/usr/lib/gcc/mips-linux-gnu/4.4.5/../../../../mips-linux-gnu/bin/ld: Dwarf Error: found dwarf version '0', this reader only handles version 2 and 3 information.
/usr/lib/gcc/mips-linux-gnu/4.4.5/../../../../mips-linux-gnu/bin/ld: Dwarf Error: found dwarf version '3072', this reader only handles version 2 and 3 information.
/buildslave/rust-buildbot/slave/nightly-dist-rustc-cross-linux/build/src/jemalloc/src/jemalloc.c:(.text.malloc_conf_init+0x98): undefined reference to `secure_getenv'

If someone found "dwarf version '3072'" something is deeply wrong.

[jimblandy]

Those failures are all covered by #129, so I don't think they reveal anything wrong with this pull request. It would be better to fix #129 and get a clean bill of health for this pull request before merging, but we have no evidence there's anything wrong with these changes.

[brson]

@alexcrichton This is a breaking change. How should the versioning be dealt with?

[alexcrichton]

Ah yes this was an interesting situation where the man page disagrees with the actual definition itself. @jimblandy could you elaborate a bit on why this should be changed? Does this type differ depending on the libc you're using?

[jimblandy]

@alexcrichton Yes, it depends on the libc. On EGLIBC the libc type mismatches the kernel type; on GLIBC they match. The libc crate matches EGLIBC.

All systems' definitions will conflict with the libc crate's soon. EGLIBC was forked from GLIBC, which was then adopted by Debian and derived distributions. However, EGLIBC has re-merged with GLIBC, and as of Debian Jesse (8.0, April 2015), Debian has switched back to GLIBC.

My immediate impetus for the pull request was simply that I ran libc-test on my machine and it failed, so I tried to investigate the failures; nothing more important than that.

[alexcrichton]

@jimblandy hm fascinating!

So is the correct conclusion to draw from that in that everyone is converging on this being unsigned? If that's the case them I'm fine with this patch as-is.

@brson if landing this then I don't think we need to worry much about breakage. This was recently added and it's highly likely that the first argument is always a constant. If breakage comes up then I think it's covered under our minor APi breaking changes policy (e.g. a cast makes it go away).

[jimblandy]

So is the correct conclusion to draw from that in that everyone is converging on this being unsigned?

Since EGLIBC is no longer being used by Debian (and hence Ubuntu), I think this is the case.

[alexcrichton]

OK cool, sounds good to me! If you want to rebase I'll merge once tests pass

[brson]

@alexcrichton I'm not yet convinced that this is an acceptable API change - it's definitely not according to semver, and skimming the API evolution RFC I don't see anything about changing function parameters being ok if it just requires a cast.

I would like some clarity on what we can and should do here. Every time we bend the rules a little it violates our contract with our users.

To be clear, please do not merge this yet.

cc @rust-lang/libs

Edit: this was recently added. Has it already been published in the existing form?

[jimblandy]

To help weigh the consequences of merging and not merging:

The eventfd definition with the bad argument type was introduced in commit 8dce9ad on 2015-12-03. Looking at the dates, it seems it was published as 0.2.3 and 0.2.4.

Without merging, the libc crate's eventfd will eventually disagree with all the actual Linux systems whose eventfd functions it's exporting. Rust will consider the first argument to be signed, whereas the kernel will see it as unsigned. This will only affect programs that accidentally pass negative values as the first argument, which will probably be very rare.

[alexcrichton]

I just grepp'd all of crates.io and no one's most recent version of their crate is using eventfd from libc. If anyone was I'd be fine just leaving as is saying "whelp, oh well", but I don't think this has been out there long enough for anyone to actually start using it.

it's definitely not according to semver

From the principles of the API evolution RFC, this isn't a breaking change under our definition of what a breaking change is.

Every time we bend the rules a little it violates our contract with our users.

We've always known that we need to take breaking changes with a grain of salt, we'd make virtually no progress if we were ironclad in following the letter of the law literally everywhere. That basically means that until we fix the compiler we can't add any APIs anywhere.

Now that being said we're the ones judging the "whether this is a breaking change in practice" and I can see where we may be bending things a little there because it's subjective. I just want to make sure we're coming at this from the angle of "let's try to minimize actual breakage" rather than "let's prevent all hypothetical breakage whatsoever".

---

So in terms of this actual change, I don't think this matters that much in terms of correctness. If this is contentious it's basically not worth it. This matches the definition of a very popular version of glibc (although it may perhaps change in the future), and the only "wrong values" are negative ones (as they get interpreted as very-large-positive values) and those are probably never actually passed into the function. Essentially, in terms of ABI and functional changes, this won't do much.

I'm fine moving this towards the "expected" definition, however, in terms of aesthetics basically. This matches what's in the man pages and also found on other distributions (and libc variants as @jimblandy has found)

---

also, since rust-lang teams don't work here:

cc @sfackler, @aturon, @huonw, @gankro, @BurntSushi, @Kimundi, @bluss

[brson]

From the principles of the API evolution RFC, this isn't a breaking change under our definition of what a breaking change is.

It is a breaking change under any reasonable definition. The policy states what minor breakage we might tolerate. This may or may not be that, but it does violate semver, and it is breaking.

That basically means that until we fix the compiler we can't add any APIs anywhere.

It means that we need to be conscientious about how our changes affect our users and weigh the pros and cons of our decisions.

For my part, I think this is a change we must make to conform to the underlying API, but we need to make it the right way. Just making a breaking API change silently in a point release is not the way. Even if it affects nobody in the world this time, next time maybe we guess wrong and it does.

I think the correct way is to do a major version bump, but I suspect we will not be willing to do that. Barring that it may be time to publicise rust-lang crate releases distributed with release notes. Barring that, well I guess we're just going to break stuff silently. In any case it's a decision that should be made carefully, not off-hand.

[BurntSushi]

I think there's another option, although perhaps less satisfying. Since this falls under "bug fix, not severe," there's no time pressure to fix it right this second. We could, for example, accept the fix but not publish a version with it until some later point in time (e.g., when there are more $reasons to bump 0.2 to 0.3, or even wait until a 1.0). I'm not sure we have the right infrastructure to pull that off, other than leave the PR unmerged until the release of 0.3.

My opinion is to not publish a change like this in a patch release. I get that it's probably not-a-big-deal in isolation, but I would definitely be concerned about the precedent it sets.

[brson]

Could we bump to 0.3 just for this? I'm not sure there are big downsides, and it's what version numbers are for.

Point releases mainly fix bugs in existing code, but that doesn't apply at all to libc since it's nothing but type definitions. IOW, users aren't expecting to receive automatic bugfixes to the versions of libc they've declared. Every release of libc could be a major bump.

[BurntSushi]

@brson You could in theory release additions to libc in a point release. A 0.3 release sounds fine to me though.

[alexcrichton]

I don't think that we've figured out how to do a major version bump of this library yet, the last 0.1 -> 0.2 transition caused quite a bit of pain and nothing about the tooling has changed since then.

This is not worth it to change in my opinion if we want to make a major version bump. There are 0 ABI implications and I highly doubt that anyone will ever actually run into this.

[BurntSushi]

@alexcrichton Isn't the reason why the last 0.1 -> 0.2 transition caused pain because it actually did have a lot of breaking changes and people were using libc = "*" everywhere? I think the former should at least not be an issue here. I feel like people have gotten better about * deps, but I don't have any numbers.

[alexcrichton]

That was certainly one source of pain, but the other is that libc is a "shared dependency" in many many places. This means that fundamentally lots of crates need to agree on the libc version because they're sharing types from libc.

When we release a new major version, any crate which updates starts a ripple effect of all downstream dependencies now needing to update as well to ensure that crates agree on types. Cargo can give better error messages here and also help structure the resolution graph a little better, but this is still a somewhat fundamental problem. The tooling on the Cargo side to ease the transition is not implemented yet.

[BurntSushi]

@alexcrichton Ah, right. Forgot about that. Good point. In that case, I'd agree a 0.3 release is not worth it for this change alone. What about leaving it unmerged and incorporating it in the next "natural" release?

[alexcrichton]

@jimblandy if you want to update this PR to just ignore the eventfd test for Linux to get tests passing locally for you (or submit an alternate PR with that), I could merge that in the meantime.

[jimblandy]

@alexcrichton Opened as PR #136.

[jimblandy]

I've rebased this pull request given that #136 has been merged.

[brson]

Per my offline conversations with @alexcrichton let's just do it. We'll put more consideration in how to deal with this type of situation, but we don't have to hold it up this time, since the practical impact is likely to be low.

[alexcrichton]

Ok, I'm going to merge this with #149 so we don't have to rebase all these PRs.