[vm/concurrency] Allow StackTrace objects to be shared

The existing lib/isolate/stacktrace_message_test was failing and also incorrectly written due to "!" in front of the expected stringification of the stacktrace. Issue dart-lang#46623 TEST=vm/dart{,_2}/isolates/fast_object_copy2_test, lib{,_2}/isolate/stacktrace_message_test Change-Id: I169d8fd7a7c3cb3b8c57e00287565e3a5c622acf Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/216041 Reviewed-by: Alexander Aprelev <aam@google.com> Commit-Queue: Martin Kustermann <kustermann@google.com>
ra1u · Oct 11, 2021 · b7909e1 · b7909e1
1 parent 9c10d2b
commit b7909e1
Show file tree

Hide file tree

Showing 6 changed files with 46 additions and 56 deletions.
diff --git a/runtime/lib/isolate.cc b/runtime/lib/isolate.cc
@@ -205,11 +205,30 @@ static ObjectPtr ValidateMessageObject(Zone* zone,
       ObjectPtr raw = working_set.RemoveLast();
 
       const intptr_t cid = raw->GetClassId();
-      // Keep the list in sync with the one in message_snapshot.cc
+      // Keep the list in sync with the one in runtime/vm/object_graph_copy.cc
       switch (cid) {
+        // Can be shared.
+        case kOneByteStringCid:
+        case kTwoByteStringCid:
+        case kExternalOneByteStringCid:
+        case kExternalTwoByteStringCid:
+        case kMintCid:
+        case kImmutableArrayCid:
+        case kNeverCid:
+        case kSentinelCid:
+        case kInt32x4Cid:
+        case kSendPortCid:
+        case kCapabilityCid:
         case kRegExpCid:
-          // Can be shared, need to be explicitly listed to prevent inspection.
+        case kStackTraceCid:
           continue;
+        // Cannot be shared due to possibly being mutable boxes for unboxed
+        // fields in JIT, but can be transferred via Isolate.exit()
+        case kDoubleCid:
+        case kFloat32x4Cid:
+        case kFloat64x2Cid:
+          continue;
+
         case kClosureCid:
           closure ^= raw;
           // Only context has to be checked.
@@ -223,15 +242,12 @@ static ObjectPtr ValidateMessageObject(Zone* zone,
     error_found = true;                                                        \
     break;
 
-          MESSAGE_SNAPSHOT_ILLEGAL(FunctionType);
+          MESSAGE_SNAPSHOT_ILLEGAL(DynamicLibrary);
           MESSAGE_SNAPSHOT_ILLEGAL(MirrorReference);
+          MESSAGE_SNAPSHOT_ILLEGAL(Pointer);
           MESSAGE_SNAPSHOT_ILLEGAL(ReceivePort);
-          MESSAGE_SNAPSHOT_ILLEGAL(StackTrace);
           MESSAGE_SNAPSHOT_ILLEGAL(UserTag);
 
-          MESSAGE_SNAPSHOT_ILLEGAL(DynamicLibrary);
-          MESSAGE_SNAPSHOT_ILLEGAL(Pointer);
-
         default:
           if (cid >= kNumPredefinedCids) {
             klass = class_table->At(cid);

diff --git a/runtime/tests/vm/dart/isolates/fast_object_copy2_test.dart b/runtime/tests/vm/dart/isolates/fast_object_copy2_test.dart
@@ -76,6 +76,7 @@ final sharableObjects = [
   RegExp('a'),
   Isolate.current.pauseCapability,
   Int32x4(1, 2, 3, 4),
+  StackTrace.current,
 ];
 
 final copyableClosures = <dynamic>[

diff --git a/runtime/tests/vm/dart_2/isolates/fast_object_copy2_test.dart b/runtime/tests/vm/dart_2/isolates/fast_object_copy2_test.dart
@@ -77,6 +77,7 @@ final sharableObjects = [
   RegExp('a'),
   Isolate.current.pauseCapability,
   Int32x4(1, 2, 3, 4),
+  StackTrace.current,
 ];
 
 final copyableClosures = <dynamic>[

diff --git a/runtime/vm/object_graph_copy.cc b/runtime/vm/object_graph_copy.cc
@@ -136,6 +136,7 @@ static ObjectPtr Marker() {
   return Object::unknown_constant().ptr();
 }
 
+// Keep in sync with runtime/lib/isolate.cc:ValidateMessageObject
 DART_FORCE_INLINE
 static bool CanShareObject(ObjectPtr obj, uword tags) {
   if ((tags & UntaggedObject::CanonicalBit::mask_in_place()) != 0) {
@@ -150,6 +151,7 @@ static bool CanShareObject(ObjectPtr obj, uword tags) {
   if (cid == kImmutableArrayCid) return true;
   if (cid == kNeverCid) return true;
   if (cid == kSentinelCid) return true;
+  if (cid == kStackTraceCid) return true;
 #if defined(DART_PRECOMPILED_RUNTIME)
   // In JIT mode we have field guards enabled which means
   // double/float32x4/float64x2 boxes can be mutable and we therefore cannot
@@ -590,11 +592,9 @@ class ObjectCopyBase {
       // those are the only non-abstract classes (so we avoid checking more cids
       // here that cannot happen in reality)
       HANDLE_ILLEGAL_CASE(DynamicLibrary)
-      HANDLE_ILLEGAL_CASE(Pointer)
-      HANDLE_ILLEGAL_CASE(FunctionType)
       HANDLE_ILLEGAL_CASE(MirrorReference)
+      HANDLE_ILLEGAL_CASE(Pointer)
       HANDLE_ILLEGAL_CASE(ReceivePort)
-      HANDLE_ILLEGAL_CASE(StackTrace)
       HANDLE_ILLEGAL_CASE(UserTag)
       default:
         return true;

diff --git a/tests/lib/isolate/stacktrace_message_test.dart b/tests/lib/isolate/stacktrace_message_test.dart
@@ -3,40 +3,26 @@
 // BSD-style license that can be found in the LICENSE file.
 
 import 'dart:isolate';
+
 import "package:expect/expect.dart";
-import "package:async_helper/async_helper.dart";
 
 // Test that StackTrace objects can be sent between isolates spawned from
 // the same isolate using Isolate.spawn.
 
-void main() {
-  asyncStart();
-  ReceivePort reply = new ReceivePort();
+void main() async {
+  final reply = ReceivePort();
   Isolate.spawn(runTest, reply.sendPort);
-  reply.first.then((pair) {
-    StackTrace? stack = pair[0];
-    String stackString = pair[1];
-    if (stack == null) {
-      print("Failed to send stack-trace");
-      print(stackString);
-      Expect.fail("Sending stack-trace");
-    }
-    Expect.equals(stackString, "!$stack");
-    print(stack);
-    asyncEnd();
-  });
+  final pair = await reply.first;
+  final stack = pair[0] as StackTrace;
+  final stackString = pair[1] as String;
+  Expect.isNotNull(stack);
+  Expect.equals(stackString, "$stack");
 }
 
 runTest(SendPort sendport) {
   try {
     throw 'sorry';
   } catch (e, stack) {
-    try {
-      sendport.send([stack, "$stack"]);
-      print("Stacktrace sent");
-    } catch (e, s) {
-      print("Stacktrace not sent");
-      sendport.send([null, "$e\n$s"]);
-    }
+    sendport.send([stack, "$stack"]);
   }
 }
diff --git a/tests/lib_2/isolate/stacktrace_message_test.dart b/tests/lib_2/isolate/stacktrace_message_test.dart
@@ -5,40 +5,26 @@
 // @dart = 2.9
 
 import 'dart:isolate';
+
 import "package:expect/expect.dart";
-import "package:async_helper/async_helper.dart";
 
 // Test that StackTrace objects can be sent between isolates spawned from
 // the same isolate using Isolate.spawn.
 
-void main() {
-  asyncStart();
-  ReceivePort reply = new ReceivePort();
+void main() async {
+  final reply = ReceivePort();
   Isolate.spawn(runTest, reply.sendPort);
-  reply.first.then((pair) {
-    StackTrace stack = pair[0];
-    String stackString = pair[1];
-    if (stack == null) {
-      print("Failed to send stack-trace");
-      print(stackString);
-      Expect.fail("Sending stack-trace");
-    }
-    Expect.equals(stackString, "!$stack");
-    print(stack);
-    asyncEnd();
-  });
+  final pair = await reply.first;
+  final stack = pair[0] as StackTrace;
+  final stackString = pair[1] as String;
+  Expect.isNotNull(stack);
+  Expect.equals(stackString, "$stack");
 }
 
 runTest(SendPort sendport) {
   try {
     throw 'sorry';
   } catch (e, stack) {
-    try {
-      sendport.send([stack, "$stack"]);
-      print("Stacktrace sent");
-    } catch (e, s) {
-      print("Stacktrace not sent");
-      sendport.send([null, "$e\n$s"]);
-    }
+    sendport.send([stack, "$stack"]);
   }
 }