Allow user override of default-enabled compile-time wasm features #22966
Conversation
The signext feature is enabled by default by clang and can be overridden with the -msign-ext/-mno-sign-ext flags at compile time. However it cannot be individually controlled at link time (it can only be lowered by setting the min browser version). This PR allows -mno-sign-ext to be used at link time as well, and override the browser version control.
src/settings_internal.js
Outdated
|
|
||
| var BULK_MEMORY = false; | ||
|
|
||
| var SIGN_EXT = true; |
There was a problem hiding this comment.
Should we name these internal settings so we can identify them easily. Maybe "F_SIGN_EXT" and "F_BULK_MEMORY"?
There was a problem hiding this comment.
That seems a little obscure to me still. Is FEATURE_SIGN_EXT too long? the only downside is for things like bulk memory that also impact how things are linked, it's not just a wasm feature. but maybe that's fine.
There was a problem hiding this comment.
These are all internal settings so we are free to change them later and we can bikeshed to our hearts content.
I would go for the short prefix myself, since its an internal thing, but either way is fine with me. Can also be a followup.
There was a problem hiding this comment.
Actually it turns out we don't need this setting, the logic can just stay in feature_matrix. I backed this out for now, can revisit if we need more settings in the upcoming PRs.
| settings.SIGN_EXT = 1 | ||
| feature_matrix.enable_feature(feature_matrix.Feature.SIGN_EXT, '-msign-ext') | ||
| elif arg == '-mno-sign-ext': | ||
| feature_matrix.disable_feature(feature_matrix.Feature.SIGN_EXT) |
There was a problem hiding this comment.
Why not do the same for bulk memory? I guess because we don't have a link time lowering yet?
There was a problem hiding this comment.
Right, link time lowering is in my local branch but not upstream yet.
There was a problem hiding this comment.
OK, perhaps we can generalize this once we do the same for bulk memory, perhaps even make it data driven? e.g.
# map command line feature flags to (setting, feature) pairs
feature_flags = {
'bulk-memory': ('BULK_MEMORY', feature_matrix.Feature.SIGN_EXT),
...
}
There was a problem hiding this comment.
Yeah, I'll come back to the best way to do this this with bulk and nontrapping once they are hooked up to feature_matrix.
|
PTAL, I also fixed explicit enabling/disabling for bulk memory here, since we already had processing for that. |
| def disable_feature(feature): | ||
| """Allow the user to disable a feature that would otherwise be on by default. | ||
| """ | ||
| disable_override_features.add(feature) |
There was a problem hiding this comment.
Can we have the same default override behavior in enable and disable, so they are parallel?
There was a problem hiding this comment.
The behavior is asymmetric in that there is no implicit disabling, only implicit enabling. So disable_feature can only be an explicit disable and will always override. But enable_feature can be explicit for a feature requested by a user, or implicit for a feature enabled as a result of another feature bumping the min browser versions.
|
Going to land this to avoid conflict with the upcoming PRs but they will be adding onto this, so any more suggestions can be addressed there. |
3 participants
The signext feature is enabled by default by clang and can be overridden
with the -msign-ext/-mno-sign-ext flags at compile time. However it
cannot be individually controlled at link time (it can only be lowered
by setting the min browser version). This PR allows -mno-sign-ext,
-msign-ext (and likewise -mbulk-memory) to be used at link time as well,
and override the browser version control.