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

Match delegate's immediate and label argument to those of branches #146

Closed
aheejin opened this issue Jan 26, 2021 · 13 comments
Closed

Match delegate's immediate and label argument to those of branches #146

aheejin opened this issue Jan 26, 2021 · 13 comments

Comments

@aheejin
Copy link
Member

aheejin commented Jan 26, 2021

I'd like to suggest a slight change to the current seemingly discussed spec in #143 so that we can treat the target of brs and delegates in a similar way in the wast format. I think this will help reading and writing the wast format and spec tests.

I'm not actually very sure if this will be a suggestion to change, because after discussions w/ @thibaudmichaud I realized my understanding was different from his and what I'd like to suggest here was actually already more in line with his V8
implementation. (Now I think we two are on the same page.)

So what I'd like to achieve is we can write the argument of delegate as labels in the wast format, as we do for br/br_ifs, and make delegate can syntactically target a try, which semantically in turn goes to that try's corresponding catch. delegate's immediate argument is translated to the labels in the same way we do for brs. Let me illustrate that with an example. I annotated catches with their corresponding try's labels just to make it more readable. The example looks a bit long, but please bear with me:

try $labelA
catch ($labelA)
  block $labelB
    try $labelC
      br 0         (= br $labelC)
      br 1         (= br $labelB)
      br 2         (= br $labelA)
      try
      delegate 0   (= delegate $labelC, which delegates to 'catch ($labelC)')
      try
      delegate 1   (= delegate $labelB, which is a block, so validation failure)
      try
      delegate 2   (= delegate $labelA, which delegates to 'catch ($labelA)', but 'catch ($labelA)' is above this instruction, so validation failure)
      try
      delegate 3   (= delegate to the caller)
    catch ($labelC)
      br 0         (= br $labelC)
      br 1         (= br $labelB)
      br 2         (= br $labelA)
      try
      delegate 0   (= delegate $labelC, which delegates to 'catch ($labelC)', but 'catch ($labelC)' is above this instruction, so validation failure)
      try
      delegate 1   (= delegate $labelB, which is a block, so validation failure)
      try
      delegate 2   (= delegate $labelA, which delegates to 'catch ($labelA)', but 'catch ($labelA)' is above this instruction, so validation failure)
      try
      delegate 3   (= delegate to the caller)
    end  ;; try $labelC
  end ;; block $labelB
end ;; try $labelA

