Skip to content

Allow user override of default-enabled compile-time wasm features #22966

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Nov 20, 2024
Merged

Allow user override of default-enabled compile-time wasm features #22966

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ff3e19a
Allow user override of default-enabled compile-time wasm features
dschuff Nov 20, 2024
86d3d4f
improve reason field
dschuff Nov 20, 2024
27935e5
also allow explicit enable override
dschuff Nov 20, 2024
a62c634
review feedback
dschuff Nov 20, 2024
7936268
Merge branch 'main' into feature_override
dschuff Nov 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions emcc.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@
from tools import cache
from tools.settings import default_setting, user_settings, settings, MEM_SIZE_SETTINGS, COMPILE_TIME_SETTINGS
from tools.utils import read_file, removeprefix
from tools import feature_matrix

logger = logging.getLogger('emcc')

Expand Down Expand Up @@ -1403,8 +1404,18 @@ def consume_arg_file():
settings.WASM_EXCEPTIONS = 0
elif arg == '-mbulk-memory':
settings.BULK_MEMORY = 1
feature_matrix.enable_feature(feature_matrix.Feature.BULK_MEMORY,
'-mbulk-memory',
override=True)
elif arg == '-mno-bulk-memory':
settings.BULK_MEMORY = 0
feature_matrix.disable_feature(feature_matrix.Feature.BULK_MEMORY)
elif arg == '-msign-ext':
feature_matrix.enable_feature(feature_matrix.Feature.SIGN_EXT,
'-msign-ext',
override=True)
elif arg == '-mno-sign-ext':
feature_matrix.disable_feature(feature_matrix.Feature.SIGN_EXT)
Copy link
Collaborator

@sbc100 sbc100 Nov 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not do the same for bulk memory? I guess because we don't have a link time lowering yet?

Copy link
Member Author

@dschuff dschuff Nov 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, link time lowering is in my local branch but not upstream yet.

Copy link
Collaborator

@sbc100 sbc100 Nov 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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),
  ...
}

Copy link
Member Author

@dschuff dschuff Nov 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

elif arg == '-fexceptions':
# TODO Currently -fexceptions only means Emscripten EH. Switch to wasm
# exception handling by default when -fexceptions is given when wasm
Expand Down
24 changes: 19 additions & 5 deletions test/test_other.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10434,23 +10434,37 @@ def compile(flags):
verify_features_sec('multivalue', True)
verify_features_sec('reference-types', True)

# Disable a feature
compile(['-mno-sign-ext', '-c'])
verify_features_sec('sign-ext', False)
# Disabling overrides default browser versions
compile(['-mno-sign-ext'])
verify_features_sec_linked('sign-ext', False)
# Disabling overrides manual browser versions
compile(['-sMIN_SAFARI_VERSION=150000', '-mno-sign-ext'])
# Disable via browser selection
compile(['-sMIN_FIREFOX_VERSION=61'])
verify_features_sec_linked('sign-ext', False)
# Manual enable overrides browser version
compile(['-sMIN_FIREFOX_VERSION=61', '-msign-ext'])
verify_features_sec_linked('sign-ext', True)

compile(['-mnontrapping-fptoint', '-c'])
verify_features_sec('nontrapping-fptoint', True)

# BIGINT causes binaryen to not run, and keeps the target_features section after link
# Setting this SAFARI_VERSION should enable bulk memory because it links in emscripten_memcpy_bulkmem
# However it does not enable nontrapping-fptoint yet because it has no effect at compile time and
# no libraries include nontrapping yet.
compile(['-sMIN_SAFARI_VERSION=150000', '-sWASM_BIGINT'])
compile(['-sMIN_SAFARI_VERSION=150000'])
verify_features_sec_linked('sign-ext', True)
verify_features_sec_linked('mutable-globals', True)
verify_features_sec_linked('multivalue', True)
verify_features_sec_linked('bulk-memory', True)
verify_features_sec_linked('nontrapping-fptoint', False)

compile(['-sMIN_SAFARI_VERSION=150000', '-mno-bulk-memory', '-sWASM_BIGINT'])
# FIXME? -mno-bulk-memory at link time does not override MIN_SAFARI_VERSION. it probably should?
verify_features_sec_linked('bulk-memory', True)
compile(['-sMIN_SAFARI_VERSION=150000', '-mno-bulk-memory'])
# -mno-bulk-memory at link time overrides MIN_SAFARI_VERSION
verify_features_sec_linked('bulk-memory', False)

def test_js_preprocess(self):
# Use stderr rather than stdout here because stdout is redirected to the output JS file itself.
Expand Down
17 changes: 16 additions & 1 deletion tools/feature_matrix.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,8 @@ class Feature(IntEnum):


default_features = {Feature.SIGN_EXT, Feature.MUTABLE_GLOBALS}
disable_override_features = set()
enable_override_features = set()

min_browser_versions = {
Feature.NON_TRAPPING_FPTOINT: {
Expand Down Expand Up @@ -93,6 +95,11 @@ class Feature(IntEnum):


def caniuse(feature):
if feature in disable_override_features:
return False
if feature in enable_override_features:
return True

min_versions = min_browser_versions[feature]

def report_missing(setting_name):
Expand All @@ -114,10 +121,12 @@ def report_missing(setting_name):
return True


def enable_feature(feature, reason):
def enable_feature(feature, reason, override=False):
"""Updates default settings for browser versions such that the given
feature is available everywhere.
"""
if override:
enable_override_features.add(feature)
for name, min_version in min_browser_versions[feature].items():
name = f'MIN_{name.upper()}_VERSION'
if settings[name] < min_version:
Expand All @@ -132,6 +141,12 @@ def enable_feature(feature, reason):
setattr(settings, name, min_version)


def disable_feature(feature):
"""Allow the user to disable a feature that would otherwise be on by default.
"""
disable_override_features.add(feature)
Copy link
Member

@kripken kripken Nov 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we have the same default override behavior in enable and disable, so they are parallel?

Copy link
Member Author

@dschuff dschuff Nov 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.



# apply minimum browser version defaults based on user settings. if
# a user requests a feature that we know is only supported in browsers
# from a specific version and above, we can assume that browser version.
Expand Down
Loading