[Memory64] Made clock_t 64-bit compatible #15500
Conversation
src/library.js
Outdated
| @@ -453,7 +453,8 @@ LibraryManager.library = { | |||
| clock__sig: 'i', | |||
| clock: function() { | |||
| if (_clock.start === undefined) _clock.start = Date.now(); | |||
| return ((Date.now() - _clock.start) * ({{{ cDefine('CLOCKS_PER_SEC') }}} / 1000))|0; | |||
| const t = ((Date.now() - _clock.start) * ({{{ cDefine('CLOCKS_PER_SEC') }}} / 1000))|0; | |||
| return {{{ to64('t') }}}; | |||
| }, | |||
There was a problem hiding this comment.
clock() returns clock_t I think. Does musl define that differently in 64-bit? That surprises me a little as usually that stuff is fixed (say ino_t is always 64-bit).
There was a problem hiding this comment.
All I know is it ended up as an i64 in the module.
Seems we have typedef long clock_t;
There was a problem hiding this comment.
Is that a typedef in musl code, or in emscripten code?
I ask because I wouldn't suggest changing musl code, but if it's in an emscripten header then maybe the simple change is to just make it always 32-bit.
There was a problem hiding this comment.
It looks like its in musl:
There was a problem hiding this comment.
Thanks @sbc100 !
Reading C docs, I don't see a requirement for the type of clock_t. So I think we are free to change that. In practice we call Date.now() which returns a result in ms. So 32 bits are likely enough for such a value. That suggests we can use int32_t for wasm64 as well.
I think that would be best. Otherwise we'd have an odd difference between wasm32 and wasm64 builds, potentially a confusing and difficult to debug one. We have not gotten any bug reports about the precision of clock_t to my knowledge, so I think it should be good enough for wasm64 too.
There was a problem hiding this comment.
We already have use of long all over the place, but ok, I'll try changing the type instead.
There was a problem hiding this comment.
I'm not opposed to long in principle - where we want wasm32 and wasm64 to diverge it sgtm - I'm just saying I think it isn't appropriate here specifically since we don't have a good reason to diverge the two targets. Do you disagree with that?
There was a problem hiding this comment.
No, it's all the same to me. I already changed the type and reverted the JS change.
|
Another approach to solving this might be to use |
|
Presumably this function is already under test in Fully agree we should move as much code to Wasm as possible, though probably separate from this PR. |
aardappel
force-pushed
the
clock
branch
2 times, most recently
from
630725e to
c2a73fc
Compare
aardappel
changed the title
No description provided.