-
Notifications
You must be signed in to change notification settings - Fork 36
Add prose for reduction rules involving thrown exceptions, #226
Add prose for reduction rules involving thrown exceptions, #226
Conversation
i.e., explaining some execution steps for `THROWadm`, `CATCHadm`, `DELEGATEadm`. The parts of this commit that involve `DELEGATEadm` may change once PR WebAssembly#220 is settled. Nonetheless, in this commit the typing rule and the reduction rule for `DELEGATEadm` are both tentatively changed to match the current [formal overview](https://github.com/WebAssembly/exception-handling/blob/main/proposals/exception-handling/Exceptions-formal-overview.md).
Gentle ping :) |
Co-authored-by: Heejin Ahn <[email protected]>
document/core/exec/instructions.rst
Outdated
\CATCHadm~\XT[\val^n~(\THROWadm~a)]~\END &\stepto& | ||
\XT[\val^n~(\THROWadm~a)] \\ | ||
\CATCHadm\{a_1^?~\instr^\ast\}\{a'^?~\instr'^\ast\}^\ast~\XT[\val^n~(\THROWadm~a)]~\END &\stepto& | ||
\CATCHadm\{a'^?~\instr'^\ast\}^\ast~\XT[\val^n~(\THROWadm~a)]~\END \\ |
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 don't think this formulation works, since you don't know what n
is without looking up a's arity in the store in every single rule. You could do that, but the previous formulation instead keeps XT around in the latter rule so that extracting the right number of values from it can be delayed until the handler is found (where you have to look it up again).
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, thank you! switching to the previous throw contexts and adding your commend as explainer.
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.
@rossberg
I'm not sure any more what this review comment means, in particular which formulation it refers to. I think I misunderstood when I thought it refers to previous formulation of throw contexts, because IIUC any non-trivial throw context would still have the exception payload values val^n
inside the square brackets with the throw (\XT[val^n (throw a)]
).
Which previous formulation are you referring to?
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 believe you can just use \XT[(throw a)]
, since the context will cover any val* anyway.
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 this case, I'm not sure how to write the reduction for a CATCHadm
with an empty list of catch clauses.
\CATCHadm~\XT[\THROWadm~a]~\END & \stepto & ???
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.
Nevermind my last comment, I think I got it right with the latest commits.
document/core/exec/instructions.rst
Outdated
\end{array} | ||
|
||
.. note:: | ||
Note that the reduction step resulting in a |CAUGHTadm| instruction is the only one that does not preserve the throw context. | ||
While a |THROWadm| propagates through the stack, the throw context |XT| is collected |
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 does not explain why, but see above.
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.
@rossberg this comment is now removed, but I'm still not sure why the rules keep the throw context, so I added a todo::
to add an explanation why or what's the meaning of apparently preserving the throw context in some of the rules.
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.
Thank you for the explanation via email, I included it as a note.
document/core/exec/instructions.rst
Outdated
|
||
5. While the stack is not empty and the top of the stack is not an :ref:`exception handler <syntax-handler>`, do: | ||
|
||
a. Pop the top element from the stack, prepending it to the :ref:`throw context <syntax-ctxt-throw>` of the exception: :math:`\XT[\val^n~(\THROWadm~a)]`. |
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 spec so far carefully avoids to use the notion of contexts in the prose. I think you can reformulate this in a way more similar to branches, where there is no need to mention XT.
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.
Removed everything after ", prepending".
document/core/exec/instructions.rst
Outdated
i. Push :math:`\CAUGHTadm\{a~\val^n\}` onto the stack. | ||
|
||
ii. Jump to the start of the instruction sequence :math:`\instr^\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.
I believe this ought to be formulated in terms of a separate step of "Entering a catch clause", mirroring the exit you have already introduced. (Same below)
document/core/exec/instructions.rst
Outdated
|
||
a. While the instruction on the top of the stack is not a label, do: | ||
|
||
i. Pop the instruction from the stack, without pushing it to |XT|. |
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 informal stack does not contain "instructions", only value, labels, frames, and now handlers and catch clauses. We probably need to find a common name for those ("control frames"?).
Also, the formulation of br
has to change, since it no longer just skips over values, it also pops EH-related control frames.
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.
Rephrased to a. Pop the top element from the stack.
Should I make a new PR rephrasing document/core/exec/runtime.rst
to add control frames anyway?
Just for the record, the formulation of br
is now changed by merged PR #233.
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 no longer think we need to define "control frame". Just pop the "top element", as you wrote above.
document/core/exec/instructions.rst
Outdated
|
||
c. Pop the label from the stack. | ||
|
||
16. Push the throw context that we collected so far :math:`\XT[\val^n~(\THROWadm~a)]` onto 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.
Yeah, I'm not sure what that would mean. I think we need a wording without XT.
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'm not sure how to search the stack for a handler without popping stuff from it. So I don't know how to describe keeping the throw context in these rules.
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.
Why do you need to keep the context in the prose? Above, it has already popped the operands val^n
, and that's all you need to keep AFAICS.
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 we need it to return the entire stack if the exception is not handled?
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.
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.
See my email: you need to allow (uncaught) exceptions as stack elements and push it as that.
Co-authored-by: Andreas Rossberg <[email protected]>
CATCHadm, DELEGATEadm, and CAUGHTadm in the prose of `exec-throwadm`. Split CAUGHTadm steps into a new section `exec-caughtadm-enter`, as suggested in review. Used the suggested name "catch clause" for `CAUGHTadm`, as well as for the clauses of a catching exception handler, also in the runtime definition of handlers.
…HTadm step, all in the prose of `exec-throwadm`. - All mentions of throw contexts are removed from the prose, which leaves only `val^n (throw a)` in case of an unhandled exception. - The loop of delegating handlers popped the last label too which is wrong, the throw should happen inside the `l+1`th surrounding block. - Forgotten to rephrase the steps pushing CAUGHTadm when section `exec-caughtadm-enter` was added in a previous commit.
document/core/exec/instructions.rst
Outdated
|
||
8. Else there is an :ref:`exception handler <syntax-handler>` :math:`H` on the top of the stack. | ||
a. Assert: there is an :ref:`exception handler <syntax-handler>` :math:`H` on the top of 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.
It seems off that this is under else
but what follows is not.
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 am changing this to
8. Else assert: there is an :ref:`exception handler <syntax-handler>` :math:`H` on the top of the stack.
This way I hope it's clear that everything that follows is under the else, given that the other branch of the "If" (7.a
) returns.
Or would you prefer that everything that follows is nested under 8.
?
document/core/exec/instructions.rst
Outdated
|
||
9. Pop the exception handler :math:`H` from the stack. | ||
|
||
10. Assert: :math:`H` is either of the form :math:`\CATCHadm\{a^?~\instr^\ast\}^k` or :math:`\DELEGATEadm\{l\}.` | ||
10. Assert: :math:`H` is either of the a catching or a delegating :ref:`exception handler <syntax-handler>`. |
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.
Is this assertion needed? That appears to be true by definition of exception handler.
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.
Removing this line.
document/core/exec/instructions.rst
Outdated
|
||
e. Else, repeat step 11 for :math:`H = \CATCHadm\{a'^?~\instr'^\ast\}^\ast`. | ||
ii. Repeat step 11 for :math:`H'`. |
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 turn this use of goto into a structured while loop. ;)
document/core/exec/instructions.rst
Outdated
|
||
13. Assert: due to :ref:`validation <valid-delegate-admin>`, the stack contains at least :math:`l+1` labels. | ||
a. Assert: :math:`H` is a delegating exception handler. |
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 nesting oddity here.
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.
Since the other "if" branch does not end with a clear "return", here I am putting the rest of the steps nested under the "Else".
document/core/exec/instructions.rst
Outdated
|
||
a. While the instruction on the top of the stack is not a label, do: | ||
15. Let :math:`L` be the :math:`l`-th label appearing on the stack, starting from the top and counting from zero. |
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 is this step? L
doesn't even seem to be used.
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.
Removing.
document/core/exec/instructions.rst
Outdated
|
||
d. Else if :math:`a_1^? = a`, then: | ||
|
||
i. Push :math:`\CAUGHTadm\{a~\val^n\}` onto the stack. | ||
i. :ref:`Enter <exec-caughtadm-enter>` :math:`\val^n~\instr^\ast` with catch clause holding the caught exception :math:`\{a~\val^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.
You defined how to enter it, but not how it is exited.
Perhaps we don't need these auxiliary definitions for something as simple as this. Just push the value, enter the instruction sequence, pop the exception.
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 I rename section _exec-caughtadm
to _exec-caughtadm-exit
or remove both sections _exec-caughtadm-enter
and _exec-caughtadm
, and merge their steps into this prose directly?
document/core/exec/instructions.rst
Outdated
|
||
.. _exec-caughtadm-enter: | ||
|
||
Entering :math:`\instr^\ast` with catch clause holding a caught exception :math:`\{a~\val^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.
See comment above.
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 sounds like the answer to my question above, is to remove both sections _exec-caughtadm-enter
and _exec-caughtadm
, and merge their steps into the exec-throwadm
prose directly?
document/core/exec/instructions.rst
Outdated
|
||
13. Assert: due to :ref:`validation <valid-delegate-admin>`, the stack contains at least :math:`l+1` labels. | ||
|
||
14. Let :math:`L` be the :math:`l`-th label appearing on the stack, starting from the top and counting from zero. |
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 would be much simpler to express after the loop below.
Co-authored-by: Andreas Rossberg <[email protected]>
In particular: - Switched to handler ::= (tagaddr? instr*)* | labelidx exn ::= tagaddr val* instr ::= … | handler_n{handler} instr* end | caught_n{exn} instr* end removing DELEGATEadm - Changed prose to just push and pop handlers and exceptions, in the runtime, in the execution steps of the instructions, in the formal overview, and partly in appendix/properties. Not done: - Did not change the notation and prose for CAUGHTadm in appendix/properties.rst, this will be done in PR WebAssembly#244.
- Also added the missing _n to CAUGHTadm. The subscript is introduced by the changes in PR WebAssembly#226
as it doesn't make sense on its own in WebAssembly#246.
* Fixes the typing rule of CAUGHTadm as suggested in: #229 (comment) * Applying suggestion from PR #241 https://github.com/WebAssembly/exception-handling/pull/241/files/201ff276be6c458e7a819662eb2862bd475f2fcd..f97c9d108a999c3c3fd30f25554a113822bed1b8#r1030097793 * Applied suggestions from code review, fixed notation, also adding the missing _n to CAUGHTadm. The subscript is introduced by the changes in PR #226
Uses the "current" version of exception results, as it will be changed by WebAssembly#226.
Co-authored-by: Andreas Rossberg <[email protected]>
…to spec-prose-exec-throwadm-catchadm-delegateadm
Co-authored-by: Andreas Rossberg <[email protected]>
i.e., prose for some execution steps for
THROWadm
,CATCHadm
,DELEGATEadm
.The parts of this commit that involve
DELEGATEadm
may change once PR #220 is settled.Nonetheless, in this commit the typing rule and the reduction rule for
DELEGATEadm
are both tentatively changed to match the current formal overview.