From af7c3900448c3f1e35477d66c30864b34b437f81 Mon Sep 17 00:00:00 2001 From: Alexander Aprelev Date: Thu, 22 Jan 2026 12:53:50 -0800 Subject: [PATCH] [vm/shared] Skip runtime deep-immutability check for statically known types. Fixes https://github.com/dart-lang/sdk/issues/61078 TEST=StreamingFlowGraphBuilder_DeeplyImmutableTypeCheck* Change-Id: I4116d5f51f3247d50d4e7fc0f87e04238a6e3151 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/474563 Reviewed-by: Alexander Markov Commit-Queue: Alexander Aprelev --- .../transformations/deeply_immutable.dart | 72 +++++++++++++-- ...eeply_immutable_runtime_check_il_test.dart | 85 ++++++++++++++++++ runtime/vm/compiler/backend/il_arm.cc | 47 +++++----- runtime/vm/compiler/backend/il_arm64.cc | 47 +++++----- runtime/vm/compiler/backend/il_ia32.cc | 51 ++++++----- runtime/vm/compiler/backend/il_riscv.cc | 47 +++++----- runtime/vm/compiler/backend/il_x64.cc | 51 ++++++----- .../frontend/kernel_binary_flowgraph.cc | 3 +- .../frontend/kernel_binary_flowgraph_test.cc | 88 +++++++++++++++++++ runtime/vm/kernel_loader.cc | 4 + runtime/vm/object.h | 11 +++ 11 files changed, 386 insertions(+), 120 deletions(-) create mode 100644 runtime/tests/vm/dart/no_deeply_immutable_runtime_check_il_test.dart diff --git a/pkg/vm/lib/modular/transformations/deeply_immutable.dart b/pkg/vm/lib/modular/transformations/deeply_immutable.dart index 4ada99b658d..d6f52e5bd5e 100644 --- a/pkg/vm/lib/modular/transformations/deeply_immutable.dart +++ b/pkg/vm/lib/modular/transformations/deeply_immutable.dart @@ -36,9 +36,25 @@ void validateLibraries( } } +class _CheckResult { + final bool isImmutable; + final bool requiresRuntimeCheck; + + const _CheckResult({ + this.isImmutable = false, + this.requiresRuntimeCheck = false, + }); +} + /// Implements the `vm:deeply-immutable` semantics. class DeeplyImmutableValidator { static const vmDeeplyImmutable = "vm:deeply-immutable"; + late final InstanceConstant vmDeeplyImmutableConstant = + InstanceConstant(coreTypes.pragmaClass.reference, [], { + coreTypes.pragmaName.fieldReference: StringConstant(vmDeeplyImmutable), + coreTypes.pragmaOptions.fieldReference: NullConstant(), + }); + static const vmShared = "vm:shared"; final CoreTypes coreTypes; final DiagnosticReporter diagnosticReporter; @@ -67,12 +83,32 @@ class DeeplyImmutableValidator { for (final cls in library.classes) { visitClass(cls); } + for (final field in library.fields) { + if (_isVmSharedField(field)) { + addDeeplyImmutableAnnotationIfNeeded(field); + } + } } void visitClass(Class node) { _validateDeeplyImmutable(node); } + // pragma("vm:deeply-immutable") on a field indicates that the field static + // type guarantees that it always have deeply-immutable value, therefore + // at a runtime there is no need to check the value being assigned to the + // field. + // This pragma is added only for "vm:shared" static fields and to all fields + // of "vm:deeply-immutable" class because those are the only ones that are + // sensitive to having deeply-immutable values in them. + _CheckResult addDeeplyImmutableAnnotationIfNeeded(Field field) { + final checkResult = _isDeeplyImmutableDartType(field.type); + if (checkResult.isImmutable && !checkResult.requiresRuntimeCheck) { + field.addAnnotation(ConstantExpression(vmDeeplyImmutableConstant)); + } + return checkResult; + } + bool _isOrExtendsNativeFieldWrapper1Class(Class? node) { while (node != null && node != nativeFieldWrapperClass1Class) { node = node.superclass; @@ -99,6 +135,11 @@ class DeeplyImmutableValidator { ); } } + for (final field in node.fields) { + if (field.isStatic && _isVmSharedField(field)) { + addDeeplyImmutableAnnotationIfNeeded(field); + } + } return; } @@ -141,11 +182,12 @@ class DeeplyImmutableValidator { // All instance fields should be non-late final and deeply immutable. for (final field in node.fields) { + final checkResult = addDeeplyImmutableAnnotationIfNeeded(field); if (field.isStatic) { // Static fields are not part of instances. continue; } - if (!_isDeeplyImmutableDartType(field.type)) { + if (!checkResult.isImmutable) { diagnosticReporter.report( codeFfiDeeplyImmutableFieldsMustBeDeeplyImmutable, field.fileOffset, @@ -164,13 +206,16 @@ class DeeplyImmutableValidator { } } - bool _isDeeplyImmutableDartType(DartType dartType) { + _CheckResult _isDeeplyImmutableDartType(DartType dartType) { if (dartType is NullType) { - return true; + return _CheckResult(isImmutable: true, requiresRuntimeCheck: false); } if (dartType is InterfaceType) { final classNode = dartType.classNode; - return _isDeeplyImmutableClass(classNode); + return _CheckResult( + isImmutable: _isDeeplyImmutableClass(classNode), + requiresRuntimeCheck: false, + ); } if (dartType is TypeParameterType) { return _isDeeplyImmutableDartType(dartType.bound); @@ -178,9 +223,9 @@ class DeeplyImmutableValidator { if (dartType is FunctionType) { // Relies on dynamic check of whether closure actually captures only // deeply-immutable values. - return true; + return _CheckResult(isImmutable: true, requiresRuntimeCheck: true); } - return false; + return _CheckResult(isImmutable: false, requiresRuntimeCheck: false); } bool _isDeeplyImmutableClass(Class node) { @@ -197,4 +242,19 @@ class DeeplyImmutableValidator { } return false; } + + bool _isVmSharedField(Field node) { + for (final annotation in node.annotations) { + if (annotation is ConstantExpression) { + final constant = annotation.constant; + if (constant is InstanceConstant && + constant.classNode == pragmaClass && + constant.fieldValues[pragmaName.fieldReference] == + StringConstant(vmShared)) { + return true; + } + } + } + return false; + } } diff --git a/runtime/tests/vm/dart/no_deeply_immutable_runtime_check_il_test.dart b/runtime/tests/vm/dart/no_deeply_immutable_runtime_check_il_test.dart new file mode 100644 index 00000000000..c7be4051ca9 --- /dev/null +++ b/runtime/tests/vm/dart/no_deeply_immutable_runtime_check_il_test.dart @@ -0,0 +1,85 @@ +// Copyright (c) 2026, 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. + +// Verifies that deeply-immutable runtme type check is omitted when types +// are statically known. + +import 'package:expect/expect.dart'; +import 'package:vm/testing/il_matchers.dart'; + +@pragma('vm:deeply-immutable') +final class FunctionFoo { + final void Function(int) baz; + FunctionFoo(this.baz); +} + +@pragma('vm:never-inline') +@pragma('vm:testing:print-flow-graph') +runtime_check_is_present() { + final ff = FunctionFoo((int x) { + print(x); + }); + ff.baz(42); +} + +@pragma('vm:deeply-immutable') +final class IntFoo { + final int baz; + IntFoo(this.baz); +} + +@pragma('vm:never-inline') +@pragma('vm:testing:print-flow-graph') +runtime_check_is_omitted() { + IntFoo(42); +} + +void main() { + runtime_check_is_present(); + runtime_check_is_omitted(); +} + +void extractAllInstructions(dynamic data, List into) { + if (data is Map) { + for (var entry in data.entries) { + if (entry.key == "o" && entry.value is String) { + into.add(entry.value); + } else { + extractAllInstructions(entry.value, into); + } + } + } else if (data is List) { + for (var entry in data) { + extractAllInstructions(entry, into); + } + } else { + if (data is int || data is String) { + // ok + } else { + print("Notice: Unhandled data: ${data.runtimeType}: $data"); + } + } +} + +bool hasCheckFieldImmutability(FlowGraph graph) { + List ils = []; + extractAllInstructions(graph.blocks(), ils); + for (String il in ils) { + if (il == "CheckFieldImmutability") return true; + } + return false; +} + +void matchIL$runtime_check_is_present(FlowGraph graph) { + Expect.isTrue( + hasCheckFieldImmutability(graph), + "should have immutability checks", + ); +} + +void matchIL$runtime_check_is_omitted(FlowGraph graph) { + Expect.isFalse( + hasCheckFieldImmutability(graph), + "should not have immutability checks", + ); +} diff --git a/runtime/vm/compiler/backend/il_arm.cc b/runtime/vm/compiler/backend/il_arm.cc index e0863d6ff2f..83c05e86c7d 100644 --- a/runtime/vm/compiler/backend/il_arm.cc +++ b/runtime/vm/compiler/backend/il_arm.cc @@ -2933,8 +2933,10 @@ void LoadCodeUnitsInstr::EmitNativeCode(FlowGraphCompiler* compiler) { LocationSummary* StoreStaticFieldInstr::MakeLocationSummary(Zone* zone, bool opt) const { const intptr_t kNumInputs = 1; - const intptr_t kNumTemps = - FLAG_experimental_shared_data && field().is_shared() ? 2 : 1; + const bool need_extra_temp = FLAG_experimental_shared_data && + field().is_shared() && + !field().has_deeply_immutable_type(); + const intptr_t kNumTemps = need_extra_temp ? 2 : 1; const bool can_call_to_throw = FLAG_experimental_shared_data; LocationSummary* locs = new (zone) LocationSummary(zone, kNumInputs, kNumTemps, @@ -2943,7 +2945,7 @@ LocationSummary* StoreStaticFieldInstr::MakeLocationSummary(Zone* zone, locs->set_in( 0, Location::RegisterLocation(CheckedStoreIntoSharedStubABI::kValueReg)); locs->set_temp(0, Location::RequiresRegister()); - if (FLAG_experimental_shared_data && field().is_shared()) { + if (need_extra_temp) { locs->set_temp(1, Location::RegisterLocation( CheckedStoreIntoSharedStubABI::kFieldReg)); } @@ -2965,26 +2967,26 @@ void StoreStaticFieldInstr::EmitNativeCode(FlowGraphCompiler* compiler) { __ LoadIsolate(temp); __ BranchIfZero(temp, slow_path->entry_label()); } else { - // TODO(dartbug.com/61078): use field static type information to decide - // whether the following value check is needed or not. - checked_store_into_shared_slow_path = - new CheckedStoreIntoSharedSlowPath(this, value); - compiler->AddSlowPathCode(checked_store_into_shared_slow_path); + if (!field().has_deeply_immutable_type()) { + checked_store_into_shared_slow_path = + new CheckedStoreIntoSharedSlowPath(this, value); + compiler->AddSlowPathCode(checked_store_into_shared_slow_path); - compiler::Label allow_store; - __ BranchIfSmi(value, &allow_store, compiler::Assembler::kNearJump); - __ ldr(temp, compiler::FieldAddress( - value, compiler::target::Object::tags_offset())); - __ TestImmediate(temp, - 1 << compiler::target::UntaggedObject::kCanonicalBit); - // If canonical bit is set, no need for runtime check. - __ b(&allow_store, NOT_ZERO); - __ TestImmediate( - temp, 1 << compiler::target::UntaggedObject::kDeeplyImmutableBit); - // If immutability bit is not set, go to runtime. - __ b(checked_store_into_shared_slow_path->entry_label(), ZERO); + compiler::Label allow_store; + __ BranchIfSmi(value, &allow_store, compiler::Assembler::kNearJump); + __ ldr(temp, compiler::FieldAddress( + value, compiler::target::Object::tags_offset())); + __ TestImmediate(temp, + 1 << compiler::target::UntaggedObject::kCanonicalBit); + // If canonical bit is set, no need for runtime check. + __ b(&allow_store, NOT_ZERO); + __ TestImmediate( + temp, 1 << compiler::target::UntaggedObject::kDeeplyImmutableBit); + // If immutability bit is not set, go to runtime. + __ b(checked_store_into_shared_slow_path->entry_label(), ZERO); - __ Bind(&allow_store); + __ Bind(&allow_store); + } } } @@ -3004,7 +3006,8 @@ void StoreStaticFieldInstr::EmitNativeCode(FlowGraphCompiler* compiler) { compiler::target::FieldTable::OffsetOf(field())); } - if (FLAG_experimental_shared_data && field().is_shared()) { + if (FLAG_experimental_shared_data && field().is_shared() && + !field().has_deeply_immutable_type()) { __ Bind(checked_store_into_shared_slow_path->exit_label()); } } diff --git a/runtime/vm/compiler/backend/il_arm64.cc b/runtime/vm/compiler/backend/il_arm64.cc index 4e14b6ed054..6202f68d481 100644 --- a/runtime/vm/compiler/backend/il_arm64.cc +++ b/runtime/vm/compiler/backend/il_arm64.cc @@ -2510,8 +2510,10 @@ void GuardFieldLengthInstr::EmitNativeCode(FlowGraphCompiler* compiler) { LocationSummary* StoreStaticFieldInstr::MakeLocationSummary(Zone* zone, bool opt) const { const intptr_t kNumInputs = 1; - const intptr_t kNumTemps = - FLAG_experimental_shared_data && field().is_shared() ? 2 : 1; + const bool need_extra_temp = FLAG_experimental_shared_data && + field().is_shared() && + !field().has_deeply_immutable_type(); + const intptr_t kNumTemps = need_extra_temp ? 2 : 1; const bool can_call_to_throw = FLAG_experimental_shared_data; LocationSummary* locs = new (zone) LocationSummary(zone, kNumInputs, kNumTemps, @@ -2520,7 +2522,7 @@ LocationSummary* StoreStaticFieldInstr::MakeLocationSummary(Zone* zone, locs->set_in( 0, Location::RegisterLocation(CheckedStoreIntoSharedStubABI::kValueReg)); locs->set_temp(0, Location::RequiresRegister()); - if (FLAG_experimental_shared_data && field().is_shared()) { + if (need_extra_temp) { locs->set_temp(1, Location::RegisterLocation( CheckedStoreIntoSharedStubABI::kFieldReg)); } @@ -2542,26 +2544,26 @@ void StoreStaticFieldInstr::EmitNativeCode(FlowGraphCompiler* compiler) { __ LoadIsolate(temp); __ BranchIfZero(temp, slow_path->entry_label()); } else { - // TODO(dartbug.com/61078): use field static type information to decide - // whether the following value check is needed or not. - checked_store_into_shared_slow_path = - new CheckedStoreIntoSharedSlowPath(this, value); - compiler->AddSlowPathCode(checked_store_into_shared_slow_path); + if (!field().has_deeply_immutable_type()) { + checked_store_into_shared_slow_path = + new CheckedStoreIntoSharedSlowPath(this, value); + compiler->AddSlowPathCode(checked_store_into_shared_slow_path); - compiler::Label allow_store; - __ BranchIfSmi(value, &allow_store, compiler::Assembler::kNearJump); - __ ldr(temp, - compiler::FieldAddress(value, - compiler::target::Object::tags_offset()), - compiler::kUnsignedByte); - // If canonical bit is set, no need for runtime check. - __ tbnz(&allow_store, temp, - compiler::target::UntaggedObject::kCanonicalBit); - // If immutability bit is not set, go to runtime. - __ tbz(checked_store_into_shared_slow_path->entry_label(), temp, - compiler::target::UntaggedObject::kDeeplyImmutableBit); + compiler::Label allow_store; + __ BranchIfSmi(value, &allow_store, compiler::Assembler::kNearJump); + __ ldr(temp, + compiler::FieldAddress(value, + compiler::target::Object::tags_offset()), + compiler::kUnsignedByte); + // If canonical bit is set, no need for runtime check. + __ tbnz(&allow_store, temp, + compiler::target::UntaggedObject::kCanonicalBit); + // If immutability bit is not set, go to runtime. + __ tbz(checked_store_into_shared_slow_path->entry_label(), temp, + compiler::target::UntaggedObject::kDeeplyImmutableBit); - __ Bind(&allow_store); + __ Bind(&allow_store); + } } } @@ -2580,7 +2582,8 @@ void StoreStaticFieldInstr::EmitNativeCode(FlowGraphCompiler* compiler) { compiler::target::FieldTable::OffsetOf(field())); } - if (FLAG_experimental_shared_data && field().is_shared()) { + if (FLAG_experimental_shared_data && field().is_shared() && + !field().has_deeply_immutable_type()) { __ Bind(checked_store_into_shared_slow_path->exit_label()); } } diff --git a/runtime/vm/compiler/backend/il_ia32.cc b/runtime/vm/compiler/backend/il_ia32.cc index 7f518593355..0082694e792 100644 --- a/runtime/vm/compiler/backend/il_ia32.cc +++ b/runtime/vm/compiler/backend/il_ia32.cc @@ -2139,8 +2139,10 @@ void CheckFieldImmutabilityInstr::EmitNativeCode(FlowGraphCompiler* compiler) { LocationSummary* StoreStaticFieldInstr::MakeLocationSummary(Zone* zone, bool opt) const { - const intptr_t kNumTemps = - FLAG_experimental_shared_data && field().is_shared() ? 2 : 1; + const bool need_extra_temp = FLAG_experimental_shared_data && + field().is_shared() && + !field().has_deeply_immutable_type(); + const intptr_t kNumTemps = need_extra_temp ? 2 : 1; const bool can_call_to_throw = FLAG_experimental_shared_data; LocationSummary* locs = new (zone) LocationSummary(zone, 1, kNumTemps, @@ -2149,7 +2151,7 @@ LocationSummary* StoreStaticFieldInstr::MakeLocationSummary(Zone* zone, locs->set_in(0, value()->NeedsWriteBarrier() ? Location::WritableRegister() : Location::RequiresRegister()); locs->set_temp(0, Location::RequiresRegister()); - if (FLAG_experimental_shared_data && field().is_shared()) { + if (need_extra_temp) { locs->set_temp(1, Location::RegisterLocation( CheckedStoreIntoSharedStubABI::kFieldReg)); } @@ -2177,28 +2179,30 @@ void StoreStaticFieldInstr::EmitNativeCode(FlowGraphCompiler* compiler) { __ LoadIsolate(temp); __ BranchIfZero(temp, slow_path->entry_label()); } else { - // TODO(dartbug.com/61078): use field static type information to decide - // whether the following value check is needed or not. - checked_store_into_shared_slow_path = - new CheckedStoreIntoSharedSlowPath(this, in); - compiler->AddSlowPathCode(checked_store_into_shared_slow_path); + if (!field().has_deeply_immutable_type()) { + checked_store_into_shared_slow_path = + new CheckedStoreIntoSharedSlowPath(this, in); + compiler->AddSlowPathCode(checked_store_into_shared_slow_path); - compiler::Label allow_store; - __ BranchIfSmi(in, &allow_store, compiler::Assembler::kNearJump); + compiler::Label allow_store; + __ BranchIfSmi(in, &allow_store, compiler::Assembler::kNearJump); - __ movl(temp, compiler::FieldAddress( - in, compiler::target::Object::tags_offset())); - __ testl(temp, compiler::Immediate( - 1 << compiler::target::UntaggedObject::kCanonicalBit)); - // If canonical bit is set, no need for runtime check. - __ j(NOT_ZERO, &allow_store); - __ testl(temp, - compiler::Immediate( - 1 << compiler::target::UntaggedObject::kDeeplyImmutableBit)); - // If immutability bit is not set, go to runtime. - __ j(ZERO, checked_store_into_shared_slow_path->entry_label()); + __ movl(temp, compiler::FieldAddress( + in, compiler::target::Object::tags_offset())); + __ testl(temp, + compiler::Immediate( + 1 << compiler::target::UntaggedObject::kCanonicalBit)); + // If canonical bit is set, no need for runtime check. + __ j(NOT_ZERO, &allow_store); + __ testl( + temp, + compiler::Immediate( + 1 << compiler::target::UntaggedObject::kDeeplyImmutableBit)); + // If immutability bit is not set, go to runtime. + __ j(ZERO, checked_store_into_shared_slow_path->entry_label()); - __ Bind(&allow_store); + __ Bind(&allow_store); + } } } @@ -2219,7 +2223,8 @@ void StoreStaticFieldInstr::EmitNativeCode(FlowGraphCompiler* compiler) { in); } - if (FLAG_experimental_shared_data && field().is_shared()) { + if (FLAG_experimental_shared_data && field().is_shared() && + !field().has_deeply_immutable_type()) { __ Bind(checked_store_into_shared_slow_path->exit_label()); } } diff --git a/runtime/vm/compiler/backend/il_riscv.cc b/runtime/vm/compiler/backend/il_riscv.cc index 01b61eba609..a7179b51a52 100644 --- a/runtime/vm/compiler/backend/il_riscv.cc +++ b/runtime/vm/compiler/backend/il_riscv.cc @@ -2742,8 +2742,9 @@ void GuardFieldLengthInstr::EmitNativeCode(FlowGraphCompiler* compiler) { LocationSummary* StoreStaticFieldInstr::MakeLocationSummary(Zone* zone, bool opt) const { const intptr_t kNumInputs = 1; - const intptr_t kNumTemps = - FLAG_experimental_shared_data && field().is_shared() ? 1 : 0; + const bool need_temp = FLAG_experimental_shared_data && field().is_shared() && + !field().has_deeply_immutable_type(); + const intptr_t kNumTemps = need_temp ? 1 : 0; const bool can_call_to_throw = FLAG_experimental_shared_data; LocationSummary* locs = new (zone) LocationSummary(zone, kNumInputs, kNumTemps, @@ -2751,7 +2752,7 @@ LocationSummary* StoreStaticFieldInstr::MakeLocationSummary(Zone* zone, : LocationSummary::kNoCall); locs->set_in( 0, Location::RegisterLocation(CheckedStoreIntoSharedStubABI::kValueReg)); - if (FLAG_experimental_shared_data && field().is_shared()) { + if (need_temp) { locs->set_temp(0, Location::RegisterLocation( CheckedStoreIntoSharedStubABI::kFieldReg)); } @@ -2772,27 +2773,28 @@ void StoreStaticFieldInstr::EmitNativeCode(FlowGraphCompiler* compiler) { __ LoadIsolate(TMP); __ BranchIfZero(TMP, slow_path->entry_label()); } else { - const Register temp = locs()->temp(0).reg(); + if (!field().has_deeply_immutable_type()) { + const Register temp = locs()->temp(0).reg(); - // TODO(dartbug.com/61078): use field static type information to decide - // whether the following value check is needed or not. - checked_store_into_shared_slow_path = - new CheckedStoreIntoSharedSlowPath(this, value); - compiler->AddSlowPathCode(checked_store_into_shared_slow_path); + checked_store_into_shared_slow_path = + new CheckedStoreIntoSharedSlowPath(this, value); + compiler->AddSlowPathCode(checked_store_into_shared_slow_path); - compiler::Label allow_store; - __ BranchIfSmi(value, &allow_store, compiler::Assembler::kNearJump); - __ lbu(TMP, compiler::FieldAddress( - value, compiler::target::Object::tags_offset())); - __ andi(temp, TMP, 1 << compiler::target::UntaggedObject::kCanonicalBit); - // If canonical bit is set, no need for runtime check. - __ bnez(temp, &allow_store); - __ andi(temp, TMP, - 1 << compiler::target::UntaggedObject::kDeeplyImmutableBit); - // If deeply immutability bit is not set, go to runtime. - __ beqz(temp, checked_store_into_shared_slow_path->entry_label()); + compiler::Label allow_store; + __ BranchIfSmi(value, &allow_store, compiler::Assembler::kNearJump); + __ lbu(TMP, compiler::FieldAddress( + value, compiler::target::Object::tags_offset())); + __ andi(temp, TMP, + 1 << compiler::target::UntaggedObject::kCanonicalBit); + // If canonical bit is set, no need for runtime check. + __ bnez(temp, &allow_store); + __ andi(temp, TMP, + 1 << compiler::target::UntaggedObject::kDeeplyImmutableBit); + // If deeply immutability bit is not set, go to runtime. + __ beqz(temp, checked_store_into_shared_slow_path->entry_label()); - __ Bind(&allow_store); + __ Bind(&allow_store); + } } } @@ -2811,7 +2813,8 @@ void StoreStaticFieldInstr::EmitNativeCode(FlowGraphCompiler* compiler) { compiler::target::FieldTable::OffsetOf(field())); } - if (FLAG_experimental_shared_data && field().is_shared()) { + if (FLAG_experimental_shared_data && field().is_shared() && + !field().has_deeply_immutable_type()) { __ Bind(checked_store_into_shared_slow_path->exit_label()); } } diff --git a/runtime/vm/compiler/backend/il_x64.cc b/runtime/vm/compiler/backend/il_x64.cc index 4f68512899b..4be40205be6 100644 --- a/runtime/vm/compiler/backend/il_x64.cc +++ b/runtime/vm/compiler/backend/il_x64.cc @@ -2514,8 +2514,10 @@ void CheckFieldImmutabilityInstr::EmitNativeCode(FlowGraphCompiler* compiler) { LocationSummary* StoreStaticFieldInstr::MakeLocationSummary(Zone* zone, bool opt) const { const intptr_t kNumInputs = 1; - const intptr_t kNumTemps = - FLAG_experimental_shared_data && field().is_shared() ? 2 : 1; + const bool need_extra_temp = FLAG_experimental_shared_data && + field().is_shared() && + !field().has_deeply_immutable_type(); + const intptr_t kNumTemps = need_extra_temp ? 2 : 1; const bool can_call_to_throw = FLAG_experimental_shared_data; LocationSummary* locs = new (zone) LocationSummary(zone, kNumInputs, kNumTemps, @@ -2524,7 +2526,7 @@ LocationSummary* StoreStaticFieldInstr::MakeLocationSummary(Zone* zone, locs->set_in( 0, Location::RegisterLocation(CheckedStoreIntoSharedStubABI::kValueReg)); locs->set_temp(0, Location::RequiresRegister()); - if (FLAG_experimental_shared_data && field().is_shared()) { + if (need_extra_temp) { locs->set_temp(1, Location::RegisterLocation( CheckedStoreIntoSharedStubABI::kFieldReg)); } @@ -2547,27 +2549,29 @@ void StoreStaticFieldInstr::EmitNativeCode(FlowGraphCompiler* compiler) { __ LoadIsolate(temp); __ BranchIfZero(temp, slow_path->entry_label()); } else { - // TODO(dartbug.com/61078): use field static type information to decide - // whether the following value check is needed or not. - checked_store_into_shared_slow_path = - new CheckedStoreIntoSharedSlowPath(this, value); - compiler->AddSlowPathCode(checked_store_into_shared_slow_path); + if (!field().has_deeply_immutable_type()) { + checked_store_into_shared_slow_path = + new CheckedStoreIntoSharedSlowPath(this, value); + compiler->AddSlowPathCode(checked_store_into_shared_slow_path); - compiler::Label allow_store; - __ BranchIfSmi(value, &allow_store, compiler::Assembler::kNearJump); - __ movq(temp, compiler::FieldAddress( - value, compiler::target::Object::tags_offset())); - __ testq(temp, compiler::Immediate( - 1 << compiler::target::UntaggedObject::kCanonicalBit)); - // If canonical bit is set, no need for runtime check. - __ j(NOT_ZERO, &allow_store); - __ testq(temp, - compiler::Immediate( - 1 << compiler::target::UntaggedObject::kDeeplyImmutableBit)); - // If deep immutability bit is not set, go to runtime. - __ j(ZERO, checked_store_into_shared_slow_path->entry_label()); + compiler::Label allow_store; + __ BranchIfSmi(value, &allow_store, compiler::Assembler::kNearJump); + __ movq(temp, compiler::FieldAddress( + value, compiler::target::Object::tags_offset())); + __ testq(temp, + compiler::Immediate( + 1 << compiler::target::UntaggedObject::kCanonicalBit)); + // If canonical bit is set, no need for runtime check. + __ j(NOT_ZERO, &allow_store); + __ testq( + temp, + compiler::Immediate( + 1 << compiler::target::UntaggedObject::kDeeplyImmutableBit)); + // If deep immutability bit is not set, go to runtime. + __ j(ZERO, checked_store_into_shared_slow_path->entry_label()); - __ Bind(&allow_store); + __ Bind(&allow_store); + } } } @@ -2588,7 +2592,8 @@ void StoreStaticFieldInstr::EmitNativeCode(FlowGraphCompiler* compiler) { value); } - if (FLAG_experimental_shared_data && field().is_shared()) { + if (FLAG_experimental_shared_data && field().is_shared() && + !field().has_deeply_immutable_type()) { __ Bind(checked_store_into_shared_slow_path->exit_label()); } } diff --git a/runtime/vm/compiler/frontend/kernel_binary_flowgraph.cc b/runtime/vm/compiler/frontend/kernel_binary_flowgraph.cc index ead06549180..a7b548cc7f8 100644 --- a/runtime/vm/compiler/frontend/kernel_binary_flowgraph.cc +++ b/runtime/vm/compiler/frontend/kernel_binary_flowgraph.cc @@ -179,10 +179,9 @@ Fragment StreamingFlowGraphBuilder::BuildFieldInitializer( instructions += Drop(); } else { const auto& klass = Class::Handle(field.Owner()); - // TODO(dartbug.com/61078): Use static type to avoid runtime check. instructions += flow_graph_builder_->StoreFieldGuarded( field, StoreFieldInstr::Kind::kInitializing, - klass.is_deeply_immutable()); + klass.is_deeply_immutable() && !field.has_deeply_immutable_type()); } return instructions; } diff --git a/runtime/vm/compiler/frontend/kernel_binary_flowgraph_test.cc b/runtime/vm/compiler/frontend/kernel_binary_flowgraph_test.cc index fe1c6f25493..9ebdbd21b3a 100644 --- a/runtime/vm/compiler/frontend/kernel_binary_flowgraph_test.cc +++ b/runtime/vm/compiler/frontend/kernel_binary_flowgraph_test.cc @@ -394,4 +394,92 @@ ISOLATE_UNIT_TEST_CASE( EXPECT_EQ(0xFEEDFEED, Integer::Cast(const_value->value()).Value()); } +ISOLATE_UNIT_TEST_CASE( + StreamingFlowGraphBuilder_DeeplyImmutableTypeCheckPresent) { + const char* kScript = R"( + @pragma("vm:deeply-immutable") + final class Foo { + final void Function(int) baz; + Foo(this.baz); + } + + @pragma("vm:entry-point", "call") + test() { + Foo((int x) { x = x + 1; } ); + } + )"; + + const auto& root_library = Library::Handle(LoadTestScript(kScript)); + EXPECT(ClassFinalizer::ProcessPendingClasses()); + const Class& foo = Class::Handle(GetClass(root_library, "Foo")); + const auto& error = foo.EnsureIsFinalized(thread); + const auto& constructor = Function::Handle( + foo.LookupConstructor(String::Handle(String::New("Foo.")))); + + EXPECT(error == Error::null()); + Invoke(root_library, "test"); + + TestPipeline pipeline(constructor, CompilerPass::kJIT); + FlowGraph* flow_graph = pipeline.RunPasses({ + CompilerPass::kComputeSSA, + }); + + auto entry = flow_graph->graph_entry()->normal_entry(); + EXPECT(entry != nullptr); + + ILMatcher cursor(flow_graph, entry); + RELEASE_ASSERT(cursor.TryMatch({ + kMatchAndMoveFunctionEntry, + kMatchAndMoveCheckStackOverflow, + kMoveDebugStepChecks, + kMatchAndMoveGuardFieldClass, + kMatchAndMoveCheckFieldImmutability, + kMatchAndMoveStoreField, + })); +} + +ISOLATE_UNIT_TEST_CASE( + StreamingFlowGraphBuilder_DeeplyImmutableTypeCheckOmitted) { + const char* kScript = R"( + @pragma("vm:deeply-immutable") + final class Foo { + final int baz; + Foo(this.baz); + } + + @pragma("vm:entry-point", "call") + test() { + Foo(42); + } + )"; + + const auto& root_library = Library::Handle(LoadTestScript(kScript)); + EXPECT(ClassFinalizer::ProcessPendingClasses()); + const Class& foo = Class::Handle(GetClass(root_library, "Foo")); + const auto& error = foo.EnsureIsFinalized(thread); + const auto& constructor = Function::Handle( + foo.LookupConstructor(String::Handle(String::New("Foo.")))); + + EXPECT(error == Error::null()); + + Invoke(root_library, "test"); + + TestPipeline pipeline(constructor, CompilerPass::kJIT); + FlowGraph* flow_graph = pipeline.RunPasses({ + CompilerPass::kComputeSSA, + }); + + auto entry = flow_graph->graph_entry()->normal_entry(); + EXPECT(entry != nullptr); + + ILMatcher cursor(flow_graph, entry); + RELEASE_ASSERT(cursor.TryMatch({ + kMatchAndMoveFunctionEntry, + kMatchAndMoveCheckStackOverflow, + kMoveDebugStepChecks, + kMatchAndMoveGuardFieldClass, + kMatchAndMoveStoreField, + })); +} + } // namespace dart diff --git a/runtime/vm/kernel_loader.cc b/runtime/vm/kernel_loader.cc index 32d19638f76..d3bdce89936 100644 --- a/runtime/vm/kernel_loader.cc +++ b/runtime/vm/kernel_loader.cc @@ -1023,6 +1023,8 @@ void KernelLoader::FinishTopLevelClassLoading( field.set_is_shared(SharedPragma::decode(pragma_bits)); field.set_is_no_sanitize_thread( NoSanitizeThreadPragma::decode(pragma_bits)); + field.set_has_deeply_immutable_type( + DeeplyImmutablePragma::decode(pragma_bits)); const AbstractType& type = T.BuildType(); // read type. field.SetFieldType(type); ReadInferredType(field, field_offset + library_kernel_offset_); @@ -1453,6 +1455,8 @@ void KernelLoader::FinishClassLoading(const Class& klass, field.set_is_shared(SharedPragma::decode(pragma_bits)); field.set_is_no_sanitize_thread( NoSanitizeThreadPragma::decode(pragma_bits)); + field.set_has_deeply_immutable_type( + DeeplyImmutablePragma::decode(pragma_bits)); ReadInferredType(field, field_offset + library_kernel_offset_); CheckForInitializer(field); // Static fields with initializers are implicitly late. diff --git a/runtime/vm/object.h b/runtime/vm/object.h index 614980593d6..c6beef02f9f 100644 --- a/runtime/vm/object.h +++ b/runtime/vm/object.h @@ -4584,6 +4584,13 @@ class Field : public Object { return untag()->kind_bits_.Read(); } + void set_has_deeply_immutable_type(bool value) const { + untag()->kind_bits_.UpdateBool(value); + } + bool has_deeply_immutable_type() const { + return untag()->kind_bits_.Read(); + } + #if defined(DART_DYNAMIC_MODULES) bool is_declared_in_bytecode() const; #else @@ -4997,6 +5004,10 @@ class Field : public Object { HasInitializerBit::kNextBit>; using NoSanitizeThreadBit = BitField; + using HasDeeplyImmutableTypeBit = + BitField; // Force this field's guard to be dynamic and deoptimize dependent code. void ForceDynamicGuardedCidAndLength() const;