-
Notifications
You must be signed in to change notification settings - Fork 27
[spec] Document execution #19
Conversation
Rendered result: https://webassembly.github.io/bulk-memory-operations/core/bikeshed |
@rossberg PTAL if you have time! |
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.
This looks mostly good, except for the correct indexing semantics.
One thing I was hoping for, though, is that with proper memory.init
and table.init
instructions we could get rid of the auxiliary init_data
and init_elem
ones, and replace their uses with the former. Is there a reason why that cannot work?
document/core/exec/instructions.rst
Outdated
|
||
12. Pop the value :math:`\I32.\CONST~s` from the stack. | ||
|
||
13. Pop the value :math:`\I32.\CONST~t` from the stack. |
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: the spec convention is to use t
for value types, and i,j,k,m,n for integers, at least if they are single-letter. So either use those or somewhat longer names.
document/core/exec/instructions.rst
Outdated
|
||
a. Trap. | ||
|
||
18. Let :math:`y` be the byte sequence :math:`\X{mem}.\DSIINIT[s \slice n]`. |
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: Use b^\ast
instead of y
.
document/core/exec/instructions.rst
Outdated
\end{array} | ||
\\[1ex] | ||
\begin{array}{lcl@{\qquad}l} | ||
S; F; (\I32.\CONST~n)~(\I32.\CONST~t)~(\I32.\CONST~s)~(\MEMORYINIT~x) &\stepto& S; F; \TRAP |
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.
This needs an "otherwise" as a side conditione, else it would always be allowed non-deterministically.
document/core/exec/modules.rst
Outdated
|
||
1. Let :math:`e` be the :ref:`element segment <syntax-elem>` to allocate. | ||
|
||
2. If :math:`e` is of the form :math:`\{ \EINIT~x^\ast \}`, then: |
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.
What does this algorithm return otherwise? Should it be epsilon, i.e., the algo returns an optional address?
document/core/exec/modules.rst
Outdated
.. math:: | ||
\begin{array}{rlll@{\qquad}l} | ||
\allocelem(S, e, \moduleinst) &=& S', \elemaddr & (\iff e = \{ \EINIT~x^\ast \}) \\[1ex] | ||
\allocelem(S, e, \moduleinst) &=& S & (\otherwise) \\[1ex] |
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.
This only returns one thing, not a pair, which seems incoherent. I think it should be epsilon.
document/core/exec/modules.rst
Outdated
|
||
1. Let :math:`d` be the :ref:`data segment <syntax-data>` to allocate. | ||
|
||
2. If :math:`d` is of the form :math:`\{ \DINIT~b^\ast \}`, then: |
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.
6. Let :math:`\funcaddr^\ast` be the the concatenation of the :ref:`function addresses <syntax-funcaddr>` :math:`\funcaddr_i` in index order. | ||
6. For each :ref:`element segment <syntax-elem>` :math:`\elem_i` in :math:`\module.\MELEM`, do: | ||
|
||
a. Let :math:`\elemaddr_i` be the :ref:`element address <syntax-elemaddr>` resulting from :ref:`allocating <alloc-elem>` :math:`\elem_i` for the :ref:`\module instance <syntax-moduleinst>` :math:`\moduleinst` defined below. |
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.
Here you assume that the vector of elemaddr_i corresponds to elem_i, which is not the case because you only get addresses for passive elements. So I think what the module instance needs to store is not am elemaddr^* but an (elemaddr^?)^*. Otherwise all the indexing will be off, too.
document/core/exec/runtime.rst
Outdated
@@ -158,6 +170,8 @@ and collects runtime representations of all entities that are imported, defined, | |||
\MITABLES & \tableaddr^\ast, \\ | |||
\MIMEMS & \memaddr^\ast, \\ | |||
\MIGLOBALS & \globaladdr^\ast, \\ | |||
\MIELEMS & \elemaddr^\ast, \\ |
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.
As mentioned above, I think these need to be optional addresses to represent indices of active segments correctly.
document/core/exec/runtime.rst
Outdated
.. math:: | ||
\begin{array}{llll} | ||
\production{(element instance)} & \eleminst &::=& | ||
\{ \ESIINIT~\vec(\funcaddr) \} \\ |
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: why not EIINIT?
document/core/exec/runtime.rst
Outdated
.. math:: | ||
\begin{array}{llll} | ||
\production{(data instance)} & \datainst &::=& | ||
\{ \DSIINIT~\vec(\byte) \} \\ |
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 Q here.
I think it's possible, and I started to do this, but it's pretty ugly unless we go with option 2 here. Here's what I was thinking:
Without option 2, though, I guess you'd need to conditionally update the state only if none of the instructions trap. I started to write this, but wasn't quite sure how to define it, and thought we may need to change it for the threads proposal anyway. |
Yes, I like that makes. Look slike the preferable route. For option 1, wouldn't the only difference be that we keep the bounds checks as pre-checks in the instantiation itself? |
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.
Sorry, dropped the ball on this review.
document/core/exec/instructions.rst
Outdated
\end{array} | ||
\\ \qquad | ||
\begin{array}[j]{@{}r@{~}l@{}} | ||
(\iff & F.\AMODULE.\MIMEMS[0] = ma \\ |
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.
It seems like ma isn't needed (similarly in prose).
document/core/exec/instructions.rst
Outdated
~\\[-1ex] | ||
\begin{array}{l} | ||
\begin{array}{lcl@{\qquad}l} | ||
S; F; (\I32.\CONST~n)~(\I32.\CONST~j)~(\I32.\CONST~i)~(\MEMORYCOPY~x) &\stepto& S; F; (\INITDATA~ma~j~b^\ast) |
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.
The index x seems unused. I think in the future we will want two indices, for source and target memory, but for now we can probably drop both from the AST.
(\iff & F.\AMODULE.\MIMEMS[0] = ma \\ | ||
\wedge & (i + n \leq |S.\SMEMS[ma].\MIDATA|) \\ | ||
\wedge & (j + n \leq |S.\SMEMS[ma].\MIDATA|) \\ | ||
\wedge & b^\ast = S.\SMEMS[ma].\MIDATA[i \slice n]) \\ |
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.
Should we add a note that this semantics implies that overlapping ranges are handled correctly?
Also, this semantics essentially prescribes buffering of the copied memory before writing begins. That will be observable once we have threads and concurrent mutation. I think it's fine to leave this for now, but I suspect we will need to tweak it. Perhaps adda TODO note?
document/core/exec/instructions.rst
Outdated
|
||
8. Let :math:`ea` be the :ref:`element segment address <syntax-elemaddr>` :math:`F.\AMODULE.\MIELEMS[x]`. | ||
|
||
9. If :math:`S.\SELEM[ea]` does not exist, then: |
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: better say "is unset" or something like that (ea exists in the list, but its value is epsilon).
document/core/exec/instructions.rst
Outdated
\end{array} | ||
\\ \qquad | ||
\begin{array}[j]{@{}r@{~}l@{}} | ||
(\iff & F.\AMODULE.\MITABLES[0] = ta \\ |
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 as above: ta is unused.
document/core/exec/instructions.rst
Outdated
~\\[-1ex] | ||
\begin{array}{l} | ||
\begin{array}{lcl@{\qquad}l} | ||
S; F; (\I32.\CONST~n)~(\I32.\CONST~j)~(\I32.\CONST~i)~(\TABLECOPY~x) &\stepto& S; F; (\INITELEM~ta~j~y^\ast) |
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 as above: x unused.
document/core/exec/instructions.rst
Outdated
.. index:: table, table instance, table address, initialize table | ||
.. _initelem: | ||
|
||
:math:`\INITELEM~\tableaddr~o~x^\ast` |
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.
Let's rename x* to a* here and below, since they're not function indices but function addresses.
document/core/exec/modules.rst
Outdated
|
||
b. For each :ref:`function index <syntax-funcidx>` :math:`x_i` in :math:`e.\EINIT`, do: | ||
a. Let :math:`\funcaddr_i` be the :ref:`function address <syntax-funcaddr>` :math:`\moduleinst.\MIFUNCS[x_i]`. |
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.
First assert that this exists.
\begin{array}[j]{@{}r@{~}l@{}} | ||
(\iff & F.\AMODULE.\MIMEMS[0] = ma \\ | ||
\wedge & F.\AMODULE.\MIDATAS[x] = da \\ | ||
\wedge & S' = S \with \SDATA[da] = \epsilon) \\ |
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.
Setting a SDATA slot to epsilon must allow for it in the definition of store, i.e., change the syntax to (\datainst^?)^* (same for element segments).
It's OK, I still haven't fixed the instantiation to use |
This is pretty out-of-date at this point, closing for now. |
This is copied and modified from this closed PR: #19
This is copied and modified from this closed PR: #19
Started to work on spec for execution. Still more work to do, but I thought it would be useful to get some early feedback.