Skip to content
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

Rename catch_br #133

Closed
rossberg opened this issue Oct 5, 2020 · 23 comments
Closed

Rename catch_br #133

rossberg opened this issue Oct 5, 2020 · 23 comments

Comments

@rossberg
Copy link
Member

rossberg commented Oct 5, 2020

The name catch_br is misleading, since it is not a branch in the regular sense. In particular, catch_br $l jumps to a different place than br $l. Possible suggestion: catch_delegate.

@aheejin
Copy link
Member

aheejin commented Oct 6, 2020

I don't mind either way. Do people have objections or other suggestions for the name? (Do we need a vote for this? I guess not, right?)

@ioannad
Copy link
Collaborator

ioannad commented Oct 6, 2020

The name catch_br is misleading, since it is not a branch in the regular sense. In particular, catch_br $l jumps to a different place than br $l. Possible suggestion: catch_delegate.

I also don't mind changing the name, but I must say I didn't realise that catch_br $l branches to a different place than br $l. I though we just have to validate that $l is indeed a try block. Is this not the case?

@aheejin
Copy link
Member

aheejin commented Oct 6, 2020

Yes, I think we should verify it too. $l should be attached to a try, and catch_br (or catch_delegate) should transfer, or delegate, the exception searching path to the corresponding catch/unwind of try specified by the label.

Whether $l is attached to try of catch is just a matter of text syntax, because in binary the argument will be an immediate integer.

EDIT: I think this comment is misleading and incorrect; I posted a new explanation here: #133 (comment)

@rossberg
Copy link
Member Author

rossberg commented Oct 6, 2020

@ioannad, a regular br jumps past the whole try statement, while catch_br goes to its catch/unwind part. The former also carries along potential arguments (depending on the type of the label), whereas the latter is agnostic of the label type.

@RossTate
Copy link
Contributor

RossTate commented Oct 6, 2020

Taking the "skip over" interpretation of catch_br in #130, catch_br should not specify what try to delegate to; it should specify how many trys to skip over. This would also provide an answer to how to specify the caller as whom to delegate to.

@rossberg
Copy link
Member Author

rossberg commented Oct 6, 2020

It does, but it counts other control constructs as well. That makes finding the target try in the control stack O(1) in an implementation instead of O(N), and doesn't require a separate try stack.

@RossTate
Copy link
Contributor

RossTate commented Oct 6, 2020

That makes sense. Note then that with the "skip over" interpretation, there is no need for the target to be a try. That is, catch_br is essentially saying "for any exceptions thrown from in here, pretend their continuation does not include the n+1 immediately enclosing control constructs".

@tlively
Copy link
Member

tlively commented Oct 6, 2020

FWIW, if we do determine that this construct is sufficiently different from br, I like delegate as the verb to use. It would be nice to use plain delegate instead of catch_delegate because 1) "try-delegate" rolls off the tongue better than "try-catch_delegate" and 2) it's more inclusive of unwind as well as catch. If delegate seems too general a name, though, I would be fine with catch_delegate as well.

@rossberg
Copy link
Member Author

rossberg commented Oct 6, 2020

@tlively, sounds good to me. Given that it's part of a larger try construct, delegate would be unambiguous enough. I mean, even end is. :)

@RossTate's suggestion about generalising this is interesting. It essentially turns this into a rethrow_in sort of construct. In fact, if we generalised rethrow that way we wouldn't even need a separate try-delegate statement, but could just replace that with a (try ... catch_all (rethrow_in $l)).

@RossTate
Copy link
Contributor

RossTate commented Oct 6, 2020

Interesting idea, @rossberg, although this catch_br is supposed to apply to unwinding as well, not just exceptions.

@aheejin
Copy link
Member

aheejin commented Oct 6, 2020

Sorry, I think I said something incorrect in #133 (comment).

What I intended was the immediate arg for catch_br (or whatever its name will be) only counts trys and not other constructs such as blocks or loops. This may require a separate try stack but I thought it was clearer. I think I implied the opposite in #133 (comment) by saying we need a validation to see if $l is really a try; I was incorrect. $l will be an immediate in binary, and if we only count trys, we don't need that validation. (Maybe we need validation for immediate values that exceeds the (try stack depth + 1), but that's a separate thing)

Do people here mean we should also count other blocks/loops as well? I'm little confused, but if there's a good enough reason, I think we can consider that.

@aheejin
Copy link
Member

aheejin commented Oct 6, 2020

@rossberg

@RossTate's suggestion about generalising this is interesting. It essentially turns this into a rethrow_in sort of construct. In fact, if we generalised rethrow that way we wouldn't even need a separate try-delegate statement, but could just replace that with a (try ... catch_all (rethrow_in $l)).

