Skip to content

Commit

Permalink
[vm] Prevent late fields from being unboxed
Browse files Browse the repository at this point in the history
When unboxed doubles are assigned to late fields, it conflicts with the
late field logic and causes a crash.

Bug: #38841
Bug: #39658
Change-Id: I641f597006114f02473a20f8c2526eeaf4fe813f
Fixes: #39658
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/127442
Commit-Queue: Liam Appelbe <liama@google.com>
Reviewed-by: Alexander Markov <alexmarkov@google.com>
  • Loading branch information
liamappelbe authored and commit-bot@chromium.org committed Dec 5, 2019
1 parent 7ac3f3e commit e20ff1e
Show file tree
Hide file tree
Showing 8 changed files with 112 additions and 35 deletions.
5 changes: 3 additions & 2 deletions runtime/vm/compiler/backend/slot_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,9 @@ TEST_CASE(SlotFromGuardedField) {
const Field& field = Field::Handle(
Field::New(String::Handle(Symbols::New(thread, "field")),
/*is_static=*/false, /*is_final=*/false, /*is_const=*/false,
/*is_reflectable=*/true, dummy_class, Object::dynamic_type(),
TokenPosition::kMinSource, TokenPosition::kMinSource));
/*is_reflectable=*/true, /*is_late=*/false, dummy_class,
Object::dynamic_type(), TokenPosition::kMinSource,
TokenPosition::kMinSource));

// Set non-trivial guarded state on the field.
field.set_guarded_cid(kSmiCid);
Expand Down
3 changes: 2 additions & 1 deletion runtime/vm/compiler/backend/type_propagator_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,8 @@ ISOLATE_UNIT_TEST_CASE(TypePropagator_Refinement) {
/*is_static=*/true,
/*is_final=*/false,
/*is_const=*/false,
/*is_reflectable=*/true, object_class, Object::dynamic_type(),
/*is_reflectable=*/true,
/*is_late=*/false, object_class, Object::dynamic_type(),
TokenPosition::kNoSource, TokenPosition::kNoSource));

FlowGraphBuilderHelper H;
Expand Down
19 changes: 9 additions & 10 deletions runtime/vm/compiler/frontend/bytecode_reader.cc
Original file line number Diff line number Diff line change
Expand Up @@ -2017,16 +2017,15 @@ void BytecodeReaderHelper::ReadFieldDeclarations(const Class& cls,
}

field = Field::New(name, is_static, is_final, is_const,
(flags & kIsReflectableFlag) != 0, script_class, type,
position, end_position);
(flags & kIsReflectableFlag) != 0, is_late, script_class,
type, position, end_position);

field.set_is_declared_in_bytecode(true);
field.set_has_pragma(has_pragma);
field.set_is_covariant((flags & kIsCovariantFlag) != 0);
field.set_is_generic_covariant_impl((flags & kIsGenericCovariantImplFlag) !=
0);
field.set_has_nontrivial_initializer(has_nontrivial_initializer);
field.set_is_late((flags & kIsLateFlag) != 0);
field.set_is_extension_member(is_extension_member);
field.set_has_initializer(has_initializer);

