Report missing section features in wasm2wat (#1197)

[rusty122]

Ran into #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

[binji]

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.

[rusty122]

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

[binji]

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.

[keithw]

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.