[wasm-dis] Restore features section in output

[aheejin]

This effectively restores pre-#7802 status, except for the case the input binary has the features section. If the binary does not have it, we restore the default features (mutable-global + sign-ext) + whatever is given in the command line before the printing. If the binary has the features section, we just restore it before the printing.

This currently does not handle the case when there is the features section and the user gives more features flags in the command line, but in the current interface it is cumbersome to handle. And we were not handling that anyway pre-#7802, so it is not a regression.

And we preserve the FeaturesSet::All setting at parsing time so @tlively's concern will not be an issue.

Because we preserve the features section, #7836 will be addressed too.

The main motivation for #7802 was call_indirect could be incorrectly printed, and that was also separately addressed by #7843.

ModuleReader::getFeaturesSectionFeatures was adopted from @tlively's commit:
ed36a8c

Because we don't set the features to Features::All at the printing time, (type $..) in (func) definitions and (table ..) in call_indirects have been removed.

Fixes #7836.

[kripken]

I'm not opposed to this, it makes sense and seems reasonably simple, but what do you think about the other option of trusting the features section over the defaults? That is,

• If the features section exists, use those features.
• If it does not, use the default enabled features (which are atm the same as LLVM).
• Either way, user-applied features from the commandline are applied at the end, overriding things etc.

I might be missing something but that seems fairly simple to do as well? We already read the features section early (in preScan(), so we can decide what to do with it before everything else,

binaryen/src/wasm/wasm-binary.cpp

Lines 2012 to 2013 in 00aae63

	} else if (sectionName == BinaryConsts::CustomSections::TargetFeatures) {
	readFeatures(oldPos, payloadLen);

[aheejin]

@kripken So the enabling of all features in the beginning was due to @tlively's concern (#7836 (comment) and #7790), so I think we can't change that?

Then the only thing that's different between this PR and your suggestion is when we don't have a features section this PR uses a blank features and you suggest we use the default one. Is my understanding correct?

[kripken]

Then the only thing that's different between this PR and your suggestion is when we don't have a features section this PR uses a blank features and you suggest we use the default one. Is my understanding correct?

For wasm-dis, I think that's right. But the 3 bullet points I wrote are kind of how I see a reasonable behavior for all the tools, as a whole. (I'm not sure if the others would need any changes or not.)

About @tlively's concern, quoting from your first link

I'm concerned about the case where the input file does not end up having a target features section, but it uses custom descriptors in its types.

I think we don't have a problem because we pre-scan the features section, as I wrote above. That is, we know and can use the features from the very start, and pass the right ones into TypeBuilder. That is the big benefit of our pre-scanning.

[tlively]

In that case I described there is no target features section to pre-scan.

[kripken]

Oh, you want to preserve the current behavior where wasm-dis doesn't do any validation, so users don't need to pass in flags - is that right? If so then wasm-dis does need to remain unique among the tools, and we need some hack for that (like skipping validation is a hack). Can we achieve that by passing FeatureSet::All to the TypeBuilder?

(Long-term, my preferred solution to this is for all the tools to consistently enable features on demand, based on what they read. So if you use atomics, we enable atomics, etc.. And no one needs to add flags for existing content in a wasm.)

[tlively]

Oh, you want to preserve the current behavior where wasm-dis doesn't do any validation, so users don't need to pass in flags - is that right? If so then wasm-dis does need to remain unique among the tools, and we need some hack for that (like skipping validation is a hack). Can we achieve that by passing FeatureSet::All to the TypeBuilder?

Right. This is effectively what we're currently doing by passing FeatureSet::All into the parser, since the parser creates the TypeBuilder.

[aheejin]

So the latest commit effectively restores pre-#7802 status, except for the case the input binary has the features section. If the binary does not have it, we restore the default features (mutable-global + sign-ext) + whatever is given in the command line before the printing. If the binary has the features section, we just restore it before the printing.

This currently does not handle the case when there is the features section and the user gives more features flags in the command line, but in the current interface it is cumbersome to handle. And we were not handling that anyway pre-#7802, so it is not a regression.

And we preserve the FeaturesSet::All setting at parsing time so @tlively's concern will not be an issue.

Because we preserve the features section, #7836 will be addressed too.

The main motivation for #7802 was call_indirect could be incorrectly printed, and that was also separately addressed by #7843.

I'll update the PR description accordingly.