wabt

wabt copied to clipboard

Ran into https://github.com/WebAssembly/wabt/issues/1197 when using wasm2wat so I took a stab at adding hints for missing features like --enable-bulk-memory.

Previously, it seems like looping through WASM sections would return early for these errors (ignoring stop_on_first_error) since the ERROR_UNLESS macro will cause ReadSections to return Error for missing section features.

With this PR, we accumulate a list of the missing features in missing_section_features and print them out with a helpful error message when returning from ReadSections - this makes it easy to extend in the future. Took some inspiration from the thread on this un-merged PR tackling the same issue. We shouldn't run into any cases of reporting the same feature missing multiple times because there's already a guard for multiple section definitions.

See the updated test cases for what this looks like - happy to update the messaging if there are any suggestions

Thanks for looking into this, and sorry for the very long review delay. Like the previous PR, I think I'd prefer something a bit more comprehensive to handle missing features (i.e. collecting all _enabled() checks that lead to errors in a separate Features struct, then display those to the user before exiting).

That said, it seems like this error hits a lot of new users, so maybe a simpler solution for the common case would be OK. In that case, I think integrating the flag directly into the error (as was done in the previous PR) would be best.

Like the previous PR, I think I'd prefer something a bit more comprehensive to handle missing features (i.e. collecting all _enabled() checks that lead to errors in a separate Features struct, then display those to the user before exiting).

Oh that's sort of what I was going for - collecting the necessary but not enabled features in a vector and reporting them at the end of parsing - this should catch both --enable-bulk-memory and --enable-exceptions and report them together before ending the parsing stage. Are there other features that I missed? I assumed this we could catch all the missing features in binary-reader.cc but maybe there needs to be a more extensive solution if missing features get caught in other parts of the codebase

I assumed this we could catch all the missing features in binary-reader.cc but maybe there needs to be a more extensive solution if missing features get caught in other parts of the codebase

Yeah, features are checked in a lot of places. binary-reader.cc checks features in quite a few other places (search for _enabled() in that file). Then features are also checked when reading the text format too.

Closing as stale, but this would still be very nice to have in some form. The error messages when a feature is not enabled are still unhelpful.