Skip to content

Commit a5936e4

Browse files
aamCommit Queue
authored and
Commit Queue
committed
[vm] Include field and local names in isolate invalid object retaining path message.
Retrieval of local names only works in JIT as it relies on availability of LocalVarDescriptors. For example, for this ``` import 'dart:isolate'; foo() { final x = RawReceivePort(); RawReceivePort().sendPort.send(() => print("$x")); } main() { foo(); } ``` error message says ``` Unhandled exception: Invalid argument(s): Illegal argument in isolate message: (object is a ReceivePort) <- field x in foo.<anonymous closure> (from file:///usr/local/google/home/aam/p/d/dart-sdk1/sdk/s1.dart) ``` Bug: #52957 TEST=ci Change-Id: I684014fd19ea82829e7df17a8a67d386551a5a82 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/314501 Reviewed-by: Ryan Macnak <[email protected]> Commit-Queue: Alexander Aprelev <[email protected]>
1 parent 2cac4ab commit a5936e4

5 files changed

+200
-28
lines changed

runtime/tests/vm/dart/isolates/send_unsupported_objects_deep_test.dart

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,11 +31,12 @@ main() async {
3131
print('$nestedList');
3232
});
3333
} catch (e) {
34+
print(e);
3435
Expect.isTrue(checkForRetainingPath(e, <String>[
3536
'NativeClass',
3637
'Baz',
3738
'Fu',
38-
'Closure',
39+
'closure',
3940
]));
4041

4142
final msg = e.toString();

runtime/tests/vm/dart/isolates/send_unsupported_objects_init_isolate_test.dart

Lines changed: 24 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,8 @@ import 'dart:isolate';
88
import 'package:async_helper/async_helper.dart';
99
import 'package:expect/expect.dart';
1010

11+
import '../use_flag_test_helper.dart';
12+
1113
import 'send_unsupported_objects_test.dart';
1214

1315
main() async {
@@ -16,18 +18,32 @@ main() async {
1618
final fu = Fu.unsendable('fu');
1719
try {
1820
// Pass a closure that captures [fu]
19-
await Isolate.spawn((arg) {
20-
arg();
21-
}, () {
22-
print('${fu.label}');
23-
Expect.fail('This closure should fail to be sent, shouldn\'t be called');
24-
});
21+
await () async {
22+
final foo1 = Foo();
23+
await () async {
24+
final foo2 = Foo();
25+
await () async {
26+
final foo3 = Foo();
27+
await Isolate.spawn((arg) {
28+
arg();
29+
}, () {
30+
print('${fu.label} $foo1 $foo2 $foo3');
31+
Expect.fail('This closure should fail to be sent, '
32+
'shouldn\'t be called');
33+
});
34+
}();
35+
}();
36+
}();
2537
} catch (e) {
2638
Expect.isTrue(checkForRetainingPath(e, <String>[
2739
'Baz',
2840
'Fu',
29-
'Context',
30-
'Closure',
41+
if (isAOTRuntime) ...[
42+
'Context',
43+
'main.<anonymous closure>'
44+
] else ...[
45+
'field fu in main.<anonymous closure>'
46+
],
3147
]));
3248
asyncEnd();
3349
}

runtime/tests/vm/dart_2/isolates/send_unsupported_objects_deep_test.dart

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,7 @@ main() async {
3737
'NativeClass',
3838
'Baz',
3939
'Fu',
40-
'Closure',
40+
'closure',
4141
]));
4242

4343
final msg = e.toString();

runtime/tests/vm/dart_2/isolates/send_unsupported_objects_init_isolate_test.dart

Lines changed: 27 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -10,26 +10,42 @@ import 'dart:isolate';
1010
import 'package:async_helper/async_helper.dart';
1111
import 'package:expect/expect.dart';
1212

13+
import '../use_flag_test_helper.dart';
14+
1315
import 'send_unsupported_objects_test.dart';
1416

1517
main() async {
1618
asyncStart();
1719

1820
final fu = Fu.unsendable('fu');
1921
try {
20-
// Pass a closure that captures [fu]
21-
await Isolate.spawn((arg) {
22-
arg();
23-
}, () {
24-
print('${fu.label}');
25-
Expect.fail('This closure should fail to be sent, shouldn\'t be called');
26-
});
22+
await () async {
23+
final foo1 = Foo();
24+
await () async {
25+
final foo2 = Foo();
26+
await () async {
27+
final foo3 = Foo();
28+
await Isolate.spawn((arg) {
29+
arg();
30+
}, () {
31+
print('${fu.label} $foo1 $foo2 $foo3');
32+
Expect.fail('This closure should fail to be sent, '
33+
'shouldn\'t be called');
34+
});
35+
}();
36+
}();
37+
}();
2738
} catch (e) {
39+
print(e);
2840
Expect.isTrue(checkForRetainingPath(e, <String>[
29-
'Baz',
30-
'Fu',
31-
'Context',
32-
'Closure',
41+
'nativeClass in Instance of \'Baz\' ',
42+
'baz in Instance of \'Fu\'',
43+
if (isAOTRuntime) ...[
44+
'Context',
45+
'main.<anonymous closure>'
46+
] else ...[
47+
'field fu in main.<anonymous closure>'
48+
],
3349
]));
3450
asyncEnd();
3551
}

runtime/vm/object_graph_copy.cc

Lines changed: 146 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1113,32 +1113,171 @@ class RetainingPath {
11131113
const Object& to_;
11141114
TraversalRules traversal_rules_;
11151115

1116+
class FindObjectVisitor : public ObjectPointerVisitor {
1117+
public:
1118+
FindObjectVisitor(IsolateGroup* isolate_group, ObjectPtr target)
1119+
: ObjectPointerVisitor(isolate_group), target_(target), index_(0) {}
1120+
1121+
void VisitPointers(ObjectPtr* from, ObjectPtr* to) override {
1122+
for (ObjectPtr* ptr = from; ptr <= to; ptr++, index_++) {
1123+
if (*ptr == target_) {
1124+
break;
1125+
}
1126+
}
1127+
}
1128+
1129+
#if defined(DART_COMPRESSED_POINTERS)
1130+
void VisitCompressedPointers(uword heap_base,
1131+
CompressedObjectPtr* from,
1132+
CompressedObjectPtr* to) override {
1133+
for (ObjectPtr* ptr = from; ptr <= to; ptr++, index_++) {
1134+
if (ptr->Decompress(heap_base) == target_) {
1135+
break;
1136+
}
1137+
}
1138+
}
1139+
#endif
1140+
1141+
intptr_t index() { return index_; }
1142+
1143+
private:
1144+
ObjectPtr target_;
1145+
intptr_t index_;
1146+
};
1147+
11161148
const char* CollectPath(MallocGrowableArray<ObjectPtr>* const working_list) {
1149+
Object& previous_object = Object::Handle(zone_);
11171150
Object& object = Object::Handle(zone_);
1151+
Field& field = Field::Handle(zone_);
11181152
Class& klass = Class::Handle(zone_);
11191153
Library& library = Library::Handle(zone_);
11201154
String& library_url = String::Handle(zone_);
1155+
Context& context = Context::Handle(zone_);
1156+
Closure& closure = Closure::Handle(zone_);
1157+
Function& function = Function::Handle(zone_);
1158+
#if !defined(DART_PRECOMPILED_RUNTIME)
1159+
Code& code = Code::Handle(zone_);
1160+
LocalVarDescriptors& var_descriptors = LocalVarDescriptors::Handle(zone_);
1161+
String& name = String::Handle(zone_);
1162+
#endif
1163+
1164+
const char* saved_context_location = nullptr;
1165+
intptr_t saved_context_object_index = -1;
1166+
intptr_t saved_context_depth = 0;
11211167
const char* retaining_path = "";
11221168

11231169
ObjectPtr raw = to_.ptr();
1124-
// Skip all remaining children until null-separator, so we get the parent
11251170
do {
1171+
previous_object = raw;
1172+
// Skip all remaining children until null-separator, so we get the parent
11261173
do {
11271174
raw = working_list->RemoveLast();
11281175
} while (raw != Object::null() && raw != from_.ptr());
11291176
if (raw == Object::null()) {
11301177
raw = working_list->RemoveLast();
11311178
object = raw;
11321179
klass = object.clazz();
1133-
library = klass.library();
1134-
if (library.IsNull()) {
1135-
retaining_path = OS::SCreate(zone_, "%s <- %s\n", retaining_path,
1136-
object.ToCString());
1180+
1181+
const char* location = object.ToCString();
1182+
1183+
if (object.IsContext()) {
1184+
context ^= raw;
1185+
if (saved_context_object_index == -1) {
1186+
// If this is the first context, remember index of the
1187+
// [previous_object] in the Context.
1188+
// We will need it later if get to see the Closure next.
1189+
saved_context_depth = 0;
1190+
for (intptr_t i = 0; i < context.num_variables(); i++) {
1191+
if (context.At(i) == previous_object.ptr()) {
1192+
saved_context_object_index = i;
1193+
break;
1194+
}
1195+
}
1196+
} else {
1197+
// Keep track of context depths in case of nested contexts;
1198+
saved_context_depth++;
1199+
}
11371200
} else {
1201+
if (object.IsInstance()) {
1202+
if (object.IsClosure()) {
1203+
closure ^= raw;
1204+
function ^= closure.function();
1205+
// Use function's class when looking for a library information.
1206+
klass ^= function.Owner();
1207+
#if defined(DART_PRECOMPILED_RUNTIME)
1208+
// Use function's name instead of closure's.
1209+
location = function.QualifiedUserVisibleNameCString();
1210+
#else // defined(DART_PRECOMPILED_RUNTIME) \
1211+
// Attempt to convert "instance <- Context+ <- Closure" into \
1212+
// "instance <- local var name in Closure".
1213+
if (!function.ForceOptimize()) {
1214+
function.EnsureHasCompiledUnoptimizedCode();
1215+
}
1216+
code ^= function.unoptimized_code();
1217+
ASSERT(!code.IsNull());
1218+
var_descriptors ^= code.GetLocalVarDescriptors();
1219+
for (intptr_t i = 0; i < var_descriptors.Length(); i++) {
1220+
UntaggedLocalVarDescriptors::VarInfo info;
1221+
var_descriptors.GetInfo(i, &info);
1222+
if (info.scope_id == -saved_context_depth &&
1223+
info.kind() ==
1224+
UntaggedLocalVarDescriptors::VarInfoKind::kContextVar &&
1225+
info.index() == saved_context_object_index) {
1226+
name ^= var_descriptors.GetName(i);
1227+
location =
1228+
OS::SCreate(zone_, "field %s in %s", name.ToCString(),
1229+
function.QualifiedUserVisibleNameCString());
1230+
// Won't need saved context location after all.
1231+
saved_context_location = nullptr;
1232+
break;
1233+
}
1234+
}
1235+
#endif // defined(DART_PRECOMPILED_RUNTIME)
1236+
} else {
1237+
// Attempt to find field name for the field that holds the
1238+
// [previous_object] instance.
1239+
FindObjectVisitor visitor(isolate_->group(),
1240+
previous_object.ptr());
1241+
raw->untag()->VisitPointers(&visitor);
1242+
field ^= klass.FieldFromIndex(visitor.index());
1243+
if (!field.IsNull()) {
1244+
location =
1245+
OS::SCreate(zone_, "%s in %s",
1246+
field.UserVisibleNameCString(), location);
1247+
}
1248+
}
1249+
}
1250+
// Saved context object index stays up for only one cycle - just to
1251+
// accommodate short chains Closure -> Context -> instance.
1252+
saved_context_object_index = -1;
1253+
saved_context_depth = -1;
1254+
}
1255+
// Add library url to the location if library is available.
1256+
library = klass.library();
1257+
if (!library.IsNull()) {
11381258
library_url = library.url();
1259+
location = OS::SCreate(zone_, "%s (from %s)", location,
1260+
library_url.ToCString());
1261+
}
1262+
1263+
if (object.IsContext()) {
1264+
// Save context string placeholder in case we don't find closure next
1265+
if (saved_context_location == nullptr) {
1266+
saved_context_location = location;
1267+
} else {
1268+
// Append saved contexts
1269+
saved_context_location = OS::SCreate(
1270+
zone_, "%s <- %s\n", saved_context_location, location);
1271+
}
1272+
} else {
1273+
if (saved_context_location != nullptr) {
1274+
// Could not use saved context, insert it into retaining path now.
1275+
retaining_path = OS::SCreate(zone_, "%s <- %s", retaining_path,
1276+
saved_context_location);
1277+
saved_context_location = nullptr;
1278+
}
11391279
retaining_path =
1140-
OS::SCreate(zone_, "%s <- %s (from %s)\n", retaining_path,
1141-
object.ToCString(), library_url.ToCString());
1280+
OS::SCreate(zone_, "%s <- %s\n", retaining_path, location);
11421281
}
11431282
}
11441283
} while (raw != from_.ptr());

0 commit comments

Comments
 (0)