Port over stdout implementation to wasmfs.cpp by ethanalee · Pull Request #15104 · emscripten-core/emscripten

ethanalee · 2021-09-22T23:17:13Z

Basic template for syscalls in wasmfs.cpp
Build with -s WASMFS flag
Please see issue New File System Implementation #15041 for background information regarding this PR

tlively

Very cool to see this implementation starting up! It would be good to add a test (in test_core.py or maybe test_other.py for now? @kripken, WDYT?) at least to show that the library is built and linked in successfully, even if there isn't much functionality to test yet.

tlively · 2021-09-22T23:28:59Z

@@ -0,0 +1,8 @@
+var WASMFS = {


Do you think we'll be filling out this implementation soon? If so, including this file might be worth it if you add a comment explaining that it is a placeholder for future work with a link to the tracking issue. Otherwise adding this file would probably be best left to when we have something to put in it.

tlively · 2021-09-22T23:29:45Z

+    state.forced_stdlibs.append('libwasmfs')
+    settings.FILESYSTEM = 0
+    settings.SYSCALLS_REQUIRE_FILESYSTEM = 0
+    # settings.JS_LIBRARIES.append((0, 'library_wasmfs.js'))


If we decide not to include library_wasmfs.js in this PR, then this could be a TODO comment mentioning that it should be added in the future instead.

tlively · 2021-09-22T23:33:10Z

 // [link]
 var ASMFS = 0;

+// If set to 1, uses new filesystem implementation


It would be good to add a note that this is still an experimental work in progress and is not expected to function correctly, probably with a link to the tracking issue.

tlively · 2021-09-22T23:35:15Z

+
+extern "C" {
+
+  __wasi_errno_t __wasi_fd_write( __wasi_fd_t fd, const __wasi_ciovec_t *iovs, size_t iovs_len, __wasi_size_t *nwritten) {


This line looks longer than 80 characters, so it would be nice to run git clang-format to standardize the formatting of the C/C++ source. @kripken or @sbc100, is that standard practice in this repo? Perhaps we should have CI check formatting like we do in Binaryen.

On the other hand, maybe clang-format does something terrible to EM_ASM macros :/

I can use clang-format if that's appropriate for sure!

tlively · 2021-09-22T23:37:29Z

+extern "C" {
+
+  __wasi_errno_t __wasi_fd_write( __wasi_fd_t fd, const __wasi_ciovec_t *iovs, size_t iovs_len, __wasi_size_t *nwritten) {
+    if (fd == 1 || fd == 2) {


This hardcoding of file descriptors 1 and 2 is temporary, right? It would be good to add a comment saying what this code does and that it is temporary behavior just to get something running at all.

I think it should be a permanent convention. We don't want to tie printing to require to pull in the filesystem. Making the filesystem somehow define stdio file handles would certainly create such a dependency.

Yes, I agree that's an important goal here.

I'm hoping that we can find a way to make that happen even without hardcoding stuff here, just relying on LLVM. But if we don't, let's hardcode.

As background, currently we avoid linking in any FS code when only stdout/stderr are used here:

emscripten/emscripten.py

Line 65 in aca7c12

def optimize_syscalls(declares, DEBUG):

For WASMFS we could perhaps do something similar. I was hoping we'd find a less hackish mechanism, perhaps using weak linking, but I think all options should be on the table.

tlively · 2021-09-22T23:39:41Z

+// // Copyright 2021 The Emscripten Authors.  All rights reserved.
+// // Emscripten is available under two separate licenses, the MIT license and the
+// // University of Illinois/NCSA Open Source License.  Both these licenses can be
+// // found in the LICENSE file.


It would probably be good to create system/lib/wasmfs/wasmfs.cpp rather than putting this under system/lib/fetch.

It would also be good to add a file-level comment saying what this file is for and what its implementation status is.

tlively · 2021-09-22T23:44:57Z

+          var ptr = HEAP32[(((iovs)+(i*8))>>2)];
+          var len = HEAP32[(((iovs)+(i*8 + 4))>>2)];


@kripken, is there a better way to write these memory accesses in an EM_ASM?

Stepping back, it would much easier to read if all this code that reads and writes memory were just normal C++ rather than JS. There shouldn't be any need to call out to JS for anything except the call to out or err, and buffer can probably be an ArrayBuffer initialized directly from a slice of the Wasm memory rather than built up one character at a time.

This is basically the best way from JS, I think. (Though the extra parens can be removed, as I commented after you posted.)

We could do more work in C++ and leave less for JS. That would add additional copying, though, so I'm not sure it's a good idea.

kripken

Great start!

For landing, let's add a test. Perhaps adding a variation on hello world,

emscripten/tests/test_core.py

Lines 310 to 314 in 592dec1

    
           @also_with_standalone_wasm() 
        
           def test_hello_world(self): 
        
             self.do_core_test('test_hello_world.c') 
        
             # must not emit this unneeded internal thing 
        
             self.assertNotContained('EMSCRIPTEN_GENERATED_FUNCTIONS', read_file('test_hello_world.js'))

One way to add a variation could be to add a decorator like that existing @also_with_standalone_wasm (so, @also_with_wasmfs). That might make sense, I think. And then we'd decorate more and more tests as things get working.

kripken · 2021-09-22T23:41:14Z

+    $WASMFS : {}
+


Suggested change

$WASMFS : {}

$WASMFS : {}

kripken · 2021-09-22T23:41:54Z

-    if (FILESYSTEM) {
+    if (WASMFS) {
+      libraries.push('library_wasmfs.js')
+    } else if (FILESYSTEM) {


As emcc.py sets FILESYSTEM=0, I don't think this is needed.

kripken · 2021-09-22T23:42:05Z


-    if (FILESYSTEM) {
+    if (WASMFS) {
+      libraries.push('library_wasmfs.js')


This could be moved up to be an else in that other if (if we keep it).

kripken · 2021-09-22T23:45:58Z

+#include <emscripten/emscripten.h>
+#include <wasi/api.h>
+
+extern "C" {


For C++ style, perhaps we can just use the clang-format settings for say LLVM or Binaryen? We can also leave that for later, but meanwhile, I'd suggest we use something more like the existing code we have, like system/lib/embind/bind.cpp (in particular: no indentation inside extern C)

IIUC, @curiousdannii exported functions need to be C functions (to avoid C++ name mangling). If this is what you're asking.

https://emscripten.org/docs/getting_started/FAQ.html?highlight=mangling#why-do-functions-in-my-c-c-source-code-vanish-when-i-compile-to-javascript-and-or-i-get-no-functions-to-process

@kripken I used clang-format for now for consistency. I think it more or less matches the formatting in bind.cpp

The main idea is that we don't indent stuff that lives inside namespace { or inside extern "C" {

Got it. I think when I ran clang-format locally it removed the indentations.

kripken · 2021-09-22T23:49:56Z

+
+  __wasi_errno_t __wasi_fd_write( __wasi_fd_t fd, const __wasi_ciovec_t *iovs, size_t iovs_len, __wasi_size_t *nwritten) {
+    if (fd == 1 || fd == 2) {
+      MAIN_THREAD_EM_ASM({


Might be worth writing a TODO here so we don't forget for later: We may not want to proxy stderr to the main thread (it doesn't show up in the HTML anyhow - doing console.warn in a worker would be fine). But that would be a change from the current FS.

kripken · 2021-09-22T23:51:03Z

+        var nwritten = $3;
+        var num = 0;
+        for (var i = 0; i < iovs_len; i++) {
+          var ptr = HEAP32[(((iovs)+(i*8))>>2)];


Suggested change

var ptr = HEAP32[(((iovs)+(i*8))>>2)];

var ptr = HEAP32[iovs + i*8 >> 2];

The JS order of operations allows this to be written without parens.

kripken · 2021-09-22T23:52:05Z

+
+  __wasi_errno_t __wasi_fd_seek(__wasi_fd_t fd, __wasi_filedelta_t offset, __wasi_whence_t whence, __wasi_filesize_t *newoffset)
+  {
+    abort();


Might be nice to make a function that gets a name and prints an error out before aborting, so the runtime error is a little clearer. But it would help mostly you @ethanalee so up to you...

I can have it print out the name of the stubbed function? @kripken . ex. "__wasi_fd_seek has been temporarily stubbed and is inert"?

I think you can: https://gcc.gnu.org/onlinedocs/gcc/Function-Names.html

kripken · 2021-09-22T23:52:37Z

+
+extern "C" {
+
+  __wasi_errno_t __wasi_fd_write( __wasi_fd_t fd, const __wasi_ciovec_t *iovs, size_t iovs_len, __wasi_size_t *nwritten) {


Suggested change

__wasi_errno_t __wasi_fd_write( __wasi_fd_t fd, const __wasi_ciovec_t *iovs, size_t iovs_len, __wasi_size_t *nwritten) {

__wasi_errno_t __wasi_fd_write(__wasi_fd_t fd, const __wasi_ciovec_t *iovs, size_t iovs_len, __wasi_size_t *nwritten) {

kripken · 2021-09-22T23:54:35Z

+    if (!WASMFS) {
+      libraries.push('library_syscall.js')
+      libraries.push('library_wasi.js')
+    }


Given that we realized this files contain non-filesystem syscalls too, we might want to remove this if. Things should still work for you, although the error messages might be less clear - basically, any time you see var FS being included, that'd mean you have a syscall you need to implement. But, if the error messages are useful, then given this is still an experimental option that seems fine to me.

I suggest instead you either:

modify these files and wrap the FS syscalls in if !WASMFS

split these out into library_wasi_fs.js and `'library_syscall_fs.js'

kripken · 2021-09-23T00:00:18Z

Oh, and it would be good to link to #15041 from the relevant PRs. Something like see #15041 in the top comment and the commit message perhaps.

kripken · 2021-09-23T00:01:01Z

(sorry for racing with your feedback @tlively ! some overlap there...)

sbc100 · 2021-09-23T00:03:30Z

+    if (!WASMFS) {
+      libraries.push('library_syscall.js')
+      libraries.push('library_wasi.js')
+    }


I suggest instead you either:

modify these files and wrap the FS syscalls in if !WASMFS

split these out into library_wasi_fs.js and `'library_syscall_fs.js'

sbc100 · 2021-09-23T00:09:56Z

@@ -0,0 +1,6 @@
+var WASMFS = {
+    $WASMFS : {}


It could be that you don't need this global object to hang stuff off.. it has proved to be an anti-pattern in some cases because it can defeat DCE.

sbc100 · 2021-09-23T00:11:16Z

+  __wasi_errno_t __wasi_fd_write( __wasi_fd_t fd, const __wasi_ciovec_t *iovs, size_t iovs_len, __wasi_size_t *nwritten) {
+    if (fd == 1 || fd == 2) {
+      MAIN_THREAD_EM_ASM({
+        var buffer = [];


Given that you have a JS library file I think it makes sense to put any large chunks of JS code there rather than inlining in C++.. for one thing it makes editing a lot easier (syntax highlighting, etc).

sbc100 · 2021-09-23T14:51:44Z

@@ -0,0 +1,55 @@
+// // Copyright 2021 The Emscripten Authors.  All rights reserved.


The issue that @curiousdannii brings up about using C++ in core libraries is worth thinking about.

The issues is that up until now one could build a C program and link agianst only C libraries and avoid the C++ runtime completely. (i.e. use emcc rather then em++.. as of today we still make C++ libraries available unless -s STRICT mode is also used but I hope to change that).

Using C++ language features should be fine but if we use C++ library features in our filesystem implementation then it makes it much hard to build a pure C application.

We do have some core libraries such as embind that are writting in C++ but those are not used to implement low level C APIs and its generally accepted that embind only works for C++ programs.

Its maybe not a huge issue, but if we do use C++ (and C++ library features in particular) that does means that users of WASMFS would need to make sure they link in libc++ and libc++abi.

Interesting point. Does that mean that even using new would be an issue, as that method is in libc++abi I think? If so that is concerning.

I do think it would be a big productivity win to use as modern a language as we can here.

Yes, I agree it nice to be able to implement system libraries in C++... and I think we should probably allow it here, but traditionally the C library implementation is implement in C .. for this reason.. otherwise it makes all program into C++ programs.

Really we are implementing part of the "OS" in C++ here but they happen to get linked into the user program.

I guess we can just have -s WASMFS imply -s DEFAULT_TO_CXX.

I guess we can just have -s WASMFS imply -s DEFAULT_TO_CXX.

Yes. But note that the goal is to eventually enable WASMFS by default, and then even remove the option so it is the only thing we support, which would mean that any program using a nontrivial file-related syscall would end up a C++ program. So we need to make a decision here I guess.

I guess it depends how much benefit we see from using C++ here. Perhaps its not the end of the world if all filesystem-using programs end up depending on C++.

Maybe we can bundle C++ runtime dependencies into the wasmfs build somehow so they don't need to be separately linked in?

DEFAULT_TO_CXX should be sufficient to guarantee they are linked it.

Could LTO versions of libc++ etc be included in the Docker build? Prior to this you'd only have to wait for it to be built when your own code was linking against libc++, but now it will be all C projects that use the FS too.

Yes, the Docker build should contains all flavors of libc++ i think. IIUC its a bug that it doesn't.

kripken · 2021-09-23T16:11:49Z

    # must not emit this unneeded internal thing
    self.assertNotContained('EMSCRIPTEN_GENERATED_FUNCTIONS', read_file('test_hello_world.js'))

+  @also_with_wasmfs


Sorry if I wasn't clear, I was suggesting adding a new decorator to the existing test. That would avoid adding a new test entirely, just one line for the decorator. (Well, and also defining the decorator the first time.)

Oh apologies. I will delete the new test I added!

kripken · 2021-09-24T15:03:08Z

@@ -0,0 +1,19 @@
+var WasmfsLibrary = {
+$WASMFS: {


Let's refactor this to avoid the single global WASMFS object. As @sbc100 mentioned that is a pattern that is not optimal because of code size.

Instead, something like

$wasmfsBuffers: [null, [], []], emscripten_wasmfs_printchar__deps: ['$wasmfsBuffers'], emscripten_wasmfs_printchar: function(stream, curr) { // use wasmfsBuffers[1] etc.

That way we define plain C functions for the C++ to call, and those functions can add dependencies on other JS things they need.

In the C++ file we'd declare

int emscripten_wasmfs_printchar(..);

and then call that - without knowing it is actually calling into JS.

Note that this also avoids the EM_ASM in the C++, which is also good.

kripken · 2021-09-24T15:06:04Z

+      const uint8_t* ptr = iovs[i].buf;
+      __wasi_size_t len = iovs[i].buf_len;
+      for (__wasi_size_t j = 0; j < len; j++) {
+        EM_ASM({ WASMFS.printChar($0, HEAPU8[$1]); }, fd, ptr + j);


It would be better to send an entire buffer to JS, and not a single character at a time, as calls to JS are not cheap.

In fact this may be an issue if we get many tiny buffers. That is something we'll need to measure at some point I guess - maybe add a TODO.

Changed it to just use the buffers that we receive via iovs

kripken · 2021-09-24T16:50:21Z

+    for (size_t i = 0; i < iovs_len; i++) {
+      const uint8_t* ptr = iovs[i].buf;
+      __wasi_size_t len = iovs[i].buf_len;
+      emscripten_wasmfs_printchar(fd, ptr, len);


The name should be updated too, as it's no longer printing a single char. How about _printbuffer?

kripken · 2021-09-24T16:52:23Z

The test errors look related to dynamic linking. I suspect that is due to recent work @sbc100 has been doing. Merging in very latest main should fix it, as then all the repos will be in sync (atm, this is using old emscripten code against newer LLVM and Binaryen, which we use the latest versions of on CI).

kripken

Great!

juj · 2021-09-25T13:31:52Z

+    }
+
+  }
+}


I would recommend implementing this whole function in C/C++. That way the state will be multithreading safe from the get-go (e.g. for behavior like fflush()ing stdout/stderr from another thread than the one(s) that have done partial printing). That method could use e.g. emscripten_console_log() and emscripten_console_error() for the printing.

For the general case I agree, but for stdout I think we need to proxy to the main thread anyhow - so that we can print it out on the page? If so, then I'm not sure how to make that multithreaded?

ethanalee added 3 commits September 22, 2021 19:24

stdout in wasmfs

6c05f32

emcc updated

7b70690

add library_wasmfs

6939f12

ethanalee force-pushed the wasmFSTemplate branch from b5db8e0 to 6939f12 Compare September 22, 2021 23:24

ethanalee requested review from kripken and tlively September 22, 2021 23:25

ethanalee added 4 commits September 22, 2021 19:26

add library_wasmfs to emcc.py

f2ce6b4

whitespace

d1b1e81

remove default list

f26043d

add blank lines

84d4bc5

tlively reviewed Sep 22, 2021

View reviewed changes

kripken reviewed Sep 22, 2021

View reviewed changes

sbc100 reviewed Sep 23, 2021

View reviewed changes

ethanalee added 3 commits September 23, 2021 11:13

addressed comments

92d5304

white space

3972910

remove parens

a301fac

kripken reviewed Sep 23, 2021

View reviewed changes

Comment thread tests/test_core.py

kripken reviewed Sep 23, 2021

View reviewed changes

Comment thread system/lib/wasmfs/wasmfs.cpp

grammar and tests fix

117be99

ethanalee mentioned this pull request Sep 23, 2021

double is reserved keyword #15110

Merged

extracted writing to stdout to library_wasmfs.js

9d4bdab

ethanalee requested review from kripken, sbc100 and tlively September 24, 2021 14:17

kripken reviewed Sep 24, 2021

View reviewed changes

changed layout of library_wasmfs, removed EM_ASM

28bcfcb

kripken reviewed Sep 24, 2021

View reviewed changes

change name to _printbuffer

d31597f

ethanalee force-pushed the wasmFSTemplate branch from 6f70329 to d31597f Compare September 24, 2021 18:02

ethanalee added 3 commits September 24, 2021 14:06

use emscripten_console_log

0ee2972

Merge remote-tracking branch 'origin/main' into wasmFSTemplate

6236f37

add html5.h header

47d4392

kripken reviewed Sep 24, 2021

View reviewed changes

Comment thread system/lib/wasmfs/wasmfs.cpp Outdated

removed errant #define, update test

13ef89f

kripken approved these changes Sep 24, 2021

View reviewed changes

sbc100 approved these changes Sep 24, 2021

View reviewed changes

Comment thread system/lib/wasmfs/wasmfs.cpp Outdated

Comment thread tests/test_core.py Outdated

ethanalee added 3 commits September 24, 2021 17:36

edited tests, comments

8ce49c4

if WASMFS is off, do not link wasmfs.cpp

d86d655

remove whitespace

d9936bf

ethanalee merged commit fac7c5d into main Sep 24, 2021

ethanalee deleted the wasmFSTemplate branch September 24, 2021 23:59

juj reviewed Sep 25, 2021

View reviewed changes

ethanalee mentioned this pull request Sep 27, 2021

Refactor __wasi_fd_write in wasmfs.cpp #15147

Merged


		extern "C" {

		__wasi_errno_t __wasi_fd_write( __wasi_fd_t fd, const __wasi_ciovec_t iovs, size_t iovs_len, __wasi_size_t nwritten) {

		var ptr = HEAP32[(((iovs)+(i*8))>>2)];
		var len = HEAP32[(((iovs)+(i*8 + 4))>>2)];

	@also_with_standalone_wasm()
	def test_hello_world(self):
	self.do_core_test('test_hello_world.c')
	# must not emit this unneeded internal thing
	self.assertNotContained('EMSCRIPTEN_GENERATED_FUNCTIONS', read_file('test_hello_world.js'))

	var ptr = HEAP32[(((iovs)+(i*8))>>2)];
	var ptr = HEAP32[iovs + i*8 >> 2];

	__wasi_errno_t __wasi_fd_write( __wasi_fd_t fd, const __wasi_ciovec_t iovs, size_t iovs_len, __wasi_size_t nwritten) {
	__wasi_errno_t __wasi_fd_write(__wasi_fd_t fd, const __wasi_ciovec_t iovs, size_t iovs_len, __wasi_size_t nwritten) {

		@@ -0,0 +1,55 @@
		// // Copyright 2021 The Emscripten Authors. All rights reserved.

+                  }
+                }
+              }

Uh oh!

Conversation

ethanalee commented Sep 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tlively left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kripken left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ethanalee Sep 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ethanalee Sep 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kripken commented Sep 23, 2021

Uh oh!

kripken commented Sep 23, 2021

ethanalee commented Sep 22, 2021 •

edited

Loading

ethanalee Sep 23, 2021 •

edited

Loading

ethanalee Sep 23, 2021 •

edited

Loading