Expand Down Expand Up @@ -2134,13 +2133,13 @@ void BytecodeReaderHelper::ReadFieldDeclarations(const Class& cls,

if (cls.is_enum_class()) {
// Add static field 'const _deleted_enum_sentinel'.
field =
Field::New(Symbols::_DeletedEnumSentinel(),
/* is_static = */ true,
/* is_final = */ true,
/* is_const = */ true,
/* is_reflectable = */ false, cls, Object::dynamic_type(),
TokenPosition::kNoSource, TokenPosition::kNoSource);
field = Field::New(Symbols::_DeletedEnumSentinel(),
/* is_static = */ true,
/* is_final = */ true,
/* is_const = */ true,
/* is_reflectable = */ false,
/* is_late = */ false, cls, Object::dynamic_type(),
TokenPosition::kNoSource, TokenPosition::kNoSource);

fields.SetAt(num_fields, field);
}
Expand Down
31 changes: 15 additions & 16 deletions runtime/vm/kernel_loader.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1187,12 +1187,11 @@ void KernelLoader::FinishTopLevelClassLoading(
const bool is_late = field_helper.IsLate();
const bool is_extension_member = field_helper.IsExtensionMember();
const Field& field = Field::Handle(
Z,
Field::NewTopLevel(name, is_final, field_helper.IsConst(), script_class,
field_helper.position_, field_helper.end_position_));
Z, Field::NewTopLevel(name, is_final, field_helper.IsConst(), is_late,
script_class, field_helper.position_,
field_helper.end_position_));
field.set_kernel_offset(field_offset);
field.set_has_pragma(has_pragma_annotation);
field.set_is_late(is_late);
field.set_is_extension_member(is_extension_member);
const AbstractType& type = T.BuildType(); // read type.
field.SetFieldType(type);
Expand Down Expand Up @@ -1541,16 +1540,15 @@ void KernelLoader::FinishClassLoading(const Class& klass,
const bool is_late = field_helper.IsLate();
const bool is_extension_member = field_helper.IsExtensionMember();
Field& field = Field::Handle(
Z,
Field::New(name, field_helper.IsStatic(), is_final,
field_helper.IsConst(), is_reflectable, script_class, type,
field_helper.position_, field_helper.end_position_));
Z, Field::New(name, field_helper.IsStatic(), is_final,
field_helper.IsConst(), is_reflectable, is_late,
script_class, type, field_helper.position_,
field_helper.end_position_));
field.set_kernel_offset(field_offset);
field.set_has_pragma(has_pragma_annotation);
field.set_is_covariant(field_helper.IsCovariant());
field.set_is_generic_covariant_impl(
field_helper.IsGenericCovariantImpl());
field.set_is_late(is_late);
field.set_is_extension_member(is_extension_member);
ReadInferredType(field, field_offset + library_kernel_offset_);
CheckForInitializer(field);
Expand All @@ -1575,13 +1573,14 @@ void KernelLoader::FinishClassLoading(const Class& klass,
// Add static field 'const _deleted_enum_sentinel'.
// This field does not need to be of type E.
Field& deleted_enum_sentinel = Field::ZoneHandle(Z);
deleted_enum_sentinel = Field::New(
Symbols::_DeletedEnumSentinel(),
/* is_static = */ true,
/* is_final = */ true,
/* is_const = */ true,
/* is_reflectable = */ false, klass, Object::dynamic_type(),
TokenPosition::kNoSource, TokenPosition::kNoSource);
deleted_enum_sentinel =
Field::New(Symbols::_DeletedEnumSentinel(),
/* is_static = */ true,
/* is_final = */ true,
/* is_const = */ true,
/* is_reflectable = */ false,
/* is_late = */ false, klass, Object::dynamic_type(),
TokenPosition::kNoSource, TokenPosition::kNoSource);
fields_.Add(&deleted_enum_sentinel);
}

Expand Down
18 changes: 14 additions & 4 deletions runtime/vm/object.cc
Original file line number Diff line number Diff line change
Expand Up @@ -3834,7 +3834,11 @@ bool Class::InjectCIDFields() const {
const AbstractType& field_type = Type::Handle(zone, Type::IntType());
for (size_t i = 0; i < ARRAY_SIZE(cid_fields); i++) {
field_name = Symbols::New(thread, cid_fields[i].field_name);
field = Field::New(field_name, true, false, true, false, *this, field_type,
field = Field::New(field_name, /* is_static = */ true,
/* is_final = */ false,
/* is_const = */ true,
/* is_reflectable = */ false,
/* is_late = */ false, *this, field_type,
TokenPosition::kMinSource, TokenPosition::kMinSource);
value = Smi::New(cid_fields[i].cid);
field.SetStaticValue(value, true);
Expand Down Expand Up @@ -8746,6 +8750,7 @@ void Field::InitializeNew(const Field& result,
bool is_final,
bool is_const,
bool is_reflectable,
bool is_late,
const Object& owner,
TokenPosition token_pos,
TokenPosition end_token_pos) {
Expand All @@ -8758,13 +8763,14 @@ void Field::InitializeNew(const Field& result,
result.set_is_final(is_final);
result.set_is_const(is_const);
result.set_is_reflectable(is_reflectable);
result.set_is_late(is_late);
result.set_is_double_initialized(false);
result.set_owner(owner);
result.set_token_pos(token_pos);
result.set_end_token_pos(end_token_pos);
result.set_has_nontrivial_initializer(false);
result.set_has_initializer(false);
result.set_is_unboxing_candidate(!is_final);
result.set_is_unboxing_candidate(!is_final && !is_late);
result.set_initializer_changed_after_initialization(false);
NOT_IN_PRECOMPILED(result.set_is_declared_in_bytecode(false));
NOT_IN_PRECOMPILED(result.set_binary_declaration_offset(0));
Expand Down Expand Up @@ -8800,29 +8806,31 @@ RawField* Field::New(const String& name,
bool is_final,
bool is_const,
bool is_reflectable,
bool is_late,
const Object& owner,
const AbstractType& type,
TokenPosition token_pos,
TokenPosition end_token_pos) {
ASSERT(!owner.IsNull());
const Field& result = Field::Handle(Field::New());
InitializeNew(result, name, is_static, is_final, is_const, is_reflectable,
owner, token_pos, end_token_pos);
is_late, owner, token_pos, end_token_pos);
result.SetFieldType(type);
return result.raw();
}

RawField* Field::NewTopLevel(const String& name,
bool is_final,
bool is_const,
bool is_late,
const Object& owner,
TokenPosition token_pos,
TokenPosition end_token_pos) {
ASSERT(!owner.IsNull());
const Field& result = Field::Handle(Field::New());
InitializeNew(result, name, true, /* is_static */
is_final, is_const, true, /* is_reflectable */
owner, token_pos, end_token_pos);
is_late, owner, token_pos, end_token_pos);
return result.raw();
}

Expand Down Expand Up @@ -10240,6 +10248,7 @@ void Library::AddMetadata(const Object& owner,
Field::Handle(zone, Field::NewTopLevel(metaname,
false, // is_final
false, // is_const
false, // is_late
owner, token_pos, token_pos));
field.SetFieldType(Object::dynamic_type());
field.set_is_reflectable(false);
Expand Down Expand Up @@ -12002,6 +12011,7 @@ void Namespace::AddMetadata(const Object& owner,
Field& field = Field::Handle(Field::NewTopLevel(Symbols::TopLevel(),
false, // is_final
false, // is_const
false, // is_late
owner, token_pos, token_pos));
field.set_is_reflectable(false);
field.SetFieldType(Object::dynamic_type());
Expand Down
3 changes: 3 additions & 0 deletions runtime/vm/object.h
Original file line number Diff line number Diff line change
Expand Up @@ -3603,6 +3603,7 @@ class Field : public Object {
bool is_final,
bool is_const,
bool is_reflectable,
bool is_late,
const Object& owner,
const AbstractType& type,
TokenPosition token_pos,
Expand All @@ -3611,6 +3612,7 @@ class Field : public Object {
static RawField* NewTopLevel(const String& name,
bool is_final,
bool is_const,
bool is_late,
const Object& owner,
TokenPosition token_pos,
TokenPosition end_token_pos);
Expand Down Expand Up @@ -3854,6 +3856,7 @@ class Field : public Object {
bool is_final,
bool is_const,
bool is_reflectable,
bool is_late,
const Object& owner,
TokenPosition token_pos,
TokenPosition end_token_pos);
Expand Down
4 changes: 2 additions & 2 deletions runtime/vm/object_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -234,7 +234,7 @@ ISOLATE_UNIT_TEST_CASE(InstanceClass) {
const Array& one_fields = Array::Handle(Array::New(1));
const String& field_name = String::Handle(Symbols::New(thread, "the_field"));
const Field& field = Field::Handle(
Field::New(field_name, false, false, false, true, one_field_class,
Field::New(field_name, false, false, false, true, false, one_field_class,
Object::dynamic_type(), TokenPosition::kMinSource,
TokenPosition::kMinSource));
one_fields.SetAt(0, field);
Expand Down Expand Up @@ -2852,7 +2852,7 @@ static RawField* CreateTestField(const char* name) {
const String& field_name =
String::Handle(Symbols::New(Thread::Current(), name));
const Field& field = Field::Handle(Field::New(
field_name, true, false, false, true, cls, Object::dynamic_type(),
field_name, true, false, false, true, false, cls, Object::dynamic_type(),
TokenPosition::kMinSource, TokenPosition::kMinSource));
return field.raw();
}
Expand Down
64 changes: 64 additions & 0 deletions tests/language_2/nnbd/syntax/late_modifier_bug_39658.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
// Copyright (c) 2019, 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.

// SharedOptions=--enable-experiment=non-nullable
import 'package:expect/expect.dart';

int initCalls = 0;
double init() {
++initCalls;
return 1.23;
}

class A {
late double? fieldWithInit = init();
late double fieldWithTrivialInit = 1.23;
late double? fieldWithNullInit = null;
late double fieldWithNoInit;
}

main() {
// Late, non-final, with init.
var a = A();
Expect.equals(0, initCalls);
Expect.equals(1.23, a.fieldWithInit);
Expect.equals(1.23, a.fieldWithTrivialInit);
Expect.equals(null, a.fieldWithNullInit);
Expect.throws(
() => a.fieldWithNoInit, (error) => error is LateInitializationError);
Expect.equals(1, initCalls);
Expect.equals(1.23, a.fieldWithInit);
Expect.equals(1.23, a.fieldWithTrivialInit);
Expect.equals(null, a.fieldWithNullInit);
Expect.throws(
() => a.fieldWithNoInit, (error) => error is LateInitializationError);
Expect.equals(1, initCalls);
a.fieldWithInit = 4.56;
a.fieldWithTrivialInit = 4.56;
a.fieldWithNullInit = 4.56;
a.fieldWithNoInit = 4.56;
Expect.equals(1, initCalls);
Expect.equals(4.56, a.fieldWithInit);
Expect.equals(4.56, a.fieldWithTrivialInit);
Expect.equals(4.56, a.fieldWithNullInit);
Expect.equals(4.56, a.fieldWithNoInit);
Expect.equals(1, initCalls);
initCalls = 0;

// Late, non-final, with init that's pre-empted by setter.
var b = A();
Expect.equals(0, initCalls);
b.fieldWithInit = 4.56;
Expect.equals(0, initCalls);
Expect.equals(4.56, b.fieldWithInit);
Expect.equals(0, initCalls);

// Late, non-final, with init that's pre-empted by null setter.
var c = A();
Expect.equals(0, initCalls);
c.fieldWithInit = null;
Expect.equals(0, initCalls);
Expect.equals(null, c.fieldWithInit);
Expect.equals(0, initCalls);
}

0 comments on commit e20ff1e

Please sign in to comment.