Update Emscripten system types

[tlively]

These changes bring the types up to parity with recent Emscripten
versions using the upstream LLVM wasm backend. These changes should be
coordinated with the upgrade of rustc's Emscripten support. See
https://internals.rust-lang.org/t/upgrading-rust-s-emscripten-support/10684

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @gnzlbg (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[gnzlbg]

@tlively sorry that it took so long to come back to this.

@sunfishcode @alexcrichton this is probably going to be a pain to land since it is a backward incompatible change to the target.

It appears to me that merging this and releasing a new libc version would be required to be able to merge a rust-lang/rust PR updating emscripten, but in the time-frame between the libc version release and the merge of an upstream PR, libc would be broken for the target.

Also, new libc versions with these changes would be broken for older Rust toolchains - do any stable Rust toolchains support the wasm32-unknown-emscripten and asmjs-unknown-emscripten targets ? If the answer is "no", I think that once there is an upstream PR in rust-lang/rust for which "bors try" passes with these changes, we can make the changes here, do a release, and update the upstream PR to the released libc version and merge it with p=infinity.

If the answer is "yes, we have some stable toolchains for x-unknown-emscripten" then... this is going to require a bit more work. We could add one such toolchain to CI, and in the build.rs detect it, and expose the old types for it, making this change exclusively for nightly for now. I'm not sure if we are going to need some logic to handle bootstrapping, I hope not.

[gnzlbg]

If the answer is "yes,

So the answer is yes, both of these targets are supported in stable Rust, and this change here would probably be a breaking change for older toolchains.

The build.rs has logic to detect the Rust toolchain version, we can gate these changes behind a cfg flag that only applies for Rust > 1.37.0 or so.

[alexcrichton]

I think it's good to acknowledge the breakage that will happen here and develop a roll-out plan accordingly, but I won't pretend like we have a policy around this for each target. If the maintainers of Emscripten recommend we upgrade/change, though, and if Emscripten itself changed, then it seems like we should follow-suit.

[tlively]

I just did a test and it turns out both fastcomp and upstream backends use the types introduced in this PR since April, so everything was just totally broken on recent Emscripten releases anyway and no one noticed. I therefore don't think we need to worry too much about breaking people.

[gnzlbg]

I just did a test and it turns out both fastcomp and upstream backends use the types introduced in this PR since April, so everything was just totally broken on recent Emscripten releases anyway and no one noticed.

Is that with the Emscripten LLVM that's shipped with the Rust toolchain ? What's the first released Rust toolchain where that change landed ?

[gnzlbg]

@tlively @alexcrichton

The moment we land this on libc, all users of the emscripten targets using a stable Rust toolchain, or an older nightly toolchain, will have to either migrate to using the latest nightly toolchain, or pin their libc to an older version.

I have no idea how many users this toolchain has beyond that it is available on stable. If it has no or few users, we can just make this change and call it a day. If it has more, we could give the change a try, and if people complain, we can roll it back. But rolling it back is going to be a pain because this change is tied to a change in rust-lang/rust.

I'd rather just avoid all these headaches by adding something like:

#[cfg(libc_emscripten_{version})]
pub type ino64_t = u32;
#[cfg(libc_emscripten_{newer_version})]
pub type ino64_t = u64;

where the libc_emscripten cfgs are created in the build.rs depending on the toolchain version somehow.

It is unfortunate to have to maintain this, but that removes the headaches, and allow us to land this here and have CI pass before landing this on rust-lang/rust (once this lands upstream, maybe we can add a second CI build job here to test the "newer_version" ?).

Alternatively, I'd rather just temporarily disable the emscripten build jobs here, land this change as is, and do a new libc release - without changing anything in rust-lang/rust upstream. We can wait one week and see if somebody complains, and if somebody complains, we just revert this here and yank the version, without having to do anything in rust-lang/rust. Thoughts ?

[gnzlbg]

@bors: try

[bors]

⌛ Trying commit d3c7de8 with merge 20ab682...

[bors]

💔 Test failed - status-azure

[alexcrichton]

I don't personally feel like I have a good grasp on the number of Rust users of the emscripten targets nor what the reaction would be to breakage. In that sense I want to be clear that I'm not assuming that there will be a huge uproar if we just land breaking changes.

If @gnzlbg you'd like to do the work smoothing over the transition, seems fine to me! I probably wouldn't personally do it but I'm always a bit too strapped for tiem...

[gnzlbg]

I don't personally feel like I have a good grasp on the number of Rust users of the emscripten targets nor what the reaction would be to breakage. In that sense I want to be clear that I'm not assuming that there will be a huge uproar if we just land breaking changes.

FWIW me neither. I have no idea how many users these backends have.

[tlively]

I just did a test and it turns out both fastcomp and upstream backends use the types introduced in this PR since April, so everything was just totally broken on recent Emscripten releases anyway and no one noticed.

Is that with the Emscripten LLVM that's shipped with the Rust toolchain ? What's the first released Rust toolchain where that change landed ?

Does Rust even ship an emscripten? My impression is that it calls out to whatever version of emscripten the user happens to have installed separately, but I could be wrong.

[alexcrichton]

Rust doesn't ship an emscripten, no, you're right in that we just use whatever emcc happens to be in PATH.

@gnzlbg FWIW these are header-level details which don't have to do with the LLVM backend, the emscripten LLVM backend in rustc is basically purely selected to ensure that the Rust-generated LLVM IR is generated by the same LLVM version as what emscripten is using internally.

[gnzlbg]

FWIW these are header-level details which don't have to do with the LLVM backend, the emscripten LLVM backend in rustc is basically purely selected to ensure that the Rust-generated LLVM IR is generated by the same LLVM version as what emscripten is using internally.

Ah! I thought we would just ship the whole thing!

Then we can't be compatible with both old and new emscripten toolchains because when building libc we don't know which one will be used right? Then this LGTM.

[alexcrichton]

Yes the standard library certainy has no recourse here. While theoretically possible that the crates.io crate could have a build script to dynamically switch, I wouldn't personally bother with such complications. I'd just merge the PR myself.

[gnzlbg]

While theoretically possible that the crates.io crate could have a build script to dynamically switch, I wouldn't personally bother with such complications.

I don't think this is worth it either.

[tlively]

It turns out the CI is broken due to an emsdk bug 🙃 Fix is up: emscripten-core/emsdk#328

[tlively]

So the tests are actually running now, which is nice. But they're failing with this output:

rust[0] = 24 != 42 (C): Rust "pthread_condattr_t" -> C
rust[1] = 25 != 1 (C): Rust "pthread_condattr_t" -> C
rust[2] = 26 != 2 (C): Rust "pthread_condattr_t" -> C
rust[3] = 27 != 3 (C): Rust "pthread_condattr_t" -> C
bad ino_t size: rust: 4 (0x4) != c 8 (0x8)
bad ino_t align: rust: 4 (0x4) != c 8 (0x8)
rust[5] = 120 != 5 (C): Rust "ino_t" -> C
rust[6] = 2 != 6 (C): Rust "ino_t" -> C
rust[7] = 0 != 7 (C): Rust "ino_t" -> C
rust [1] = 254 != 1 (C): C "ino_t" -> Rust
rust [2] = 253 != 2 (C): C "ino_t" -> Rust
rust [3] = 252 != 3 (C): C "ino_t" -> Rust
bad field size st_ino of stat: rust: 4 (0x4) != c 8 (0x8)
bad field size st_ino of stat64: rust: 4 (0x4) != c 8 (0x8)
bad field size d_ino of dirent: rust: 4 (0x4) != c 8 (0x8)

I get that the ino_t should be updated to u64, but what does that pthread_condattr_t stuff mean? I double checked, and the size of pthread_condattr_t should be 4 bytes, which is what the code says.

[tlively]

False alarm, fixing ino_t was enough. Yay! It looks like the bsd errors are unrelated, so I’m hoping this should be good to go now.

[gnzlbg]

It looks like the bsd errors are unrelated, so I’m hoping this should be good to go now.

Yes, the *BSD errors are being resolved upstream. This LGTM.

but what does that pthread_condattr_t stuff mean?

It means that when passing an initialized pthread_condattr_t with some byte pattern to C, the C code receiving it by argument did not read the same byte pattern, which obviously happened because ino_t had a different size. These tests shouldn't probably run if the size of the rust type and the C type differ...

[tlively]

@gnzlbg What are the next steps for this PR?

[gnzlbg]

It can land once CI is green, but CI is currently broken because of an unrelated bug in rust-lang/rust. I'll check that out and will approve this as soon as that's fixed.

[gnzlbg]

rust-lang/rust#64019 is in the homu queue with the fix, should land on nightly sometime soon: https://buildbot2.rust-lang.org/homu/queue/rust

[alexcrichton]

Since that can often take some time, can the freebsd builders be pinned to an older working nightly so this can land in the meantime?

[gnzlbg]

Sure!

[gnzlbg]

@bors: r+

[bors]

📌 Commit 06c980e has been approved by gnzlbg

[bors]

⌛ Testing commit 06c980e with merge a131ca3...

[bors]

💔 Test failed - checks-cirrus-freebsd-12

[bors]

☀️ Test successful - checks-cirrus-freebsd-11, checks-cirrus-freebsd-12, status-azure
Approved by: gnzlbg
Pushing a131ca3 to master...