Skip to content

Add a workaround for Safari 9 ARM iOS right shift by non-immediate zero JIT bug #7191

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

Jump to bottom
Merged
merged 1 commit into from
Oct 25, 2018
Merged

Add a workaround for Safari 9 ARM iOS right shift by non-immediate zero JIT bug #7191

merged 1 commit into from
Oct 25, 2018

Conversation

juj
Copy link
Collaborator

@juj juj commented Sep 25, 2018

@juj juj added the iOS label Sep 25, 2018
@kripken
Copy link
Member

kripken commented Sep 28, 2018

Perhaps this could be part of LEGACY_VM_SUPPORT?

cc @Brion

@juj
Copy link
Collaborator Author

juj commented Oct 3, 2018

I'd like not to put this as part of LEGACY_VM_SUPPORT, since it would be nicer to keep a more fine grained control. E.g. developer might be interesting in supporting old desktop Internet Explorer 9/10/11 users, but not care about Safari on iOS, or other way around. Instead it would be nice to go to opposite direction of a "catch-all" LEGACY_VM_SUPPORT setting, and use feature and/or browser specific things instead. That way developers can tackle code size in a fine grained way even when they need backward compatibility.

@kripken
Copy link
Member

kripken commented Oct 4, 2018 •
edited
Loading

@juj I do still think there is a benefit to a catch-all option, people that just want to support all older browsers. But I agree that it shouldn't be the only way for users that want specific things out of that. So how about adding this new option as you suggested, and also enabling it when LEGACY_VM_SUPPORT?

@juj
Copy link
Collaborator Author

juj commented Oct 8, 2018

Yeah, having a enable-all sounds good as well. I'll update this PR and add an option for that.

@juj juj force-pushed the workaround_ios_9_right_shift_zero_bug branch 2 times, most recently from 9597030 to fcab882 Compare October 10, 2018 09:11
@juj
Copy link
Collaborator Author

juj commented Oct 12, 2018

Pushed updated version of this. @kripken does this look good now?

kripken
kripken approved these changes Oct 12, 2018
View reviewed changes
Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

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

lgtm with that name fixed

emcc.py Outdated
@@ -1099,6 +1099,8 @@ def check(input_file):
if shared.Settings.LEGACY_VM_SUPPORT:
# legacy vms don't have wasm
assert not shared.Settings.WASM, 'LEGACY_VM_SUPPORT is only supported for asm.js, and not wasm. Build with -s WASM=0'
shared.Settings.POLYFILL_OLD_MATH_FUNCTIONS = 1
shared.Settings.SUPPORT_IOS_9 = 1
Copy link
Member

Choose a reason for hiding this comment

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

The IOS option has a different name here than elsewhere, which is WORKAROUND_IOS_9_RIGHT_SHIFT_BUG

@juj juj force-pushed the workaround_ios_9_right_shift_zero_bug branch from fcab882 to 97eccad Compare October 23, 2018 12:07
@juj juj force-pushed the workaround_ios_9_right_shift_zero_bug branch from 97eccad to 6a90bb4 Compare October 25, 2018 09:24
@juj juj merged commit b8b8cc7 into emscripten-core:incoming Oct 25, 2018
@juj juj mentioned this pull request Jan 28, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kripken kripken kripken approved these changes

Assignees
No one assigned
Labels
iOS
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@juj @kripken