[dart2wasm] Don't compile catch blocks multiple times for JS exceptions

Currently when a Dart `catch` block can catch both Dart and JS
exceptions, we compile the `catch` body once in a Wasm `catch` block,
once in a Wasm `catch_all` block.

This is because a Dart exception needs to be caught in a `catch` with
the right tag, to be able to get the exception and stack trace values,
and JS exceptions need to be caught in `catch_all` and they come without
error values and stack traces.

With this CL we generate one Wasm block per Dart `catch` block. Wasm
`catch` and `catch_all` blocks only do type tests and jump to the right
Wasm `block` when a type test passes.

This allows using the same block for multiple Wasm `catch` and
`catch_all` blocks.

When jumping to the block for a Dart `catch` we pass the error value and
stack trace to the block. As before, when the caught exception is a JS
exception, we pass an empty `JavaScriptError` as the error value and the
call stack of the Dart `catch` as the stack trace.

We also replace Wasm `rethrow` instruction with `throw` when compiling
Dart `rethrow` statements. This change is necessary as the blocks for
Dart `catch` blocks are no longer enclosed by a Wasm `try`, and it also
makes it easier to switch to the new exception handling proposal, which
doesn't have a `rethrow` instruction.

This changes Wasm exceptions reported to the console in uncaught
exceptions, but when we switch to the new exception handling
instructions we will recover the stack traces, as `throw_ref` doesn't
update the stack trace of the error value.

Change-Id: I732c0192af158611d5f0a584182a48b0e13ff83a
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/410321
Commit-Queue: Ömer Ağacan <[email protected]>
Reviewed-by: Martin Kustermann <[email protected]>

Author: osa1

pkg/dart2wasm/lib/code_generator.dart

@@ -90,7 +90,8 @@ abstract class AstCodeGenerator
   final LinkedHashMap<LabeledStatement, List<w.Label>> breakFinalizers =
       LinkedHashMap();
 
-  final List<w.Label> tryLabels = [];
+  final List<({w.Local exceptionLocal, w.Local stackTraceLocal})>
+      tryBlockLocals = [];
 
   final Map<SwitchCase, w.Label> switchLabels = {};
 