(catches can be either catch or catch_all, it doesn't really matter here. unwind will be treated the same.)

So what I propose is basically this: Treat delegate's immediate/label in the same way as br's immediate/label. delegate also can target any of block/loop/try, but if the target is a block or loop it is a validation failure. (I think this was discussed somewhat already in #143).

If the target is a try, if the try's corresponding catch is below the delegate as in the first delegate 0 in my example, it delegates to that catch. If the corresponding catch is above the delegate as in the second delegate 0 in my example, we can't delegate to somewhere upwards, so it is a validation failure. Please notice the difference between two delegate 0's interpretation in my example. But the advantage of this format is we can maintain a single control flow stack, and use labels and immediates interchangeably in the wast format as we already do for br/br_if. In my example brs are just written to show that when br's immediate and delegate's immediate are the same, their label arguments will also be the same.

Also, when there are multiple catch/catch_alls per try, they are treated as a chunk. So

try $labelA
catch tag_a ($labelA)
  try
  delegate 0  ;; validation failure
catch tag_b
  ...
catch_all
  ...
end

delegate 0 here targets $labelA's all catches as a chunk. Because catch tag_a is above this delegate 0, this is rather a validation failure, and it does not delegate to catch tag_b or catch_all below.

I'm not necessarily trying to discuss the formal spec yet; if we are all on the same page on the operational semantics in informal English, I think we can proceed to the format version.

WDYT? cc @thibaudmichaud @backes @ioannad @takikawa @tlively

@tlively
Copy link
Member

tlively commented Jan 26, 2021

Sounds good to me! I think this is in line with my own previous assumptions as well.

@thibaudmichaud
Copy link
Collaborator

Thanks! Sounds like we are still on the same page.

@backes
Copy link
Member

backes commented Jan 27, 2021

Thanks for the write-up. Looks good to me!

@rossberg
Copy link
Member

Sounds good, that already was my assumption as well.

@takikawa
Copy link
Collaborator

takikawa commented Feb 3, 2021

Thanks for the clarification! It sounds good to me and I think it matches my mental model of the semantics.

I just wanted to get clarification on one thing, sorry if I missed this somewhere in the write-up. If a delegate N targets an outer try-delegate block, that should also validate right? Judging from the write-up where it says "delegate also can target any of block/loop/try", I was assuming this is the case, but I wanted to make sure.

@aheejin
Copy link
Member Author

aheejin commented Feb 3, 2021

@takikawa Yes, delegate can target either a try-catch or a try-delegate. (If if targets something else, such as blocks or loops, it will be a validation error.)

@ioannad
Copy link
Collaborator

ioannad commented Feb 3, 2021

Full agreement from me too, @aheejin.

aheejin added a commit to aheejin/binaryen that referenced this issue Feb 8, 2021
This adds support for reading/writing of the new 'delegate' instruction
in the folded wast format, the stack IR format, and the binary format in
Binaryen. We don't have a format spec written down yet, but please refer
to WebAssembly/exception-handling#137 and
WebAssembly/exception-handling#146 for the informal semantics.
In the current version of spec `delegate` is basically a rethrow, but
with branch-like immediate argument so that it can bypass other
catches/delegates in between.

'delegate' is not represented a new `Expression`, but it is rather an
option within a `Try` class, like `catch`/`catch_all`.

`delegate` semantically targets an outer `catch` or `delegate`, but we
write `delegate` target as a `try` label because we only give labels to
block-like scoping expressions. So far we has not given `Try` a label
and used inner blocks or a wrapping block in case a branch targets the
`try`. But in case of `delegate`, it can syntactically only target `try`
and if it targets blocks or loops it is a validation failure.

So after discussions in WebAssembly#3497, we give `Try` a label but this label can
only be targeted by `delegate`s. Unfortunately this makes parsing and
writing of `Try` expression somewhat complicated. Also there is one
special case; if the immediate argument of `try` is the same as the
depth of control flow stack, this means the 'delegate' delegates to the
caller. To handle this case this adds a fake label
`DELEGATE_CALLER_TARGET`, and when writing it back to the wast format
writes it as an immediate value, unlike other cases in which we write
labels.

This uses `DELEGATE_FIELD_SCOPE_NAME_DEF/USE` to represent `try`'s label
and `delegate`'s target. There are many cases that `try` and
`delegate`'s labels need to be treated in the same way as block and
branch labels, such as for hashing or comparing. But there are routines
in which we automatically assume all label uses are branches. I thought
about adding a new kind of defines such as
`DELEGATE_FIELD_TRY_NAME_DEF/USE`, but I think it will also involve some
duplication of existing routines or classes. So at the moment this PR
chooses to use the existing `DELEGATE_FIELD_SCOPE_NAME_DEF/USE` for
`try` and `delegate` labels and makes only necessary amount of changes
in branch-utils. We can revisit this decision later if necessary.

This only supports reading and writing and has not been tested against
any optimization passes yet.
aheejin added a commit to aheejin/binaryen that referenced this issue Feb 8, 2021
This adds support for reading/writing of the new 'delegate' instruction
in the folded wast format, the stack IR format, and the binary format in
Binaryen. We don't have a format spec written down yet, but please refer
to WebAssembly/exception-handling#137 and
WebAssembly/exception-handling#146 for the informal semantics.
In the current version of spec `delegate` is basically a rethrow, but
with branch-like immediate argument so that it can bypass other
catches/delegates in between.

'delegate' is not represented a new `Expression`, but it is rather an
option within a `Try` class, like `catch`/`catch_all`.

One special thing about `delegate` is, even though it is written
_within_ a `try` in the folded wat format, like
```wasm
(try
  (do
    ...
  )
  (delegate $l)
)
```
In the unfolded wat format or in the binary format, `delegate` serves as
a scope end instruction so there is no separate `end`:
```wasm
try
  ...
delegate $l
```

`delegate` semantically targets an outer `catch` or `delegate`, but we
write `delegate` target as a `try` label because we only give labels to
block-like scoping expressions. So far we has not given `Try` a label
and used inner blocks or a wrapping block in case a branch targets the
`try`. But in case of `delegate`, it can syntactically only target `try`
and if it targets blocks or loops it is a validation failure.

So after discussions in WebAssembly#3497, we give `Try` a label but this label can
only be targeted by `delegate`s. Unfortunately this makes parsing and
writing of `Try` expression somewhat complicated. Also there is one
special case; if the immediate argument of `try` is the same as the
depth of control flow stack, this means the 'delegate' delegates to the
caller. To handle this case this adds a fake label
`DELEGATE_CALLER_TARGET`, and when writing it back to the wast format
writes it as an immediate value, unlike other cases in which we write
labels.

This uses `DELEGATE_FIELD_SCOPE_NAME_DEF/USE` to represent `try`'s label
and `delegate`'s target. There are many cases that `try` and
`delegate`'s labels need to be treated in the same way as block and
branch labels, such as for hashing or comparing. But there are routines
in which we automatically assume all label uses are branches. I thought
about adding a new kind of defines such as
`DELEGATE_FIELD_TRY_NAME_DEF/USE`, but I think it will also involve some
duplication of existing routines or classes. So at the moment this PR
chooses to use the existing `DELEGATE_FIELD_SCOPE_NAME_DEF/USE` for
`try` and `delegate` labels and makes only necessary amount of changes
in branch-utils. We can revisit this decision later if necessary.

Many of changes to the existing test cases are because now all `try`s
are automatically assigned a label. They will be removed in
`RemoveUnusedNames` pass in the same way as block labels if not targeted
by any delegates.

This only supports reading and writing and has not been tested against
any optimization passes yet.
aheejin added a commit to aheejin/binaryen that referenced this issue Feb 10, 2021
This adds support for reading/writing of the new 'delegate' instruction
in the folded wast format, the stack IR format, and the binary format in
Binaryen. We don't have a format spec written down yet, but please refer
to WebAssembly/exception-handling#137 and
WebAssembly/exception-handling#146 for the informal semantics.
In the current version of spec `delegate` is basically a rethrow, but
with branch-like immediate argument so that it can bypass other
catches/delegates in between.

'delegate' is not represented a new `Expression`, but it is rather an
option within a `Try` class, like `catch`/`catch_all`.

One special thing about `delegate` is, even though it is written
_within_ a `try` in the folded wat format, like
```wasm
(try
  (do
    ...
  )
  (delegate $l)
)
```
In the unfolded wat format or in the binary format, `delegate` serves as
a scope end instruction so there is no separate `end`:
```wasm
try
  ...
delegate $l
```

`delegate` semantically targets an outer `catch` or `delegate`, but we
write `delegate` target as a `try` label because we only give labels to
block-like scoping expressions. So far we has not given `Try` a label
and used inner blocks or a wrapping block in case a branch targets the
`try`. But in case of `delegate`, it can syntactically only target `try`
and if it targets blocks or loops it is a validation failure.

So after discussions in WebAssembly#3497, we give `Try` a label but this label can
only be targeted by `delegate`s. Unfortunately this makes parsing and
writing of `Try` expression somewhat complicated. Also there is one
special case; if the immediate argument of `try` is the same as the
depth of control flow stack, this means the 'delegate' delegates to the
caller. To handle this case this adds a fake label
`DELEGATE_CALLER_TARGET`, and when writing it back to the wast format
writes it as an immediate value, unlike other cases in which we write
labels.

This uses `DELEGATE_FIELD_SCOPE_NAME_DEF/USE` to represent `try`'s label
and `delegate`'s target. There are many cases that `try` and
`delegate`'s labels need to be treated in the same way as block and
branch labels, such as for hashing or comparing. But there are routines
in which we automatically assume all label uses are branches. I thought
about adding a new kind of defines such as
`DELEGATE_FIELD_TRY_NAME_DEF/USE`, but I think it will also involve some
duplication of existing routines or classes. So at the moment this PR
chooses to use the existing `DELEGATE_FIELD_SCOPE_NAME_DEF/USE` for
`try` and `delegate` labels and makes only necessary amount of changes
in branch-utils. We can revisit this decision later if necessary.

Many of changes to the existing test cases are because now all `try`s
are automatically assigned a label. They will be removed in
`RemoveUnusedNames` pass in the same way as block labels if not targeted
by any delegates.

This only supports reading and writing and has not been tested against
any optimization passes yet.
aheejin added a commit to aheejin/binaryen that referenced this issue Feb 10, 2021
This adds support for reading/writing of the new 'delegate' instruction
in the folded wast format, the stack IR format, and the binary format in
Binaryen. We don't have a format spec written down yet, but please refer
to WebAssembly/exception-handling#137 and
WebAssembly/exception-handling#146 for the informal semantics.
In the current version of spec `delegate` is basically a rethrow, but
with branch-like immediate argument so that it can bypass other
catches/delegates in between.

'delegate' is not represented a new `Expression`, but it is rather an
option within a `Try` class, like `catch`/`catch_all`.

One special thing about `delegate` is, even though it is written
_within_ a `try` in the folded wat format, like
```wasm
(try
  (do
    ...
  )
  (delegate $l)
)
```
In the unfolded wat format or in the binary format, `delegate` serves as
a scope end instruction so there is no separate `end`:
```wasm
try
  ...
delegate $l
```

`delegate` semantically targets an outer `catch` or `delegate`, but we
write `delegate` target as a `try` label because we only give labels to
block-like scoping expressions. So far we has not given `Try` a label
and used inner blocks or a wrapping block in case a branch targets the
`try`. But in case of `delegate`, it can syntactically only target `try`
and if it targets blocks or loops it is a validation failure.

So after discussions in WebAssembly#3497, we give `Try` a label but this label can
only be targeted by `delegate`s. Unfortunately this makes parsing and
writing of `Try` expression somewhat complicated. Also there is one
special case; if the immediate argument of `try` is the same as the
depth of control flow stack, this means the 'delegate' delegates to the
caller. To handle this case this adds a fake label
`DELEGATE_CALLER_TARGET`, and when writing it back to the wast format
writes it as an immediate value, unlike other cases in which we write
labels.

This uses `DELEGATE_FIELD_SCOPE_NAME_DEF/USE` to represent `try`'s label
and `delegate`'s target. There are many cases that `try` and
`delegate`'s labels need to be treated in the same way as block and
branch labels, such as for hashing or comparing. But there are routines
in which we automatically assume all label uses are branches. I thought
about adding a new kind of defines such as
`DELEGATE_FIELD_TRY_NAME_DEF/USE`, but I think it will also involve some
duplication of existing routines or classes. So at the moment this PR
chooses to use the existing `DELEGATE_FIELD_SCOPE_NAME_DEF/USE` for
`try` and `delegate` labels and makes only necessary amount of changes
in branch-utils. We can revisit this decision later if necessary.

Many of changes to the existing test cases are because now all `try`s
are automatically assigned a label. They will be removed in
`RemoveUnusedNames` pass in the same way as block labels if not targeted
by any delegates.

This only supports reading and writing and has not been tested against
any optimization passes yet.
aheejin added a commit to aheejin/binaryen that referenced this issue Feb 10, 2021
This adds support for reading/writing of the new 'delegate' instruction
in the folded wast format, the stack IR format, the poppy IR format, and
the binary format in Binaryen. We don't have a format spec written down
yet, but please refer to WebAssembly/exception-handling#137 and
WebAssembly/exception-handling#146 for the informal semantics. In the
current version of spec `delegate` is basically a rethrow, but with
branch-like immediate argument so that it can bypass other
catches/delegates in between.

'delegate' is not represented a new `Expression`, but it is rather an
option within a `Try` class, like `catch`/`catch_all`.

One special thing about `delegate` is, even though it is written
_within_ a `try` in the folded wat format, like
```wasm
(try
  (do
    ...
  )
  (delegate $l)
)
```
In the unfolded wat format or in the binary format, `delegate` serves as
a scope end instruction so there is no separate `end`:
```wasm
try
  ...
delegate $l
```

`delegate` semantically targets an outer `catch` or `delegate`, but we
write `delegate` target as a `try` label because we only give labels to
block-like scoping expressions. So far we has not given `Try` a label
and used inner blocks or a wrapping block in case a branch targets the
`try`. But in case of `delegate`, it can syntactically only target `try`
and if it targets blocks or loops it is a validation failure.

So after discussions in WebAssembly#3497, we give `Try` a label but this label can
only be targeted by `delegate`s. Unfortunately this makes parsing and
writing of `Try` expression somewhat complicated. Also there is one
special case; if the immediate argument of `try` is the same as the
depth of control flow stack, this means the 'delegate' delegates to the
caller. To handle this case this adds a fake label
`DELEGATE_CALLER_TARGET`, and when writing it back to the wast format
writes it as an immediate value, unlike other cases in which we write
labels.

This uses `DELEGATE_FIELD_SCOPE_NAME_DEF/USE` to represent `try`'s label
and `delegate`'s target. There are many cases that `try` and
`delegate`'s labels need to be treated in the same way as block and
branch labels, such as for hashing or comparing. But there are routines
in which we automatically assume all label uses are branches. I thought
about adding a new kind of defines such as
`DELEGATE_FIELD_TRY_NAME_DEF/USE`, but I think it will also involve some
duplication of existing routines or classes. So at the moment this PR
chooses to use the existing `DELEGATE_FIELD_SCOPE_NAME_DEF/USE` for
`try` and `delegate` labels and makes only necessary amount of changes
in branch-utils. We can revisit this decision later if necessary.

Many of changes to the existing test cases are because now all `try`s
are automatically assigned a label. They will be removed in
`RemoveUnusedNames` pass in the same way as block labels if not targeted
by any delegates.

This only supports reading and writing and has not been tested against
any optimization passes yet.
takikawa added a commit to takikawa/wabt that referenced this issue Feb 11, 2021
As clarified in:

  WebAssembly/exception-handling#146

the delegate target cannot be a `block` or `loop`. This commit
also adds a failure test that covers several cases discussed
in that issue.
@aheejin
Copy link
Member Author

aheejin commented Feb 11, 2021

Can we assume rethrow follows the same rule? The current explainer (#137) says


Associated with the rethrow instruction is a label. The label is used to
disambiguate which exception is to be rethrown, when inside nested catch blocks.
The label is the relative block depth to the corresponding try block for which
the catching block appears.

For example consider the following:

try
  ...
catch 1
  ...
  block
    ...
    try
      ...
    catch 2
      ...
      try
        ...
      catch 3
        ...
        rethrow N
      end
    end
  end
  ...
end

In this example, N is used to disambiguate which caught exception is being
rethrown. It could rethrow any of the three caught expceptions. Hence,
rethrow 0 corresponds to the exception caught by catch 3, rethrow 1
corresponds to the exception caught by catch 2, and rethrow 3 corresponds
to the exception caught by catch 1.

Note that rethrow 2 is not allowed because it does not reference a try
instruction. Rather, it references a block instruction.


I didn't invent this rule for the explainer myself; I simply restored the old rule from the very first version of the spec, which is similar to the current spec except the current spec has unwind and delegate additionally. But this looks in line with the rule we agreed on for delegate as well. So the example above can be rewritten in the text format using labels:

try $label1
  ...
catch ($label1)
  ...
  block $label2
    ...
    try $label3
      ...
    catch ($label3)
      ...
      try $label4
        ...
      catch ($label4)
        ...
        ;; These two rethrow the exception caught by catch ($label4)
        rethrow $label4
        rethrow 0
        ;; These two rethrow the exception caught by catch ($label3)
        rethrow $label3
        rethrow 1
        ;; These correspond to the block $label2, so validation failure
        rethrow $label2
        rethrow 2
        ;; These two rethrow the exception caught by catch ($label1)
        rethrow $label1
        rethrow 3
      end
    end
  end
  ...
end

Do we also agree on this?

@aheejin
Copy link
Member Author

aheejin commented Feb 11, 2021

Oh, also, to make rethrow follow the same immediate/label rule with branches and delegates, when rethrow is between try and catch. I added some more lines to the previous example. Here all other cases are the same, but rethrow 0 is a validation failure.

try $label1
  ...
catch ($label1)
  ...
  block $label2
    ...
    try $label3
      ...
    catch ($label3)
      ...
      try $label4
        ...
        ;; These two refers to 'catch (label$4)', but it is not within 'catch (label$4)', so validation failure
        rethrow $label4
        rethrow 0
        ;; These two rethrow the exception caught by catch ($label3)
        rethrow $label3
        rethrow 1
        ;; These correspond to the block $label2, so validation failure
        rethrow $label2
        rethrow 2
        ;; These two rethrow the exception caught by catch ($label1)
        rethrow $label1
        rethrow 3
      catch ($label4)
        ...
        ;; These two rethrow the exception caught by catch ($label4)
        rethrow $label4
        rethrow 0
        ;; These two rethrow the exception caught by catch ($label3)
        rethrow $label3
        rethrow 1
        ;; These correspond to the block $label2, so validation failure
        rethrow $label2
        rethrow 2
        ;; These two rethrow the exception caught by catch ($label1)
        rethrow $label1
        rethrow 3
      end
    end
  end
  ...
end

@thibaudmichaud
Copy link
Collaborator

Thanks for the examples, they still match my expectation and V8's implementation so far.

@takikawa
Copy link
Collaborator

takikawa commented Feb 11, 2021

@aheejin Thanks for clarifying this. Based on previous examples from this PR (example 2 there) my understanding had been that the current interpretation was that rethrow's argument only counts catch blocks.

But I'm also happy to implement this interpretation of the rethrow argument which aligns it with br, delegate, etc. because it is simpler and consistent with everything else too.

aheejin added a commit to WebAssembly/binaryen that referenced this issue Feb 12, 2021
This adds support for reading/writing of the new `delegate` instruction
in the folded wast format, the stack IR format, the poppy IR format, and
the binary format in Binaryen. We don't have a formal spec written down
yet, but please refer to WebAssembly/exception-handling#137 and
WebAssembly/exception-handling#146 for the informal semantics. In the
current version of spec `delegate` is basically a rethrow, but with
branch-like immediate argument so that it can bypass other
catches/delegates in between.

`delegate` is not represented as a new `Expression`, but it is rather
an option within a `Try` class, like `catch`/`catch_all`.

One special thing about `delegate` is, even though it is written
_within_ a `try` in the folded wat format, like
```wasm
(try
  (do
    ...
  )
  (delegate $l)
)
```
In the unfolded wat format or in the binary format, `delegate` serves as
a scope end instruction so there is no separate `end`:
```wasm
try
  ...
delegate $l
```

`delegate` semantically targets an outer `catch` or `delegate`, but we
write `delegate` target as a `try` label because we only give labels to
block-like scoping expressions. So far we have not given `Try` a label
and used inner blocks or a wrapping block in case a branch targets the
`try`. But in case of `delegate`, it can syntactically only target `try`
and if it targets blocks or loops it is a validation failure.

So after discussions in #3497, we give `Try` a label but this label can
only be targeted by `delegate`s. Unfortunately this makes parsing and
writing of `Try` expression somewhat complicated. Also there is one
special case; if the immediate argument of `try` is the same as the
depth of control flow stack, this means the 'delegate' delegates to the
caller. To handle this case this adds a fake label
`DELEGATE_CALLER_TARGET`, and when writing it back to the wast format
writes it as an immediate value, unlike other cases in which we write
labels.

This uses `DELEGATE_FIELD_SCOPE_NAME_DEF/USE` to represent `try`'s label
and `delegate`'s target. There are many cases that `try` and
`delegate`'s labels need to be treated in the same way as block and
branch labels, such as for hashing or comparing. But there are routines
in which we automatically assume all label uses are branches. I thought
about adding a new kind of defines such as
`DELEGATE_FIELD_TRY_NAME_DEF/USE`, but I think it will also involve some
duplication of existing routines or classes. So at the moment this PR
chooses to use the existing `DELEGATE_FIELD_SCOPE_NAME_DEF/USE` for
`try` and `delegate` labels and makes only necessary amount of changes
in branch-utils. We can revisit this decision later if necessary.

Many of changes to the existing test cases are because now all `try`s
are automatically assigned a label. They will be removed in
`RemoveUnusedNames` pass in the same way as block labels if not targeted
by any delegates.

This only supports reading and writing and has not been tested against
any optimization passes yet.

---

Original unfolded wat file to generate test/try-delegate.wasm:
```wasm
(module
  (event $e)
  (func
    try
      try
      delegate 0
    catch $e
    end)

  (func
    try
      try
      catch $e
        i32.const 0
        drop
        try
        delegate 1
      end
    catch $e
    end
  )
)
```
aheejin added a commit to llvm/llvm-project that referenced this issue Feb 12, 2021
I previously assumed `delegate`'s immediate argument computation
followed a different rule than that of branches, but we agreed to make
it the same
(WebAssembly/exception-handling#146). This
removes the need for a separate `DelegateStack` in both CFGStackify and
InstPrinter.

When computing the immediate argument, we use a different function for
`delegate` computation because in MIR `DELEGATE`'s instruction's
destination is the destination catch BB or delegate BB, and when it is a
catch BB, we need an additional step of getting its corresponding `end`
marker.

Reviewed By: tlively, dschuff

Differential Revision: https://reviews.llvm.org/D96525
@aheejin
Copy link
Member Author

aheejin commented Feb 12, 2021

@takikawa Sorry, yeah, I was mistaken about the rule too, but while discussing the impl with @thibaudmichaud I realized the Explainer says it uses the same rule with branches (and delegates).

aheejin added a commit to llvm/llvm-project that referenced this issue Feb 13, 2021
Previously we assumed `rethrow`'s argument was always 0, but it turned
out `rethrow` follows the same rule with `br` or `delegate`:
WebAssembly/exception-handling#137
WebAssembly/exception-handling#146 (comment)

Currently `rethrow`s generated by our backend always rethrow the
exception caught by the innermost enclosing catch, so this adds a
function to compute that and replaces `rethrow`'s argument with its
computed result.

This also renames `EHPadStack` in `InstPrinter` to `TryStack`, because
in CFGStackify we use `EHPadStack` to mean the range between
`catch`~`end`, while in `InstPrinter` we used it to mean the range
between `try`~`catch`, so choosing different names would look clearer.
Doesn't contain any functional changes in `InstPrinter`.

Reviewed By: dschuff

Differential Revision: https://reviews.llvm.org/D96595
aheejin added a commit to aheejin/binaryen that referenced this issue Feb 14, 2021
I was previously mistaken about `rethrow`'s argument rule and thoought
it only counted `catch`'s depth. But it turns out it follows the same
rule `delegate`'s label: the immediate argument follows the same rule as
when computing branch labels, but it only can target `try` labels
(semantically it targets that `try`'s corresponding `catch`); otherwise
it will be a validation failure. Unlike `delegate`, `rethrow`'s label
denotes not where to rethrow, but which exception to rethrow. For
example,
```wasm
try $l0
catch ($l0)
  try $l1
  catch ($l1)
   rethrow $l0 ;; rethrow the exception caught by 'catch ($l0)'
  end
end
```

Refer to this comment for the more detailed informal semantics:
WebAssembly/exception-handling#146 (comment)
aheejin added a commit to aheejin/binaryen that referenced this issue Feb 14, 2021
I was previously mistaken about `rethrow`'s argument rule and thought
it only counted `catch`'s depth. But it turns out it follows the same
rule with `delegate`'s label: the immediate argument is comptuted in the
same way as branch labels, but it only can target `try` labels
(semantically it targets that `try`'s corresponding `catch`); otherwise
it will be a validation failure. Unlike `delegate`, `rethrow`'s label
denotes not where to rethrow, but which exception to rethrow. For
example,
```wasm
try $l0
catch ($l0)
  try $l1
  catch ($l1)
   rethrow $l0 ;; rethrow the exception caught by 'catch ($l0)'
  end
end
```

Refer to this comment for the more detailed informal semantics:
WebAssembly/exception-handling#146 (comment)
aheejin added a commit to aheejin/binaryen that referenced this issue Feb 14, 2021
I was previously mistaken about `rethrow`'s argument rule and thought
it only counted `catch`'s depth. But it turns out it follows the same
rule with `delegate`'s label: the immediate argument is comptuted in the
same way as branch labels, but it only can target `try` labels
(semantically it targets that `try`'s corresponding `catch`); otherwise
it will be a validation failure. Unlike `delegate`, `rethrow`'s label
denotes not where to rethrow, but which exception to rethrow. For
example,
```wasm
try $l0
catch ($l0)
  try $l1
  catch ($l1)
   rethrow $l0 ;; rethrow the exception caught by 'catch ($l0)'
  end
end
```

Refer to this comment for the more detailed informal semantics:
WebAssembly/exception-handling#146 (comment)
aheejin added a commit to aheejin/binaryen that referenced this issue Feb 14, 2021
I was previously mistaken about `rethrow`'s argument rule and thought
it only counted `catch`'s depth. But it turns out it follows the same
rule with `delegate`'s label: the immediate argument is comptuted in the
same way as branch labels, but it only can target `try` labels
(semantically it targets that `try`'s corresponding `catch`); otherwise
it will be a validation failure. Unlike `delegate`, `rethrow`'s label
denotes not where to rethrow, but which exception to rethrow. For
example,
```wasm
try $l0
catch ($l0)
  try $l1
  catch ($l1)
   rethrow $l0 ;; rethrow the exception caught by 'catch ($l0)'
  end
end
```

Refer to this comment for the more detailed informal semantics:
WebAssembly/exception-handling#146 (comment)

This also reverses some of `delegateTarget` -> `exceptionTarget` changes
done in WebAssembly#3562 in the validator. Label validation rules apply differently
for `delegate` and `rethrow` for try-catch. For example, this is valid:
```wasm
try $l0
  try
  delegate $l0
catch ($l0)
end
```
But this is NOT valid:
```wasm
try $l0
catch ($l0)
  try
  delegate $l0
end
```
So `try`'s label should be used within try-catch range (not catch-end
range) for `delegate`s.

But for the `rethrow` the rule is different. For example, this is valid:
```wasm
try $l0
catch ($l0)
  rethrow $l0
end
```
But this is NOT valid:
```wasm
try $l0
  rethrow $l0
catch ($l0)
end
```
So the `try`'s label should be used within catch-end range instead.
aheejin added a commit to WebAssembly/binaryen that referenced this issue Feb 17, 2021
I was previously mistaken about `rethrow`'s argument rule and thought
it only counted `catch`'s depth. But it turns out it follows the same
rule `delegate`'s label: the immediate argument follows the same rule as
when computing branch labels, but it only can target `try` labels
(semantically it targets that `try`'s corresponding `catch`); otherwise
it will be a validation failure. Unlike `delegate`, `rethrow`'s label
denotes not where to rethrow, but which exception to rethrow. For
example,
```wasm
try $l0
catch ($l0)
  try $l1
  catch ($l1)
   rethrow $l0 ;; rethrow the exception caught by 'catch ($l0)'
  end
end
```

Refer to this comment for the more detailed informal semantics:
WebAssembly/exception-handling#146 (comment)

---

This also reverts some of `delegateTarget` -> `exceptionTarget` changes
done in #3562 in the validator. Label validation rules apply differently
for `delegate` and `rethrow` for try-catch. For example, this is valid:
```wasm
try $l0
  try
  delegate $l0
catch ($l0)
end
```
But this is NOT valid:
```wasm
try $l0
catch ($l0)
  try
  delegate $l0
end
```
So `try`'s label should be used within try-catch range (not catch-end
range) for `delegate`s.

But for the `rethrow` the rule is different. For example, this is valid:
```wasm
try $l0
catch ($l0)
  rethrow $l0
end
```
But this is NOT valid:
```wasm
try $l0
  rethrow $l0
catch ($l0)
end
```
So the `try`'s label should be used within catch-end range instead.
@ioannad
Copy link
Collaborator

ioannad commented Feb 19, 2021

@aheejin SGTM too! I am updating the formal-spec-overview now, and it seems the semantics as agreed in this issue do make things simpler. Although we're back to a new attribute (this time I call it kind) of labels surrounding instructions in try blocks or catch blocks, that's ok imho. No more Caught stack so 👍 :)

binji pushed a commit to WebAssembly/wabt that referenced this issue Feb 24, 2021
As clarified in:

  WebAssembly/exception-handling#146

the delegate target cannot be a `block` or `loop`. This commit
also adds a failure test that covers several cases discussed
in that issue.
aheejin added a commit to aheejin/exception-handling that referenced this issue Sep 7, 2021
This adds the changes decided in WebAssembly#176 and adds a modified version from
WebAssembly#146 (comment)
for clarification. (The example is not the same by the way; the `catch`
from the outermost `try` has been moved)

Closes WebAssembly#176.
aheejin added a commit to aheejin/exception-handling that referenced this issue Sep 7, 2021
This adds the changes decided in WebAssembly#176 and adds a modified version from
WebAssembly#146 (comment)
for clarification. (The example is not the same by the way; the `catch`
from the outermost `try` has been moved)

Closes WebAssembly#176.
aheejin added a commit that referenced this issue Sep 15, 2021
This adds the changes decided in #176 and adds a modified version from
#146 (comment)
for clarification. (The example is not the same by the way; the `catch`
from the outermost `try` has been moved)
mem-frob pushed a commit to draperlaboratory/hope-llvm-project that referenced this issue Oct 7, 2022
I previously assumed `delegate`'s immediate argument computation
followed a different rule than that of branches, but we agreed to make
it the same
(WebAssembly/exception-handling#146). This
removes the need for a separate `DelegateStack` in both CFGStackify and
InstPrinter.

When computing the immediate argument, we use a different function for
`delegate` computation because in MIR `DELEGATE`'s instruction's
destination is the destination catch BB or delegate BB, and when it is a
catch BB, we need an additional step of getting its corresponding `end`
marker.

Reviewed By: tlively, dschuff

Differential Revision: https://reviews.llvm.org/D96525
mem-frob pushed a commit to draperlaboratory/hope-llvm-project that referenced this issue Oct 7, 2022
Previously we assumed `rethrow`'s argument was always 0, but it turned
out `rethrow` follows the same rule with `br` or `delegate`:
WebAssembly/exception-handling#137
WebAssembly/exception-handling#146 (comment)

Currently `rethrow`s generated by our backend always rethrow the
exception caught by the innermost enclosing catch, so this adds a
function to compute that and replaces `rethrow`'s argument with its
computed result.

This also renames `EHPadStack` in `InstPrinter` to `TryStack`, because
in CFGStackify we use `EHPadStack` to mean the range between
`catch`~`end`, while in `InstPrinter` we used it to mean the range
between `try`~`catch`, so choosing different names would look clearer.
Doesn't contain any functional changes in `InstPrinter`.

Reviewed By: dschuff

Differential Revision: https://reviews.llvm.org/D96595
aheejin added a commit to aheejin/exception-handling that referenced this issue Feb 18, 2023
The current explainer has a detailed example on `delegate`, which was
adapted from
WebAssembly#146 (comment),
but not the one for `rethrow` from the same issue
(WebAssembly#146 (comment)).
This adds the detailed `rethrow` example to the explainer, with the
some cosmetic changes to the label names and comments.
aheejin added a commit that referenced this issue Feb 23, 2023
The current explainer has a detailed example on `delegate`, which was
adapted from
#146 (comment),
but not the one for `rethrow` from the same issue
(#146 (comment)).
This adds the detailed `rethrow` example to the explainer, with the
some cosmetic changes to the label names and comments.
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

7 participants