From 622747502e32f578e097e79d5bee34b55d87c9c6 Mon Sep 17 00:00:00 2001 From: Aart Bik Date: Wed, 4 Sep 2019 00:52:27 +0000 Subject: [PATCH] [vm/interpreter] Enforce consistent interpreter/compiler unboxing view Rationale: Both the interpreter and compiler had some logic on when fields could be unboxed. Rather than duplicating this logic, this CL uses the same methods to get a more consistent view of unboxing. Note that this add a slight overhead to the interpreter, but only a slight. We could avoid this by duplicating the logic to enforce consistency, but this has the danger of getting similar issues in the future. https://github.com/dart-lang/sdk/issues/37821 Change-Id: Icf0c1ad518d75c63e9da72e1f75eaf1cf903587f Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/115301 Reviewed-by: Alexander Markov Reviewed-by: Ben Konyi Commit-Queue: Aart Bik --- runtime/vm/interpreter.cc | 29 +++++++++---- runtime/vm/interpreter.h | 4 ++ .../language_2/vm/regression_37821_test.dart | 41 +++++++++++++++++++ 3 files changed, 65 insertions(+), 9 deletions(-) create mode 100755 tests/language_2/vm/regression_37821_test.dart diff --git a/runtime/vm/interpreter.cc b/runtime/vm/interpreter.cc index 71aa86cd161..dea9ec38ba9 100644 --- a/runtime/vm/interpreter.cc +++ b/runtime/vm/interpreter.cc @@ -13,6 +13,7 @@ #include "vm/compiler/assembler/assembler.h" #include "vm/compiler/assembler/disassembler_kbc.h" +#include "vm/compiler/backend/flow_graph_compiler.h" #include "vm/compiler/frontend/bytecode_reader.h" #include "vm/compiler/jit/compiler.h" #include "vm/cpu.h" @@ -389,6 +390,9 @@ Interpreter::Interpreter() } } #endif + // Make sure interpreter's unboxing view is consistent with compiler. + supports_unboxed_doubles_ = FlowGraphCompiler::SupportsUnboxedDoubles(); + supports_unboxed_simd128_ = FlowGraphCompiler::SupportsUnboxedSimd128(); } Interpreter::~Interpreter() { @@ -2317,7 +2321,7 @@ SwitchDispatch: (field->ptr()->is_nullable_ != kNullCid) && Field::UnboxingCandidateBit::decode(field->ptr()->kind_bits_); classid_t guarded_cid = field->ptr()->guarded_cid_; - if (unboxing && (guarded_cid == kDoubleCid)) { + if (unboxing && (guarded_cid == kDoubleCid) && supports_unboxed_doubles_) { double raw_value = Double::RawCast(value)->ptr()->value_; ASSERT(*(reinterpret_cast(instance->ptr()) + offset_in_words) == null_value); // Initializing store. @@ -2329,7 +2333,8 @@ SwitchDispatch: instance->StorePointer( reinterpret_cast(instance->ptr()) + offset_in_words, box, thread); - } else if (unboxing && (guarded_cid == kFloat32x4Cid)) { + } else if (unboxing && (guarded_cid == kFloat32x4Cid) && + supports_unboxed_simd128_) { simd128_value_t raw_value; raw_value.readFrom(Float32x4::RawCast(value)->ptr()->value_); ASSERT(*(reinterpret_cast(instance->ptr()) + @@ -2342,7 +2347,8 @@ SwitchDispatch: instance->StorePointer( reinterpret_cast(instance->ptr()) + offset_in_words, box, thread); - } else if (unboxing && (guarded_cid == kFloat64x2Cid)) { + } else if (unboxing && (guarded_cid == kFloat64x2Cid) && + supports_unboxed_simd128_) { simd128_value_t raw_value; raw_value.readFrom(Float64x2::RawCast(value)->ptr()->value_); ASSERT(*(reinterpret_cast(instance->ptr()) + @@ -3078,20 +3084,23 @@ SwitchDispatch: (field->ptr()->is_nullable_ != kNullCid) && Field::UnboxingCandidateBit::decode(field->ptr()->kind_bits_); classid_t guarded_cid = field->ptr()->guarded_cid_; - if (unboxing && (guarded_cid == kDoubleCid)) { + if (unboxing && (guarded_cid == kDoubleCid) && supports_unboxed_doubles_) { + ASSERT(FlowGraphCompiler::SupportsUnboxedDoubles()); double raw_value = Double::RawCast(value)->ptr()->value_; // AllocateDouble places result at SP[0] if (!AllocateDouble(thread, raw_value, pc, FP, SP)) { HANDLE_EXCEPTION; } - } else if (unboxing && (guarded_cid == kFloat32x4Cid)) { + } else if (unboxing && (guarded_cid == kFloat32x4Cid) && + supports_unboxed_simd128_) { simd128_value_t raw_value; raw_value.readFrom(Float32x4::RawCast(value)->ptr()->value_); // AllocateFloat32x4 places result at SP[0] if (!AllocateFloat32x4(thread, raw_value, pc, FP, SP)) { HANDLE_EXCEPTION; } - } else if (unboxing && (guarded_cid == kFloat64x2Cid)) { + } else if (unboxing && (guarded_cid == kFloat64x2Cid) && + supports_unboxed_simd128_) { simd128_value_t raw_value; raw_value.readFrom(Float64x2::RawCast(value)->ptr()->value_); // AllocateFloat64x2 places result at SP[0] @@ -3193,20 +3202,22 @@ SwitchDispatch: (field->ptr()->is_nullable_ != kNullCid) && Field::UnboxingCandidateBit::decode(field->ptr()->kind_bits_); classid_t guarded_cid = field->ptr()->guarded_cid_; - if (unboxing && (guarded_cid == kDoubleCid)) { + if (unboxing && (guarded_cid == kDoubleCid) && supports_unboxed_doubles_) { double raw_value = Double::RawCast(value)->ptr()->value_; RawDouble* box = *(reinterpret_cast(instance->ptr()) + offset_in_words); ASSERT(box != null_value); // Non-initializing store. box->ptr()->value_ = raw_value; - } else if (unboxing && (guarded_cid == kFloat32x4Cid)) { + } else if (unboxing && (guarded_cid == kFloat32x4Cid) && + supports_unboxed_simd128_) { simd128_value_t raw_value; raw_value.readFrom(Float32x4::RawCast(value)->ptr()->value_); RawFloat32x4* box = *(reinterpret_cast(instance->ptr()) + offset_in_words); ASSERT(box != null_value); // Non-initializing store. raw_value.writeTo(box->ptr()->value_); - } else if (unboxing && (guarded_cid == kFloat64x2Cid)) { + } else if (unboxing && (guarded_cid == kFloat64x2Cid) && + supports_unboxed_simd128_) { simd128_value_t raw_value; raw_value.readFrom(Float64x2::RawCast(value)->ptr()->value_); RawFloat64x2* box = *(reinterpret_cast(instance->ptr()) + diff --git a/runtime/vm/interpreter.h b/runtime/vm/interpreter.h index fd40ec50cc6..4889a5a7750 100644 --- a/runtime/vm/interpreter.h +++ b/runtime/vm/interpreter.h @@ -275,7 +275,11 @@ class Interpreter { bool is_debugging_; #endif // !PRODUCT + bool supports_unboxed_doubles_; + bool supports_unboxed_simd128_; + friend class InterpreterSetjmpBuffer; + DISALLOW_COPY_AND_ASSIGN(Interpreter); }; diff --git a/tests/language_2/vm/regression_37821_test.dart b/tests/language_2/vm/regression_37821_test.dart new file mode 100755 index 00000000000..fd7ef2d2ac3 --- /dev/null +++ b/tests/language_2/vm/regression_37821_test.dart @@ -0,0 +1,41 @@ +// 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. + +// VMOptions=--deterministic + +import "package:expect/expect.dart"; + +import 'dart:typed_data'; + +// Found by DartFuzzing: inconsistent view of unboxing +// https://github.com/dart-lang/sdk/issues/37821 + +double var5 = 0.5521203015696288; + +class X0 { + double fld0_1 = 0.44794902547497595; + void run() { + for (int loc1 = 0; loc1 < 11; loc1++) { + var5 = fld0_1; + } + fld0_1 -= 20; + } +} + +class X1 extends X0 { + void run() { + super.run(); + } +} + +@pragma("vm:never-inline") +void checkMe(double value) { + Expect.approxEquals(0.44794902547497595, value); +} + +main() { + Expect.approxEquals(0.5521203015696288, var5); + new X1().run(); + checkMe(var5); +}