[spec] Adapt OOB behaviour in spec; store typing #129
Conversation
There was a problem hiding this comment.
Thanks for doing this! Overall it looks great.
I did an amateur pass through here in order to keep this moving. I believe I understand the execution semantics part of the spec fairly well. I have less of solid understanding of store typing and extension semantics, so I only verified that the new text matched the existing text's form well.
| \\ \qquad | ||
| (\otherwise) | ||
| \\[1ex] | ||
| S; F; (\I32.\CONST~d)~\val~(\I32.\CONST~n+1)~\MEMORYFILL |
There was a problem hiding this comment.
Out of curiosity, why the choice of 'n+1' and 'n' here? As opposed to 'n' and 'n-1'.
There was a problem hiding this comment.
Ah, because that avoids having to put n > 0 as an extra side condition and it makes the inductive nature of the definition more apparent.
document/core/exec/instructions.rst
Outdated
| @@ -722,13 +716,15 @@ Memory Instructions | |||
|
|
|||
| 5. Let :math:`\X{mem}` be the :ref:`memory instance <syntax-meminst>` :math:`S.\SMEMS[\X{ma}]`. | |||
|
|
|||
| 1. Let :math:`F` be the :ref:`current <exec-notation-textual>` :ref:`frame <syntax-frame>`. | |||
There was a problem hiding this comment.
I believe this should be dropped.
document/core/exec/instructions.rst
Outdated
|
|
||
| 25. Push the value :math:`\I32.\CONST~(cnt-1)` to the stack. | ||
| 24. Push the value :math:`\I32.\CONST~(n-1)` to the stack. |
There was a problem hiding this comment.
Numbering needs to be corrected here.
document/core/exec/instructions.rst
Outdated
|
|
||
| c. Push the value :math:`\I32.\CONST~1` to the stack. | ||
| e. Push the value :math:`\I32.\CONST~(s+n-1)` to the stack. |
There was a problem hiding this comment.
Ditto the store stack argument ordering issue from above.
|
|
||
| a. Push the value :math:`\I32.\CONST~dst` to the stack. | ||
| d. Execute the instruction :math:`\TABLESET`. |
There was a problem hiding this comment.
I don't think table.set has a full definition in this spec yet, but if it will match reftypes then we need to push the destination offset first. Like with i32.store.
There was a problem hiding this comment.
It actually has a temporary one below as and administrative instruction. But yes, this is superceded by the reftype proposal.
There was a problem hiding this comment.
Oh nice, I missed there was an entry in execution for it. I took a look at it and it does first pop funcelem, then the offset. So we need to push the dest offset before source offset here, like with memory.copy. This needs a change in the 'else' branch and the formal semantics as well.
|
|
||
| 14. If :math:`d \leq s`, then: | ||
|
|
||
| a. Push the value :math:`\I32.\CONST~d` to the stack. |
There was a problem hiding this comment.
I believe you meant to load from the source offset here.
document/core/exec/instructions.rst
Outdated
|
|
||
| 25. Push the value :math:`\I32.\CONST~(cnt-1)` to the stack. | ||
| 24. Push the value :math:`\I32.\CONST~(n-1)` to the stack. |
There was a problem hiding this comment.
Numbering needs to be corrected here.
|
|
||
| 19. Push the value :math:`\I32.\CONST~dst` to the stack. | ||
| 20. Push the value :math:`\funcelem` to the stack. |
There was a problem hiding this comment.
It may be more clear to reorder to 18, 20, then 19 so that the use of the funcelem value is right next to the definition.
Additionally, it's a bit odd to use funcelem here as a value, but I understand that's due to reftypes being a separate proposal. Is the intention for this to be an administrative instruction like table.get/set in the mean time?
There was a problem hiding this comment.
Yes, exactly. This is all a temporary hack. We screwed up the dependencies for these proposals.
Co-Authored-By: Ryan Hunt <rhunt@eqrion.net>
|
Thanks for that thorough review! |
|
I took another look and left one more comment. After that, this and the interpreter change should be ready to go! |
This does the necessary spec changes to address #127 and #128:
Baseline is #126; all changes outside spec dir are from that PR.
Any volunteers for reviewing?