emscripten-core / emscripten

Emscripten: An LLVM-to-WebAssembly Compiler
Other
25.37k stars 3.25k forks source link

64 bit time_t #17393

Closed hoodmane closed 2 years ago

hoodmane commented 2 years ago

As discussed in https://github.com/pyodide/pyodide/issues/2841, 2038 is only 16 years from now so it would be great to be able to represent dates further into the future as unix timestamps. In interest of this, we would like to be able to opt into a 64 bit time_t (or have it as the default).

tiran commented 1 year ago

I believe that the change broke some stat or utime() related APIs in Emscripten. CPython's test suite is failing on EMSDK 3.1.6 and TOT, e.g.

 ======================================================================
FAIL: test_utime (test.test_os.UtimeTests.test_utime)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/runner/work/cpython-wasm-test/cpython-wasm-test/cpython/Lib/test/test_os.py", line 805, in test_utime
    self._test_utime(set_time)
  File "/home/runner/work/cpython-wasm-test/cpython-wasm-test/cpython/Lib/test/test_os.py", line 793, in _test_utime
    self.assertAlmostEqual(st.st_atime, atime_ns * 1e-9, delta=1e-6)
AssertionError: 1.1182583010295808e+17 != 1.002003 within 1e-06 delta (1.1182583010295808e+17 difference)
sbc100 commented 1 year ago

Ha, that's ironic/unfortunate given that its was python folks who wanted this changed. Any ideas why it would be failing?

tiran commented 1 year ago

Not yet, but I have a gut feeling that the utimensat syscall code should be changed to use bigint and 64bit ints.

https://github.com/emscripten-core/emscripten/blob/8c047b122416fbf87dc8110c61bfc1161ad40ef7/src/library_syscall.js#L957-L978

tiran commented 1 year ago

timespec struct definition depends on size of time_t

system/lib/libc/musl/include/alltypes.h.in:STRUCT timespec { time_t tv_sec; int :8*(sizeof(time_t)-sizeof(long))*(__BYTE_ORDER==4321); long tv_nsec; int :8*(sizeof(time_t)-sizeof(long))*(__BYTE_ORDER!=4321); };

sbc100 commented 1 year ago

Hmm.. it looks like the JS filesystem layer assumes timestamps are JS numbers, so we will not be able fully represent i64 times. Do you know if its reasonable for the value passed to utimens to be truncated such that value returns by stat that follows it looks some precision?

If not then I think I might just revert this change since 32-bit time_t is more compatible with the JS layer (which represents time as JS number (double) number of seconds): https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date/getTime

sbc100 commented 1 year ago

Hmm.. looks like it may be ok to truncate: https://www.qnx.com/developers/docs/7.1/#com.qnx.doc.neutrino.lib_ref/topic/u/utimensat.html:

"The time that's actually recorded depends on the resolution in the filesystem's data structures. For example, no matter what you put in the tv_nsec field, the time that a FAT filesystem records uses a 2-second granularity. The recorded time is the greatest value supported by the filesystem that isn't greater than the specified time."

sbc100 commented 1 year ago

Still, I'm not sure it work the extra cost in code complexity and output side to have to deal with i64 here rather than i32.

sbc100 commented 1 year ago

Playing around with linux is looks like its currently capping tv_sec at 0x37fffffff (2446-05-10 15:38:53.000000000). Specifying anything bigger than this results in stat returning that value... so I guess its fine if we cap ours at 2^53-1.

tiran commented 1 year ago
>>> datetime.datetime.fromtimestamp(2**53 - 1)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ValueError: year 285428751 is out of range

Yeah, a cap at 2^53-1 looks fine to me, too! :+1:

sbc100 commented 1 year ago

Hopefully https://github.com/emscripten-core/emscripten/pull/17459 will fix you failure

hoodmane commented 1 year ago

Thanks @sbc100, @tiran!

tiran commented 1 year ago

I have created a GH repo with daily GHA to run smoke tests of EMSDK tip-of-tree with CPython 3.11 and main branch, https://github.com/tiran/cpython-wasm-test/actions

tiran commented 1 year ago

Thanks! The PR fixed most broken tests of CPython's test suite. The functions localtime_js and _gmtime_js need some attention, too. They still parse time_t as i32. Python expects that localtime() fails with EOVERFLOW when the input is out of supported range.

https://github.com/tiran/cpython-wasm-test/runs/7403750856

tiran commented 1 year ago

Fixed in https://github.com/emscripten-core/emscripten/pull/17471 . The PR also adds some tests for edge cases.

tiran commented 1 year ago

Good news! CPython's test suite is passing again with Emscripten tot-upstream.