Switch to 64bit timestamps in libc #16401
Conversation
|
In emscripten we care a lot about code size, and also about passing 64-bit values over the system call boundary (since JS support for 64-bit integers is complex/expensive). So we may want to hold off on this upgrade longer than other platform, but we should take a look at how many syscalls use timestamps and what effect his change has on code size. Is the motivation for this change that you are concerned about the imminent arrival or 2038? Or is it more about consistency with other platforms / upstream musl? |
|
@sbc100 I am porting https://github.com/intel/SGXDataCenterAttestationPrimitives/tree/master/QuoteVerification/QVL which is a library that heavily depends on attestation certificates that often have end dates at year >2036 (which is a high security concern where I work at), which is why I had to fork emscripten to support them (otherwise they just overflow to the negatives, which heavily breaks attestation certificate validation) I am okay with holding off 64bit timestamps in emscripten in general for code size concerns, because my port depends on my fork of emscripten anyway, and I think I can manage updating my fork with upstream emscripten in the meantime. However, I don't believe it is much of a code size concern, I don't think much code uses time_t anyway in the wild, other than the sort of code that I'm dealing with, in which 32bit timestamp is a big concern. In which case I don't think it doesn't involves that big of a code size increase anyway. I might be wrong though, I don't know much about time_t in emscripten usage in the wild. |
|
I've seen my commit doesn't pass the tests. I can fix the emscripten code / update the tests myself, however I haven't been able to run the test-browser-chrome or test-browser-firefox well on my machine yet... |
|
I would focus on the core and other tests.. its likely that fixing them will fix all your issues. Given that you have real world need for 64-bit time_t I think this change seem necessary. Lets see what it looks like the tests pass. |
cchudant
force-pushed
the
main
branch
2 times, most recently
from
16e4d0f to
e63aeb0
Compare
| difftime__sig: 'dii', | ||
| difftime: function(time1, time0) { | ||
| return time1 - time0; | ||
| }, |
There was a problem hiding this comment.
Perhaps we can land the moving of these two functions to native code separately, before landing this PR.
The difftime change looks like a no-brainer.. the time change probably is too, but could effect code size.
There was a problem hiding this comment.
Yes. The time change is responsible for the 2/3% code size regression on the test-other test suite.
I am very tempted to remove most of the date stuff from the JS side, and just have a single _time_js function which just returns Date.now() as a double.
This should help with bundle size and performance, because we won't have to pass i64s on the wasm/js boundary anymore
There was a problem hiding this comment.
Yup, this was what I was worried about with increasing the width of time_t. Any refactors that move stuff into native code without regressing size sounds good to me.
Can I recommend a PR that just moves time() and timediff() first.. rather than trying to make one big refactor.
There was a problem hiding this comment.
I tooks inspiration from your suggestions and created a PR to move a whole bunch of these functions into native code: #16439. I'm not sure if here is still work to do to avoid passing time_t values but this seems like a good improvment.
There was a problem hiding this comment.
Ah, sorry for the (big) delay in my response.
I was working on something very similar, but didn't have time to finish
This is great! I will continue my work on this PR asap
Hello!
With the current 32bit time_t, timestamps overflow at year 2038.
Musl has switched to 64bit time_t back in 2019 for every 32bit archs to avoid this issue, but since emscripten has a custom arch, this change has never been downstreamed.
This pull request mirrors the changes in the commit from musl, also setting
suseconds_tto 64bit and_REDIR_TIME64for consistency.However, I haven't found any instance of
time_tbeing used in the arch/emscripten directory of musl, and the rest of musl should be forward compatible withtime_tbeing 64bit.There may be backward-compatibility issues I overlooked.