From ef720983530e04819b6fda0659ed7a3fdb190060 Mon Sep 17 00:00:00 2001 From: Martin Kustermann Date: Mon, 10 Sep 2018 15:31:09 +0000 Subject: [PATCH] [VM] Add new SymbolConstant to package:kernel/ast.dart The canonicalization of symbols needs to take library privacy into account. Since the Symbol class itself only has a [_name] field but does not reference in which library the symbol came from, the [_name] must be mangled. Mangling is done by backends and so we make a new [SymbolConstant] which the backends can desugar by doing appropriate mangling and construction of a [Symbol] instance. Fixes https://github.com/dart-lang/sdk/issues/34396 Change-Id: I5ddb5331ce79a0b942807929d4b8f1050a9899e7 Reviewed-on: https://dart-review.googlesource.com/73883 Commit-Queue: Martin Kustermann Reviewed-by: Alexander Markov Reviewed-by: Vyacheslav Egorov --- pkg/dev_compiler/lib/src/kernel/compiler.dart | 2 ++ .../lib/src/kernel/constants.dart | 8 ----- pkg/kernel/binary.md | 20 +++++++----- pkg/kernel/lib/ast.dart | 28 +++++++++++++++++ pkg/kernel/lib/binary/ast_from_binary.dart | 6 ++++ pkg/kernel/lib/binary/ast_to_binary.dart | 16 ++++++++++ pkg/kernel/lib/binary/tag.dart | 15 ++++----- pkg/kernel/lib/transformations/constants.dart | 16 +++++++--- pkg/kernel/lib/visitor.dart | 4 +++ .../lib/vm/constants_native_effects.dart | 7 ----- .../type_flow/summary_collector.dart | 9 ++++++ .../type_flow/transformer.dart | 5 +++ .../compiler/frontend/constant_evaluator.cc | 31 ++++++++++++++++++- .../vm/compiler/frontend/constant_evaluator.h | 4 +++ runtime/vm/kernel_binary.h | 15 ++++----- tests/language_2/vm/regress_34396_helper.dart | 8 +++++ tests/language_2/vm/regress_34396_test.dart | 17 ++++++++++ 17 files changed, 170 insertions(+), 41 deletions(-) create mode 100644 tests/language_2/vm/regress_34396_helper.dart create mode 100644 tests/language_2/vm/regress_34396_test.dart diff --git a/pkg/dev_compiler/lib/src/kernel/compiler.dart b/pkg/dev_compiler/lib/src/kernel/compiler.dart index f9ce5c81a29..1746d303b5d 100644 --- a/pkg/dev_compiler/lib/src/kernel/compiler.dart +++ b/pkg/dev_compiler/lib/src/kernel/compiler.dart @@ -4975,6 +4975,8 @@ class ProgramCompiler extends Object @override defaultConstant(Constant node) => _emitInvalidNode(node); @override + visitSymbolConstant(node) => defaultConstant(node); + @override visitMapConstant(node) => defaultConstant(node); @override visitListConstant(node) => defaultConstant(node); diff --git a/pkg/dev_compiler/lib/src/kernel/constants.dart b/pkg/dev_compiler/lib/src/kernel/constants.dart index 60a79c748d3..8838cae8eb3 100644 --- a/pkg/dev_compiler/lib/src/kernel/constants.dart +++ b/pkg/dev_compiler/lib/src/kernel/constants.dart @@ -303,14 +303,6 @@ class _ConstantsBackend implements ConstantsBackend { nativeName, typeArguments, positionalArguments, namedArguments) => throw StateError('unreachable'); // DDC does not use VM native syntax - @override - buildSymbolConstant(StringConstant value) { - return InstanceConstant( - coreTypes.internalSymbolClass.reference, - const [], - {symbolNameField.reference: value}); - } - @override lowerMapConstant(constant) => constant; diff --git a/pkg/kernel/binary.md b/pkg/kernel/binary.md index 8f800cc21a5..9aa8b603f4a 100644 --- a/pkg/kernel/binary.md +++ b/pkg/kernel/binary.md @@ -131,7 +131,7 @@ type CanonicalName { type ComponentFile { UInt32 magic = 0x90ABCDEF; - UInt32 formatVersion = 11; + UInt32 formatVersion = 12; Library[] libraries; UriSource sourceMap; List canonicalNames; @@ -865,39 +865,45 @@ type StringConstant extends Constant { StringReference value; } -type MapConstant extends Constant { +type SymbolConstant extends Constant { Byte tag = 5; + Option library; + StringReference name; +} + +type MapConstant extends Constant { + Byte tag = 6; DartType keyType; DartType valueType; List<[ConstantReference, ConstantReference]> keyValueList; } type ListConstant extends Constant { - Byte tag = 6; + Byte tag = 7; DartType type; List values; } type InstanceConstant extends Constant { - Byte tag = 7; + Byte tag = 8; CanonicalNameReference class; List typeArguments; List<[FieldReference, ConstantReference]> values; } type PartialInstantiationConstant extends Constant { - Byte tag = 8; + Byte tag = 9; ConstantReference tearOffConstant; List typeArguments; } type TearOffConstant extends Constant { - Byte tag = 9; + Byte tag = 10; CanonicalNameReference staticProcedureReference; } type TypeLiteralConstant extends Constant { - Byte tag = 10; + Byte tag = 11; DartType type; } diff --git a/pkg/kernel/lib/ast.dart b/pkg/kernel/lib/ast.dart index 59c482365e6..03a709a756f 100644 --- a/pkg/kernel/lib/ast.dart +++ b/pkg/kernel/lib/ast.dart @@ -5073,6 +5073,34 @@ class StringConstant extends PrimitiveConstant { DartType getType(TypeEnvironment types) => types.stringType; } +class SymbolConstant extends Constant { + final String name; + final Reference libraryReference; + + SymbolConstant(this.name, this.libraryReference); + + visitChildren(Visitor v) {} + + accept(ConstantVisitor v) => v.visitSymbolConstant(this); + acceptReference(Visitor v) => v.visitSymbolConstantReference(this); + + String toString() { + return libraryReference != null + ? '#${libraryReference.asLibrary.importUri}::$name' + : '#$name'; + } + + int get hashCode => name.hashCode ^ libraryReference.hashCode; + + bool operator ==(Object other) => + identical(this, other) || + (other is SymbolConstant && + other.name == name && + other.libraryReference == libraryReference); + + DartType getType(TypeEnvironment types) => types.symbolType; +} + class MapConstant extends Constant { final DartType keyType; final DartType valueType; diff --git a/pkg/kernel/lib/binary/ast_from_binary.dart b/pkg/kernel/lib/binary/ast_from_binary.dart index db54b4ac507..70684ea2091 100644 --- a/pkg/kernel/lib/binary/ast_from_binary.dart +++ b/pkg/kernel/lib/binary/ast_from_binary.dart @@ -206,6 +206,12 @@ class BinaryBuilder { return new DoubleConstant(readDouble()); case ConstantTag.StringConstant: return new StringConstant(readStringReference()); + case ConstantTag.SymbolConstant: + Reference libraryReference; + if (readAndCheckOptionTag()) { + libraryReference = readLibraryReference(); + } + return new SymbolConstant(readStringReference(), libraryReference); case ConstantTag.MapConstant: final DartType keyType = readDartType(); final DartType valueType = readDartType(); diff --git a/pkg/kernel/lib/binary/ast_to_binary.dart b/pkg/kernel/lib/binary/ast_to_binary.dart index 528aeb9519c..7c0795ac122 100644 --- a/pkg/kernel/lib/binary/ast_to_binary.dart +++ b/pkg/kernel/lib/binary/ast_to_binary.dart @@ -164,6 +164,10 @@ class BinaryPrinter implements Visitor, BinarySink { } else if (constant is StringConstant) { writeByte(ConstantTag.StringConstant); writeStringReference(constant.value); + } else if (constant is SymbolConstant) { + writeByte(ConstantTag.SymbolConstant); + writeOptionalReference(constant.libraryReference); + writeStringReference(constant.name); } else if (constant is MapConstant) { writeByte(ConstantTag.MapConstant); writeDartType(constant.keyType); @@ -1876,6 +1880,16 @@ class BinaryPrinter implements Visitor, BinarySink { throw new UnsupportedError('serialization of StringConstant references'); } + @override + void visitSymbolConstant(SymbolConstant node) { + throw new UnsupportedError('serialization of SymbolConstants'); + } + + @override + void visitSymbolConstantReference(SymbolConstant node) { + throw new UnsupportedError('serialization of SymbolConstant references'); + } + @override void visitPartialInstantiationConstant(PartialInstantiationConstant node) { throw new UnsupportedError( @@ -1997,6 +2011,8 @@ class ConstantIndexer extends RecursiveVisitor { if (constant is StringConstant) { stringIndexer.put(constant.value); + } else if (constant is SymbolConstant) { + stringIndexer.put(constant.name); } else if (constant is DoubleConstant) { stringIndexer.put('${constant.value}'); } else if (constant is IntConstant) { diff --git a/pkg/kernel/lib/binary/tag.dart b/pkg/kernel/lib/binary/tag.dart index beb113c8993..aea6b2a6e52 100644 --- a/pkg/kernel/lib/binary/tag.dart +++ b/pkg/kernel/lib/binary/tag.dart @@ -127,7 +127,7 @@ class Tag { /// Internal version of kernel binary format. /// Bump it when making incompatible changes in kernel binaries. /// Keep in sync with runtime/vm/kernel_binary.h, pkg/kernel/binary.md. - static const int BinaryFormatVersion = 11; + static const int BinaryFormatVersion = 12; } abstract class ConstantTag { @@ -136,10 +136,11 @@ abstract class ConstantTag { static const int IntConstant = 2; static const int DoubleConstant = 3; static const int StringConstant = 4; - static const int MapConstant = 5; - static const int ListConstant = 6; - static const int InstanceConstant = 7; - static const int PartialInstantiationConstant = 8; - static const int TearOffConstant = 9; - static const int TypeLiteralConstant = 10; + static const int SymbolConstant = 5; + static const int MapConstant = 6; + static const int ListConstant = 7; + static const int InstanceConstant = 8; + static const int PartialInstantiationConstant = 9; + static const int TearOffConstant = 10; + static const int TypeLiteralConstant = 11; } diff --git a/pkg/kernel/lib/transformations/constants.dart b/pkg/kernel/lib/transformations/constants.dart index cf7654addd0..2d202818007 100644 --- a/pkg/kernel/lib/transformations/constants.dart +++ b/pkg/kernel/lib/transformations/constants.dart @@ -1025,8 +1025,9 @@ class ConstantEvaluator extends RecursiveVisitor { } visitSymbolLiteral(SymbolLiteral node) { - final value = canonicalize(new StringConstant(node.value)); - return canonicalize(backend.buildSymbolConstant(value)); + final libraryReference = + node.value.startsWith('_') ? libraryOf(node).reference : null; + return canonicalize(new SymbolConstant(node.value, libraryReference)); } visitInstantiation(Instantiation node) { @@ -1193,6 +1194,15 @@ class ConstantEvaluator extends RecursiveVisitor { } return value; } + + Library libraryOf(TreeNode node) { + // The tree structure of the kernel AST ensures we always have an enclosing + // library. + while (true) { + if (node is Library) return node; + node = node.parent; + } + } } /// Holds the necessary information for a constant object, namely @@ -1270,8 +1280,6 @@ abstract class ConstantsBackend { List typeArguments, List positionalArguments, Map namedArguments); - Constant buildSymbolConstant(StringConstant value); - Constant lowerListConstant(ListConstant constant); Constant lowerMapConstant(MapConstant constant); } diff --git a/pkg/kernel/lib/visitor.dart b/pkg/kernel/lib/visitor.dart index 13ae2ade99f..3133b2008d6 100644 --- a/pkg/kernel/lib/visitor.dart +++ b/pkg/kernel/lib/visitor.dart @@ -269,6 +269,7 @@ class ConstantVisitor { R visitIntConstant(IntConstant node) => defaultConstant(node); R visitDoubleConstant(DoubleConstant node) => defaultConstant(node); R visitStringConstant(StringConstant node) => defaultConstant(node); + R visitSymbolConstant(SymbolConstant node) => defaultConstant(node); R visitMapConstant(MapConstant node) => defaultConstant(node); R visitListConstant(ListConstant node) => defaultConstant(node); R visitInstanceConstant(InstanceConstant node) => defaultConstant(node); @@ -321,6 +322,7 @@ class Visitor extends TreeVisitor R visitIntConstant(IntConstant node) => defaultConstant(node); R visitDoubleConstant(DoubleConstant node) => defaultConstant(node); R visitStringConstant(StringConstant node) => defaultConstant(node); + R visitSymbolConstant(SymbolConstant node) => defaultConstant(node); R visitMapConstant(MapConstant node) => defaultConstant(node); R visitListConstant(ListConstant node) => defaultConstant(node); R visitInstanceConstant(InstanceConstant node) => defaultConstant(node); @@ -345,6 +347,8 @@ class Visitor extends TreeVisitor defaultConstantReference(node); R visitStringConstantReference(StringConstant node) => defaultConstantReference(node); + R visitSymbolConstantReference(SymbolConstant node) => + defaultConstantReference(node); R visitMapConstantReference(MapConstant node) => defaultConstantReference(node); R visitListConstantReference(ListConstant node) => diff --git a/pkg/kernel/lib/vm/constants_native_effects.dart b/pkg/kernel/lib/vm/constants_native_effects.dart index 0de29bec7fb..44f43eb8919 100644 --- a/pkg/kernel/lib/vm/constants_native_effects.dart +++ b/pkg/kernel/lib/vm/constants_native_effects.dart @@ -91,13 +91,6 @@ class VmConstantsBackend implements ConstantsBackend { throw 'No native effect registered for constant evaluation: $nativeName'; } - Constant buildSymbolConstant(StringConstant value) { - return new InstanceConstant( - internalSymbolClass.reference, - const [], - {symbolNameField.reference: value}); - } - Constant lowerMapConstant(MapConstant constant) { // The _ImmutableMap class is implemented via one field pointing to a list // of key/value pairs -- see runtime/lib/immutable_map.dart! diff --git a/pkg/vm/lib/transformations/type_flow/summary_collector.dart b/pkg/vm/lib/transformations/type_flow/summary_collector.dart index 0ed585c44ff..13ff6e99c54 100644 --- a/pkg/vm/lib/transformations/type_flow/summary_collector.dart +++ b/pkg/vm/lib/transformations/type_flow/summary_collector.dart @@ -535,6 +535,10 @@ class SummaryCollector extends RecursiveVisitor { Type get _stringType => _cachedStringType ??= new Type.cone(_environment.stringType); + Type _cachedSymbolType; + Type get _symbolType => + _cachedSymbolType ??= new Type.cone(_environment.symbolType); + Type _cachedNullType; Type get _nullType => _cachedNullType ??= new Type.nullable(new Type.empty()); @@ -1258,6 +1262,11 @@ class ConstantAllocationCollector extends ConstantVisitor { return summaryCollector._stringType; } + @override + visitSymbolConstant(SymbolConstant constant) { + return summaryCollector._symbolType; + } + @override Type visitMapConstant(MapConstant node) { throw 'The kernel2kernel constants transformation desugars const maps!'; diff --git a/pkg/vm/lib/transformations/type_flow/transformer.dart b/pkg/vm/lib/transformations/type_flow/transformer.dart index 098e79d2d03..b108e0b5a50 100644 --- a/pkg/vm/lib/transformations/type_flow/transformer.dart +++ b/pkg/vm/lib/transformations/type_flow/transformer.dart @@ -939,6 +939,11 @@ class _TreeShakerConstantVisitor extends ConstantVisitor { @override visitStringConstant(StringConstant constant) {} + @override + visitSymbolConstant(SymbolConstant constant) { + // The Symbol class and it's _name field are always retained. + } + @override visitMapConstant(MapConstant node) { throw 'The kernel2kernel constants transformation desugars const maps!'; diff --git a/runtime/vm/compiler/frontend/constant_evaluator.cc b/runtime/vm/compiler/frontend/constant_evaluator.cc index e5e9367d682..92f88fd5ade 100644 --- a/runtime/vm/compiler/frontend/constant_evaluator.cc +++ b/runtime/vm/compiler/frontend/constant_evaluator.cc @@ -1050,19 +1050,33 @@ ConstantHelper::ConstantHelper(Zone* zone, const_evaluator_(helper, type_translator, active_class, nullptr), translation_helper_(helper->translation_helper_), skip_vmservice_library_(skip_vmservice_library), + symbol_class_(Class::Handle(zone)), + symbol_name_field_(Field::Handle(zone)), temp_type_(AbstractType::Handle(zone)), temp_type_arguments_(TypeArguments::Handle(zone)), temp_type_arguments2_(TypeArguments::Handle(zone)), temp_type_arguments3_(TypeArguments::Handle(zone)), temp_object_(Object::Handle(zone)), + temp_string_(String::Handle(zone)), temp_array_(Array::Handle(zone)), temp_instance_(Instance::Handle(zone)), temp_field_(Field::Handle(zone)), temp_class_(Class::Handle(zone)), + temp_library_(Library::Handle(zone)), temp_function_(Function::Handle(zone)), temp_closure_(Closure::Handle(zone)), temp_context_(Context::Handle(zone)), - temp_integer_(Integer::Handle(zone)) {} + temp_integer_(Integer::Handle(zone)) { + temp_library_ = Library::InternalLibrary(); + ASSERT(!temp_library_.IsNull()); + + symbol_class_ = temp_library_.LookupClass(Symbols::Symbol()); + ASSERT(!symbol_class_.IsNull()); + + symbol_name_field_ = + symbol_class_.LookupInstanceFieldAllowPrivate(Symbols::_name()); + ASSERT(!symbol_name_field_.IsNull()); +} const Array& ConstantHelper::ReadConstantTable() { const intptr_t number_of_constants = helper_.ReadUInt(); @@ -1111,6 +1125,21 @@ const Array& ConstantHelper::ReadConstantTable() { H.Canonicalize(H.DartString(helper_.ReadStringReference())); break; } + case kSymbolConstant: { + Tag initializer_tag = helper_.ReadTag(); + if (initializer_tag == kSomething) { + const NameIndex index = helper_.ReadCanonicalNameReference(); + temp_library_ = H.LookupLibraryByKernelLibrary(index); + } else { + temp_library_ = Library::null(); + } + const String& symbol = + H.DartIdentifier(temp_library_, helper_.ReadStringReference()); + temp_instance_ = Instance::New(symbol_class_, Heap::kOld); + temp_instance_.SetField(symbol_name_field_, symbol); + temp_instance_ = H.Canonicalize(temp_instance_); + break; + } case kListConstant: { temp_type_arguments_ = TypeArguments::New(1, Heap::kOld); const AbstractType& type = type_translator_.BuildType(); diff --git a/runtime/vm/compiler/frontend/constant_evaluator.h b/runtime/vm/compiler/frontend/constant_evaluator.h index f7b3b61459b..f71e48f5ba1 100644 --- a/runtime/vm/compiler/frontend/constant_evaluator.h +++ b/runtime/vm/compiler/frontend/constant_evaluator.h @@ -166,15 +166,19 @@ class ConstantHelper { ConstantEvaluator const_evaluator_; TranslationHelper& translation_helper_; NameIndex skip_vmservice_library_; + Class& symbol_class_; + Field& symbol_name_field_; AbstractType& temp_type_; TypeArguments& temp_type_arguments_; TypeArguments& temp_type_arguments2_; TypeArguments& temp_type_arguments3_; Object& temp_object_; + String& temp_string_; Array& temp_array_; Instance& temp_instance_; Field& temp_field_; Class& temp_class_; + Library& temp_library_; Function& temp_function_; Closure& temp_closure_; Context& temp_context_; diff --git a/runtime/vm/kernel_binary.h b/runtime/vm/kernel_binary.h index 158081726ce..2ffb315e728 100644 --- a/runtime/vm/kernel_binary.h +++ b/runtime/vm/kernel_binary.h @@ -17,7 +17,7 @@ namespace kernel { // package:kernel/binary.md. static const uint32_t kMagicProgramFile = 0x90ABCDEFu; -static const uint32_t kBinaryFormatVersion = 11; +static const uint32_t kBinaryFormatVersion = 12; // Keep in sync with package:kernel/lib/binary/tag.dart #define KERNEL_TAG_LIST(V) \ @@ -138,12 +138,13 @@ enum ConstantTag { kIntConstant = 2, kDoubleConstant = 3, kStringConstant = 4, - kMapConstant = 5, - kListConstant = 6, - kInstanceConstant = 7, - kPartialInstantiationConstant = 8, - kTearOffConstant = 9, - kTypeLiteralConstant = 10, + kSymbolConstant = 5, + kMapConstant = 6, + kListConstant = 7, + kInstanceConstant = 8, + kPartialInstantiationConstant = 9, + kTearOffConstant = 10, + kTypeLiteralConstant = 11, }; static const int SpecializedIntLiteralBias = 3; diff --git a/tests/language_2/vm/regress_34396_helper.dart b/tests/language_2/vm/regress_34396_helper.dart new file mode 100644 index 00000000000..5e61334c854 --- /dev/null +++ b/tests/language_2/vm/regress_34396_helper.dart @@ -0,0 +1,8 @@ +// Copyright (c) 2018, 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. + +library regress_34396_helper; + +get privateSymbol => #_privateSymbol; +get privateSymbolSame => #_privateSymbol; diff --git a/tests/language_2/vm/regress_34396_test.dart b/tests/language_2/vm/regress_34396_test.dart new file mode 100644 index 00000000000..ead2b0bdcf9 --- /dev/null +++ b/tests/language_2/vm/regress_34396_test.dart @@ -0,0 +1,17 @@ +// Copyright (c) 2018, 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. + +import 'package:expect/expect.dart'; + +import 'regress_34396_helper.dart' as helper; + +main() { + Expect.isFalse(#_privateSymbol == helper.privateSymbol); + Expect.isFalse(#_privateSymbol == helper.privateSymbolSame); + Expect.isFalse(identical(#_privateSymbol, helper.privateSymbol)); + Expect.isFalse(identical(#_privateSymbol, helper.privateSymbolSame)); + + Expect.isTrue(helper.privateSymbol == helper.privateSymbolSame); + Expect.isTrue(identical(helper.privateSymbol, helper.privateSymbolSame)); +}