-
Notifications
You must be signed in to change notification settings - Fork 9
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
Fix Wasm-1.0/2.0 spec #48
Conversation
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.
Thanks!
rule Eval/done: | ||
z; val* ~>* z; val* | ||
|
||
rule Eval/step: | ||
z; admininstr* ~>* z''; val* | ||
-- Step: z; admininstr* ~> z'; admininstr'* | ||
-- Eval: z'; admininstr' ~>* z''; val* |
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.
Declaratively, it would be more natural to have this more general relation (while leaving Eval_expr as is except invoking Steps):
rule Eval/done: | |
z; val* ~>* z; val* | |
rule Eval/step: | |
z; admininstr* ~>* z''; val* | |
-- Step: z; admininstr* ~> z'; admininstr'* | |
-- Eval: z'; admininstr' ~>* z''; val* | |
relation Steps: config ~>* config hint(show "E") | |
rule Steps/refl: | |
z; admininstr* ~>* z; admininstr* | |
rule Steps/trans: | |
z; admininstr* ~>* z''; admininstr''* | |
-- Step: z; admininstr* ~> z'; admininstr'* | |
-- Steps: z'; admininstr' ~>* z''; admininstr''* |
Would that work in the backend as well?
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.
Yes, it should work well without any problem.
(It was already handled in hardcoded way. The rule premise Eval_expr: lhs ~> rhs
is rougly translated to the AL statement 1. Let rhs be the result of computing $eval_expr(lhs)
, where eval_expr
is a manually written algorithm)
I pushed a new commit introducing |
|
||
relation Eval: config ~>* state; val* hint(show "E-expr") | ||
relation Eval_expr: state; expr ~>* state; val* hint(show "E-expr") | ||
relation Steps: config ~>* config hint(show "E") |
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: move up to other relations.
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.
In that case, should I also move rules for Steps
above rules for Step
?
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.
I would move the relation decl after the others, so the rules can stay here, I think.
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.
Oh I see, I thought you meant moving Steps
relation above other Step
relations.
|
||
relation Eval: config ~>* state; val* hint(show "E-expr") | ||
relation Eval_expr: state; expr ~>* state; val* hint(show "E-expr") | ||
relation Steps: config ~>* config hint(show "E") |
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.
Same here.
Clarify order of the memory.copy immediates
This PR introduces following changes for Wasm 1.0 / 2.0 specs.
x* x'**
properly so that they are correctly parsed (This issue seems to be happening repeatedly. Perhaps, changing parser so that they are parsed as intended even without parenthesis would be a better option in long term.)instrX*
intoexprX
atinstantiate
functionStep_read
rule premises intoEval_expr
atinstantiate
function. (Step_read
requires exactly one reduction step to occur, i.e., disallows already-value instructions likeI32.CONST 0
.)