-
Notifications
You must be signed in to change notification settings - Fork 36
Conversation
@rossberg I can probably take a look at this while I'm in the air on Sunday. |
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'd be OK merging this PR into exnref-1b
before review and then reviewing the merged branch as a PR on the main branch. (AFAICT there is no PR to merge exnref-1b
into the main branch yet, and I already noticed a typo there).
There is: #285 ;) Happy to do it in either order. |
Thinking again, it probably makes sense not to block #285, so people see an up-to-date explainer and tests etc. I rather hold off on merging this one until @conrad-watt or somebody else did a proper review. |
Apologies for the delay, on this now. |
EDIT: I originally complained about a build warning here, but I was building on the wrong branch |
Instruction(r'\DROP', r'\hex{1A}', r'[t] \to []', r'valid-drop', r'exec-drop'), | ||
Instruction(r'\SELECT', r'\hex{1B}', r'[t~t~\I32] \to [t]', r'valid-select', r'exec-select'), | ||
Instruction(r'\SELECT~t', r'\hex{1C}', r'[t~t~\I32] \to [t]', r'valid-select', r'exec-select'), | ||
Instruction(None, r'\hex{1D}'), | ||
Instruction(None, r'\hex{1E}'), | ||
Instruction(None, r'\hex{1F}'), | ||
Instruction(r'\TRYTABLE~\X{bt}', r'\hex{1F}', r'[t_1^\ast] \to [t_2^\ast]', r'valid-try_table', r'exec-try_table'), |
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.
Are we no longer listing the catch
opcodes separately?
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.
They are no longer in the space of instruction opcodes, but just flag bytes in a vector.
@@ -59,16 +59,12 @@ Results | |||
} | |||
|
|||
|
|||
:ref:`Results <syntax-result>` :math:`\val^\ast~(\THROWadm~\tagaddr)` | |||
:ref:`Results <syntax-result>` :math:`\XT[(\REFEXNADDR~a)~\THROWREF]` |
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 feels kind of nasty to classify an instruction inside a context as a "result". Can you walk me through why you don't want to use something like \THROWadm
any 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.
The introduction of the context is actually unrelated to replacing THROWadm, its absence was simply wrong before. We need it here since there is no reduction rule that can eliminate it on the outermost level (outside a function). We can't simply add a rule XT[exn] ⇝ exn, since that would introduce overlapping rules.
I see two ways to fix this in the future: (1) switching to a bubbling semantics for exceptions (avoiding overlap), or (2) introducing a separate global reduction relation like we have with threads. The former should go along with changes to other parts of the semantics to stay coherent, the latter I'd rather do after we have merged with threads. Hence I don't want to do either in this PR.
@@ -1280,6 +1290,7 @@ | |||
.. |vdashglobalinst| mathdef:: \xref{appendix/properties}{valid-globalinst}{\vdash} |
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.
line 1283 (above): .. |vdashval| mathdef:: \xref{appendix/properties}{valid-val}{\vdash}
should be exec/modules
rather than appendix/properties
(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.
Indeed, fixed.
\iff & F.\AMODULE.\MITAGS[x] = a \\ | ||
\land & S.\STAGS[a].\TAGITYPE = [t^n] \toF [] \\ | ||
\land & \X{exn} = \{ \EITAG~a, \EIFIELDS~\val^n \} \\ | ||
\land & S' = S \with \SEXNS = S.\SEXNS~\X{exn} ) \\ |
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 it necessary to allocate exceptions in the store? Since they're immutable, can the abstract semantics not just treat them as ordinary values?
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.
References need internal identity, because it can become observable in the host, e.g. in JS. This is analogous to functions, which are likewise immutable.
(The lack of exn identity was one of the problems we faced with the JS API spec in the old proposal.)
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 for the review!
Instruction(r'\DROP', r'\hex{1A}', r'[t] \to []', r'valid-drop', r'exec-drop'), | ||
Instruction(r'\SELECT', r'\hex{1B}', r'[t~t~\I32] \to [t]', r'valid-select', r'exec-select'), | ||
Instruction(r'\SELECT~t', r'\hex{1C}', r'[t~t~\I32] \to [t]', r'valid-select', r'exec-select'), | ||
Instruction(None, r'\hex{1D}'), | ||
Instruction(None, r'\hex{1E}'), | ||
Instruction(None, r'\hex{1F}'), | ||
Instruction(r'\TRYTABLE~\X{bt}', r'\hex{1F}', r'[t_1^\ast] \to [t_2^\ast]', r'valid-try_table', r'exec-try_table'), |
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.
They are no longer in the space of instruction opcodes, but just flag bytes in a vector.
@@ -59,16 +59,12 @@ Results | |||
} | |||
|
|||
|
|||
:ref:`Results <syntax-result>` :math:`\val^\ast~(\THROWadm~\tagaddr)` | |||
:ref:`Results <syntax-result>` :math:`\XT[(\REFEXNADDR~a)~\THROWREF]` |
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 introduction of the context is actually unrelated to replacing THROWadm, its absence was simply wrong before. We need it here since there is no reduction rule that can eliminate it on the outermost level (outside a function). We can't simply add a rule XT[exn] ⇝ exn, since that would introduce overlapping rules.
I see two ways to fix this in the future: (1) switching to a bubbling semantics for exceptions (avoiding overlap), or (2) introducing a separate global reduction relation like we have with threads. The former should go along with changes to other parts of the semantics to stay coherent, the latter I'd rather do after we have merged with threads. Hence I don't want to do either in this PR.
\iff & F.\AMODULE.\MITAGS[x] = a \\ | ||
\land & S.\STAGS[a].\TAGITYPE = [t^n] \toF [] \\ | ||
\land & \X{exn} = \{ \EITAG~a, \EIFIELDS~\val^n \} \\ | ||
\land & S' = S \with \SEXNS = S.\SEXNS~\X{exn} ) \\ |
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.
References need internal identity, because it can become observable in the host, e.g. in JS. This is analogous to functions, which are likewise immutable.
(The lack of exn identity was one of the problems we faced with the JS API spec in the old proposal.)
@@ -1280,6 +1290,7 @@ | |||
.. |vdashglobalinst| mathdef:: \xref{appendix/properties}{valid-globalinst}{\vdash} |
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.
Indeed, fixed.
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 the longer term, I'd like to revisit the way the exception "value" is formalised, but beyond that things seem fine.
* Reintroduce exnref * Fix type_of * Simplify parser * Implement 1b * Change opcodes for catch/catch_all to avoid conflict * Put catch clauses first * Remove obsolete Delegating cases * Change exn type opcode to -0x17 * Switch to B' variant * [interpreter] Add boilerplate for ref.exn result patterns * [ci] Deactivate node run, since it can't handle try_table yet * Try -> TryTable in AST * [spec] Update spec for option B' (#283) * Deactivate Bikeshed * [spec/tests] Specification of legacy exceptions + tests (#284) * [legacy] Create specification doc for legacy exception handling * [test] Create infra for legacy tests
This updates the spec document to option B'.
@ioannad, would you be interested in reviewing this change? I know it is rather large, but it's deleting more than it's adding. :)