emscripten-core / emscripten-core/emscripten

What should `makeSetValue`/`makeGetValue` do for i64?

Open
#18,687 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
C++
Stars
27.6k
Forks
3.6k
Avg merge
1d 6h
Merged PRs (30d)
109

Description

Right now there seems to be mix behaviors, which doesn't seem like a great situation.

Within the code base there seems to be very limited use of these types. For `makeGetValue` I only see a single use, in the getValue utility function:

```
$ git grep makeGetValue.*i64
src/library_getvalue.js: case 'i64': return {{{ makeGetValue('ptr', '0', 'i64') }}};
```

For `makeSetValue` there are a few more uses:

```
$ git grep makeSetValue.*i64
src/library_getvalue.js: case 'i64': {{{ makeSetValue('ptr', '0', 'value', 'i64') }}}; break;
src/library_sdl.js: {{{ makeSetValue('ptr', C_STRUCTS.SDL_TouchFingerEvent.touchId, 'touch.deviceID', 'i64') }}};
src/library_sdl.js: {{{ makeSetValue('ptr', C_STRUCTS.SDL_TouchFingerEvent.fingerId, 'touch.identifier', 'i64') }}};
src/library_syscall.js: {{{ makeSetValue('buf', C_STRUCTS.stat.st_size, 'stat.size', 'i64') }}};
src/library_syscall.js: {{{ makeSetValue('buf', C_STRUCTS.stat.st_atim.tv_sec, 'Math.floor(atime / 1000)', 'i64') }}};
src/library_syscall.js: {{{ makeSetValue('buf', C_STRUCTS.stat.st_mtim.tv_sec, 'Math.floor(mtime / 1000)', 'i64') }}};
src/library_syscall.js: {{{ makeSetValue('buf', C_STRUCTS.stat.st_ctim.tv_sec, 'Math.floor(ctime / 1000)', 'i64') }}};
src/library_syscall.js: {{{ makeSetValue('buf', C_STRUCTS.stat.st_ino, 'stat.ino', 'i64') }}};
src/library_syscall.js: {{{ makeSetValue('dirp + pos', C_STRUCTS.dirent.d_ino, 'id', 'i64') }}};
src/library_syscall.js: {{{ makeSetValue('dirp + pos', C_STRUCTS.dirent.d_off, '(idx + 1) * struct_size', 'i64') }}};
src/library_wasi.js: {{{ makeSetValue('newOffset', '0', 'stream.position', 'i64') }}};
src/library_wasi.js: // TODO {{{ makeSetValue('pbuf', C_STRUCTS.__wasi_fdstat_t.fs_rights_base, '?', 'i64') }}};
src/library_wasi.js: // TODO {{{ makeSetValue('pbuf', C_STRUCTS.__wasi_fdstat_t.fs_rights_inheriting, '?', 'i64') }}};
src/library_wasmfs_jsimpl.js: {{{ makeSetValue('size_p', 0, 'size', 'i64') }}};
src/library_wasmfs_opfs.js: {{{ makeSetValue('sizePtr', 0, 'size', 'i64') }}};
src/library_wasmfs_opfs.js: {{{ makeSetValue('sizePtr', 0, 'size', 'i64') }}};
src/library_webgpu.js: {{{ makeSetValue('limitsOutPtr', 'limitsPtr + limitOffset', 'limitValue', 'i64') }}};
src/library_websocket.js: {{{ makeSetValue('bufferedAmount', '0', 'socket.bufferedAmount', 'i64') }}};
```

From my reading of the code `makeGetValue` with `i64` but without MEMORY64 simply doesn't do that right thing, so its good that we don't have any users of that code.

`makeSetValue` does do what is perhaps the right thing bug it generates a lot of rather complicated code. I basically expands to this:

```
(tempI64 = [value>>>0,(tempDouble=value,(+(Math.abs(tempDouble))) >= 1.0 ? (tempDouble > 0.0 ? ((Math.min((+(Math.floor((tempDouble)/4294967296.0))), 4294967295.0))|0)>>>0 : (~~((+(Math.ceil((tempDouble - +(((~~(tempDouble)))>>>0))/4294967296.0)))))>>>0) : 0)],HEAP32[((ptr)>>2)] = tempI64[0],HEAP32[(((ptr)+(4))>>2)] = tempI64[1])
```

The goal of this code as far as I can tell (which is generated here: https://github.com/emscripten-core/emscripten/blob/d91e1279cea285f96ab25c9d7d0334db0a446011/src/parseTools.js#L210-L243) is to try to allow numbers outside of the i53/safeInteger range to stored and retrieved, but its imprecise and involves rounding when these values are not withing the i53 range.

My question is, should we be preferring `i53` in the above cases? Are there any cases were we want to allow for rounded values outside of the i53 range?

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.