Skip to content

[dart2wasm] Support try/catch/finally in sync* functions #51343

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

Closed
askeksa-google opened this issue Feb 9, 2023 · 9 comments
Closed

[dart2wasm] Support try/catch/finally in sync* functions #51343

askeksa-google opened this issue Feb 9, 2023 · 9 comments
Assignees
Labels
area-dart2wasm Issues for the dart2wasm compiler.

Comments

@askeksa-google
Copy link

The sync* implementation in dart2wasm currently does not support try statements.

To implement these via the sync* CFG, each try block needs to be split into individual Wasm try blocks covering each of the sync* CFG blocks that are covered by the original try.

The catch blocks need not be duplicated. They can be separate CFG blocks targeted by the try blocks.

Also, finally blocks can probably be reused between the various control flow chains (normal completion, exception thrown, return from function) rather than being duplicated as in the normal implementation. One local for each finally block could contain the target index of where the execution needs to go after the execution of the finally block completes. Care must be taken to make this interact properly with normal finally blocks (those not containing any yield or yield* and therefore translated by the normal code generator). Either the CFG-based finally statements must properly maintain the stack of finalizers for the normal code generator, such that it properly duplicates these finalizers on return or exceptions, or all try/finally statements in a sync* functions must be translated via the CFG, even if they do not contain any yield or yield*.

@askeksa-google askeksa-google added the area-dart2wasm Issues for the dart2wasm compiler. label Feb 9, 2023
@osa1 osa1 self-assigned this Apr 4, 2023
@osa1
Copy link
Member

osa1 commented Apr 11, 2023

This is being implemented in https://dart-review.googlesource.com/c/sdk/+/294480.

@osa1
Copy link
Member

osa1 commented Apr 18, 2023

One local for each finally block could contain the target index of where the execution needs to go after the execution of the finally block completes

In this implementation do we need separate blocks for return, break N, and rethrow?

An alternative is to have two variables (which can be combined into one) for the whole CFG:

  1. One for how many parent finalizers to run
  2. One for how to continue after the last finalizer

In a finalizer, after running the finalizer code, we check (1). If it's non-zero we decrement it and continue with the parent finalizer. If it's zero we check (2). If the value is 0 we return. If it's 1 we rethrow. Otherwise the value minus one gives us the target block index (used in break).

I don't know how easy it is to create a synthetic local currently (can I update Closures.captures and expect the context struct to be updated/generated as expected?) but depending on that this may be easier to implement, we just add these two variables as fields in _SuspendState.

We can also combine these two variables into one by adding a single int field and using low and high bits for the variables.

@osa1
Copy link
Member

osa1 commented Apr 18, 2023

We discussed this with @askeksa-google offline, I'm implementing the plan with two suspend state varibles described in my comment above.

@osa1
Copy link
Member

osa1 commented May 12, 2023

The approach I describe above does not work, we need a stack of continuation variables for nested finalizers. For example:

(Code below is async but try-finally implementation is the same one in my sync* CL above)

void main() async {
  print('1');
  try {
    print('2');
  } finally {
    print('3');
    try {
      print('4');
    } finally {
      print('5');
    }
    print('6');
  }
  print('7');
}

Here in the nested finalizer we need to save parent finalizer's continuation and restore it on fallthrough (i.e. when the nested finalizer does not override the continuation with throw, return, or break/continue). With the idea described above we have one continuation variable which we override in the nested finalizer. Because the nested finalizer does not override the control flow, after the finalizer block we should continue according to the parent finalizer's continuation, but that continuation is lost because we've overridden it when entering the nested finalizer.

So we need a stack of continuation variables. This can be implemented by (conceptually) adding a local for each finally block. When we enter a finally block we set the continuation local the current continuation (return, break, re-throw). The variable will then be used if the current finalizer block does not override the control flow (fallthrough).

I think we still need a global (within a function) "number of finalizers to run" variable as described in my previous comment, to be able to implement break N when there are finalizers between the labeled block and the current statements. In these cases the continuation of the first finalizers will be "run X finalizers, then jump to block N". If the finalizer doesn't override it the fallthrough code will decrement X by one, set the parent finalizer block's fallthrough continuation, and jump to it.

The code that jumps to a finalizer block will set the finalizer's continuation variable. The only difference here is that we will have a variable specific to the finalizer block, not a single variable.

Normally these continuation variables don't need to be in the context (heap-allocated sync/async function state) always, but currently we put all locals in the context, so I'll try a simpler implementation that doesn't require creating a kernel variable declaration for finally blocks and instead try to generate context fields for them before code generation.