When we have two-phase, rethrow within a catch block will run after both two phase unwinding is finished, and will not be accessible to first phase search. This is the reason I proposed catch_br rather than adding a label to existing rethrow. (I once suggested adding a label to rethrow to fix unwind mismatch (#74), but that was when we weren't considering two-phase.)

@tlively I think try-delegate sounds good too.

@takikawa
Copy link
Collaborator

takikawa commented Oct 6, 2020

Do people here mean we should also count other blocks/loops as well? I'm little confused, but if there's a good enough reason, I think we can consider that.

I had also thought that (counting other blocks/loops) was the intended behavior until this thread. I think if the catch_br name is kept it would make sense to make the index numbering consistent with br and count the other control constructs. If the name is changed, maybe this point of consistency isn't as important but it does seem easier to implement without the additional stack.

aheejin added a commit to aheejin/exception-handling that referenced this issue Oct 7, 2020
This updates the explainer text according to the new spec we agreed in
the 09-15-2020 CG meeting. Some of the text is taken from [the first
version](https://github.com/WebAssembly/exception-handling/blob/master/proposals/old/Exceptions-v1.md)
of the proposal. I renamed `catch_br` to `delegate` based on the
perceived consensus in WebAssembly#133.

There are some minor issues that need clarification, some of which is
currently being discussed in the repo:
- `rethrow`'s immmediate (or label) argument. I made it match the status
  in the first proposal for now. I believe keeping it makes it more
  versatile and lets us handle more languages.
- It is not clear whether `rethrow` and `delegate`'s label (=immediate)
  argument should count non-try block-like structures, such as blocks
  and loops. It is currently being discussed for `delegate`.
- `delegate`'s opcode should be decided. We exhausted all control-flow
  opcodes that begin with `0x0`.
aheejin added a commit to aheejin/exception-handling that referenced this issue Oct 7, 2020
This updates the explainer text according to the new spec we agreed in
the 09-15-2020 CG meeting. Some of the text is taken from [the first
version](https://github.com/WebAssembly/exception-handling/blob/master/proposals/old/Exceptions-v1.md)
of the proposal. I renamed `catch_br` to `delegate` based on the
perceived consensus in WebAssembly#133.

There are some minor issues that need clarification, some of which is
currently being discussed in the repo:
- `rethrow`'s immmediate (or label) argument. I made it match the status
  in the first proposal for now. I believe keeping it makes it more
  versatile and lets us handle more languages.
- It is not clear whether `rethrow` and `delegate`'s label (=immediate)
  argument should count non-try block-like structures, such as blocks
  and loops. It is currently being discussed for `delegate`.
- `delegate`'s opcode should be decided. We exhausted all control-flow
  opcodes that begin with `0x0`.
@aheejin
Copy link
Member

aheejin commented Oct 12, 2020

I first thought the label of delegate can only target trys and thus its immediate argument only count trys, but maybe the consistency is more important. If we interpret that in the same way as we do for other branches, VMs don't need to main a separate stack I guess, right? Of course, there's one difference from branches, that the target try shouldn't mean the end of it but catch/unwind of it.

Do people agree with this..?

@ioannad
Copy link
Collaborator

ioannad commented Oct 30, 2020

The new proposal overview has renamed catch_br to delegate and there seems to be agreement (or no objections) to have the immediate argument of delegate target any control constructs, so implementation is simpler, so I suppose we can close this.

I just want to make sure @aheejin agrees with the semantics given in this comment, i.e., that

That is, catch_br [delegate] is essentially saying "for any exceptions thrown from in here, pretend their continuation does not include the n+1 immediately enclosing control constructs".

With emphasis on the n+1 part. That would also resolve a question we have with respect to the meaning of delegate 0.

@aheejin
Copy link
Member

aheejin commented Nov 4, 2020

@ioannad

I just want to make sure @aheejin agrees with the semantics given in this comment, i.e., that

That is, catch_br [delegate] is essentially saying "for any exceptions thrown from in here, pretend their continuation does not include the n+1 immediately enclosing control constructs".

With emphasis on the n+1 part. That would also resolve a question we have with respect to the meaning of delegate 0.

Sorry for the delayed reply. I think this makes sense. Then how do you think delegate 0 should behave? It would be targeting itself, so is that gonna be an infinite loop? (I guess this is not what you meant?) Or should it behave equivalent to

try
  ...
catch_all
end

?

@ioannad
Copy link
Collaborator

ioannad commented Nov 4, 2020

I just want to make sure @aheejin agrees with the semantics given in this comment, i.e., that

That is, catch_br [delegate] is essentially saying "for any exceptions thrown from in here, pretend their continuation does not include the n+1 immediately enclosing control constructs".

With emphasis on the n+1 part. That would also resolve a question we have with respect to the meaning of delegate 0.

Sorry for the delayed reply. I think this makes sense. Then how do you think delegate 0 should behave? It would be targeting itself, so is that gonna be an infinite loop? (I guess this is not what you meant?)

@aheejin, I understood that comment's phrase "does not include the n+1 immediately enclosing control constructs" as starting to count outside of the try...delegate n itself. So with the "n+1"-interpretation, try bt instr* delegate 0 does not target itself but the first control construct enclosing it. Did I misunderstand?

Having said that, I was surprised to see that comment with the "n+1"-interpretation. We had understood delegate 0 as either an illegal instruction (because of the loop you mentioned), or as a special case: for example some sort of "don't catch", or "catch_all" as you said in the quote below, or anything convenient for the toolchain.

I was about to ask whether to make delegate 0 illegal or special case it, when I saw the said comment. That's why I wanted to know whether you agree with that "n+1"-interpretation or not.

Or should it behave equivalent to

try
  ...
catch_all
end

?

@aheejin
Copy link
Member

aheejin commented Nov 5, 2020

@ioannad Sorry, I think I meant "n"-interpretation and not "n+1" one. So delegate 0 would be a special case that is a validation failure, or it should be considered as something like try ... catch_all end. The reason I think we should choose "n" interpretation is that's what we do for other block-like instructions. We think brs target a label, so

block $label0
  br $label0
end

In this code, even though we write it as br $label0, it will be emitted in binary form as br 0. So 0 means the innermost enclosing context for block-like instructions. (This also applies to brs within trys.)

So we can also think delegate as targetting try's labels:

try $label1
  try $label0
    ...
  delegate $label1
catch $tag
end

So here delegate $label1 (= delegate 1) would mean delegating to catch $tag, and delegate 0 would mean delegating to itself, which will be invalid. I'm OK with either that we invalidate it or we treat it as just catching the exception and doing nothing.

@rossberg
Copy link
Member Author

rossberg commented Nov 5, 2020

@aheejin, but delegate is not a standalone instruction, it's part of the structured try-delegate construct. The clause does not occur within the try-block, it delimits it. So it would actually be more consistent if 0 referred not to its own inner block but to the first enclosing block, as with other labels. (It's also generally desirable to avoid the existence of illegal edge case values where possible.)

@aheejin
Copy link
Member

aheejin commented Nov 5, 2020

@rossberg Yeah, maybe that makes more sense. And we don't need to special-case delegate 0 then. What do you think @ioannad?

The clause does not occur within the try-block, it delimits it.

I think I understood the drift of your comment, but I'm not exactly sure what this means.

@rossberg
Copy link
Member Author

rossberg commented Nov 5, 2020

I mean that the delegate marks the end of the block, like end elsewhere. So, the label is not actually inside the block this encloses.

@ioannad
Copy link
Collaborator

ioannad commented Nov 5, 2020

@aheejin I agree that this makes sense. I'm happy we don't have to special case delegate 0.

@aheejin
Copy link
Member

aheejin commented Nov 5, 2020

Yeah, then let's close this. I'll update the explainer about this. Thanks.

@aheejin aheejin closed this as completed Nov 5, 2020
ioannad pushed a commit to ioannad/exception-handling that referenced this issue Feb 23, 2021
aheejin added a commit that referenced this issue Feb 25, 2021
This updates the explainer text according to the new spec we agreed in
the 09-15-2020 CG meeting and discussions afterwards.

The following are modifications and clarifications we made after the
09-15-2020 CG meeting, and the relevant issue posts, if any:
https://github.com/WebAssembly/meetings/blob/master/main/2020/CG-09-15.md

- `catch_br` wasm renamed to `delegate` (#133)
- `rethrow` gains an immediate argument (#126)
- Removed dependences on the reference types proposal and the multivalue
  proposal. The multivalue proposal was previously listed as dependent
  because 1. `try` is basically a `block`, so it can have multivalue
  input/output 2. `br_on_exn` can extract multiple values from a
  `block`. We don't have `br_on_exn` anymore, and I'm not sure 1 is a
  strong enough reason to make it a dependence.
- Mention `rethrow` cannot rethrow exceptions caught by `unwind` (#142
  and #137)
- Mention some runtimes, especially web VMs, can attach stack traces to
  the exception object, implying stack traces are not required for all
  VMs
- Update label/validation rules for `delegate` and `rethrow` (#146)
- Finalize opcodes for `delegate` (0x18) and `catch_all` (0x19) (#145
  and #147)

I believe this resolves many previous issue threads, so I'll close them.
Please reopen them if you think there are things left for discussions in
those issues.

Resolves #113, resolves #126, resolves #127, resolves #128, resolves
#130, resolves #142, resolves #145, resolves #146, resolves #147.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants