Change exit() to Fatal(); make it possible to throw on Fatal().

[brson]

This patch makes binaryen easier to call from other applications by making more errors recoverable instead of early-exiting.

The main thing it does is change three calls to exit on I/O errors into calls to Fatal(), which is an existing custom abstraction for handling unrecoverable errors. Currently Fatal's destructor calls _Exit(1).

My intent is to make it possible for Fatal to not exit, but to throw, allowing an embedding application to catch the exception.

Because the previous early exits were exiting with error code EXIT_FAILURE, I also changed Fatal to exit with EXIT_FAILURE. The test suite continues to pass so I assume this is ok.

Next I changed Fatal to buffer its error message until the destructor instead of immediately printing it to stderr. This is for ease of patching Fatal to throw instead.

Finally, I also included the patch I need to make Fatal throw when THROW_ON_FATAL is defined at compile time. I can carry this patch out of tree, but it is a small patch, so perhaps you will be willing to take it. I am happy to remove it.

The aforementioned patch throws in a dtor, which is so strongly discouraged that it requires a special noexcept(false) annotation in c++17, the pitfall being that it is easy to throw during unwinding, which causes an abort. This is a special type though that only exists on error paths, and this solution is the smallest solution that might work for my use case. I have accounted for double throws by inserting an explicit abort in that case.

I have run the test suite successfully, but have not manually tested that fatal errors print the same as previously. I have tested that the patch throws exceptions that can be recovered in my project https://github.com/brson/wasm-opt-rs

Fixes #4938

[kripken]

Sounds good to me!

We should document that flag perhaps. At least in the changelog, but maybe there could be a section in the readme for emdedding? That section might also link to the rust bindings actually.

[brson]

I'll add some docs as requested. The explicit abort is I think not strictly necessary. I suspect but haven't tested, that c++ behaves like rust, and aborts on a double throw. So this branch is just an indication to readers that we know throwing in destructors is bad and have thought about it. I will remove it if you prefer, maybe just leave a comment about double throws.

…

On Wed, Sep 28, 2022, 11:59 AM Alon Zakai ***@***.***> wrote: ***@***.**** commented on this pull request. Sounds good to me! We should document that flag perhaps. At least in the changelog, but maybe there could be a section in the readme for emdedding? That section might also link to the rust bindings actually. ------------------------------ In src/support/utilities.h <#5087 (comment)>: > } +#else + // This variation is a best-effort attempt to make fatal errors recoverable + // for embedders of Binaryen as a library, namely wasm-opt-rs. + [[noreturn]] ~Fatal() noexcept(false) { + if (std::uncaught_exceptions()) { Is this part necessary? What bad thing happens if you just run the else arm unconditionally? — Reply to this email directly, view it on GitHub <#5087 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABD6DQUSH7OZE5UEM2YC43WARTMRANCNFSM6AAAAAAQXICJRY> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[kripken]

I see, thanks. That goes beyond my understanding of C++ exceptions 😄 I'd be ok either with removing it or a comment that explains it.

[brson]

I've updated the THROW_ON_FATAL patch with comments about throwing in destructors and removed the explicit abort; and added a commit that mentions it in the changelog.

I have contacted you on discord about how to best document it.

I think there's a lint error yet that I am waiting on ci again to look at.

[kripken]

This seems reasonable for now. Perhaps we can improve things further later but I don't have an idea how atm, and there is no harm in adding this now to help the Rust bindings.

[brson]

Updated for the lint.