@@ -903,33 +904,48 @@ abstract class AstCodeGenerator
 
   @override
   void visitTryCatch(TryCatch node) {
-    // It is not valid dart to have a try without a catch.
+    // It is not valid Dart to have a try without a catch.
     assert(node.catches.isNotEmpty);
 
-    // We lower a [TryCatch] to a wasm try block.
-    w.Label try_ = b.try_();
-    translateStatement(node.body);
-    b.br(try_);
+    final w.RefType exceptionType = translator.topInfo.nonNullableType;
+    final w.RefType stackTraceType =
+        translator.stackTraceInfo.repr.nonNullableType;
+
+    final w.Label wrapperBlock = b.block();
+
+    // Create a block target for each Dart `catch` block, to be able to share
+    // code when generating a `catch` and `catch_all` for the same Dart `catch`
+    // block, when the block can catch both Dart and JS exceptions.
+    // The `end` for the Wasm `try` block works as the first exception handler
+    // target.
+    List<w.Label> catchBlockLabels = List.generate(node.catches.length - 1,
+        (i) => b.block([], [exceptionType, stackTraceType]),
+        growable: true);
+
+    w.Label try_ = b.try_([], [exceptionType, stackTraceType]);
+    catchBlockLabels.add(try_);
 
-    // Note: We must wait to add the try block to the [tryLabels] stack until
-    // after we have visited the body of the try. This is to handle the case of
-    // a rethrow nested within a try nested within a catch, that is we need the
-    // rethrow to target the last try block with a catch.
-    tryLabels.add(try_);
+    catchBlockLabels = catchBlockLabels.reversed.toList();
+
+    translateStatement(node.body);
+    b.br(wrapperBlock);
 
     // Stash the original exception in a local so we can push it back onto the
     // stack after each type test. Also, store the stack trace in a local.
-    w.Local thrownException = addLocal(translator.topInfo.nonNullableType);
-    w.Local thrownStackTrace =
-        addLocal(translator.stackTraceInfo.repr.nonNullableType);
+    w.Local thrownException = addLocal(exceptionType);
+    w.Local thrownStackTrace = addLocal(stackTraceType);
+
+    tryBlockLocals.add(
+        (exceptionLocal: thrownException, stackTraceLocal: thrownStackTrace));
 
-    void emitCatchBlock(Catch catch_, bool emitGuard) {
+    void emitCatchBlock(
+        w.Label catchBlockTarget, Catch catch_, bool emitGuard) {
       // For each catch node:
       //   1) Create a block for the catch.
       //   2) Push the caught exception onto the stack.
       //   3) Add a type test based on the guard of the catch.
       //   4) If the test fails, we jump to the next catch. Otherwise, we
-      //      execute the body of the catch.
+      //      jump to the block for the body of the catch.
       w.Label catchBlock = b.block();
       DartType guard = catch_.guard;
 
@@ -942,29 +958,10 @@ abstract class AstCodeGenerator
         b.br_if(catchBlock);
       }
 
-      final VariableDeclaration? exceptionDeclaration = catch_.exception;
-      if (exceptionDeclaration != null) {
-        initializeVariable(exceptionDeclaration, () {
-          b.local_get(thrownException);
-          // Type test passed, downcast the exception to the expected type.
-          translator.convertType(
-            b,
-            thrownException.type,
-            translator.translateType(exceptionDeclaration.type),
-          );
-        });
-      }
-
-      final VariableDeclaration? stackTraceDeclaration = catch_.stackTrace;
-      if (stackTraceDeclaration != null) {
-        initializeVariable(
-            stackTraceDeclaration, () => b.local_get(thrownStackTrace));
-      }
-
-      translateStatement(catch_.body);
+      b.local_get(thrownException);
+      b.local_get(thrownStackTrace);
+      b.br(catchBlockTarget);
 
-      // Jump out of the try entirely if we enter any catch block.
-      b.br(try_);
       b.end(); // end catchBlock.
     }
 
@@ -974,26 +971,29 @@ abstract class AstCodeGenerator
 
     b.local_set(thrownStackTrace);
     b.local_set(thrownException);
-    for (final Catch catch_ in node.catches) {
+    for (int catchBlockIndex = 0;
+        catchBlockIndex < node.catches.length;
+        catchBlockIndex += 1) {
+      final catch_ = node.catches[catchBlockIndex];
       // Only insert type checks if the guard is not `Object`
       final bool shouldEmitGuard =
           catch_.guard != translator.coreTypes.objectNonNullableRawType;
-      emitCatchBlock(catch_, shouldEmitGuard);
+      emitCatchBlock(
+          catchBlockLabels[catchBlockIndex], catch_, shouldEmitGuard);
       if (!shouldEmitGuard) {
         // If we didn't emit a guard, we won't ever fall through to the
         // following catch blocks.
         break;
       }
     }
+
     // Rethrow if all the catch blocks fall through
     b.rethrow_(try_);
 
     // If we have a catches that are generic enough to catch a JavaScript
     // error, we need to put that into a catch_all block.
-    final Iterable<Catch> catchAllCatches = node.catches
-        .where((c) => guardCanMatchJSException(translator, c.guard));
-
-    if (catchAllCatches.isNotEmpty) {
+    if (node.catches
+        .any((c) => guardCanMatchJSException(translator, c.guard))) {
       // This catches any objects that aren't dart exceptions, such as
       // JavaScript exceptions or objects.
       b.catch_all();
@@ -1007,14 +1007,21 @@ abstract class AstCodeGenerator
       call(translator.javaScriptErrorFactory.reference);
       b.local_set(thrownException);
 
-      for (final c in catchAllCatches) {
+      for (int catchBlockIndex = 0;
+          catchBlockIndex < node.catches.length;
+          catchBlockIndex += 1) {
+        final catch_ = node.catches[catchBlockIndex];
+        if (!guardCanMatchJSException(translator, catch_.guard)) {
+          continue;
+        }
         // Type guards based on a type parameter are special, in that we cannot
         // statically determine whether a JavaScript error will always satisfy
         // the guard, so we should emit the type checking code for it. All
         // other guards will always match a JavaScript error, however, so no
         // need to emit type checks for those.
-        final bool shouldEmitGuard = c.guard is TypeParameterType;
-        emitCatchBlock(c, shouldEmitGuard);
+        final bool shouldEmitGuard = catch_.guard is TypeParameterType;
+        emitCatchBlock(
+            catchBlockLabels[catchBlockIndex], catch_, shouldEmitGuard);
         if (!shouldEmitGuard) {
           // If we didn't emit a guard, we won't ever fall through to the
           // following catch blocks.
@@ -1026,8 +1033,36 @@ abstract class AstCodeGenerator
       b.rethrow_(try_);
     }
 
-    tryLabels.removeLast();
-    b.end(); // end try_.
+    for (Catch catch_ in node.catches) {
+      b.end();
+      b.local_set(thrownStackTrace);
+      b.local_set(thrownException);
+
+      final VariableDeclaration? exceptionDeclaration = catch_.exception;
+      if (exceptionDeclaration != null) {
+        initializeVariable(exceptionDeclaration, () {
+          b.local_get(thrownException);
+          // Type test passed, downcast the exception to the expected type.
+          translator.convertType(
+            b,
+            thrownException.type,
+            translator.translateType(exceptionDeclaration.type),
+          );
+        });
+      }
+
+      final VariableDeclaration? stackTraceDeclaration = catch_.stackTrace;
+      if (stackTraceDeclaration != null) {
+        initializeVariable(
+            stackTraceDeclaration, () => b.local_get(thrownStackTrace));
+      }
+
+      translateStatement(catch_.body);
+      b.br(wrapperBlock);
+    }
+
+    tryBlockLocals.removeLast();
+    b.end(); // end tryWrapper
   }
 
   @override
@@ -2721,7 +2756,10 @@ abstract class AstCodeGenerator
 
   @override
   w.ValueType visitRethrow(Rethrow node, w.ValueType expectedType) {
-    b.rethrow_(tryLabels.last);
+    final exceptionLocals = tryBlockLocals.last;
+    b.local_get(exceptionLocals.exceptionLocal);
+    b.local_get(exceptionLocals.stackTraceLocal);
+    b.throw_(translator.getExceptionTag(b.module));
     return expectedType;
   }