From b759d096cb0f428534bfee21eb2a242e8f9ce358 Mon Sep 17 00:00:00 2001 From: Alexander Aprelev Date: Wed, 13 Aug 2025 08:46:24 -0700 Subject: [PATCH] [vm/shared] Enforce pragma('vm:shared') annotations for captured local vars. Fixes https://github.com/dart-lang/sdk/issues/61287 TEST=ffi/isolate_group_bound_captured_local_test Change-Id: I9dc1e8aaf9e99d8ec9ad730cf1cd89ae3b4ed148 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/444923 Commit-Queue: Alexander Aprelev Reviewed-by: Ryan Macnak --- runtime/vm/ffi_callback_metadata.cc | 10 +++ runtime/vm/object.cc | 34 ++++++++++ runtime/vm/object.h | 6 ++ runtime/vm/raw_object.h | 8 ++- runtime/vm/scopes.cc | 28 +++++++++ runtime/vm/scopes.h | 17 ++++- runtime/vm/symbols.h | 1 + ...olate_group_bound_captured_local_test.dart | 63 +++++++++++++++++++ tests/ffi/isolate_group_bound_init_test.dart | 8 ++- tests/ffi/run_isolate_group_run_test.dart | 1 + 10 files changed, 171 insertions(+), 5 deletions(-) create mode 100644 tests/ffi/isolate_group_bound_captured_local_test.dart diff --git a/runtime/vm/ffi_callback_metadata.cc b/runtime/vm/ffi_callback_metadata.cc index b85ca6ae3de..06f9036f033 100644 --- a/runtime/vm/ffi_callback_metadata.cc +++ b/runtime/vm/ffi_callback_metadata.cc @@ -398,6 +398,16 @@ void FfiCallbackMetadata::EnsureOnlyTriviallyImmutableValuesInClosure( for (intptr_t i = 0; i < context.num_variables(); i++) { ValidateTriviallyImmutabilityOfAnObject(zone, &obj, context.At(i)); } + + if (!function.does_close_over_only_final_and_shared_vars()) { + const String& error = String::Handle( + zone, + String::New( + "Only final and 'vm:shared' variables can be captured by isolate " + "group callbacks.")); + Exceptions::ThrowArgumentError(error); + UNREACHABLE(); + } } } diff --git a/runtime/vm/object.cc b/runtime/vm/object.cc index f0c3a534c08..fbb77833494 100644 --- a/runtime/vm/object.cc +++ b/runtime/vm/object.cc @@ -8586,6 +8586,27 @@ void Function::set_awaiter_link(Function::AwaiterLink link) const { UNREACHABLE(); } +bool Function::does_close_over_only_final_and_shared_vars() const { + if (IsClosureFunction()) { + const Object& obj = Object::Handle(untag()->data()); + ASSERT(!obj.IsNull()); + return ClosureData::Cast(obj).does_close_over_only_final_and_shared_vars(); + } + UNREACHABLE(); +} + +void Function::set_does_close_over_only_final_and_shared_vars( + bool value) const { + if (IsClosureFunction()) { + const Object& obj = Object::Handle(untag()->data()); + ASSERT(!obj.IsNull()); + ClosureData::Cast(obj).set_does_close_over_only_final_and_shared_vars( + value); + return; + } + UNREACHABLE(); +} + ClosurePtr Function::implicit_static_closure() const { if (IsImplicitStaticClosureFunction()) { const Object& obj = Object::Handle(untag()->data()); @@ -12128,6 +12149,19 @@ void ClosureData::set_awaiter_link(Function::AwaiterLink link) const { link.index); } +bool ClosureData::does_close_over_only_final_and_shared_vars() const { + return untag() + ->packed_fields_ + .Read(); +} + +void ClosureData::set_does_close_over_only_final_and_shared_vars( + bool value) const { + untag() + ->packed_fields_ + .Update(value); +} + ClosureDataPtr ClosureData::New() { ASSERT(Object::closure_data_class() != Class::null()); return Object::Allocate(Heap::kOld); diff --git a/runtime/vm/object.h b/runtime/vm/object.h index a5840b3307f..e9c78f2443f 100644 --- a/runtime/vm/object.h +++ b/runtime/vm/object.h @@ -3302,6 +3302,9 @@ class Function : public Object { (awaiter_link().depth != UntaggedClosureData::kNoAwaiterLinkDepth); } + void set_does_close_over_only_final_and_shared_vars(bool value) const; + bool does_close_over_only_final_and_shared_vars() const; + // Enclosing function of this local function. FunctionPtr parent_function() const; @@ -4393,6 +4396,9 @@ class ClosureData : public Object { Function::AwaiterLink awaiter_link() const; void set_awaiter_link(Function::AwaiterLink link) const; + bool does_close_over_only_final_and_shared_vars() const; + void set_does_close_over_only_final_and_shared_vars(bool value) const; + // Enclosing function of this local function. PRECOMPILER_WSR_FIELD_DECLARATION(Function, parent_function) diff --git a/runtime/vm/raw_object.h b/runtime/vm/raw_object.h index bd80dd6a407..78c7e0f72be 100644 --- a/runtime/vm/raw_object.h +++ b/runtime/vm/raw_object.h @@ -1521,7 +1521,10 @@ class UntaggedClosureData : public UntaggedObject { using PackedAwaiterLinkIndex = BitField; - + using DoesCloseOverOnlySharedFields = + BitField; friend class Function; friend class UnitDeserializationRoots; }; @@ -2470,7 +2473,8 @@ class UntaggedContext : public UntaggedObject { V(Late) \ V(Nullable) \ V(Invisible) \ - V(AwaiterLink) + V(AwaiterLink) \ + V(Shared) class UntaggedContextScope : public UntaggedObject { RAW_HEAP_OBJECT_IMPLEMENTATION(ContextScope); diff --git a/runtime/vm/scopes.cc b/runtime/vm/scopes.cc index f0fe0df70a0..3c6ab3322f5 100644 --- a/runtime/vm/scopes.cc +++ b/runtime/vm/scopes.cc @@ -462,6 +462,8 @@ ContextScopePtr LocalScope::PreserveOuterScope( LocalVariable* awaiter_link = nullptr; + bool does_capture_only_final_and_shared_vars = true; + // Create a descriptor for each referenced captured variable of enclosing // functions to preserve its name and its context allocation information. int captured_idx = 0; @@ -500,6 +502,13 @@ ContextScopePtr LocalScope::PreserveOuterScope( if (is_awaiter_link) { awaiter_link = variable; } + + bool is_shared = variable->ComputeIfShared(library); + context_scope.SetIsSharedAt(captured_idx, is_shared); + if (!is_shared && !variable->is_final()) { + does_capture_only_final_and_shared_vars = false; + } + captured_idx++; } } @@ -523,6 +532,11 @@ ContextScopePtr LocalScope::PreserveOuterScope( } } + if (!function.IsNull()) { + function.set_does_close_over_only_final_and_shared_vars( + does_capture_only_final_and_shared_vars); + } + return context_scope.ptr(); } @@ -542,6 +556,7 @@ LocalScope* LocalScope::RestoreOuterScope(const ContextScope& context_scope) { String::ZoneHandle(context_scope.NameAt(i)), static_type, context_scope.KernelOffsetAt(i), inferred_type); variable->set_is_awaiter_link(context_scope.IsAwaiterLinkAt(i)); + variable->set_is_shared(context_scope.IsSharedAt(i)); variable->set_is_captured(); variable->set_index(VariableIndex(context_scope.ContextIndexAt(i))); if (context_scope.IsFinalAt(i)) { @@ -693,6 +708,19 @@ bool LocalVariable::ComputeIfIsAwaiterLink(const Library& library) { return is_awaiter_link_ == IsAwaiterLink::kLink; } +bool LocalVariable::ComputeIfShared(const Library& library) { + if (is_shared_ == IsShared::kUnknown) { + RELEASE_ASSERT(annotations_offset_ != kNoKernelOffset); + Thread* T = Thread::Current(); + Zone* Z = T->zone(); + const auto& metadata = Object::Handle( + Z, kernel::EvaluateMetadata(library, annotations_offset_, + /* is_annotations_offset = */ true)); + set_is_shared(FindPragmaInMetadata(T, metadata, Symbols::vm_shared())); + } + return is_shared_ == IsShared::kShared; +} + bool LocalVariable::Equals(const LocalVariable& other) const { if (HasIndex() && other.HasIndex() && (index() == other.index())) { if (is_captured() == other.is_captured()) { diff --git a/runtime/vm/scopes.h b/runtime/vm/scopes.h index 4e3d9d53089..996375b87fa 100644 --- a/runtime/vm/scopes.h +++ b/runtime/vm/scopes.h @@ -104,7 +104,8 @@ class LocalVariable : public ZoneAllocated { late_init_offset_(0), type_check_mode_(kDoTypeCheck), index_(), - is_awaiter_link_(IsAwaiterLink::kNotLink) { + is_awaiter_link_(IsAwaiterLink::kNotLink), + is_shared_(IsShared::kNotShared) { DEBUG_ASSERT(static_type.IsNotTemporaryScopedHandle()); ASSERT(static_type.IsFinalized()); ASSERT(inferred_type != nullptr); @@ -129,6 +130,8 @@ class LocalVariable : public ZoneAllocated { annotations_offset_ = offset; is_awaiter_link_ = (offset == kNoKernelOffset) ? IsAwaiterLink::kNotLink : IsAwaiterLink::kUnknown; + is_shared_ = + (offset == kNoKernelOffset) ? IsShared::kNotShared : IsShared::kUnknown; } const AbstractType& static_type() const { return static_type_; } @@ -151,6 +154,11 @@ class LocalVariable : public ZoneAllocated { bool is_late() const { return IsLateBit::decode(bitfield_); } void set_is_late() { bitfield_ = IsLateBit::update(true, bitfield_); } + bool ComputeIfShared(const Library& library); + void set_is_shared(bool value) { + is_shared_ = value ? IsShared::kShared : IsShared::kNotShared; + } + intptr_t late_init_offset() const { return late_init_offset_; } void set_late_init_offset(intptr_t late_init_offset) { late_init_offset_ = late_init_offset; @@ -264,6 +272,13 @@ class LocalVariable : public ZoneAllocated { }; IsAwaiterLink is_awaiter_link_; + enum class IsShared { + kUnknown, + kNotShared, + kShared, + }; + IsShared is_shared_; + friend class LocalScope; DISALLOW_COPY_AND_ASSIGN(LocalVariable); }; diff --git a/runtime/vm/symbols.h b/runtime/vm/symbols.h index e1a6bc8002d..3b0e0c6e249 100644 --- a/runtime/vm/symbols.h +++ b/runtime/vm/symbols.h @@ -577,6 +577,7 @@ class ObjectPointerVisitor; V(vm_notify_debugger_on_exception, "vm:notify-debugger-on-exception") \ V(vm_prefer_inline, "vm:prefer-inline") \ V(vm_recognized, "vm:recognized") \ + V(vm_shared, "vm:shared") \ V(vm_testing_print_flow_graph, "vm:testing:print-flow-graph") \ V(vm_trace_entrypoints, "vm:testing.unsafe.trace-entrypoints-fn") \ V(vm_unsafe_no_interrupts, "vm:unsafe:no-interrupts") \ diff --git a/tests/ffi/isolate_group_bound_captured_local_test.dart b/tests/ffi/isolate_group_bound_captured_local_test.dart new file mode 100644 index 00000000000..8113aee26c5 --- /dev/null +++ b/tests/ffi/isolate_group_bound_captured_local_test.dart @@ -0,0 +1,63 @@ +// Copyright (c) 2025, the Dart project authors. Please see the AUTHORS file +// for details. All rights reserved. Use of this source code is governed by a +// BSD-style license that can be found in the LICENSE file. + +// Test that exception is thrown when captured variables doesn't have +// pragma('vm:shared') annotation. +// +// VMOptions=--experimental-shared-data + +import 'dart:async'; +import 'dart:ffi'; +import 'dart:isolate'; + +import 'package:dart_internal/isolate_group.dart' show IsolateGroup; + +import "package:expect/async_helper.dart"; +import "package:expect/expect.dart"; + +typedef CallbackNativeType = Void Function(Int64, Int32); + +Future testCapturedLocalVarNoDecoration() async { + int foo_result = 42; + Expect.throws(() { + final callback = NativeCallable.isolateGroupBound(( + int a, + int b, + ) { + foo_result += (a * b); + }); + }, (e) => e.toString().contains("variables can be captured")); +} + +Future testCapturedLocalVarPragmaVmShared() async { + @pragma('vm:shared') + int foo_result = 42; + final callback = NativeCallable.isolateGroupBound(( + int a, + int b, + ) { + foo_result += (a * b); + }); + callback.close(); +} + +Future testCapturedLocalVarFinal() async { + final int foo_result = 42; + final callback = NativeCallable.isolateGroupBound(( + int a, + int b, + ) { + foo_result + (a * b); + }); + callback.close(); +} + +main(args, message) async { + asyncStart(); + await testCapturedLocalVarNoDecoration(); + await testCapturedLocalVarPragmaVmShared(); + await testCapturedLocalVarFinal(); + asyncEnd(); + print("All tests completed :)"); +} diff --git a/tests/ffi/isolate_group_bound_init_test.dart b/tests/ffi/isolate_group_bound_init_test.dart index b9a0f0fa18d..28fe475244a 100644 --- a/tests/ffi/isolate_group_bound_init_test.dart +++ b/tests/ffi/isolate_group_bound_init_test.dart @@ -62,8 +62,10 @@ testInitStrings() async { for (int i = 0; i < nWorkers; i++) { Isolate.spawn( (sendPort) { + @pragma('vm:shared') + final sp = sendPort; IsolateGroup.runSync(() { - sendPort.send(shared_late_final_string); + sp.send(shared_late_final_string); }); }, rp.sendPort, @@ -100,9 +102,11 @@ testInitThrows() async { for (int i = 0; i < nWorkers; i++) { Isolate.spawn( (sendPort) { + @pragma('vm:shared') + final sp = sendPort; IsolateGroup.runSync(() { try { - sendPort.send(shared_late_final_throw); + sp.send(shared_late_final_throw); } catch (e) { Expect.equals("165", e); rethrow; diff --git a/tests/ffi/run_isolate_group_run_test.dart b/tests/ffi/run_isolate_group_run_test.dart index da3a520a6ec..19242d64dbd 100644 --- a/tests/ffi/run_isolate_group_run_test.dart +++ b/tests/ffi/run_isolate_group_run_test.dart @@ -54,6 +54,7 @@ String string_foo = ""; SendPort? sp; StringMethodTearoffTest() { + @pragma('vm:shared') final stringTearoff = "abc".toString; IsolateGroup.runSync(() { stringTearoff;