From 7e1fbe8f341161ca57f95f01b263643d139eb14a Mon Sep 17 00:00:00 2001 From: Leszek Swirski Date: Mon, 26 Aug 2019 17:00:57 +0200 Subject: [PATCH] [ic] Check Double representation on store For stores with Double feedback, StoreIC needs to check that the representation is still Double before doing the store, in case it accidentally tries to write to an object or worse, mutate a non-mutable HeapNumber. Bug: v8:9606 Bug: chromium:997485 Change-Id: I51e0953b40f752648c5e86b8644c23baf636367e Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1768373 Commit-Queue: Leszek Swirski Reviewed-by: Igor Sheludko Cr-Commit-Position: refs/heads/master@{#63402} --- src/codegen/code-stub-assembler.h | 10 ++ src/ic/accessor-assembler.cc | 19 ++++ test/mjsunit/regress/regress-997485.js | 127 +++++++++++++++++++++++++ 3 files changed, 156 insertions(+) create mode 100644 test/mjsunit/regress/regress-997485.js diff --git a/src/codegen/code-stub-assembler.h b/src/codegen/code-stub-assembler.h index 6410368d9a..c9861062ff 100644 --- a/src/codegen/code-stub-assembler.h +++ b/src/codegen/code-stub-assembler.h @@ -2731,6 +2731,16 @@ class V8_EXPORT_PRIVATE CodeStubAssembler return Word32Equal(Word32And(word32, const_mask), const_mask); } + // Returns true if the bit field |BitField| in |word32| is equal to a given. + // constant |value|. Avoids a shift compared to using DecodeWord32. + template + TNode IsEqualInWord32(TNode word32, + typename BitField::FieldType value) { + TNode masked_word32 = + Word32And(word32, Int32Constant(BitField::kMask)); + return Word32Equal(masked_word32, Int32Constant(BitField::encode(value))); + } + // Returns true if any of the |T|'s bits in given |word| are set. template TNode IsSetWord(SloppyTNode word) { diff --git a/src/ic/accessor-assembler.cc b/src/ic/accessor-assembler.cc index 8a102016b6..ef261b9abe 100644 --- a/src/ic/accessor-assembler.cc +++ b/src/ic/accessor-assembler.cc @@ -16,6 +16,7 @@ #include "src/objects/heap-number.h" #include "src/objects/module.h" #include "src/objects/objects-inl.h" +#include "src/objects/property-details.h" #include "src/objects/smi.h" namespace v8 { @@ -1663,6 +1664,24 @@ Node* AccessorAssembler::PrepareValueForStore(Node* handler_word, Node* holder, if (representation.IsDouble()) { value = TryTaggedToFloat64(value, bailout); + // We have to check that the representation is still Double. Checking the + // value is nor enough, as we could have transitioned to Tagged but still + // be holding a HeapNumber, which would no longer be allowed to be mutable. + + // TODO(leszeks): We could skip the representation check in favor of a + // constant value check in StoreNamedField here, but then StoreNamedField + // would need an IsHeapNumber check in case both the representation changed + // and the value is no longer a HeapNumber. + TNode descriptor_entry = + Signed(DecodeWord(handler_word)); + TNode descriptors = LoadMapDescriptors(LoadMap(holder)); + TNode details = + LoadDetailsByDescriptorEntry(descriptors, descriptor_entry); + + GotoIfNot(IsEqualInWord32( + details, Representation::kDouble), + bailout); + } else if (representation.IsHeapObject()) { GotoIf(TaggedIsSmi(value), bailout); diff --git a/test/mjsunit/regress/regress-997485.js b/test/mjsunit/regress/regress-997485.js new file mode 100644 index 0000000000..bcc1664222 --- /dev/null +++ b/test/mjsunit/regress/regress-997485.js @@ -0,0 +1,127 @@ +// Copyright 2019 the V8 project authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +// Flags: --allow-natives-syntax + +(function doubleToTaggedWithTaggedValueStoresCorrectly() { + + function setX_Double(o) { o.x = 4.2; } + + function foo() { + // o.x starts off as Double + const o = { x: 0.1 }; + + // Write to it a few times with setX_Double, to make sure setX_Double has + // Double feedback. + setX_Double(o); + setX_Double(o); + + // Transition o.x to Tagged. + o.x = {}; + + // setX_Double will still have Double feedback, so make sure it works with + // the new Tagged representation o.x. + setX_Double(o); + + assertEquals(o.x, 4.2); + } + + %EnsureFeedbackVectorForFunction(setX_Double); + foo(); + +})(); + +(function doubleToTaggedWithDoubleValueDoesNotMutate() { + + function setX_Double(o) { o.x = 4.2; } + + function foo() { + // o.x starts off as Double + const o = { x: 0.1 }; + + // Write to it a few times with setX_Double, to make sure setX_Double has + // Double feedback. + setX_Double(o); + setX_Double(o); + + // Transition o.x to Tagged. + o.x = {}; + + // Write the HeapNumber val to o.x. + const val = 1.25; + o.x = val; + + // setX_Double will still have Double feedback, which expects to be able to + // mutate o.x's HeapNumber, so make sure it does not mutate val. + setX_Double(o); + + assertEquals(o.x, 4.2); + assertNotEquals(val, 4.2); + } + + %EnsureFeedbackVectorForFunction(setX_Double); + foo(); + +})(); + +(function doubleToTaggedWithTaggedValueStoresSmiCorrectly() { + + function setX_Smi(o) { o.x = 42; } + + function foo() { + // o.x starts off as Double + const o = { x: 0.1 }; + + // Write to it a few times with setX_Smi, to make sure setX_Smi has + // Double feedback. + setX_Smi(o); + setX_Smi(o); + + // Transition o.x to Tagged. + o.x = {}; + + // setX_Smi will still have Double feedback, so make sure it works with + // the new Tagged representation o.x. + setX_Smi(o); + + assertEquals(o.x, 42); + } + + %EnsureFeedbackVectorForFunction(setX_Smi); + foo(); + +})(); + +(function doubleToTaggedWithSmiValueDoesNotMutate() { + + function setX_Smi(o) { o.x = 42; } + + function foo() { + // o.x starts off as Double + const o = { x: 0.1 }; + + // Write to it a few times with setX_Smi, to make sure setX_Smi has + // Double feedback. + setX_Smi(o); + setX_Smi(o); + + // Transition o.x to Tagged. + o.x = {}; + + // Write the HeapNumber val to o.x. + const val = 1.25; + o.x = val; + + // setX_Smi will still have Double feedback, which expects to be able to + // mutate o.x's HeapNumber, so make sure it does not mutate val. + setX_Smi(o); + + assertEquals(o.x, 42); + assertNotEquals(val, 42); + } + + %EnsureFeedbackVectorForFunction(setX_Smi); + foo(); + +})();