Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WasmFS JS API: Implement truncate #19543

Merged
merged 26 commits into from
Jun 13, 2023
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions emcc.py
Original file line number Diff line number Diff line change
Expand Up @@ -2322,6 +2322,8 @@ def phase_linker_setup(options, state, newargs):
'_wasmfs_pwrite',
'_wasmfs_rename',
'_wasmfs_mkdir',
'_wasmfs_truncate',
'_wasmfs_ftruncate',
'_wasmfs_unlink',
'_wasmfs_chdir',
'_wasmfs_rmdir',
Expand Down
17 changes: 15 additions & 2 deletions src/library_wasmfs.js
Original file line number Diff line number Diff line change
Expand Up @@ -275,8 +275,21 @@ FS.createPreloadedFile = FS_createPreloadedFile;
// TDOO: chown
// TODO: lchown
// TODO: fchown
// TODO: truncate
// TODO: ftruncate
truncate: (path, len) => {
if (len < 0) {
throw new FS.ErrnoError({{{ cDefs.EINVAL }}});
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@jameshu15869 jameshu15869 Jun 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

@kleisauke kleisauke Jun 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

off_t is defined here:

#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.

return FS.handleError(withStackSave(() => {
var pathBuffer = stringToUTF8OnStack(path);
return __wasmfs_truncate(pathBuffer, len);
}));
},
ftruncate: (fd, len) => {
if (len < 0) {
throw new FS.ErrnoError({{{ cDefs.EINVAL }}});
}
return FS.handleError(__wasmfs_ftruncate(fd, len));
},
// TODO: utime
findObject: (path) => {
var result = __wasmfs_identify(path);
Expand Down
16 changes: 16 additions & 0 deletions system/lib/wasmfs/js_api.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,22 @@ int _wasmfs_unlink(char* path) {
return __syscall_unlinkat(AT_FDCWD, (intptr_t)path, 0);
}

int _wasmfs_truncate(char *path, off_t length) {
int err = __syscall_truncate64((intptr_t)path, length);
if (err == -1) {
return errno;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@jameshu15869 jameshu15869 Jun 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return err;
}

int _wasmfs_ftruncate(int fd, off_t size) {
int err = __syscall_ftruncate64(fd, size);
if (err == -1) {
return errno;
}
return err;
}

int _wasmfs_chdir(char* path) { return __syscall_chdir((intptr_t)path); }

int _wasmfs_symlink(char* old_path, char* new_path) {
Expand Down
81 changes: 81 additions & 0 deletions test/fs/test_fs_js_api.c
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
#include <emscripten/emscripten.h>
#include <stdio.h>
#include <sys/stat.h>
#include <assert.h>

int main() {
/********** test FS.open() **********/
Expand Down Expand Up @@ -28,6 +30,84 @@ int main() {
assert(createFileNotHere && createFileNotHere.fd >= 0);
);

/********** test FS.truncate() **********/
EM_ASM(
FS.writeFile('truncatetest', 'a=1\nb=2\n');
);

struct stat s;
stat("truncatetest", &s);
assert(s.st_size == 8);

EM_ASM(
FS.truncate('truncatetest', 2);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

);
stat("truncatetest", &s);
assert(s.st_size == 2);

EM_ASM(
FS.truncate('truncatetest', 10);
);
stat("truncatetest", &s);
assert(s.st_size == 10);

EM_ASM(
var truncateStream = FS.open('truncatetest', 'w');
#if WASMFS
FS.ftruncate(truncateStream, 4);
#else
FS.ftruncate(truncateStream.fd, 4);
#endif
);
stat("truncatetest", &s);
assert(s.st_size == 4);

EM_ASM(
var ex;
try {
FS.truncate('truncatetest', -10);
} catch(err) {
ex = err;
}

assert(ex.name === "ErrnoError" && ex.errno === 28 /* EINVAL */);
);

EM_ASM(
var ex;
try {
var truncateStream = FS.open('truncatetest', 'w');
#if WASMFS
FS.ftruncate(truncateStream, -10);
#else
FS.ftruncate(truncateStream.fd, -10);
#endif
} catch(err) {
ex = err;
}

assert(ex.name === "ErrnoError" && ex.errno === 28 /* EINVAL */);
);

EM_ASM(
var ex;
try {
FS.truncate('nonexistent', 10);
} catch(err) {
ex = err;
}
assert(ex.name === "ErrnoError" && ex.errno === 44 /* ENOENT */);

var ex;
try {
FS.ftruncate(99, 10);
} catch(err) {
ex = err;
}

assert(ex.name === "ErrnoError" && ex.errno === 8 /* EBADF */);
);

/********** test FS.rename() **********/
EM_ASM(
FS.mkdir('renamedir');
Expand Down Expand Up @@ -130,6 +210,7 @@ int main() {
assert(ex.name === "ErrnoError" && ex.errno === 8 /* EBADF */)
);

remove("truncatetest");
remove("testfile");
remove("renametestfile");
remove("readtestfile");
Expand Down