[WasmFS] Fix FS.readFile + WASM_BIGINT #17316

kleisauke · 2022-06-25T16:13:56Z

_wasmfs_read_file wrote a 32-bit unsigned integer, while
makeGetValue in JS read this as a 64-bit integer.

This caused the length to be returned incorrectly when linking
with -sWASM_BIGINT. Fix this by ensuring that _wasmfs_read_file
will write a 64-bit integer.

Resolves: #17444.

The wrong type was used for `makeGetValue` causing the length to be returned incorrectly when linking with `-sWASM_BIGINT`.

tests/test_other.py

kripken · 2022-06-27T23:38:50Z

src/library_wasmfs.js

-      // The integer length is returned in the first 8 bytes of the buffer.
-      var length = {{{ makeGetValue('buf', '0', 'i64') }}};
+      // The integer length resides in the first 8 bytes of the buffer.
+      var length = {{{ makeGetValue('buf', '0', 'i32') }}};


Hmm, how about changing

emscripten/system/lib/wasmfs/js_api.cpp

Line 37 in ae675c6

*(uint32_t*)result = size;

to write an i64? I think that might be better. The size can be more than 32 bits, and we are all set up for that, just the wrong type is written in that C++.

Yeah, WasmFS currently uses 32-bit sizes in a lot of places where it should be using 64-bit sizes...

Commit 567cfd3 ensures that it writes an i64 in C++, and that it is read out as an i64 in JS. I've updated the PR description accordingly.

kripken

Nice!

lgtm % last comment

system/lib/wasmfs/js_api.cpp

`off_t` should already be a 64-bit type, even on wasm32, so we can safely use the normal types and APIs to get large file support.

…orator" This reverts commit f19dcad.

kleisauke · 2022-07-01T17:19:07Z

src/library_wasmfs.js

      var length = {{{ makeGetValue('buf', '0', 'i64') }}};

      // Default return type is binary.
      // The buffer contents exist 8 bytes after the returned pointer.
+#if WASM_BIGINT
+      // The length as BigInt must be converted to a Number.
+      var ret = new Uint8Array(HEAPU8.subarray(buf + 8, buf + 8 + Number(length)));


I was considering using $bigintToI53Checked here instead of Number(length).

emscripten/src/library_int53.js

Lines 127 to 136 in 5003f94

$MAX_INT53: '{{{ Math.pow(2, 53) }}}',

$MIN_INT53: '-{{{ Math.pow(2, 53) }}}',

// Counvert a bigint value (usually coming from Wasm->JS call) into an int53

// JS Number. This is used when we have an incoming i64 that we know is a

// pointer or size_t and is expected to be withing the int53 range.

// Returns NaN if the incoming bigint is outside the range.

$bigintToI53Checked__deps: ['$MAX_INT53', '$MIN_INT53'],

$bigintToI53Checked: function(num) {

return (num < MIN_INT53 || num > MAX_INT53) ? NaN : Number(num);

},

That is, something similar to {{{ receiveI64ParamAsI53('length', -cDefine('EOVERFLOW')) }}}.

emscripten/src/parseTools.js

Lines 1118 to 1122 in 5003f94

