-
Notifications
You must be signed in to change notification settings - Fork 36
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
formal spec overview for the 3rd exception handling proposal #143
Conversation
Thanks for writing up this updated semantics, @ioannad! This PR will be a useful reference for ongoing discussions. |
Don't formal spec docs live in document/ directory? Do you plan to make a separate "real" formal spec there, and this is something else for discussions? |
I like having this document available for discussion because it is much easier to read and much more focused on EH issues than the raw formal spec documents would be. |
I put this together with the "English spec overview" because I thought of this as also an overview (a formal version), because it's written in Markdown, because it contains all the exception handling (EH) additions (except opcodes) and only the EH additions, and because its purpose is just for the discussions, until we reach consensus on its contents. When there is consensus on these contents, then I will update the "real" formal spec, which is more involved, with formatting and prose. I could move it somewhere in the Or I could instead update WDYT? |
Oh, sorry, now I understand, and I think having an introduction for the formal spec here is good. |
|
||
To verify that a `try...delegate l` instruction refers to a label introduced by a try block (I'll call this a try-label), introduce a type attribute to labels in the validation context, which type attribute is set to `try` when the label is a try-label. The original notation `label [t*]` will then be a shortcut for `label {result [t*], type ε}`. | ||
|
||
Moreover, to verify that the `rethrow l` instruction refers to a label introduced by a catch block (call this a catch-label), we allow this type attribute of labels in the validation context to be set to `catch` when the label is a catch label. This addition is reflected in the execution rules, by the administrative instruction `caught` which introduces a label around the instructions of the catching try-block's catch instructions. |
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.
So the intention here is, as in the case of delegate
, rethrow
can target any label including labels for blocks, loops, and try-delegates, but they are just validation failures, right?
The concept of catch label is a little new. Can we consider it in the same as try label? Then we have this problem:
try $label0
rethrow $label0 ;; cannot be done, because it's not within catch below
catch
end
But delegate
has another problem anyway if it we say it takes a try block:
try $label0
catch
try
...
delegate $label0 ;; cannot be done, because $label0's catch is not below but above here
end
delegate
can be considered to target a catch label too, but in this case it can be considered to target try
:
try $label1
try $label0
try
...
delegate $label0
delegate $label1
end
So my question is
- For
rethrow
: If we introduce a concept of catch label, should we also mention that that this works in the same way as its correspondingtry
block for other branch-like constructs? - For
delegate
: The concept of try block may not work in something like my second example, but as we can see in my third example, it is different from catch label. Should we introduce a new concept?
I might be mistaken on something, so please let me know.
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.
Gentle ping :)
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 I'm late replying @aheejin!
The concept of catch label is a little new. Can we consider it in the same as try label? Then we have this problem:
try $label0 rethrow $label0 ;; cannot be done, because it's not within catch below catch end
I used catch labels just as a helper to make sure that the rethrow
is inside a catch block, not as something one can use in the text format. In your example, rethrow
is not inside a catch block, so that example would not validate.
So, to your question 1, I did not intend a catch
label to be the same as a try
label.
I'm not sure I understand the problem with delegate
, I'll try to look at this again later today or tomorrow.
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.
@aheejin I see now what you mean, yes, you are right.
In the second example, the label from the catch block would interfere with delegate
, when it shouldn't. Also rethrow
should not interfere with other labels, it should just count the stack of nested catch blocks it is in - to rethrow an exception, an exception must be caught, so we can only rethrow n
the exception caught by the n
th surrounding catch block.
I think to solve this, the catch-labels should be removed, and a separate concept of a caught
-stack should be added to the runtime, described by a parallel to blocks:
C^0 ::= val^m [_] instr
C^{n+1} ::= caught{exnaddr val^k} C^n end
Also a new stack caught
to validation contexts, which gets an element "caught
" prepended whenever we catch an exception by a catch
block.
About the 3rd example, did you omit a catch in the outer try block on purpose, meaning that try
s without catches could be allowed, as @tlively suggested in #131 ? In that case, in your 3rd example, if we replace ...
by throw $exn
then the example would reduce to throw $exn
. I think it would be a nice simplification to the syntax to allow catchless try blocks. They are equivalent to try ... catch_all rethrow 0 end
AFAICT, so no new behaviour would be introduced with this simplification.
If we additionally allow delegate
to target any label, as @RossTate suggested, then we can have the same effect also without a try block, as he noted, and we wouldn't have to make any changes to labels. I admit I am not so fond of adding this auxiliary information to labels, I just thought I had to.
To clarify I'll commit the changes I'm suggesting later today, in three separate patches:
- add
caught
-stack to the runtime and to validation contexts and remove catch-labels (which actually should have been called "caught" labels but anyway) - allow
try
s withoutcatch
es - remove auxiliary
type
of labels and allowdelegate
to reference any label
WDYT?
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 just pushed the promised commits. The first one's commit message contains the question "Should the administrative instruction caught
be removed from the right hand side of rethrow's execution step?", because I ran out of time and I won't be able to think about it until the end of next week. Does anyone have any thoughts on this?
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.
@aheejin: I just want to clarify that the commits I pushed are just so that it's clear what I meant with my comment above. These commits are independent from each other, so one could be accepted and the next one not without trouble, 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.
Sorry for the delayed reply. The catch-less try was a typo, sorry. I didn't mean we have a catchless try. We can maybe consider it, but I personally have a preference against it, as I said in #131. What I tried to convey in that third example was that we can't make delegate
target outer catch
, because delegte
can also target an outer try-delegate
.
|
||
## Validation (Typing) | ||
|
||
To verify that a `try...delegate l` instruction refers to a label introduced by a try block (I'll call this a try-label), introduce a type attribute to labels in the validation context, which type attribute is set to `try` when the label is a try-label. The original notation `label [t*]` will then be a shortcut for `label {result [t*], type ε}`. |
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 is no need for delegate
to specifically reference a try
block. That is, delegate
does not mean to use a particular handler but rather to ignore the handlers in between. (I suspect delegate
will likely be implemented either by subtracting code ranges or by popping handlers depending on the backend.) Such a relaxation is also helpful, as one can skip over all handlers, i.e. delegate to the caller, without having to wrap the function body with a try
and an empty handler.
C, label [t2*], caught all |- instr_2* : [] -> [t2*] | ||
---------------------------------------------------- | ||
try bt instr_1* unwind instr_2* : bt |
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.
C, label [t2*], caught all |- instr_2* : [] -> [t2*] | |
---------------------------------------------------- | |
try bt instr_1* unwind instr_2* : bt | |
C, label [t2*] |- instr_2* : [] -> [] | |
---------------------------------------------------- | |
try bt instr_1* unwind instr_2* end : bt |
This was discussed in #142, but just to reiterate for the sake of having this suggestion be self contained: A purpose of unwind
is to specify unwinders in a way that works well with non-exceptional non-local control constructs. To that end rethrow
should not work within unwind
(which was the case in the earlier formalization), as it may not make sense to rethrow
other non-local control constructs from within a different dynamic scope. Instead, unwind
automatically resumes unwinding at the end
, and so instr_2*
does not need to output [t2*]
.
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.
Issue #142 is still open, and (unfortunately) it does not seem to me that there is general agreement on unwind
. About the return values, in the closed issue #129, it seems it was decided that unwind blocks should indeed return values.
About rethrow
in unwind
blocks or not, I can't find that discussion, can you please link to it?
By the way, the earlier formalisation was wrong, as it did not reflect the spec that @aheejin presented and that we voted on.
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 catching the missing end
typo!
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.
@RossTate, about the unwind
block's instructions returning values or not: now I see that I was perhaps too quick to close #129, since you don't seem to agree with @aheejin's proposal there. Since unwind
ends with a rethrow 0
, any return values seem indeed unnecessary to me as well. @aheejin what do you think? The try block can still have any blocktype, it's just that the unwind block's instr_2
shouldn't return any values, since they will be discarded by the rethrow
in the end anyway.
Having said that, either way it doesn't seem to make much of a difference. Even if instr_2*
returns any values, even not matching t2*
, even then it wouldn't really make a difference, 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.
@RossTate about rethrow
inside an unwind
block's instr_2*
, I can't see much difference from having a throw
inside instr_2*
. Looking around the issues, I only saw arguments saying that a rethrow
would end the unwinding process, but so would a br
or throw
or return
. I didn't manage to find a discussion wrt rethrow
of other non-local control constructs. Please help me out find the relevant comments/discussions, I'm a bit lost in the several issues containing mixed information.
Having said that, please note that the rethrow 0
resulting from the current unwind
, is creating a new exception, so it's throwing within a different dynamic scope, IIUC. I do understand that unwind
should probably be laid out more carefully than what we do here, and this is exactly why I suggested that we postpone it to an extension of the MVP, to avoid making mistakes we can't take back anymore, wrt backwards compatibility. I don't think that people should expect the MVP to be WebAssembly's "final word on exceptions" (or non-local control flow in general - as @phoe suggested), but the "first word on exceptions" (or non-local control flow). :)
But, as @aheejin says, I'm also ok to keep unwind
, if it helps us get the spec settled soon so we can continue our implementations. (Btw, @takikawa and I already started landing first patches for try-catch
and throw
in SpiderMonkey.)
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.
If we can place br
that returns a value within unwind
, I'm not sure why the type of unwind
block has to be [] -> []
?
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.
br
and the block's end
are control paths with different targets in the case of unwind, so they have different type constraints (that's also the case with loop
btw). An unwind block will drop any remaining value on the floor at end
, so it's pointless to require any values there -- all that would do is force producers to push dummy values for no reason.
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 I'm late to the party here, but I would expect all three of @takikawa's examples to validate because of implicit unreachable polymorphic behavior due to the implicit rethrow
at the end of the unwind
clause, just as they would if the rethrow
were made explicit as a separate instruction at the end of the block.
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.
@tlively, that would be possible as well, but is useless in this case, since unlike branches or throws, an end
cannot occur in the middle of a larger "expression". Stack polymorphism is useful for examples like
(... larger exp (i32.div_u (x) (br_if $bailout (i32.eqz (y))) (y)) ...)
or something like that. That cannot be constructed with a delimiter like end
.
Not saying that we can't do it, but it would be a (minor) relaxation without practical value.
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 agree there’s no practical value. Not making that relaxation would be fine with me.
|
||
To verify that a `try...delegate l` instruction refers to a label introduced by a try block (I'll call this a try-label), introduce a type attribute to labels in the validation context, which type attribute is set to `try` when the label is a try-label. The original notation `label [t*]` will then be a shortcut for `label {result [t*], type ε}`. | ||
|
||
Moreover, to verify that the `rethrow l` instruction refers to a label introduced by a catch block (call this a catch-label), we allow this type attribute of labels in the validation context to be set to `catch` when the label is a catch label. This addition is reflected in the execution rules, by the administrative instruction `caught` which introduces a label around the instructions of the catching try-block's catch instructions. |
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 for the delayed reply. The catch-less try was a typo, sorry. I didn't mean we have a catchless try. We can maybe consider it, but I personally have a preference against it, as I said in #131. What I tried to convey in that third example was that we can't make delegate
target outer catch
, because delegte
can also target an outer try-delegate
.
bt = [t1*] -> [t2*] | ||
C, label [t2*] |- instr* : [t1*] -> [t2*] | ||
C_label(l) =/= ε | ||
----------------------------------------- | ||
try bt instr* delegate l : bt |
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 seems we are using a regular labels for delegate
. Can we handle this case then?
try $label0
catch
try
...
delegate $label0 ;; cannot be done, because $label0's catch is not below but above here
end
Is this in line with the @RossTate's suggestion? His suggestion looks like we don't need to use labels at all. Am I interpreting it correctly, @RossTate?
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.
That's in line with my suggestion. The number just indicates how many syntactic enclosures to skip out of. A convention in the text format could be that you can label a syntactic enclosure and reference that label in the delegate
rather than have to compute the number of syntactic enclosures between.
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.
@ioannad What do you think? If we are on the same page on this I'll update the explainer to match this.
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.
Sorry what I meant was not catch label but try label. In my example the label $label0
is attached to try
. So it should be a valid try label, but the code itself should be invalid. I'm not sure how you can describe that code using try label, and that's the question I asked.
Did your last commit contain any changes related to this? I don't see any, so...
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.
@aheejin, apologies for the late response. Let me start by saying that I do agree that your example should be impossible. Thank you for clarifying, I see what the problem is, and I also see that we actually do need caught
to have a label after all, in order to accommodate situations such as:
try bt
throw $e
catch $e
....
br 0 ;; ---
end ;; |
;; <------- break to here
Tricky situation, I'll have to think about how to express this formally.
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.
Just to check if we are in the same page, here are questions. I'll write my current state of understanding. Please let me know if it doesn't match with yours. try
block
try
throw $e
delegate N
end
catch
end In this code,
try
catch ;; outer
block
try
catch ;; inner
rethrow N
end
end
end In my understanding, in this code, Is my understanding for both problems correct? CC'ing other people who are working on the VM implementation to be sure. @thibaudmichaud @takikawa Edit: The |
I interpreted @RossTate's comment:
to mean that targeting a non-try block is valid and delegates to the next enclosing try-catch block. In your example this would mean that N=0 is the same as N=1. The implementation would be trivial to fix so I am not advocating for one interpretation over the other, I just want to make sure we are on the same page too. I thought N=2 would be a validation failure since it does not target a try-catch block. Delegating to the caller also makes sense but I would like to know if there is a motivation for it, since it requires some special case in the implementation. |
I believe one of the use cases is to delegate to the caller, so I had in mind that |
I also think either interpretation of @thibaudmichaud The functionality to rethrow to the caller is necessary. This instruction was basically added to solve unwinding mismatch problems after control flow linearization by placing |
I agree with all your (latest) interpretations of @aheejin's examples. So I agree that these examples should be as @aheejin expects, except that in the first example I tried to write down these and other example reductions to add to the formal-overview document, but I have still to figure out some subtleties with the labels and delegate, as I mentioned in this comment above. However, I am on holidays until the end of the year, so I'll come back with an updated formalism in early 2021. :) |
Yes, in my first example I also wrote
Happy holidays! |
Happy new year! |
Happy new year everyone! Thanks for the clarifications in this thread! I like the |
Delegating to the current control depth is valid and rethrows the exception to the caller. See WebAssembly/exception-handling#143. [email protected] CC=[email protected] Bug: v8:8091 Change-Id: I6f14663751736ec6de29eefebfccdf5eb9e955e2 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2617081 Commit-Queue: Thibaud Michaud <[email protected]> Reviewed-by: Clemens Backes <[email protected]> Cr-Commit-Position: refs/heads/master@{#71974}
Happy new year folks. I was on holiday so I missed the ping on this thread, but I agree with @backes about the |
This PR updates the support of exception handling to the latest proposal (that is compatible with future 2-phase exception handling) described in WebAssembly/exception-handling#137 and WebAssembly/exception-handling#143. * Adds back tagged `catch $e`, `catch_all`, and `rethrow N` from a previous version of wabt, but with updates to match the current spec (e.g., `catch_all` shares an opcode with `else`, `rethrow`'s depth indexes only catch blocks, etc). * Adds `unwind` and `delegate` instructions. * Removes `exnref` and `br_on_exn`. * Updates relevant tests. There are some details that could still change (e.g., maybe how `delegate`'s depth is validated), but I'd be happy to submit further PRs if the spec details change.
Hi all, apologies for the long delay, among other things I got caught up with starting to implement the proposal in IonMonkey (WIP). I just pushed a version of the formal spec that I think matches @aheejin's #137 and the current consensus. I also added a new file with examples from comments in issues and what these examples reduce to, to make sure everyone is happy with the spec. The first example I made up to include all new instructions, and it's the only one with a full reduction trace. I'd be happy to get any corrections, suggestions, reviews etc. |
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 great! Just a few minor oversights, I think. And some minor simplifications you could make.
bt = [t1*] -> [t2*] | ||
C, label {result [t2*], kind try} ⊢ instr* : [t1*] -> [t2*] | ||
C_label(l) =/= ε | ||
C_label(l).kind = try |
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, the first premise about C_label(l)
is redundant.
F; throw x ↪ F; throw a (iff F_exn(x) = a) | ||
|
||
caught_m{a val^n} B^{l+1}[rethrow l] end | ||
↪ caught_m{a val^n} B^{l+1}[val^n (throw 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.
Don't you need to extend B^k
with caught
for this to be correct?
I'm not sure why it's B^{l+1}
instead of B^l
, though.
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.
To be honest this has been bothering me lately: as it is now, even in the main spec, any instructions could be between two labels in a B^l
, or am I misunderstanding something?
Unless you mean that rethrow
's immediate l
should only count the catches, which is not the case anymore (I missed this at first as it's a bit hidden in a different issue).
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 used B^{l+1}
because B^0
does not contain / is not pointing to- a label, while rethrow 0
should point to a surrounding label.
Also note that, in the execution rules, caught_m
gets introduced with a label after it (the +1), to compensate for the evaporating try-label when catching the exception by a handler. Does this make sense?
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.
To be honest this has been bothering me lately: as it is now, even in the main spec, any instructions could be between two labels in a
B^l
, or am I misunderstanding something?
Not sure I follow. A branch can skip over any instruction. What would be an example of an odd context?
I used
B^{l+1}
becauseB^0
does not contain / is not pointing to- a label, whilerethrow 0
should point to a surrounding label.
Intuitively, the B^l
captures the l label binders that you skip over with a branch. So if l=0 then this context should indeed not contain any label. The target label is the one explicit in the rule, surrounding B.
However:
Also note that, in the execution rules,
caught_m
gets introduced with a label after it (the +1), to compensate for the evaporating try-label when catching the exception by a handler. Does this make sense?
Ah, yes, this explains why we need a +1 here: the catch actually is outside its associated label, so we actually need to skip over that as well to get to the catch.
It's a bit odd, though. I suspect that this use of labels creates an issue with the soundness proof. In particular, what is the administrative typing rule for catch_n? It can't bind the label, since that is bound by label_n. But label_n won't attach the catch kind, so any contained rethrow won't type-check anymore after reducing try to catch, and we lose preservation.
Maybe this can be hacked by changing the reduction rule for try-catch so that it binds the label outside the catch. Then the typing rule for catch_n could amend the label kind of the innermost label in its environment. (And we wouldn't have the +1.)
Something to think about. I'd encourage you to write out the typing rules for the administrative instructions, to check that it all hangs together.
(I guess the more baroque nature of the new design is showing here.)
↪ val^n (throw a) | ||
|
||
|
||
try bt instr* unwind instr'* end ↪ try bt instr* catch_all instr'* rethrow 0 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.
:)
F; throw x ↪ F; throw a (iff F_exn(x) = a) | ||
|
||
caught_m{a val^n} B^{l+1}[rethrow l] end | ||
↪ caught_m{a val^n} B^{l+1}[val^n (throw 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.
To be honest this has been bothering me lately: as it is now, even in the main spec, any instructions could be between two labels in a
B^l
, or am I misunderstanding something?
Not sure I follow. A branch can skip over any instruction. What would be an example of an odd context?
I used
B^{l+1}
becauseB^0
does not contain / is not pointing to- a label, whilerethrow 0
should point to a surrounding label.
Intuitively, the B^l
captures the l label binders that you skip over with a branch. So if l=0 then this context should indeed not contain any label. The target label is the one explicit in the rule, surrounding B.
However:
Also note that, in the execution rules,
caught_m
gets introduced with a label after it (the +1), to compensate for the evaporating try-label when catching the exception by a handler. Does this make sense?
Ah, yes, this explains why we need a +1 here: the catch actually is outside its associated label, so we actually need to skip over that as well to get to the catch.
It's a bit odd, though. I suspect that this use of labels creates an issue with the soundness proof. In particular, what is the administrative typing rule for catch_n? It can't bind the label, since that is bound by label_n. But label_n won't attach the catch kind, so any contained rethrow won't type-check anymore after reducing try to catch, and we lose preservation.
Maybe this can be hacked by changing the reduction rule for try-catch so that it binds the label outside the catch. Then the typing rule for catch_n could amend the label kind of the innermost label in its environment. (And we wouldn't have the +1.)
Something to think about. I'd encourage you to write out the typing rules for the administrative instructions, to check that it all hangs together.
(I guess the more baroque nature of the new design is showing here.)
Thank you so much @rossberg for the review and ongoing discussion. I pushed the changes you suggested, and I will take a closer look at the typing of the administrative rules and the implications for the soundness proof. |
Sorry it was Thanksgiving holidays and I was OOO for some more days! I'll take a final look shortly. |
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 for the long wait! Most of the comments are just simple uppercase/lowercase spelling nits, but I still have some questions. Please bear with me if they are too basic, as I haven't spent much time with formal semantics 😅
``` | ||
B^0 ::= val* [_] instr* | ||
B^k ::= catch_m{ tagaddr^? instr* }* B^k end | caught_m{ tagaddr val* } B^k end | ||
| delegate{ labelidx } B^k 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.
By the way does this support val*
between label_n
and catch_m
/caught_m
/delegate
? For example,
val* (label_n{instr*} (catch_m{ tagaddr? instr* }* B^k end) end
This is supported, but
val* (label_n{instr*} val* (catch_m{ tagaddr? instr* }* B^k end) end
is this supported? Or should we?
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 is supported, but
val* (label_n{instr*} val* (catch_m{ tagaddr? instr* }* B^k end) end
is this supported? Or should we?
The formal-overview does support this, but I can't see any way to reduce to it.
AFAICT I don't think it is a problem to support this case, even if it were possible.
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.
My question was, I didn't think the current block context rules can produce
val* (label_n{instr*} val* (catch_m{ tagaddr? instr* }* B^k end) end
meaning val*
between a label
and catch
/caught
/delegate
. I might have been mistaken. But I think what was suggested in #143 (comment) can create this sequence, so I don't think this will be a problem.
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 for being late as well!
S;C, labels [t*] ⊢ catch_n{a? instr'*}* instr* end : []→[t*] | ||
|
||
S;C, labels [t*] ⊢ instr* : []→[t*] | ||
C.labels[l] = [t'*] |
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 should actually be
C.labels[l] = [t'*] | |
C.labels[l+1] = [t'*] |
Otherwise, it would be referring to a different label than the original try-delegate.
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 think the +1 is missing because of the behaviour of "breaking inside the label". This way we also cover the case of delegating to the function body. WDYT?
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 you sure? If you have
try ... delegate $l
you'll reduce to
label{} (delegate{$l} ... end) end
When you type this, the outer label
adds an entry to the labels environment before you get to this typing rule, unlike with the try-delegate
before the reduction. So you have to skip over that prior label in this rule.
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 Apologies for the delay, last week ended up unexpectedly way too full.
Is it possible that the +1
you're missing is hidden in the labels [t*]
attached to C
in the other two judgements? The typing rule for try-delegate gives exactly the premises of this typing rule for adm.delegate, which in turn matches the typing for adm.label, to derive the type for that last expression in the end - I think.
More concretely, if I'm not mistaken, in your question we have a derivation of
(1) C ⊢ try bt instr* delegate $l : []→[t^n]
and because try bt instr* delegate $l
reduces to label_n{} (delegate{$l} instr* end) end
we should be able to derive that C ⊢ label_n{} (delegate{$l} instr* end) end : []→[t^n]
.
If this is what you ask to show, then from the derivation of (1) we know the following.
(2) C ⊢ bt : []→[t^n]
(3) C, labels [t^n] ⊢ instr* : []→[t^n]
(4) C.labels[l] = [t'*]
By having (3) and (4) as the premises for the typing rule of adm.delegate we derive
C, labels [t^n] ⊢ delegate{l} instr* end : []→[t^n]
which we can use as a premise to the typing rule for adm.label to get the desired
C ⊢ label_n{} (delegate{l} instr* end) end : []→[t^n]
WDYT?
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 problem I see is that (4) is not the premise you'll need when arriving at the adm.delegate rule. Instead you'll need
(4') C'[l'] = [t'*]
with C' = C, labels [t^n]
and some l'
, because that's the context you now enter the rule with. The only suitable l'
that allows you to prove this from (4) is l+1
, I believe.
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.
If we change the current rule
F; val^n (try bt instr* delegate l)
↪ F; label_m{} (delegate{l} val^n instr* end) end
to
F; val^n (try bt instr* delegate l)
↪ F; delegate(l) (label_m{} val^n instr* end) end
meaning, if we put the label created by the try-delegate itself within the scope of adm.delegate
, can this confusion be helped?
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.
Continuing the discussion with a suggestion in #205 .
Co-authored-by: Heejin Ahn <[email protected]> Co-authored-by: Andreas Rossberg <[email protected]>
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.
Apologies again for the many "single comments" (which I realised send lots of notification emails, oops). I will bundle my responses with the "submit review" feature.
There are still a couple of comments I haven't replied to yet, but will do by tomorrow the latest.
@ioannad Are you planning to land this soon? I think we can address a few things mentioned in the latest round and land this. |
Apologies for the long delay @aheejin! I am now available again for this. My plan is to finish by Monday adjusting the last bits we talked about, then we can finally land this. I will then update the formal document #180, possibly initially without the prose (just rebase and update to reflect the recent changes of this formal-overview). Filling in the prose and last couple of details in the formal document should not take more than 1-2 weeks I think, and I would probably prefer to do that in a separate PR, (we could talk about this in #180 if necessary) |
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.
Pushing these changes shortly.
Apologies for the delay, I am feeling unwell this week. Will still need a couple weeks for the formal document but counting from now.
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.
@aheejin @rossberg : I merged the current stand of this repo and went through all the discussions once more to make sure I didn't forget anything, resolving what was obviously resolved.
If you are ok with my answers to the couple of unresolved conversations, I think we can go ahead and merge this. Updating #180 to reflect this formal overview will follow shortly after that (partially without prose first, PR with the rest hopefully by next week).
S;C, labels [t*] ⊢ catch_n{a? instr'*}* instr* end : []→[t*] | ||
|
||
S;C, labels [t*] ⊢ instr* : []→[t*] | ||
C.labels[l] = [t'*] |
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 think the +1 is missing because of the behaviour of "breaking inside the label". This way we also cover the case of delegating to the function body. WDYT?
F; throw x ↪ F; throw a (if F.module.tagaddrs[x]=a) | ||
|
||
caught_m{a val^n} B^l[rethrow l] end | ||
↪ caught_m{a val^n} B^l[val^n (throw 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.
Can we consider this resolved, or am I missing the error?
This PR has so many comments it's become difficult to navigate. Since we agree at least 99% that this is ok I'll go ahead and merge it. If something needs to be changed still, it could be part of a followup. Thank you for the reviews and discussions! |
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 for being late, I have a couple of follow-up comments.
C, labels catch [t2*] ⊢ instr'* : [t*]→[t2*])* | ||
(C, labels catch [t2*] ⊢ instr''* : []→[t2*])? |
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.
C, labels catch [t2*] ⊢ instr'* : [t*]→[t2*])* | |
(C, labels catch [t2*] ⊢ instr''* : []→[t2*])? | |
C, labels (catch [t2*]) ⊢ instr'* : [t*]→[t2*])* | |
(C, labels (catch [t2*]) ⊢ instr''* : []→[t2*])? |
### Instructions | ||
Validation contexts now hold a list of tag types, one for each tag known to them. | ||
``` | ||
C ::= { ..., 'tags' tagtype*} |
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 also modify the definition of C
on line 46. Maybe move that here as well?
S;C, labels [t*] ⊢ catch_n{a? instr'*}* instr* end : []→[t*] | ||
|
||
S;C, labels [t*] ⊢ instr* : []→[t*] | ||
C.labels[l] = [t'*] |
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 you sure? If you have
try ... delegate $l
you'll reduce to
label{} (delegate{$l} ... end) end
When you type this, the outer label
adds an entry to the labels environment before you get to this typing rule, unlike with the try-delegate
before the reduction. So you have to skip over that prior label in this rule.
Sorry I didn't wait longer @rossberg ! Tomorrow I'll make a new PR with your suggestions and take a closer look to the typing rule for the administrative delegate. If in the meantime you see something else that should go in that next PR, please feel free to comment here with it.
|
Sorry for the late reply! Thanks for merging; it looks good to me. I made a few cosmetic fixes in #204. |
This is not the suggestion given in that last review of WebAssembly#143: https://github.com/WebAssembly/exception-handling/pull/143/files#r759907998 but a potential fix of the issue raised there.
This is not the suggestion given in that last review of WebAssembly#143: https://github.com/WebAssembly/exception-handling/pull/143/files#r759907998 but a potential fix of the issue raised there.
This is not the suggestion given in that last review of WebAssembly#143: https://github.com/WebAssembly/exception-handling/pull/143/files#r759907998 but a potential fix of the issue raised there.
This is not the suggestion given in that last review of #143: https://github.com/WebAssembly/exception-handling/pull/143/files#r759907998 but a potential fix of the issue raised there.
…ests) [Currently](https://github.com/WebAssembly/exception-handling/blob/main/proposals/exception-handling/Exceptions-formal-overview.md) `(try ... delegate l)` reduces to `(label_n{} ( delegate{l} ... end ) end)`, so by putting a label outside (i.e., before) the administrative `delegate{l}`. An idea proposed in past [unresolved](WebAssembly#205 (comment)) [discussions](https://github.com/WebAssembly/exception-handling/pull/143/files#r812476148) of WebAssembly#205 and WebAssembly#143, is to simplify and improve the formalism by instead putting the delegate label inside (i.e., after) the `delegate{l}`. So instead to reduce to `(delegate{l} ( label_n{} ... end ) end)`. TL;DR ----- I can't seem to make it work. This PR explored an approach to implement this idea, in the [formal overview file ](https://github.com/WebAssembly/exception-handling/blob/main/proposals/exception-handling/Exceptions-formal-overview.md), as well as in the interpreter, but had failing tests which I wasn't able to fix. Perhaps I'm overseeing some other solution or approach, or there is some mistake in the interpreter implementation and/or argumentation below. Details ------- I think the problem is as follows. With the current [definition of block contexts](https://webassembly.github.io/exception-handling/core/exec/runtime.html#block-contexts), the instruction sequence `B^l[ delegate{l} T[val^n (throw a)] end ]` not only is ambiguous, but doesn't work with a try-catch label located outside of the try-catch. Take for example the following possible reduction. ``` (try (try (throw x) delegate 0) catch x end) ↪ (label_0{} (catch{a_x ε} (delegate{0} (label_0{} (throw a_x) end) end) end) end) ``` The intention for this delegate is to throw inside the handler `catch{a_x ε}` and be caught there. However, a possible `B^0` for the reduction of `delegate{0}` is `B^0 = [_] catch{a_x ε}`, in which case the reduction rule gives the following. ``` ↪ (label_0{} (throw a_x) end) ↪ (throw a_x) ``` The issue here seems to be that there is no label between the `delegate{l}` and the `catch{...}`. Perhaps there is a different change we can easily make it work, for example changing control contexts or block contexts? Failing tests ............. We can observe the above wrong behaviour also in the interpreter tests, although this could be fixable somehow. In particular, the first commit of this PR has the formal overview changes also implemented in the execution steps of the interpreter (in `interpreter/exec/eval.ml`): - The reduction of `try ... delegate l` puts the `label{}` after the `delegate{l}`. - The reduction of `delegate{l}` does not pattern match for an initial label. I tried to minimise a failing test from `test/core/try_delegate.wast` in the file `test/core/try_delegate_minimal_fail.wast`. To reproduce the failure build the interpreter and run the above test file as follows, for example from a Linux terminal in the base directory of the repository: ``` cd interpreter make ./wasm ../test/core/try_delegate_minimal_fail.wast ``` See also comments in the test file.
This is an attempt to formally describe @aheejin's 3rd proposal, which she presented to the Wasm CG, and which was voted to be the new EH proposal, on September 18, 2020. This is not formal spec that I have developed, but a formal description of this 3rd proposal.
This is a much reworked form of the first attempt on this formal spec overview edited accordingly. It is in the form of a document as @tlively requested, to make discussion on specific points easier.
This formal spec overview is roughly in the style of the 2nd proposal's formal spec overview by @rossberg.
I would be happy if anyone could point out things that are wrong or that I misunderstood.