WebAssembly
file.cpp calls `exit` instead of throwing exception
I am calling binaryen's ModuleReader and ModuleWriter APIs from Rust and discovered that there are three cases in file.cpp where it calls exit. This isn't suitable for embedding in other applications. Can it be changed to throw an exception instead? Callers already need to deal with ParseException and MapParseException so this doesn't seem too burdensome a change.
If it's ok to change this to throw exceptions, would following the same pattern as MapException be acceptable? I am willing to make this change myself.
I think this makes sense. My one concern is that in general we are trying to move to have less exception-handling code in Binaryen, in particular so that the wasm port can be built with -fno-exceptions. But I think this change would not impact that, as in -fno-exceptions builds the exceptions would just exit or abort I guess. And there should be no other downside to using exceptions for ModuleReader/Writer errors AFAICT. But perhaps people that know more about C++ than me can confirm if there's really no downside though.
Not using exceptions is generally better for rust too, so if there's a different error handling pattern you'd like I'm happy to do that instead.
It doesn't seem like anyone else has strong opinions here, so I think using C++ exceptions for file.cpp is fine.
We have all kinds of error handling patterns in Binaryen. Another to consider would be something like BuildResult from wasm-type.h. https://github.com/WebAssembly/binaryen/blob/main/src/wasm-type.h#L610-L642. Alternatively, we could move Result and BuildResult (https://github.com/WebAssembly/binaryen/blob/main/src/wat-parser.h#L26-L67) to a more general header and use those, or we could pull in a std::expected implementation from C++23 (https://en.cppreference.com/w/cpp/header/expected).
I would mildly prefer any of those options to using exceptions, but also using exceptions here is certainly the simplest short-term fix. It would be nice to go through and standardize our error handling as a code health project at some point.
Pulling the Result and MaybeResult types out of wat-parser.h and reusing them looks like something I can handle. I don't quite understand how to use BuildResult on first read though.
After spending a day working on this I discovered the Fatal type, which is used in quite a few places, some of which also are possibly places it might be desirable for the process to not abort in an embedded context.
For my purposes, as an alternative to refactoring binaryen to propagate more non-exception errors, which will make the code increasingly more verbose, I could also convert the errors in file-io.cpp to use Fatal, and just apply a simple patch to convert Fatal to throw an exception that I can catch.
I know from the comments in Fatal that the function is expected to break deadlocks in some cases, which would not happen if converted to a throw, so there are some tradeoffs.