Skip to content

[EH] Support reading/writing of delegate #3561

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

Merged
merged 5 commits into from
Feb 12, 2021
Merged

Conversation

aheejin
Copy link
Member

@aheejin aheejin commented 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 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

(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:

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 delegates. 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 trys
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:

(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 aheejin requested review from kripken and tlively February 10, 2021 17:27
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.
@@ -0,0 +1,23 @@
(module
Copy link
Member Author

@aheejin aheejin Feb 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The unfolded wat file used to generate try-delegate.wasm file is this:

(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
  )
)

This is to test whether delegate within try body or catch body works. When parsing we create a block within each of try and catch body, but we shouldn't make those delegates target those blocks because it will be a validation failure, so we have to remap those delegates to the correct try.

)
)
)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not related to delegate but I just found out we don't have any tests that test nested try-catch, so I added one here

Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! This is simpler and cleaner than I'd thought could be done, given the complexity.

Basically lgtm, aside from the comments.

Comment on lines +5864 to +5909
// For simplicity, we create an inner block within the catch body too, but the
// one within the 'catch' *must* be omitted when we write out the binary back
// later, because the 'catch' instruction pushes a value onto the stack and
// the inner block does not support block input parameters without multivalue
// support.
// try
// ...
// catch $e ;; Pushes value(s) onto the stack
// block ;; Inner block. Should be deleted when writing binary!
// use the pushed value
// end
// end
//
// But when input binary code is like
// try
// ...
// catch $e
// br 0
// end
//
// 'br 0' accidentally happens to target the inner block, creating code like
// this in Binaryen IR, making the inner block not deletable, resulting in a
// validation error:
// (try
// ...
// (catch $e
// (block $label0 ;; Cannot be deleted, because there's a branch to this
// ...
// (br $label0)
// )
// )
// )
//
// When this happens, we fix this by creating a block that wraps the whole
// try-catch, and making the branches target that block instead, like this:
// (block $label ;; New enclosing block, new target for the branch
// (try
// ...
// (catch $e
// (block ;; Now this can be deleted when writing binary
// ...
// (br $label0)
// )
// )
// )
// )
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment block was just moved

@aheejin aheejin merged commit f826df6 into WebAssembly:main Feb 12, 2021
@aheejin aheejin deleted the delegate branch February 12, 2021 05:42
aheejin added a commit to aheejin/binaryen that referenced this pull request Feb 12, 2021
This updates C and binaryen.js API to match the new `Try` structure to
support `delegate`, added in WebAssembly#3561.
aheejin added a commit to aheejin/binaryen that referenced this pull request Feb 12, 2021
This updates C and binaryen.js API to match the new `Try` structure to
support `delegate`, added in WebAssembly#3561. Now `try` can take a name (which can
be null) like a block, and also has an additional `delegateTarget` field
argument which should only be used for try-delegate and otherwise null.
aheejin added a commit to aheejin/binaryen that referenced this pull request Feb 12, 2021
This updates C and binaryen.js API to match the new `Try` structure to
support `delegate`, added in WebAssembly#3561. Now `try` can take a name (which can
be null) like a block, and also has an additional `delegateTarget` field
argument which should only be used for try-delegate and otherwise null.

This also adds several more variant of `makeTry` methods in
wasm-builder. Some are for making try-delegate and some are for
try-catch(_all).
aheejin added a commit that referenced this pull request Feb 13, 2021
This updates C and binaryen.js API to match the new `Try` structure to
support `delegate`, added in #3561. Now `try` can take a name (which can
be null) like a block, and also has an additional `delegateTarget` field
argument which should only be used for try-delegate and otherwise null.

This also adds several more variant of `makeTry` methods in
wasm-builder. Some are for making try-delegate and some are for
try-catch(_all).
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

Successfully merging this pull request may close these issues.

2 participants