Skip to content

Fix i32 unsigned range tests.#138

Merged
sunfishcode merged 1 commit intomasterfrom
really-unsigned
Oct 28, 2015
Merged

Fix i32 unsigned range tests.#138
sunfishcode merged 1 commit intomasterfrom
really-unsigned

Conversation

@sunfishcode
Copy link
Member

When testing whether a value is in the i32 unsigned range, use the
unsigned max value (aka minus_one), rather than max_int, which is the
signed max value.

@ghost
Copy link

ghost commented Oct 15, 2015

It appears Int64.of_int32 sign extends, so this might not be want is needed for wasm32.

Might it be better to spec the maximum index rather than memory size to support 4GB cleanly?

Is seems really confusing to be using the wasm storage types in the spec for general computations. Can the spec just use arbitrate precision integer maths.

@sunfishcode
Copy link
Member Author

Arg, good catch. I changed them to proper unsigned conversions now.

This caused some tests to expose the problem that the initial memory size is not rounded up to a page size. I've now added a patch which fixes that too.

The spec is using int64 as a "sufficiently large integer math" type, even when the index size is i32. In fact, the spec doesn't support wasm64 at all right now.

Copy link
Member

Choose a reason for hiding this comment

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

Nit: -1l (here and below)

Copy link
Member Author

Choose a reason for hiding this comment

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

When I make that change, I get:

File "spec/eval.ml", line 117, characters 14-38:
Error: This expression has type I32.t -> I64.t
but an expression was expected of type int

which I don't understand.

Copy link
Member

Choose a reason for hiding this comment

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

Oh yes, it's parsing the - as a binary operator. Need to put parens around the -1l.

@rossberg
Copy link
Member

LGTM modulo nits

Copy link
Member

Choose a reason for hiding this comment

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

I don't think this comment is necessary; we do this all over the place.

@sunfishcode sunfishcode force-pushed the really-unsigned branch 2 times, most recently from 5f097c2 to 9509588 Compare October 16, 2015 15:03
@sunfishcode
Copy link
Member Author

Looking at this again, since OCaml doesn't actually support unsigned integers, and since we do a proper check for unsigned overflow in the grow_memory check, I don't think we need the main change that was being made here. I removed this from the branch, and now all that remains is just the trivial test function renaming, which I'll now merge.

sunfishcode added a commit that referenced this pull request Oct 28, 2015
Rename some test functions, for consistency with other functions.
@sunfishcode sunfishcode merged commit dfe961c into master Oct 28, 2015
@sunfishcode sunfishcode deleted the really-unsigned branch October 28, 2015 16:09
Connicpu pushed a commit to Connicpu/wasm-spec that referenced this pull request Jun 7, 2020
ngzhian pushed a commit to ngzhian/spec that referenced this pull request Nov 4, 2021
dhil pushed a commit to dhil/webassembly-spec that referenced this pull request Mar 2, 2023
This moves all EH proposals into exception-handling/ directory, to
maintain the same level of directory structure with other proposals.
Also this renames Exceptions-v3.md to Exceptions-v2.md;
Exceptions-v2-Level-1.md was actually the same as that file with some
added context, and we so far have had two different proposals, not
three. Also this fixes some links.
alexcrichton pushed a commit to WebAssembly/wide-arithmetic that referenced this pull request Sep 19, 2025
Resolved minor issues of prose spec diffs
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.

3 participants