Skip to content

Fix invalid store offsets in memset polyfill #1787

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 2 commits into from
Apr 6, 2021
Merged

Fix invalid store offsets in memset polyfill #1787

merged 2 commits into from
Apr 6, 2021

Conversation

dcodeIO
Copy link
Member

@dcodeIO dcodeIO commented Apr 6, 2021

Some of the stores in memset are invalid for certain arguments due to enforcing a constant offset where we'd instead want the whole offset to wrap around. Traced this back to #1103 where I assumed that we'd be using lowMemoryUnused anyhow (i.e. never storing to the low 1024 bytes of memory), which actually isn't always the case in memset.

Discovered in #1785.

  • I've read the contributing guidelines

@dcodeIO
Copy link
Member Author

dcodeIO commented Apr 6, 2021

The change here essentially reverts to the state before #1103.

@dcodeIO dcodeIO requested a review from MaxGraey April 6, 2021 13:13
@MaxGraey
Copy link
Member

MaxGraey commented Apr 6, 2021

Hmm, it seems this cause to significant regression in term of code size and perhaps performance. I prefer solve (fix) this on binaryen side instead just revert all changes

@MaxGraey
Copy link
Member

MaxGraey commented Apr 6, 2021

Case store<u8>(-2, 0, 3) possible only if you manually set memory.fill(0, 0, 2). But usually it's not possible in managed runtime when we have memoryBase >= 8. So I guess better add fix to binaryen which normalize store<u8>(-2, 0, 3) to store<u8>(1, 0, 0)

@dcodeIO
Copy link
Member Author

dcodeIO commented Apr 6, 2021

I am not sure that this can be fixed on the Binaryen side, since what we are emitting (store with explicit constant offset) is already invalid for certain, even though uncommon, inputs. I agree that the code size regression here is unfortunate, but at least it's correct.

Copy link
Member

@MaxGraey MaxGraey left a comment

Choose a reason for hiding this comment

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

Alright, now it much better

@dcodeIO dcodeIO merged commit 9b21306 into master Apr 6, 2021
@dcodeIO dcodeIO deleted the issue-1785 branch June 1, 2021 15:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants