-
Notifications
You must be signed in to change notification settings - Fork 6
[interpreter] Add page type to IR of memory types #50
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
Conversation
fitzgen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, although I am definitely not an ocaml programmer.
@rossberg do you want to lend another pair of eyes to this?
I'm winging it--trad coding, if you can believe it! |
rossberg
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM mostly, but why did you not order the arguments of MemoryT consistently with TableT and with the text format, i.e., put the page type last?
Done. |
| let GlobalT (_mut, t) = gt in | ||
| check_valtype c t at | ||
|
|
||
| let check_memorytype (c : context) (mt : memorytype) at = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't you need to check here that pt is either 1 or 0x10000?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
interpreter/valid/valid.ml
Outdated
| require (I64.le_u min max) at | ||
| "size minimum must not be greater than maximum" | ||
|
|
||
| let check_pagesize (PageT ps) at = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: check_pagetype, since that's what you named the thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
|
AFAICT the missing parts now are just the parser, decoder, and encoder, and then it should actually be able to pass tests. The bounds-check machinery is already based on byte-sized memories and doesn't really concern itself with the pagesize, except in the places I already modified. |
This PR is just to get the ball rolling by adding a page type to a memory type.