Fix: time_t is 64 bits on Emscripten #3569
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @JohnTitor (or someone else) soon. Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (
|
|
At risk of asking a dumb question, how does this work with respect to backwards compatibility? What happens if one uses this patched libc with an older emscripten? |
|
To answer my own question: I think you probably need to put a branch in But obviously I defer to the libc maintainers. |
| // time_t was type: | ||
| // | ||
| // - long from v1.5.4 (2013/08/08) until v3.1.11 | ||
| // - int from v3.1.11 (2022/05/20) until v3.1.16 | ||
| // - _Int64 from v3.1.16 (2022/07/14) | ||
| // | ||
| // And it's still int64 today. The change from long to int was marked as a | ||
| // nonfunctional change since wasm64 was considered to be not working yet in | ||
| // v3.1.11. So it suffices to say it's 32 bit until v3.1.16 and 64 bit after. |
There was a problem hiding this comment.
@sbc100 does this look correct to you?
There was a problem hiding this comment.
(Hey Sam, long time! Hope you're doing well!)
There was a problem hiding this comment.
What! Hi. What a pleasure to see your name pop up in my inbox. I hope you are doing well too.
hoodmane
force-pushed
the
emscripten-time_t-64-bit
branch
from
78018cb
to
2b10b44
Compare
|
For libc v0.3, I think it's fine to bump up the minimum supported emscripten version to reduce our complexity. The release date mentioned in OP seems enough to bump to me. |
|
Is there an explicit minimum supported version somewhere? I don't find it. |
|
No, we could define it on v0.3 if you like. |
|
FWIW, Rust currently specifies Emscripten 2.0.5 as the minimum version it supports, see: Bumping that to version 3.1.16 would probably be fine. |
|
Is there any policy around the supported version of Emscripten on rustc? Maybe it's good time to ask to bump on rustc as well. |
Honestly, with the amount of breaking ABI changes emscripten development is doing to their libc, they can’t really ask for anyone to support a wide range of versions. Of course, the reason they are so liberally doing those changes is because, practically speaking, C programs are statically linked against emscripten libc: It all goes into a single wasm bundle. There is normally no risk that an emscripten program ends up running against an older (or newer) version of emscripten’s libc. On the flipside, this means there are no real reasons for anyone to be on older versions of emscripten. There is even an emscripten-equivalent to The attemps that were made to support different emcc versions in this repo lead to convoluted structure (not covered by automatic testing, either) and don’t even work without Imho under these considerations it would be very reasonable for Rust to only ever support one "quite recent" version of emscripten and no others. |
And make the changes in commit 63b0d67 unconditional. Supersedes: rust-lang#3569. Cherry-picks a couple of changes from: rust-lang#3307. Depends on: rust-lang/rust#131533.
|
I think this can be closed since IIUC #3962 makes this change. Please reopen if this isn't correct. |
Since July 8, 2022:
emscripten-core/emscripten#17401 https://github.com/emscripten-core/emscripten/blob/main/system/lib/libc/musl/arch/emscripten/bits/alltypes.h?plain=1#L83
Fixes #3306