Refactor __wasi_fd_write in wasmfs.cpp

[ethanalee]

• Refactor __wasi_fd_write to only use C++
• Utilizes emscripten_console_log instead of calling out to JS

Addresses Jukka's comment in #15104

[ethanalee]

error: undefined symbol: _ZNSt3__212basic_stringIcNS_11char_traitsIcEENS_9allocatorIcEEE9push_backEc (referenced by top-level compiled C/C++ code)

This error occurs in tests when -nostdlib++ is specified. We should consider what to do when users that do not want to include libc++

[kripken]

Could this be a std::vector<char>? That would avoid including code for string support.

[ethanalee]

Would this also not bring in libc++ as well if vector is part of stdlib?

[ethanalee]

wasm-ld: error: emscripten/cache/sysroot/lib/wasm32-emscripten/libwasmfs.a(wasmfs.o): undefined symbol: typeinfo for std::length_error
wasm-ld: error: emscripten/cache/sysroot/lib/wasm32-emscripten/libwasmfs.a(wasmfs.o): undefined symbol: vtable for std::length_error

[ethanalee]

error: undefined symbol: _ZNKSt3__220__vector_base_commonILb1EE20__throw_length_errorEv (referenced by top-level compiled C/C++ code)

[ethanalee]

wasm-ld: error: emscripten/cache/sysroot/lib/wasm32-emscripten/libwasmfs.a(wasmfs.o): undefined symbol: operator delete(void*)
wasm-ld: error: emscripten/cache/sysroot/lib/wasm32-emscripten/libwasmfs.a(wasmfs.o): undefined symbol: std::__2::__vector_base_common<true>::__throw_length_error() const
wasm-ld: error: emscripten/cache/sysroot/lib/wasm32-emscripten/libwasmfs.a(wasmfs.o): undefined symbol: operator new(unsigned long)

[sbc100]

Doesn't simply building without -nostdlib++ fix that?

[ethanalee]

Yes, I didn't use both. I was just trying out Alon's suggestion to use -fno-exceptions to see if the throw symbol warning would be resolved

[kripken]

@sbc100

I'd be ok with only doing this when WASMFS was not in settings.

Would you be ok, though, if in the long term we needed to keep passing some C++ stdlibs to the linker, always? When WASMFS becomes the default, that will be required, if we allow C++ in WASMFS itself.

[sbc100]

Yes, that is a little unfortunate. I was hoping we could so something like:

if settings.WASMFS:
  settings.DEFAULT_TO_CXX=1

But if that means pretty much all users will get DEFAULT_TO_CXX.. that is a little unfortunate. Removing libc++/libstdc++ from C programs is more of a nice-to-have.. its really not that important. If we can write WASMFS in C that would be good but it seems like that would slow us down quite lot, right? Probably not worth it.

[kripken]

Yes, there are tradeoffs here... I think writing WASMFS using modern C++ would be good (unique_ptr, etc. would avoid a lot of bugs). But it does mean that turning WASMFS on by default would mean DEFAULT_TO_CXX everywhere.

For completeness, we've also thrown around the idea of writing WASMFS in Rust or another language that would be better than C. But the build system integration work for that would be pretty large, sadly.

If you're ok with this then I think we can proceed here. But we should also look to see if we can avoid DEFAULT_TO_CXX somehow. I wonder if we can bundle the C++ requirements of WASMFS inside libwasmfs - those should be very few and very trivial, things like stubs for new/delete that just call malloc/free, etc. We'd avoid practically all C++ that uses stuff that is not in headers.

[kripken]

Hopefully we can avoid that error by not using a std::string here as I just commented. However, the larger issue is that we don't want to link in C++ stuff here as much as possible. That is, we want to make WASMFS the default option at some point in the future, and so anything that library needs would need to be linked in by default.

Using header-only stuff like std::vector should be fine (as then nothing is linked in). And perhaps we can live with things like new/delete as always being passed to the linker? But if we need significantly more, that would raise questions about our ability to use C++ for WASMFS I guess...

[tlively]

Code LGTM besides comments. I am also ok with linking in C++ library stuff as necessary. The benefits of using C++ are worth the costs (although we don't have a great measurement of the costs).

[tlively]

LGTM if tests pass and @kripken approves.

[tlively]

@ethanalee do you have an idea of how much extra code std::unordered_map, std::shared_ptr, and std::unique_ptr would bring in? I expect that those will largely cover what we want to use.

[sbc100]

@ethanalee do you have an idea of how much extra code std::unordered_map, std::shared_ptr, and std::unique_ptr would bring in? I expect that those will largely cover what we want to use.

IIUC, almost of STL is implemented purely in headers.. its only a few things like new and delete that come in from libraries.

Note that adding -lc++ and -lc++abi only links in what is needed so there is not real harm in including them at link time other than my desire for code hygiene. It can be slightly confusing for users such as myself to see -lc++ on the link line when I'm building a simple hello world C program.. but I don't know of any larger downsides to non-C++-using programs being linked against these libraries. wasm-ld will not include anything you don't use (Linking with -Wl,--whole-archive is the only why I can think of that that would happen and most folks don't do that).

[ethanalee]

@ethanalee do you have an idea of how much extra code std::unordered_map, std::shared_ptr, and std::unique_ptr would bring in? I expect that those will largely cover what we want to use.

For std::unique_ptr

None

For std::shared_ptr

emscripten/cache/sysroot/lib/wasm32-emscripten/libwasmfs.a(wasmfs.o): undefined symbol: std::__2::__shared_weak_count::__release_weak()
emscripten/cache/sysroot/lib/wasm32-emscripten/libwasmfs.a(wasmfs.o): undefined symbol: std::__2::__shared_weak_count::__release_weak()
emscripten/cache/sysroot/lib/wasm32-emscripten/libwasmfs.a(wasmfs.o): undefined symbol: vtable for std::__2::__shared_count
emscripten/cache/sysroot/lib/wasm32-emscripten/libwasmfs.a(wasmfs.o): undefined symbol: vtable for __cxxabiv1::__class_type_info

For std::unordered_map

emscripten/cache/sysroot/lib/wasm32-emscripten/libwasmfs.a(wasmfs.o): undefined symbol: std::__2::__next_prime(unsigned long)

[kripken]

This kind of worries me, to be honest. It seems like we might end up forgetting it, and then users would hit problems that don't occur in the test suite. That's a risk when working around issues in the test suite like this.

One possible idea is to move this to emcc.py, using the flag @sbc100 mentioned to link as cxx. We'll need that anyhow I think. However, I'm also ok with landing this if we put that on a TODO somewhere we are sure we won't forget.

[sbc100]

You mean using -s DEFAULT_TO_CXX=0 here instead of -nostdlib++? Sure we could try that. I'll open a PR to do that.

[kripken]

(Maybe what you meant, but to be sure I meant not here and instead in emcc.py - then it will work outside of the test suite too.)

[sbc100]

Are you suggesting we move forward with changing the default to DEFAULT_TO_CXX=0 for all users? This is the plan as layed out in #11121 but it could be a breaking change for many users and I'd like to start by issuing a warning.

The point of this line here is to opt into this new stricter behaviour within the test suite before we apply it to all users.

[kripken]

No, sorry, I mean to add something like this in emcc.py:

if WASMFS:
  DEFAULT_TO_CXX = 1

That would affect no one but a few tests atm.

[sbc100]

Yes that sounds like the correct solution.

[ethanalee]

We can land this as is for now. I will keep the todo and add this setting in the following PR

[kripken]

Followup PR sounds good.

[kripken]

Followup PR sounds good.