Skip to content
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.

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

Add sign-ext lowering pass when target browser doesn't support it #17689

Merged
merged 1 commit into from
Nov 16, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 4 additions & 1 deletion ChangeLog.md
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,10 @@ See docs/process.md for more on how version tagging works.
support these older engines you can use these settings
(`-sMIN_SAFARI_VERSION=120000` and/or `-sMIN_EDGE_VERSION=44`) to revert to
the previous defaults, which will result in the new proposals being disabled.
(#17690)
Note that in order to avoid support for the sign-extension emscripten uses
a binaryen pass, so targeting older browsers requires the running of wasm-opt
and is therefore incompatible with `ERROR_ON_WASM_CHANGES_AFTER_LINK` (i.e.
fast linking). (#17690)
- Added `--reproduce` command line flag (or equivalently `EMCC_REPRODUCE`
environment variable). This options specifies the name of a tar file into
which emscripten will copy all of the input files along with a response file
Expand Down
23 changes: 7 additions & 16 deletions emcc.py
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@
from tools.response_file import substitute_response_files
from tools.minimal_runtime_shell import generate_minimal_runtime_html
import tools.line_endings
from tools import feature_matrix
from tools import js_manipulation
from tools import wasm2c
from tools import webassembly
Expand Down Expand Up @@ -466,21 +467,6 @@ def apply_user_settings():
settings.LTO = 0 if value else 'full'


# 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.
def apply_min_browser_versions():

def default_min_browser_version(browser, version):
default_setting(f'MIN_{browser.upper()}_VERSION', version)

if settings.WASM_BIGINT:
default_min_browser_version('Safari', 150000)
default_min_browser_version('Edge', 79)
Copy link
Member

Choose a reason for hiding this comment

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

unrelated change?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually its needed, since setting the edge version to anything by -1 currently means no features are supported. i.e. the edge version currently means only EdgeHTML and not modern Edge.

Copy link
Member

Choose a reason for hiding this comment

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

I don't follow. First, why can't we use a single number for both browsers? I thought their version numbers did not overlap, and were monotonically increasing.

Second, what does it mean to not mention Edge here at all? Will anyone setting any value of Edge get sign-ext lowered out?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The proposal is that for now the edge version only refers to the old edge browser, and if the features of modern edge ever diverge from chromium we can start to support MIN_EDGE_VERSION being distinct from MIN_CHROME_VERSION

Not mentioning edge at all there just means that default of "-1" (not supported at all) will prevail.

Right now I'm not considering edge at all in the feature_matix.py because we currently assume that folks can use the MIN_CHROMIUM_VERSION to target modern versions of edge. Instead of removing this line I could instead extend feature_matrix.py to include support for MIN_EDGE_VERSION (but it would just be identical what is already there for chromium).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK, I reworked feature_matrix.py such that this change is no longer needed.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, then the Chrome version refers to modern Edge too? Sgtm, but I wasn't aware their features had even been in sync so far. Is that the case in practice?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If you looks at the versions in https://en.wikipedia.org/wiki/Microsoft_Edge (expand "New Edge release history") you will see that edge versions correspond 1-to-1 with blink versions.. to from a JS/wasm perspective one would expect them to always match.

default_min_browser_version('Firefox', 68)
# Chrome has BigInt since v67 which is less than default min version.


def is_ar_file_with_missing_index(archive_file):
# We parse the archive header outselves because llvm-nm --print-armap is slower and less
# reliable.
Expand Down Expand Up @@ -614,6 +600,11 @@ def get_binaryen_passes():
passes += ['--safe-heap']
if settings.MEMORY64 == 2:
passes += ['--memory64-lowering']
# sign-ext is enabled by default by llvm. If the target browser settings don't support
# this we lower it away here using a binaryen pass.
if not feature_matrix.caniuse(feature_matrix.Feature.SIGN_EXT):
logger.debug('lowering sign-ext feature due to incompatiable target browser engines')
passes += ['--signext-lowering']
if optimizing:
passes += ['--post-emscripten']
if optimizing:
Expand Down Expand Up @@ -2748,7 +2739,7 @@ def get_full_import_name(name):
if settings.WASM_EXCEPTIONS:
settings.EXPORTED_FUNCTIONS += ['___cpp_exception', '___cxa_increment_exception_refcount', '___cxa_decrement_exception_refcount', '___thrown_object_from_unwind_exception']

apply_min_browser_versions()
feature_matrix.apply_min_browser_versions()

if settings.SIDE_MODULE:
# For side modules, we ignore all REQUIRED_EXPORTS that might have been added above.
Expand Down
4 changes: 3 additions & 1 deletion src/settings.js
Original file line number Diff line number Diff line change
Expand Up @@ -1968,7 +1968,9 @@ var SEPARATE_DWARF_URL = '';
// not in others like split-dwarf).
// When this flag is turned on, we error at link time if the build requires any
// changes to the wasm after link. This can be useful in testing, for example.
// [link]
// Some example of features that require post-link wasm changes are:
// - Lowering i64 to i32 pairs at the JS boundary (See WASM_BIGINT)
// - Lowering sign-extnesion operation when targeting older browsers.
var ERROR_ON_WASM_CHANGES_AFTER_LINK = false;

// Abort on unhandled excptions that occur when calling exported WebAssembly
Expand Down
6 changes: 6 additions & 0 deletions test/other/test_signext_lowering.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
#include <stdio.h>
#include <stdint.h>

int main(int argc, char* argv[]) {
printf("%lld\n", (int64_t)argc);
}
29 changes: 29 additions & 0 deletions test/test_other.py
Original file line number Diff line number Diff line change
Expand Up @@ -12724,3 +12724,32 @@ def test_reproduce(self):
response = response.replace('\\', '/')
response = response.replace(root, '<root>')
self.assertTextDataIdentical(expected, response)

def test_signext_lowering(self):
cmd = [EMCC, test_file('other/test_signext_lowering.c'), '-sWASM_BIGINT']

# We expect ERROR_ON_WASM_CHANGES_AFTER_LINK to fail due lowering of sign-ext being
# required for safari 12.
output = self.expect_fail(cmd + ['-sERROR_ON_WASM_CHANGES_AFTER_LINK', '-sMIN_SAFARI_VERSION=120000'])
self.assertContained('error: changes to the wasm are required after link, but disallowed by ERROR_ON_WASM_CHANGES_AFTER_LINK', output)
self.assertContained('--signext-lowering', output)

# Same for firefox
output = self.expect_fail(cmd + ['-sERROR_ON_WASM_CHANGES_AFTER_LINK', '-sMIN_FIREFOX_VERSION=61'])
self.assertContained('error: changes to the wasm are required after link, but disallowed by ERROR_ON_WASM_CHANGES_AFTER_LINK', output)
self.assertContained('--signext-lowering', output)

# Same for chrome
output = self.expect_fail(cmd + ['-sERROR_ON_WASM_CHANGES_AFTER_LINK', '-sMIN_CHROME_VERSION=73'])
self.assertContained('error: changes to the wasm are required after link, but disallowed by ERROR_ON_WASM_CHANGES_AFTER_LINK', output)
self.assertContained('--signext-lowering', output)

# Running the same command with safari 14.1 should succeed because no lowering
# is required.
self.run_process(cmd + ['-sERROR_ON_WASM_CHANGES_AFTER_LINK', '-sMIN_SAFARI_VERSION=140100'])
self.assertEqual('1\n', self.run_js('a.out.js'))

# Running without ERROR_ON_WASM_CHANGES_AFTER_LINK but with safari 12
# should successfully lower sign-ext.
self.run_process(cmd + ['-sMIN_SAFARI_VERSION=120000', '-o', 'lowered.js'])
self.assertEqual('1\n', self.run_js('lowered.js'))
88 changes: 88 additions & 0 deletions tools/feature_matrix.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
# Copyright 2022 The Emscripten Authors. All rights reserved.
# Emscripten is available under two separate licenses, the MIT license and the
# University of Illinois/NCSA Open Source License. Both these licenses can be
# found in the LICENSE file.

"""Utilities for mapping browser versions to webassembly features."""

import logging
from enum import IntEnum, auto

from .settings import settings, user_settings

logger = logging.getLogger('feature_matrix')


class Feature(IntEnum):
NON_TRAPPING_FPTOINT = auto()
SIGN_EXT = auto()
BULK_MEMORY = auto()
MUTABLE_GLOBALS = auto()
JS_BIGINT_INTEGRATION = auto()


default_features = {Feature.SIGN_EXT, Feature.MUTABLE_GLOBALS}

min_browser_versions = {
Feature.NON_TRAPPING_FPTOINT: {
'chrome': 75,
'firefox': 65,
'safari': 150000,
},
Feature.SIGN_EXT: {
'chrome': 74,
'firefox': 62,
'safari': 140100,
},
Feature.BULK_MEMORY: {
'chrome': 75,
'firefox': 79,
'safari': 150000,
},
Feature.MUTABLE_GLOBALS: {
'chrome': 74,
'firefox': 61,
'safari': 120000,
},
Feature.JS_BIGINT_INTEGRATION: {
'chrome': 67,
'firefox': 68,
'safari': 150000,
},
}


def caniuse(feature):
min_versions = min_browser_versions[feature]
if settings.MIN_CHROME_VERSION < min_versions['chrome']:
return False
# For edge we just use the same version requirements as chrome since,
# at least for modern versions of edge, they share version numbers.
if settings.MIN_EDGE_VERSION < min_versions['chrome']:
return False
if settings.MIN_FIREFOX_VERSION < min_versions['firefox']:
return False
if settings.MIN_SAFARI_VERSION < min_versions['safari']:
return False
sbc100 marked this conversation as resolved.
Show resolved Hide resolved
# IE don't support any non-MVP features
if settings.MIN_IE_VERSION != 0x7FFFFFFF:
return False
return True


def enable_feature(feature):
"""Updates default settings for browser versions such that the given
feature is available everywhere.
"""
for name, min_version in min_browser_versions[feature].items():
name = f'MIN_{name.upper()}_VERSION'
if settings[name] < min_version and name not in user_settings:
setattr(settings, name, min_version)


# 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.
def apply_min_browser_versions():
if settings.WASM_BIGINT:
enable_feature(Feature.JS_BIGINT_INTEGRATION)