Skip to content

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

Merged
merged 10 commits into from
Oct 25, 2024
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
9 changes: 9 additions & 0 deletions emcc.py
Original file line number Diff line number Diff line change
Expand Up @@ -384,6 +384,15 @@ def get_clang_flags(user_args):
if '-mbulk-memory' not in user_args:
flags.append('-mbulk-memory')

# In emscripten we currently disable bulk memory by default.
# This should be removed/updated when we als update the default browser targets.
if '-mbulk-memory' not in user_args and '-mno-bulk-memory' not in user_args:
# Bulk memory may be enabled via threads or directly via -s.
if not settings.BULK_MEMORY:
flags.append('-mno-bulk-memory')
if '-mnontrapping-fptoint' not in user_args and '-mno-nontrapping-fptoint' not in user_args:
flags.append('-mno-nontrapping-fptoint')

if settings.RELOCATABLE and '-fPIC' not in user_args:
flags.append('-fPIC')

Expand Down
45 changes: 45 additions & 0 deletions test/test_other.py
Original file line number Diff line number Diff line change
Expand Up @@ -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?
Copy link
Member Author

@dschuff dschuff Oct 24, 2024

Choose a reason for hiding this comment

The 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', '''
Expand Down
18 changes: 18 additions & 0 deletions tools/webassembly.py
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,12 @@ class DylinkType(IntEnum):
IMPORT_INFO = 4


class TargetFeaturePrefix(IntEnum):
USED = 0x2b
DISALLOWED = 0x2d
REQUIRED = 0x3d
Copy link
Member

Choose a reason for hiding this comment

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

Do we still emit REQUIRED? I can't find anywhere in LLVM we emit it after llvm/llvm-project@3f34e1b. cc @tlively

Copy link
Member Author

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The 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 REQUIRED. It would be a nice cleanup to remove it entirely here and in the tool conventions doc.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Amazing, thank you!



class InvalidWasmError(BaseException):
pass

Expand Down Expand Up @@ -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())
Copy link
Member

Choose a reason for hiding this comment

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

What does an argument to TargetFeaturePrefix do here?

Copy link
Member Author

Choose a reason for hiding this comment

The 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:
Expand Down
Loading