-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Explicitly set bulk memory and nontrapping-fptoint clang flags #22751
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
Changes from 8 commits
7cc2972
4ca45b2
5b956b4
6ed1380
5c5da10
9257c5b
dfb9e50
8ef0ed7
4575042
f0ebef2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10268,6 +10268,51 @@ def test_wasm_features_section(self, args): | |
self.run_process([EMCC, test_file('hello_world.c'), '-O2'] + args) | ||
self.verify_custom_sec_existence('a.out.wasm', 'target_features', False) | ||
|
||
def test_wasm_features(self): | ||
# Test that wasm features are explicitly enabled or disabled based on target engine version | ||
def verify_features_sec(feature, expect_in, linked=False): | ||
with webassembly.Module('a.out.wasm' if linked else 'hello_world.o') as module: | ||
features = module.get_target_features() | ||
if expect_in: | ||
self.assertTrue(feature in features and | ||
features[feature] == webassembly.TargetFeaturePrefix.USED, | ||
f'{feature} missing from wasm file') | ||
else: | ||
self.assertFalse(feature in features, | ||
f'{feature} unexpectedly found in wasm file') | ||
|
||
def verify_features_sec_linked(feature, expect_in): | ||
return verify_features_sec(feature, expect_in, linked=True) | ||
|
||
def compile(flags): | ||
self.run_process([EMCC, test_file('hello_world.c')] + flags) | ||
|
||
compile(['-c']) | ||
verify_features_sec('bulk-memory', False) | ||
verify_features_sec('nontrapping-fptoint', False) | ||
verify_features_sec('sign-ext', True) | ||
verify_features_sec('mutable-globals', True) | ||
verify_features_sec('multivalue', True) | ||
verify_features_sec('reference-types', 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']) | ||
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? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. one weirdness we have currently is that the -m flags (including -mno-signext currently) do not work at link time to disable features (and e.g. do not trigger the lowering passes). Only the the feature_matrix/browser version code does this. So there's currently no way to specify the feature behavior at link time on a granular level. I'm not sure what exactly the use case for this would be, but it wouldn't surprise me too much if there were one. Maybe we should think about that. (although it doesn't need to be this PR). |
||
verify_features_sec_linked('bulk-memory', True) | ||
|
||
def test_js_preprocess(self): | ||
# Use stderr rather than stdout here because stdout is redirected to the output JS file itself. | ||
create_file('lib.js', ''' | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -162,6 +162,12 @@ class DylinkType(IntEnum): | |
IMPORT_INFO = 4 | ||
|
||
|
||
class TargetFeaturePrefix(IntEnum): | ||
USED = 0x2b | ||
DISALLOWED = 0x2d | ||
REQUIRED = 0x3d | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we still emit There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure. Seems OK to include it in this enum though. For now we really only check for USED in the test. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This whole system is slightly overengineered (my fault 🫣), and I'm not aware of any uses of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Amazing, thank you! |
||
|
||
|
||
class InvalidWasmError(BaseException): | ||
pass | ||
|
||
|
@@ -561,6 +567,18 @@ def get_function_type(self, idx): | |
func_type = self.get_function_types()[idx - self.num_imported_funcs()] | ||
return self.get_types()[func_type] | ||
|
||
def get_target_features(self): | ||
section = self.get_custom_section('target_features') | ||
self.seek(section.offset) | ||
assert self.read_string() == 'target_features' | ||
features = {} | ||
self.read_byte() # ignore feature count | ||
while self.tell() < section.offset + section.size: | ||
prefix = TargetFeaturePrefix(self.read_byte()) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What does an argument to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It just constructs the enum with that value. |
||
feature = self.read_string() | ||
features[feature] = prefix | ||
return features | ||
|
||
|
||
def parse_dylink_section(wasm_file): | ||
with Module(wasm_file) as module: | ||
|
Uh oh!
There was an error while loading. Please reload this page.