WasmFS JS API: Implement truncate #19543

jameshu15869 · 2023-06-06T21:52:57Z

This PR implements truncate and ftruncate. Tests were also added to test_fs_js_api.

One thing I noticed was that both the man pages and doTruncate syscall note that truncate and ftruncate take in size as type off_t, but __syscall_truncate64 and __syscall_ftruncate64 take in size as a uint64_t. This means that passing in negative numbers directly to __syscall_truncate64 and __syscall_ftruncate64 gives a weird number and no error, so I had to add a manual check in library_wasmfs.js, which I didn't do for the other methods. Was there a specific reason that the two syscalls take in uint64_t instead of off_t?

…into library-wasmfs-truncate

sbc100 · 2023-06-06T22:45:01Z

test/fs/test_fs_js_api.c

+    assert(s.st_size == 8);
+
+    EM_ASM(
+        FS.truncate('truncatetest', 2);


As with all these changes it would be good to look for other tests that already use this API, if any? If we were previously disabling, or not running, those tests under WASMFS it would be good to enable them now.

I didn't see anything explicitly calling FS.truncate() in the test dir, so I added them to test_fs_js_api for now. Are there any places besides the test directory that could potentially contain disabled tests?

No, it would only be in the test dir. I guess we had not tests for that API. Thanks for looking into it. Hopefully after we are done with this we will have much better test coverage for the JS API!

sbc100 · 2023-06-06T22:46:22Z

src/library_wasmfs.js

+    truncate: (path, len) => {
+      if (len < 0) {
+        throw new FS.ErrnoError({{{ cDefs.EINVAL }}});
+      }


If one calls truncate("foo", -1) I would probably expect it to try to resize of MAX_INT or something like that.. so maybe we don't need this check and we can just let the underlying call fail?

That's what I was thinking, but I had one test that tried calling truncate with -10. I tried printing out the value the syscall receives, which was 4294867286. But instead of setting the return value to -1 and setting errno accordingly, I think the syscall aborts.

Aborted() RuntimeError: Aborted(). Build with -sASSERTIONS for more info. at abort (/home/ubuntu/Documents/emsdk/emscripten/out/test/test_fs_js_api.js:1:5674) at _abort (/home/ubuntu/Documents/emsdk/emscripten/out/test/test_fs_js_api.js:1:14749) at <anonymous>:wasm-function[187]:0xd2a9 at <anonymous>:wasm-function[197]:0xe13e at <anonymous>:wasm-function[172]:0xba9a at <anonymous>:wasm-function[178]:0xce81 at _wasmfs_truncate (<anonymous>:wasm-function[261]:0x13101)

This is the error. It doesn't seem to match the pattern of how we've been returning JS API errors before (Normally the syscall returns -1 and I can return errno to JS, then throw an error from JS with an error name and code). Is this close to what you were saying by letting the call fail?

I agree we don't want to syscall aborting like that. If you build with -g or --profiling-funcs you should get functions names for those wasm functions in the backtrace BTW.

This is strange because doTruncate does return -EINVAL when its input is negative. It would be good to get to the bottom of this!

I noticed that doTruncate takes in off_t size, but truncate64 and ftruncate64 take in size as a uint64_t, so is it possible that the size is always positive in doTruncate?

I think this is needed for the Node backend, as it silently accepts negative offsets rather than failing with EINVAL, see: nodejs/node#35632.

Though, it looks like setSize() is currently not implemented for the Node backend:

emscripten/system/lib/wasmfs/backends/node_backend.cpp

Lines 120 to 122 in 48b0645

int setSize(off_t size) override {

WASMFS_UNREACHABLE("TODO: implement NodeFile::setSize");

}

(i.e. this PR won't work for -sNODERAWFS + -sWASMFS)

That's really interesting. I don't think I've been testing with NODEFS or NODERAWFS. Should I start adding those flags to test_fs_js_api?

If setSize() (and insertMove() due to FS.rename()) is supported in WasmFS' Node backend (which is probably better as a follow-up), you could do:

--- a/test/test_core.py +++ b/test/test_core.py @@ -6017,6 +6017,7 @@ Module = { def test_fs_writeFile(self): self.do_run_in_out_file_test('fs/test_writeFile.cpp') + @also_with_noderawfs def test_fs_js_api(self): self.set_setting("FORCE_FILESYSTEM") self.do_runf(test_file('fs/test_fs_js_api.c'), 'success')

After that, you can verify with:

$ ./test/runner wasmfs.test_fs_js_api* core2.test_fs_js_api*

Whether the test passes on both memory and Node backend, either with or without WasmFS.

After going back to the problem this morning, I realized I never checked that the right value was getting passed to js_api.cpp. I originally had _wamsfs_truncate in js_api.cpp take in size as type off_t, and for some reason negative values would not be passed in correctly. After changing off_t to long, the tests pass without making any changes to syscalls.cpp. I also tried passing in an int64_t to js_api.cpp, but that didn't seem to work. I initially wanted to use off_t to match the POSIX API, but could there be a potential problem with using that type in js_api.cpp?

Out of curiosity, I tried printing the values in syscalls.cpp, and I could print uint64_t as a signed integer using %lld. When the uint64_t then gets passed to doTruncate (Which takes in size as type off_t), does C automatically force the uint64_t to become signed? What if the unsigned value is larger than the signed max?

off_t is defined here:

emscripten/system/lib/libc/musl/arch/emscripten/bits/alltypes.h

Lines 240 to 243 in 48b0645

#if defined(__NEED_off_t) && !defined(__DEFINED_off_t)

typedef _Int64 off_t;

#define __DEFINED_off_t

#endif

So, it corresponds to int64_t. I just opened PR #19559 for this.

sbc100 · 2023-06-06T22:50:12Z

Regarding the syscall interface, I think for consistency all the native syscalls take primitive types.. see system/lib/libc/musl/arch/emscripten/syscall_arch.h. If changing the types of those functions to off_t doesn't break anything and causes your code to be simpler that seems reasonable to me.

tlively · 2023-06-07T20:10:08Z

system/lib/wasmfs/js_api.cpp

+  if (err == -1) {
+    return errno;
+  }


Syscalls don't set errno, so this doesn't look right. Syscalls return negative error codes and libc handles those by setting errno appropriately. I would expect this code to be checking whether err is a negative error code (not necessarily -1) instead.

Is it possible that errno is being set by libc behind the scenes? I've been doing this for the other js_api.cpp functions and the error codes match with the legacy API.

Ah, no, all those places should be fixed. For example, in _wasmfs_mknod, it checks whether __syscall_mknodat returns -1, and if so returns errno. But __syscall_mknodat never sets errno and doesn't return -1 on errors; instead, it either returns -EINVAL or -EPERM or other negative error codes that doOpen can return.

I see what went wrong now. The whole time I thought the syscalls were returning -1, so the errno values were returning correctly - turns out the if statement is always false, so the methods were just returning the syscall return value and I never checked the actual return values of the syscalls before, which made all the tests pass. Those issues have all been addressed in this PR.

tlively

LGTM % separable comment.

tlively · 2023-06-08T19:08:36Z

system/lib/wasmfs/js_api.cpp

+int _wasmfs_truncate(char *path, long size) {
+  return __syscall_truncate64((intptr_t)path, size);
+}
+
+int _wasmfs_ftruncate(int fd, long size) {
+  return __syscall_ftruncate64(fd, size);
+}
+


Since there's no extra logic necessary here, is it possible to skip these wrappers and just call the underlying syscall directly from the JS?

That makes sense, although I'm having trouble figuring out how to import the syscalls to library_wasmfs.js. I've tried using __syscall_truncate64 and ___syscall_truncate64 (Three underscores) and neither worked. Normally, I would add _wasmfs_truncate to emcc.py, but I don't think it worked. How should I go about making the syscall available to call?

Maybe that could be a followup.

When calling __syscall_truncate64 from JS you would use an extra underscore... but when adding it to the settings.REQUIRED_EXPORTS list in emcc.py you would not.

Doing this as a follow-up sounds good to me.

jameshu15869 · 2023-06-08T20:05:10Z

As a side note, should we worry about the issue with using long in place of some 64 bit integer (Either uint64_t, int64_t, or off_t)? Would it make sense to wait for #19559 and try truncate and ftruncate with those types?

sbc100 · 2023-06-08T20:09:01Z

I'm hoping we can land #19559 today BTW.. since that tests already passed.

kripken · 2023-06-13T16:33:48Z

emcc.py

@@ -2338,6 +2338,8 @@ def phase_linker_setup(options, state, newargs):
        '_wasmfs_read',
        '_wasmfs_pread',
        '_wasmfs_symlink',
+        '__syscall_truncate64',
+        '__syscall_ftruncate64',


Was there a reason to use the raw syscalls and not a _wasmfs_* wrapper? Wrappers have the benefit that changes to the syscalls don't affect them.

Thomas suggested a little earlier that since truncate doesn't have any additional processing, it might make sense to directly use the raw syscall. However, I do see your point that a wrapper would be beneficial. Would it be preferable to go back to calling the wrapper?

I think for now you should continue to use wrapper functions and we can separately/later consider weather they are actually needed since (IIUC) these are not the only once that are simple pass-through functions?

Don't block landing anything on this conversation, but I still think pass-through wrappers aren't that useful. If we update the syscalls (which should be extremely rare), then we would still have to update their callers. Whether those callers are in C or JS doesn't make much of a difference as far as I can tell.

kripken · 2023-06-13T16:34:21Z

system/lib/wasmfs/js_api.cpp

-    return -errno;
-  }
-  return err;
+  return __syscall_openat(AT_FDCWD, (intptr_t)path, flags, mode);


Are these bugfixes?

We realized that this check isn't needed, since the syscalls do not actually set errno. The tests were passing before because err was never equal to -1, so the js_api.cpp function directly returned the error without using errno, giving the desired effect.

Can you make a separate PR for this cleanup stuff?

jameshu15869 added 6 commits June 2, 2023 12:39

implement truncate and ftruncate with tests

e0d56a1

handle negative size

f305495

fix merge conflicts

58bd56a

Merge branch 'main' of https://github.com/emscripten-core/emscripten …

cfc0213

…into library-wasmfs-truncate

fix merge conflicts

b354716

clean up

408fcc3

sbc100 reviewed Jun 6, 2023

View reviewed changes

jameshu15869 and others added 2 commits June 7, 2023 14:35

Merge branch 'main' into library-wasmfs-truncate

5eaaeba

loggin

08773f5

tlively reviewed Jun 7, 2023

View reviewed changes

jameshu15869 added 6 commits June 8, 2023 09:50

passing tests with logging

044e8e4

clean up

a0a3aa6

fix merge conflicts

ad54b31

fix js_api returns

23c4326

clean up test

e857699

remove extra line

c1b3f46

tlively approved these changes Jun 8, 2023

View reviewed changes

jameshu15869 added 3 commits June 8, 2023 16:48

call syscall from js

d3a8326

working with int32_t

279f9a5

int64_t not working

c924202

kripken mentioned this pull request Jun 9, 2023

Fix splitI64 so it can be used to send values from JS libraries into wasm #19575

Merged

jameshu15869 added 5 commits June 10, 2023 11:47

working u53toi64

750a978

clean up

a0e4ca5

clean up test

b1703ff

clean up

0cabd03

use spliti64

b262202

jameshu15869 added 3 commits June 13, 2023 10:04

clean up

058dfa6

remove unused import

f057bc4

remove white space

1d18411

kripken reviewed Jun 13, 2023

View reviewed changes

use wrapper calls instead of raw syscals

bce2613

jameshu15869 mentioned this pull request Jun 13, 2023

WasmFS JS API: Implement allocate #19602

Merged

kripken approved these changes Jun 13, 2023

View reviewed changes

kripken merged commit 90858c8 into emscripten-core:main Jun 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WasmFS JS API: Implement truncate #19543

WasmFS JS API: Implement truncate #19543

jameshu15869 commented Jun 6, 2023

sbc100 Jun 6, 2023

jameshu15869 Jun 7, 2023

sbc100 Jun 7, 2023

sbc100 Jun 6, 2023

jameshu15869 Jun 7, 2023 •

edited

Loading

sbc100 Jun 7, 2023

tlively Jun 7, 2023

jameshu15869 Jun 7, 2023

kleisauke Jun 8, 2023 •

edited

Loading

jameshu15869 Jun 8, 2023

kleisauke Jun 8, 2023

jameshu15869 Jun 8, 2023

kleisauke Jun 8, 2023

sbc100 commented Jun 6, 2023

tlively Jun 7, 2023

jameshu15869 Jun 7, 2023

tlively Jun 8, 2023

jameshu15869 Jun 8, 2023 •

edited

Loading

tlively left a comment

tlively Jun 8, 2023

jameshu15869 Jun 8, 2023

sbc100 Jun 8, 2023

tlively Jun 9, 2023

jameshu15869 commented Jun 8, 2023

sbc100 commented Jun 8, 2023

kripken Jun 13, 2023

jameshu15869 Jun 13, 2023 •

edited

Loading

sbc100 Jun 13, 2023

tlively Jun 13, 2023

kripken Jun 13, 2023

jameshu15869 Jun 13, 2023 •

edited

Loading

sbc100 Jun 13, 2023

	int setSize(off_t size) override {
	WASMFS_UNREACHABLE("TODO: implement NodeFile::setSize");
	}

	#if defined(__NEED_off_t) && !defined(__DEFINED_off_t)
	typedef _Int64 off_t;
	#define __DEFINED_off_t
	#endif

WasmFS JS API: Implement truncate #19543

WasmFS JS API: Implement truncate #19543

Conversation

jameshu15869 commented Jun 6, 2023

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

jameshu15869 Jun 7, 2023 • edited Loading

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

kleisauke Jun 8, 2023 • edited Loading

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

sbc100 commented Jun 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jameshu15869 Jun 8, 2023 • edited Loading

Choose a reason for hiding this comment

tlively 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

jameshu15869 commented Jun 8, 2023

sbc100 commented Jun 8, 2023

Choose a reason for hiding this comment

jameshu15869 Jun 13, 2023 • edited Loading

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

jameshu15869 Jun 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jameshu15869 Jun 7, 2023 •

edited

Loading

kleisauke Jun 8, 2023 •

edited

Loading

jameshu15869 Jun 8, 2023 •

edited

Loading

jameshu15869 Jun 13, 2023 •

edited

Loading

jameshu15869 Jun 13, 2023 •

edited

Loading