function receiveI64ParamAsI53(name, onError) {

if (WASM_BIGINT) {

// Just convert the bigint into a double.

return `${name} = bigintToI53Checked(${name}); if (isNaN(${name})) return ${onError};`;

}

But I think this is redundant, since that is only a issue when using >= ~8GB buffers (_malloc(Number.MAX_SAFE_INTEGER + 1)). I'm not aware of any VM that currently supports such large buffers.

Agreed, that size buffer should not be a problem.

But actually I think that using readI53FromU64 may be the best thing here. We do actually want a JS number here for the reason you said, and that seems like the simplest way to get it. Then we wouldn't need an ifdef here: readI53FromU64 returns a Number either way.

@sbc100 what do you think?

Perhaps we should modify makeGetValue above such that we can pass i53 instead of i64 ? This would differ from i64 in that the return value would always be a Number?

@sbc100 Nice idea! sgtm

Oh, there is a difference between the signed and unsigned case though, which is why we have both readI53FromU64 and readI53FromI64, so just i53 is not enough information I think.

We already support i64 and u64 in makeGetValue so we can support i53 and u53 too.

Oh, I forgot that, i53/u53 might make sense then.

One issue is that functions need to include the helper function as a dep (and to the developer it's less visible without explicitly calling the dep). But maybe the auto-dep stuff you recently added could make that work automatically.

We have i53ConversionDeps for this purpose. Can makeGetValue not just call bigintToI53Checked... I don't see any singed/unsigned version of bigintToI53Checked..

Commit 8efae3e incorporates this (but this will probably conflict with PR #17459). According to POSIX, off_t is a signed integer type, so readI53FromU64 is not appropriate here.

Note that this will not catch overflows, see e.g. these TODO items:

emscripten/src/library_int53.js

Line 84 in a41b1a3

// TODO: Add $readI53FromI64Signaling() variant.

emscripten/src/library_int53.js

Line 91 in a41b1a3

// TODO: Add $readI53FromU64Signaling() variant.

kripken

Great, thanks!

@sbc100 did you want to take another look?

sbc100 · 2022-07-18T19:41:29Z

system/lib/wasmfs/js_api.cpp

@@ -22,6 +22,8 @@ __wasi_fd_t wasmfs_create_file(char* pathname, mode_t mode, backend_t backend);
 // The buffer will also contain the file length.
 // Caller must free the returned pointer.
 void* _wasmfs_read_file(char* path) {
+  static_assert(sizeof(off_t) == 8, "File offset type must be 64-bit");


Do we need this assert here.. I'm pretty sure if this ever changed all kind of stuff would break.

Possibly the assert is a little redundant, but it's static so no overhead, and also it is helpful for the reader, I think, as it indicates this is a contract with the JS side that must not change.

sbc100

Nice!

sbc100 · 2022-07-18T19:43:05Z

src/parseTools.js

@@ -408,6 +408,9 @@ function makeGetValue(ptr, pos, type, noNeedFirst, unsigned, ignore, align) {
  }

  const offset = calcFastOffset(ptr, pos, noNeedFirst);
+  if (type === 'i53' || type === 'u53') {
+    return 'readI53From' + (unsigned ? 'U' : 'I') + '64(' + offset + ')';
+  }


I already have another use of this in #17459 !

kripken · 2022-07-18T19:43:38Z

Let's maybe consider the assert in a followup, as there is a user that has hit the bug, so let me land this fix now.

[WasmFS] Fix FS.readFile + WASM_BIGINT

4f6b0a5

The wrong type was used for `makeGetValue` causing the length to be returned incorrectly when linking with `-sWASM_BIGINT`.

kripken reviewed Jun 27, 2022

View reviewed changes

Incorporate review comment

567cfd3

kripken approved these changes Jun 28, 2022

View reviewed changes

system/lib/wasmfs/js_api.cpp Outdated Show resolved Hide resolved

kleisauke added 6 commits June 28, 2022 18:54

Prefer to use the non-64 types/APIs

adc486a

`off_t` should already be a 64-bit type, even on wasm32, so we can safely use the normal types and APIs to get large file support.

Merge remote-tracking branch 'upstream/main'

c464b53

Update after commit a0ed598

638c30d

Add wasm_bigint parameter to the @wasmfs_all_backends decorator

f19dcad

Fix flake8

04c373f

Revert "Add wasm_bigint parameter to the @wasmfs_all_backends dec…

15112a4

…orator" This reverts commit f19dcad.

kleisauke commented Jul 1, 2022

View reviewed changes

kleisauke mentioned this pull request Jul 15, 2022

WasmFS: FS.readFile doesn't work with BigInt #17444

Closed

kleisauke added 2 commits July 18, 2022 11:00

Incorporate review comment

8efae3e

Merge remote-tracking branch 'upstream/main'

19f049a

kripken approved these changes Jul 18, 2022

View reviewed changes

sbc100 reviewed Jul 18, 2022

View reviewed changes

sbc100 approved these changes Jul 18, 2022

View reviewed changes

sbc100 reviewed Jul 18, 2022

View reviewed changes

kripken merged commit e79f10e into emscripten-core:main Jul 18, 2022

kleisauke deleted the wasmfs-fix-readfile-bigint branch July 19, 2022 19:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WasmFS] Fix FS.readFile + WASM_BIGINT #17316

[WasmFS] Fix FS.readFile + WASM_BIGINT #17316

kleisauke commented Jun 25, 2022 •

edited

Loading

kripken Jun 27, 2022

tlively Jun 27, 2022

kleisauke Jun 28, 2022

kripken left a comment

kleisauke Jul 1, 2022

kripken Jul 12, 2022

sbc100 Jul 12, 2022

kripken Jul 12, 2022

kripken Jul 12, 2022

sbc100 Jul 12, 2022

kripken Jul 12, 2022 •

edited

Loading

sbc100 Jul 12, 2022

kleisauke Jul 18, 2022

kripken left a comment

sbc100 Jul 18, 2022

kripken Jul 18, 2022

sbc100 left a comment

sbc100 Jul 18, 2022

kripken commented Jul 18, 2022

	$MAX_INT53: '{{{ Math.pow(2, 53) }}}',
	$MIN_INT53: '-{{{ Math.pow(2, 53) }}}',
	// Counvert a bigint value (usually coming from Wasm->JS call) into an int53
	// JS Number. This is used when we have an incoming i64 that we know is a
	// pointer or size_t and is expected to be withing the int53 range.
	// Returns NaN if the incoming bigint is outside the range.
	$bigintToI53Checked__deps: ['$MAX_INT53', '$MIN_INT53'],
	$bigintToI53Checked: function(num) {
	return (num < MIN_INT53 \|\| num > MAX_INT53) ? NaN : Number(num);
	},

	function receiveI64ParamAsI53(name, onError) {
	if (WASM_BIGINT) {
	// Just convert the bigint into a double.
	return `${name} = bigintToI53Checked(${name}); if (isNaN(${name})) return ${onError};`;
	}

[WasmFS] Fix FS.readFile + WASM_BIGINT #17316

[WasmFS] Fix FS.readFile + WASM_BIGINT #17316

Conversation

kleisauke commented Jun 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kripken left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kripken Jul 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kripken left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbc100 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kripken commented Jul 18, 2022

kleisauke commented Jun 25, 2022 •

edited

Loading

kripken Jul 12, 2022 •

edited

Loading