@osa1
Copy link
Member

osa1 commented May 12, 2023

I think we still need a global (within a function) "number of finalizers to run" variable as described in my previous comment

We can avoid this variable if we set continuation variables of all parent finalizers when changing control flow (instead of just the continuation variable of the parent finalizer). For example, break N can set continuation of the next finalizer as "jump to <block of parent finalizer>", and then continuation of the next finalizer's parent as "jump to <block for continuation of N>".

@osa1
Copy link
Member

osa1 commented Jul 6, 2023

We have a correct implementation of this in AsyncCodeGenerator, we should either reuse the logic somehow or copy-paste-modify the code from AsyncCodeGenerator.

@meg4cyberc4t
Copy link

meg4cyberc4t commented May 15, 2024

Hello @osa1!
Now (when building web assembly in stable version) I catch this unimplemented exception in my project.
Libraries that use try/catch in sync* cannot be build for wasm.
I see that the development of this is abandoned, but could you please return to it?

@osa1
Copy link
Member

osa1 commented May 15, 2024

Hi @meg4cyberc4t.

This is not really abandoned, we just had better things to work on as sync* is not that widely used. I'll try to get back to this soon.

@osa1
Copy link
Member

osa1 commented May 16, 2024

I'm fixing the remaining sync* missing features and issues in https://dart-review.googlesource.com/c/sdk/+/366663.

osa1 added a commit to osa1/sdk that referenced this issue May 17, 2024
…x sync*

This is the last part of the series of patches to implement missing
sync* features and fix bugs.

Move common code generation functions between async and sync* code
generators to the state_machine library, with the name
`StateMachineCodeGenerator`.

This class allows overriding parts that differ between the async and
sync* code generators.

Fixes tests:

- co19/Language/Statements/Yield_and_Yield_Each/Yield_Each/execution_sync_t05
- language/sync_star/generator3_test/test1
- language/sync_star/generator3_test/test2
- language/sync_star/sync_star_exception_iterator_test
- language/sync_star/sync_star_exception_nested_test
- language/sync_star/sync_star_exception_test
- language/sync_star/sync_star_exception_current_test

Fixes dart-lang#51343.
Fixes dart-lang#51342.
osa1 added a commit to osa1/sdk that referenced this issue May 17, 2024
…x sync*

This is the last part of the series of patches to implement missing
sync* features and fix bugs.

Move common code generation functions between async and sync* code
generators to the state_machine library, with the name
`StateMachineCodeGenerator`.

This class allows overriding parts that differ between the async and
sync* code generators.

Fixes tests:

- co19/Language/Statements/Yield_and_Yield_Each/Yield_Each/execution_sync_t05
- language/sync_star/generator3_test/test1
- language/sync_star/generator3_test/test2
- language/sync_star/sync_star_exception_iterator_test
- language/sync_star/sync_star_exception_nested_test
- language/sync_star/sync_star_exception_test
- language/sync_star/sync_star_exception_current_test

Fixes dart-lang#51343.
Fixes dart-lang#51342.

Change-Id: Ife6eab43b2721b003ebf9bc0f03796748fd5df46
copybara-service bot pushed a commit that referenced this issue May 29, 2024
This cherry-picks commits:

6de879e - [dart2wasm] Fix exception handling in async functions
7e237a1 - [dart2wasm] Small refactoring in async code generator
eabb2b3 - [dart2wasm] Catch JS exceptions in async functions
e44bc22 - [dart2wasm] Fix bug in restoration of `this` in async functions.
350954a - [dart2wasm] Fix `this` restoration code in sync* handling.
8ccb412 - [dart2wasm] Move type parameter bounds checks & parameter type check logic together with logic setting up variables
e7dde83 - [dart2wasm] Port VM fix for #52083 (sync*)
3863e78 - [dart2wasm] Move yield finder to a shared library
829261e - [dart2wasm] Move async compiler utilities to state_machine library
fab56db - [dart2wasm] Move common code generation routines to state_machine, fix sync*

Bugs: #55347, #55457, #51343, #51342
Cherry-pick: https://dart-review.googlesource.com/c/sdk/+/368300
Cherry-pick-request: #55847
Change-Id: I0a4186533fbdf4c5727911295ad48696a90f715f
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/368300
Commit-Queue: Ömer Ağacan <[email protected]>
Reviewed-by: Martin Kustermann <[email protected]>
Commit-Queue: Martin Kustermann <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-dart2wasm Issues for the dart2wasm compiler.
Projects
None yet
Development

No branches or pull requests

3 participants