Simplifying feature handling

[tlively]

Here's how we currently handle features:

1. The command line flags are parsed, producing a set of features to enable (enabledFeatures) and a set of features to disable (disabledFeatures).

2. The input module is parsed, creating a baseline feature set from the target feature section, if it exists. If it doesn't exist, the baseline feature set is MVP.

3. ToolOptions::applyFeatures is called, adding enabledFeatures to the baseline and removing disabledFeatures.

binaryen/src/tools/tool-options.h

Lines 143 to 157 in 6829433

	void applyFeatures(Module& module) const {
	if (hasFeatureOptions) {
	if (!detectFeatures && module.hasFeaturesSection) {
	FeatureSet optionsFeatures = FeatureSet::MVP;
	optionsFeatures.enable(enabledFeatures);
	optionsFeatures.disable(disabledFeatures);
	if (module.features != optionsFeatures) {
	Fatal() << "module features do not match specified features. "
	<< "Use --detect-features to resolve.";
	}
	}
	module.features.enable(enabledFeatures);
	module.features.disable(disabledFeatures);
	}
	}

By default, applyFeatures tries to protect the user from mistakes by erroring out if the command line features are not identical to the target feature section modules. However, the user can avoid this error by explicitly accepting the target features section as a baseline using the --detect-features flag. I have never personally needed to use this flag and I would be shocked if anyone else has either. Generally, if the target features section exists, it contains the exact features you want and there is never any reason to override or modify it.

That suggests that we could get rid of the --detect-features flag, downgrade that hard error about mismatching feature sets to a warning, and have the target features section "win out" over the command line arguments. That would be kind of weird though, since normally you want to respect the users's explicit desires when possible. It just so happens that the user is likely to be wrong about what they want in this case. With this change, the rare users who want to override the default behavior would have to strip the target features section and try again.

Another consideration is that the ability to modify the baseline feature set might become more useful in the future. For example, we could give wasm-opt the ability to polyfill new features (e.g. GC and typed function references) in terms of old features (e.g. MVP Wasm and basic reference types). In that case, removing features from the baseline feature set could trigger the polyfill. Adding features to the baseline might also be useful once we have feature detection since it would allow for specializing binaries for a broader baseline feature set and removing unnecessary fallback code paths.

So overall I think our feature system is actually pretty good and future-proof, but perhaps it could be better documented?

[kripken]

the user can avoid this error by explicitly accepting the target features section as a baseline using the --detect-features flag

What does "accept as a baseline" mean?

[tlively]

Use as the base feature set to be modified by the subsequent --enable-* and --disable-* feature flags.

[kripken]

The flag name seems confusing then. What it is detecting the features from? The user's flags?

Btw, I have needed to use that flag when debugging fuzz testcases, but I don't remember exactly why...

[tlively]

It's supposed to mean detecting features from the target features section. There are three "baseline" feature options, which reset all features to a known state regardless of any previous feature flags.

• --mvp-features means turn all features off.
• --all-features means turn all features on.
• --detect-features means set all features to their target features section values.

[tlively]

Btw, I have needed to use that flag when debugging fuzz testcases, but I don't remember exactly why...

I've definitely seen the error telling me to use it when debugging fuzz test cases, but the better fix is usually to just delete all of the feature flags from your command line.

[kripken]

Perhaps the name could be changed, then. It seems like it doesn't really "detect" features from that section, rather it allows adding more things on top of those? That is, we by default already load the features from that section, but disallow more changes.

[tlively]

Yeah, I'd be fine with renaming it. Perhaps --read-features?

[kripken]

Hmm, again, the problem is that we always read the features? The flag only changes whether we allow user changes on top.

How about --relax-features..? 🤷

[tlively]

In my mental model, we don't use the target feature section by default when the user passes feature flags, we just check it for consistency with the user's flags. So for me this flag is meant to opt in to using the target features section.

[kripken]

I'm confused. How does it opt in to using the target features section? It just disables a possible error, doesn't it? If so, could it be --ignore-features-section? Or --verify-features-section?

But I suspect our mental models here are very different, which is the cause of the issue. So in theory a name that removes the ambiguity that allows two models would be good. But OTOH if we're just debating the name of a flag that is rarely used, maybe it's not worth changing.

[tlively]

I think figuring out a system that is intuitive for everyone is important. I certainly don't want to be the only one who thinks the current system makes sense.

The flag currently opts into using the target features section as a baseline to be modified by subsequent feature flags. In contrast, the default baseline when the user uses feature flags is MVP.

I see where you're coming from, though. Perhaps --update-features? With that name change, I would also want to change the current behavior to stop clearing the previously set feature flags since it no longer suggests overriding previous options.

[kripken]

I'm running into this again now with wasm-reduce. Currently that tool passes --detect-features. That will use the target features section, or MVP. On fuzzer testcases, the section is present, so things just work. But on real-world wasm files that use features but have no feature section, it errors. OTOH using --all-features was removed due to #2813

I don't have a great idea here, but this is pretty bad as basically the reducer has not been usable on non-fuzzer testcases that use features. If we don't have a big idea, I think we should go back to -all there, and live with the downsides mentioned in #2813

[tlively]

I had run into that a couple times and I manually recreated the target features section on the input by running wasm-opt with the necessary feature flags and -emit-target-features, but that's kind of annoying to do.

[tlively]

This was resolved in #3960.