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

Rename resize_memory to grow_memory, following the design. #134

Merged
merged 4 commits into from
Oct 14, 2015

Conversation

sunfishcode
Copy link
Member

The previous code interpreted resize_memory as an absolute size; this
patch also changes it to be a delta as described in the design.

Also, refactor resizing.wast to be independent of the host page size.

The previous code interpreted `resize_memory` as an absolute size; this
patch also changes it to be a delta as described in the design.

Also, refactor resizing.wast to be independent of the host page size.
@rossberg
Copy link
Member

Looks good to me, other than the question about trap vs OOM.

@sunfishcode
Copy link
Member Author

Yes, a dedicated exception would be better. Also, this makes me notice bugs due to not using unsigned arithmetic. I reused I64's unsigned operators here to fix this, though that breaks its encapsulation a little, it seems significantly more convenient than alternatives.

Patch updated. PTAL, thanks!

@rossberg
Copy link
Member

LGTM, although I find the name AddressOverflow a bit weird in this context. How about GrowNegative or s.th like that?

@sunfishcode
Copy link
Member Author

What do you think of the name MemorySizeOverflow?

@rossberg
Copy link
Member

Hm, I'm confused in what sense it is an overflow at all.

@sunfishcode
Copy link
Member Author

When the mathematical value of size + delta is too great to be represented as an unsigned number in the result type, it's an unsigned overflow.

@rossberg
Copy link
Member

Ah, sorry, you are right, I missed that due to these values being assumed to be unsigned, an overflow is the only reason that new_size < old_size can fail.

Okay, in that case, SizeOverflow or just Overflow sounds good to me (I wouldn't include the Memory prefix, since that's the module's name already).

Sorry for the noise!

@sunfishcode
Copy link
Member Author

Renamed AddressOverflow to SizeOverflow. Merging with LGTM above and the rename. Thanks!

sunfishcode added a commit that referenced this pull request Oct 14, 2015
Rename `resize_memory` to `grow_memory`, following the design.
@sunfishcode sunfishcode merged commit bb9735b into master Oct 14, 2015
@sunfishcode sunfishcode deleted the grow-memory branch October 14, 2015 16:14
@ghost
Copy link

ghost commented Oct 14, 2015

Would I be reading this right that the maximum memory this supports is 2GB-1?

Is this just an implementation limitation for the current spec tools, or has a decision been made that the size is interpreted as a signed number?

@sunfishcode
Copy link
Member Author

This patch is attempting to interpret the size as unsigned everywhere, and even corrects some code that previously interpreted it as signed. Did I miss any places?

@sunfishcode
Copy link
Member Author

Ah, looking at it again, I did miss some places. max_int is the max signed int. I'll submit a new PR to fix that.

alexcrichton pushed a commit to alexcrichton/spec that referenced this pull request Nov 18, 2019
[test] Integrate load extend and 64x2 conversions ops from WAVM
Connicpu pushed a commit to Connicpu/wasm-spec that referenced this pull request May 11, 2020
Connicpu pushed a commit to Connicpu/wasm-spec that referenced this pull request Jun 7, 2020
After the spec change in WebAssembly#126, byte copying order is not observable.
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